| Message ID | 20260701132555.66035-2-david.plowman@raspberrypi.com |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
Actually I want to drop this patch. There are things not right about it. Will have another think! David On Wed, 1 Jul 2026 at 14:25, David Plowman <david.plowman@raspberrypi.com> wrote: > > This allows camera images to be written to offset locations within > buffers (which must have be allocated with sufficient space > available). > > Firstly, the RPi::Stream class tracks associated "ispOutputIndex" for > output streams, that is, whether it corresponds to ISP output branch 0 > or branch 1. > > Secondly, we pass buffer offsets on to libpisp for each request. > > The libpisp version is moved on to 1.6.0 which is required for this > change. > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > --- > .../pipeline/rpi/common/rpi_stream.h | 10 ++++++++-- > src/libcamera/pipeline/rpi/pisp/meson.build | 2 +- > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 19 +++++++++++++++++++ > subprojects/libpisp.wrap | 2 +- > 4 files changed, 29 insertions(+), 4 deletions(-) > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > index 300a352a..51931867 100644 > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > @@ -97,14 +97,14 @@ public: > using StreamFlags = Flags<StreamFlag>; > > Stream() > - : flags_(StreamFlag::None), id_(0), swDownscale_(0) > + : flags_(StreamFlag::None), id_(0), swDownscale_(0), ispOutputIndex_(-1) > { > } > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > : flags_(flags), name_(name), > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0), > - swDownscale_(0) > + swDownscale_(0), ispOutputIndex_(-1) > { > } > > @@ -125,6 +125,9 @@ public: > > void setExportedBuffer(FrameBuffer *buffer); > > + void setIspIndex(int ispIndex) { ispOutputIndex_ = ispIndex; } > + int getIspIndex() const { return ispOutputIndex_; } > + > int allocateBuffers(unsigned int count); > int queueBuffer(FrameBuffer *buffer); > void returnBuffer(FrameBuffer *buffer); > @@ -181,6 +184,9 @@ private: > * as the stream needs to maintain ownership of these buffers. > */ > std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_; > + > + /* For output streams, the ISP branch for this stream (otherwise -1). */ > + int ispOutputIndex_; > }; > > /* > diff --git a/src/libcamera/pipeline/rpi/pisp/meson.build b/src/libcamera/pipeline/rpi/pisp/meson.build > index 178df94c..121f93ef 100644 > --- a/src/libcamera/pipeline/rpi/pisp/meson.build > +++ b/src/libcamera/pipeline/rpi/pisp/meson.build > @@ -5,7 +5,7 @@ libcamera_internal_sources += files([ > ]) > > librt = cc.find_library('rt', required : true) > -libpisp_dep = dependency('libpisp', fallback : ['libpisp', 'libpisp_dep']) > +libpisp_dep = dependency('libpisp', version : '>=1.6.0', fallback : ['libpisp', 'libpisp_dep']) > > libcamera_deps += [libpisp_dep, librt] > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > index c9d89d58..23f4c14d 100644 > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > @@ -1524,6 +1524,7 @@ int PiSPCameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConf > beEnables |= PISP_BE_RGB_ENABLE_OUTPUT0; > ispIndex = 0; > } > + stream->setIspIndex(ispIndex); > > format = outStreams[i].format; > bool needs32BitConversion = adjustDeviceFormat(format); > @@ -2306,6 +2307,24 @@ void PiSPCameraData::tryRunPipeline() > Request *request = requestQueue_.front(); > ASSERT(request->metadata().empty()); > > + /* Pass the plane offsets of any output buffers to the Back End ISP. */ > + for (auto const &[stream, buffer] : request->buffers()) { > + int ispIndex = static_cast<const RPi::Stream *>(stream)->getIspIndex(); > + if (ispIndex >= 0) { > + /* > + * PiSP takes only 2 offsets; offset 3 (where required) is assumed > + * to be the same as offset 2. > + */ > + unsigned int offset = buffer->planes()[0].offset; > + unsigned int offset2 = 0; > + if (buffer->planes().size() > 1) > + offset2 = buffer->planes()[1].offset; > + > + pisp_be_output_format_extra extra{ 0, 0, { offset, offset2 } }; > + be_->SetOutputFormatExtra(ispIndex, extra); > + } > + } > + > /* See if a new ScalerCrop value needs to be applied. */ > applyScalerCrop(request->controls()); > > diff --git a/subprojects/libpisp.wrap b/subprojects/libpisp.wrap > index b92de484..1a3c3b77 100644 > --- a/subprojects/libpisp.wrap > +++ b/subprojects/libpisp.wrap > @@ -2,5 +2,5 @@ > > [wrap-git] > url = https://github.com/raspberrypi/libpisp.git > -revision = v1.5.0 > +revision = v1.6.0 > depth = 1 > -- > 2.47.3 >
On Thu, Jul 02, 2026 at 12:55:26PM +0100, David Plowman wrote: > Actually I want to drop this patch. There are things not right about > it. Will have another think! I think we also need to consider it in the context of Barnabás buffers split from requests, which turns the implicit buffer queues into pools and therefore drops the guarantee that FrameBuffer objects will be used in the order they're given to the camera. > On Wed, 1 Jul 2026 at 14:25, David Plowman wrote: > > > > This allows camera images to be written to offset locations within > > buffers (which must have be allocated with sufficient space > > available). > > > > Firstly, the RPi::Stream class tracks associated "ispOutputIndex" for > > output streams, that is, whether it corresponds to ISP output branch 0 > > or branch 1. > > > > Secondly, we pass buffer offsets on to libpisp for each request. > > > > The libpisp version is moved on to 1.6.0 which is required for this > > change. > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > --- > > .../pipeline/rpi/common/rpi_stream.h | 10 ++++++++-- > > src/libcamera/pipeline/rpi/pisp/meson.build | 2 +- > > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 19 +++++++++++++++++++ > > subprojects/libpisp.wrap | 2 +- > > 4 files changed, 29 insertions(+), 4 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > index 300a352a..51931867 100644 > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > @@ -97,14 +97,14 @@ public: > > using StreamFlags = Flags<StreamFlag>; > > > > Stream() > > - : flags_(StreamFlag::None), id_(0), swDownscale_(0) > > + : flags_(StreamFlag::None), id_(0), swDownscale_(0), ispOutputIndex_(-1) > > { > > } > > > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > > : flags_(flags), name_(name), > > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0), > > - swDownscale_(0) > > + swDownscale_(0), ispOutputIndex_(-1) > > { > > } > > > > @@ -125,6 +125,9 @@ public: > > > > void setExportedBuffer(FrameBuffer *buffer); > > > > + void setIspIndex(int ispIndex) { ispOutputIndex_ = ispIndex; } > > + int getIspIndex() const { return ispOutputIndex_; } > > + > > int allocateBuffers(unsigned int count); > > int queueBuffer(FrameBuffer *buffer); > > void returnBuffer(FrameBuffer *buffer); > > @@ -181,6 +184,9 @@ private: > > * as the stream needs to maintain ownership of these buffers. > > */ > > std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_; > > + > > + /* For output streams, the ISP branch for this stream (otherwise -1). */ > > + int ispOutputIndex_; > > }; > > > > /* > > diff --git a/src/libcamera/pipeline/rpi/pisp/meson.build b/src/libcamera/pipeline/rpi/pisp/meson.build > > index 178df94c..121f93ef 100644 > > --- a/src/libcamera/pipeline/rpi/pisp/meson.build > > +++ b/src/libcamera/pipeline/rpi/pisp/meson.build > > @@ -5,7 +5,7 @@ libcamera_internal_sources += files([ > > ]) > > > > librt = cc.find_library('rt', required : true) > > -libpisp_dep = dependency('libpisp', fallback : ['libpisp', 'libpisp_dep']) > > +libpisp_dep = dependency('libpisp', version : '>=1.6.0', fallback : ['libpisp', 'libpisp_dep']) > > > > libcamera_deps += [libpisp_dep, librt] > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > index c9d89d58..23f4c14d 100644 > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > @@ -1524,6 +1524,7 @@ int PiSPCameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConf > > beEnables |= PISP_BE_RGB_ENABLE_OUTPUT0; > > ispIndex = 0; > > } > > + stream->setIspIndex(ispIndex); > > > > format = outStreams[i].format; > > bool needs32BitConversion = adjustDeviceFormat(format); > > @@ -2306,6 +2307,24 @@ void PiSPCameraData::tryRunPipeline() > > Request *request = requestQueue_.front(); > > ASSERT(request->metadata().empty()); > > > > + /* Pass the plane offsets of any output buffers to the Back End ISP. */ > > + for (auto const &[stream, buffer] : request->buffers()) { > > + int ispIndex = static_cast<const RPi::Stream *>(stream)->getIspIndex(); > > + if (ispIndex >= 0) { > > + /* > > + * PiSP takes only 2 offsets; offset 3 (where required) is assumed > > + * to be the same as offset 2. > > + */ > > + unsigned int offset = buffer->planes()[0].offset; > > + unsigned int offset2 = 0; > > + if (buffer->planes().size() > 1) > > + offset2 = buffer->planes()[1].offset; > > + > > + pisp_be_output_format_extra extra{ 0, 0, { offset, offset2 } }; > > + be_->SetOutputFormatExtra(ispIndex, extra); > > + } > > + } > > + > > /* See if a new ScalerCrop value needs to be applied. */ > > applyScalerCrop(request->controls()); > > > > diff --git a/subprojects/libpisp.wrap b/subprojects/libpisp.wrap > > index b92de484..1a3c3b77 100644 > > --- a/subprojects/libpisp.wrap > > +++ b/subprojects/libpisp.wrap > > @@ -2,5 +2,5 @@ > > > > [wrap-git] > > url = https://github.com/raspberrypi/libpisp.git > > -revision = v1.5.0 > > +revision = v1.6.0 > > depth = 1
Hi Laurent Happy to pause this for a bit, but maybe just to recap what seem to be the sticking points: 1. As Dave pointed out elsewhere, V4L2 explicitly doesn't propagate the offsets for "capture" buffers, which is what these are. This seems like a bit of a blocker! 2. libpisp, on the other hand, trivially does exactly what we need. "All" that is required is to get an offset number to it from somewhere... 3. The code above gets the number from the plane offset, which doesn't feel super-lovely to me. Putting something in the StreamConfiguration might be tidier, but I can imagine objections from other PHs because (a) it would require work and (b) might not be supportable anyway. David On Thu, 2 Jul 2026 at 13:01, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Jul 02, 2026 at 12:55:26PM +0100, David Plowman wrote: > > Actually I want to drop this patch. There are things not right about > > it. Will have another think! > > I think we also need to consider it in the context of Barnabás buffers > split from requests, which turns the implicit buffer queues into pools > and therefore drops the guarantee that FrameBuffer objects will be used > in the order they're given to the camera. > > > On Wed, 1 Jul 2026 at 14:25, David Plowman wrote: > > > > > > This allows camera images to be written to offset locations within > > > buffers (which must have be allocated with sufficient space > > > available). > > > > > > Firstly, the RPi::Stream class tracks associated "ispOutputIndex" for > > > output streams, that is, whether it corresponds to ISP output branch 0 > > > or branch 1. > > > > > > Secondly, we pass buffer offsets on to libpisp for each request. > > > > > > The libpisp version is moved on to 1.6.0 which is required for this > > > change. > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > --- > > > .../pipeline/rpi/common/rpi_stream.h | 10 ++++++++-- > > > src/libcamera/pipeline/rpi/pisp/meson.build | 2 +- > > > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 19 +++++++++++++++++++ > > > subprojects/libpisp.wrap | 2 +- > > > 4 files changed, 29 insertions(+), 4 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > index 300a352a..51931867 100644 > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > @@ -97,14 +97,14 @@ public: > > > using StreamFlags = Flags<StreamFlag>; > > > > > > Stream() > > > - : flags_(StreamFlag::None), id_(0), swDownscale_(0) > > > + : flags_(StreamFlag::None), id_(0), swDownscale_(0), ispOutputIndex_(-1) > > > { > > > } > > > > > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > > > : flags_(flags), name_(name), > > > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0), > > > - swDownscale_(0) > > > + swDownscale_(0), ispOutputIndex_(-1) > > > { > > > } > > > > > > @@ -125,6 +125,9 @@ public: > > > > > > void setExportedBuffer(FrameBuffer *buffer); > > > > > > + void setIspIndex(int ispIndex) { ispOutputIndex_ = ispIndex; } > > > + int getIspIndex() const { return ispOutputIndex_; } > > > + > > > int allocateBuffers(unsigned int count); > > > int queueBuffer(FrameBuffer *buffer); > > > void returnBuffer(FrameBuffer *buffer); > > > @@ -181,6 +184,9 @@ private: > > > * as the stream needs to maintain ownership of these buffers. > > > */ > > > std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_; > > > + > > > + /* For output streams, the ISP branch for this stream (otherwise -1). */ > > > + int ispOutputIndex_; > > > }; > > > > > > /* > > > diff --git a/src/libcamera/pipeline/rpi/pisp/meson.build b/src/libcamera/pipeline/rpi/pisp/meson.build > > > index 178df94c..121f93ef 100644 > > > --- a/src/libcamera/pipeline/rpi/pisp/meson.build > > > +++ b/src/libcamera/pipeline/rpi/pisp/meson.build > > > @@ -5,7 +5,7 @@ libcamera_internal_sources += files([ > > > ]) > > > > > > librt = cc.find_library('rt', required : true) > > > -libpisp_dep = dependency('libpisp', fallback : ['libpisp', 'libpisp_dep']) > > > +libpisp_dep = dependency('libpisp', version : '>=1.6.0', fallback : ['libpisp', 'libpisp_dep']) > > > > > > libcamera_deps += [libpisp_dep, librt] > > > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > index c9d89d58..23f4c14d 100644 > > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > @@ -1524,6 +1524,7 @@ int PiSPCameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConf > > > beEnables |= PISP_BE_RGB_ENABLE_OUTPUT0; > > > ispIndex = 0; > > > } > > > + stream->setIspIndex(ispIndex); > > > > > > format = outStreams[i].format; > > > bool needs32BitConversion = adjustDeviceFormat(format); > > > @@ -2306,6 +2307,24 @@ void PiSPCameraData::tryRunPipeline() > > > Request *request = requestQueue_.front(); > > > ASSERT(request->metadata().empty()); > > > > > > + /* Pass the plane offsets of any output buffers to the Back End ISP. */ > > > + for (auto const &[stream, buffer] : request->buffers()) { > > > + int ispIndex = static_cast<const RPi::Stream *>(stream)->getIspIndex(); > > > + if (ispIndex >= 0) { > > > + /* > > > + * PiSP takes only 2 offsets; offset 3 (where required) is assumed > > > + * to be the same as offset 2. > > > + */ > > > + unsigned int offset = buffer->planes()[0].offset; > > > + unsigned int offset2 = 0; > > > + if (buffer->planes().size() > 1) > > > + offset2 = buffer->planes()[1].offset; > > > + > > > + pisp_be_output_format_extra extra{ 0, 0, { offset, offset2 } }; > > > + be_->SetOutputFormatExtra(ispIndex, extra); > > > + } > > > + } > > > + > > > /* See if a new ScalerCrop value needs to be applied. */ > > > applyScalerCrop(request->controls()); > > > > > > diff --git a/subprojects/libpisp.wrap b/subprojects/libpisp.wrap > > > index b92de484..1a3c3b77 100644 > > > --- a/subprojects/libpisp.wrap > > > +++ b/subprojects/libpisp.wrap > > > @@ -2,5 +2,5 @@ > > > > > > [wrap-git] > > > url = https://github.com/raspberrypi/libpisp.git > > > -revision = v1.5.0 > > > +revision = v1.6.0 > > > depth = 1 > > -- > Regards, > > Laurent Pinchart
On Fri, Jul 03, 2026 at 10:01:15AM +0100, David Plowman wrote: > Hi Laurent > > Happy to pause this for a bit, but maybe just to recap what seem to be > the sticking points: > > 1. As Dave pointed out elsewhere, V4L2 explicitly doesn't propagate > the offsets for "capture" buffers, which is what these are. This seems > like a bit of a blocker! > > 2. libpisp, on the other hand, trivially does exactly what we need. > "All" that is required is to get an offset number to it from > somewhere... I agree with the above, but FrameBuffer::Plane::offset does not need to be blindly propagated to v4l2_buffer.m.planes.m.offset. The pipeline handler can use the offset to program the ISP through a different mechanism. > 3. The code above gets the number from the plane offset, which doesn't > feel super-lovely to me. Putting something in the StreamConfiguration > might be tidier, but I can imagine objections from other PHs because > (a) it would require work and (b) might not be supportable anyway. I'm not very concerned about (a). My first concern about the control you initially proposed is that interactions with other API elements (including FrameBuffer::Plane::offset, as well as other controls) are not defined. FrameBuffer::Plane::offset was designed to tell where in the DMA buffer the camera should store data. I don't want to add another mechanism without considering the existing one. Maybe we would conclude after design discussions that FrameBuffer::Plane::offset should be replaced by a control, but those design discussions never happened. > On Thu, 2 Jul 2026 at 13:01, Laurent Pinchart wrote: > > On Thu, Jul 02, 2026 at 12:55:26PM +0100, David Plowman wrote: > > > Actually I want to drop this patch. There are things not right about > > > it. Will have another think! > > > > I think we also need to consider it in the context of Barnabás buffers > > split from requests, which turns the implicit buffer queues into pools > > and therefore drops the guarantee that FrameBuffer objects will be used > > in the order they're given to the camera. > > > > > On Wed, 1 Jul 2026 at 14:25, David Plowman wrote: > > > > > > > > This allows camera images to be written to offset locations within > > > > buffers (which must have be allocated with sufficient space > > > > available). > > > > > > > > Firstly, the RPi::Stream class tracks associated "ispOutputIndex" for > > > > output streams, that is, whether it corresponds to ISP output branch 0 > > > > or branch 1. > > > > > > > > Secondly, we pass buffer offsets on to libpisp for each request. > > > > > > > > The libpisp version is moved on to 1.6.0 which is required for this > > > > change. > > > > > > > > Signed-off-by: David Plowman <david.plowman@raspberrypi.com> > > > > --- > > > > .../pipeline/rpi/common/rpi_stream.h | 10 ++++++++-- > > > > src/libcamera/pipeline/rpi/pisp/meson.build | 2 +- > > > > src/libcamera/pipeline/rpi/pisp/pisp.cpp | 19 +++++++++++++++++++ > > > > subprojects/libpisp.wrap | 2 +- > > > > 4 files changed, 29 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > index 300a352a..51931867 100644 > > > > --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h > > > > @@ -97,14 +97,14 @@ public: > > > > using StreamFlags = Flags<StreamFlag>; > > > > > > > > Stream() > > > > - : flags_(StreamFlag::None), id_(0), swDownscale_(0) > > > > + : flags_(StreamFlag::None), id_(0), swDownscale_(0), ispOutputIndex_(-1) > > > > { > > > > } > > > > > > > > Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) > > > > : flags_(flags), name_(name), > > > > dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0), > > > > - swDownscale_(0) > > > > + swDownscale_(0), ispOutputIndex_(-1) > > > > { > > > > } > > > > > > > > @@ -125,6 +125,9 @@ public: > > > > > > > > void setExportedBuffer(FrameBuffer *buffer); > > > > > > > > + void setIspIndex(int ispIndex) { ispOutputIndex_ = ispIndex; } > > > > + int getIspIndex() const { return ispOutputIndex_; } > > > > + > > > > int allocateBuffers(unsigned int count); > > > > int queueBuffer(FrameBuffer *buffer); > > > > void returnBuffer(FrameBuffer *buffer); > > > > @@ -181,6 +184,9 @@ private: > > > > * as the stream needs to maintain ownership of these buffers. > > > > */ > > > > std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_; > > > > + > > > > + /* For output streams, the ISP branch for this stream (otherwise -1). */ > > > > + int ispOutputIndex_; > > > > }; > > > > > > > > /* > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/meson.build b/src/libcamera/pipeline/rpi/pisp/meson.build > > > > index 178df94c..121f93ef 100644 > > > > --- a/src/libcamera/pipeline/rpi/pisp/meson.build > > > > +++ b/src/libcamera/pipeline/rpi/pisp/meson.build > > > > @@ -5,7 +5,7 @@ libcamera_internal_sources += files([ > > > > ]) > > > > > > > > librt = cc.find_library('rt', required : true) > > > > -libpisp_dep = dependency('libpisp', fallback : ['libpisp', 'libpisp_dep']) > > > > +libpisp_dep = dependency('libpisp', version : '>=1.6.0', fallback : ['libpisp', 'libpisp_dep']) > > > > > > > > libcamera_deps += [libpisp_dep, librt] > > > > > > > > diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > > index c9d89d58..23f4c14d 100644 > > > > --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > > +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp > > > > @@ -1524,6 +1524,7 @@ int PiSPCameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConf > > > > beEnables |= PISP_BE_RGB_ENABLE_OUTPUT0; > > > > ispIndex = 0; > > > > } > > > > + stream->setIspIndex(ispIndex); > > > > > > > > format = outStreams[i].format; > > > > bool needs32BitConversion = adjustDeviceFormat(format); > > > > @@ -2306,6 +2307,24 @@ void PiSPCameraData::tryRunPipeline() > > > > Request *request = requestQueue_.front(); > > > > ASSERT(request->metadata().empty()); > > > > > > > > + /* Pass the plane offsets of any output buffers to the Back End ISP. */ > > > > + for (auto const &[stream, buffer] : request->buffers()) { > > > > + int ispIndex = static_cast<const RPi::Stream *>(stream)->getIspIndex(); > > > > + if (ispIndex >= 0) { > > > > + /* > > > > + * PiSP takes only 2 offsets; offset 3 (where required) is assumed > > > > + * to be the same as offset 2. > > > > + */ > > > > + unsigned int offset = buffer->planes()[0].offset; > > > > + unsigned int offset2 = 0; > > > > + if (buffer->planes().size() > 1) > > > > + offset2 = buffer->planes()[1].offset; > > > > + > > > > + pisp_be_output_format_extra extra{ 0, 0, { offset, offset2 } }; > > > > + be_->SetOutputFormatExtra(ispIndex, extra); > > > > + } > > > > + } > > > > + > > > > /* See if a new ScalerCrop value needs to be applied. */ > > > > applyScalerCrop(request->controls()); > > > > > > > > diff --git a/subprojects/libpisp.wrap b/subprojects/libpisp.wrap > > > > index b92de484..1a3c3b77 100644 > > > > --- a/subprojects/libpisp.wrap > > > > +++ b/subprojects/libpisp.wrap > > > > @@ -2,5 +2,5 @@ > > > > > > > > [wrap-git] > > > > url = https://github.com/raspberrypi/libpisp.git > > > > -revision = v1.5.0 > > > > +revision = v1.6.0 > > > > depth = 1
diff --git a/src/libcamera/pipeline/rpi/common/rpi_stream.h b/src/libcamera/pipeline/rpi/common/rpi_stream.h index 300a352a..51931867 100644 --- a/src/libcamera/pipeline/rpi/common/rpi_stream.h +++ b/src/libcamera/pipeline/rpi/common/rpi_stream.h @@ -97,14 +97,14 @@ public: using StreamFlags = Flags<StreamFlag>; Stream() - : flags_(StreamFlag::None), id_(0), swDownscale_(0) + : flags_(StreamFlag::None), id_(0), swDownscale_(0), ispOutputIndex_(-1) { } Stream(const char *name, MediaEntity *dev, StreamFlags flags = StreamFlag::None) : flags_(flags), name_(name), dev_(std::make_unique<V4L2VideoDevice>(dev)), id_(0), - swDownscale_(0) + swDownscale_(0), ispOutputIndex_(-1) { } @@ -125,6 +125,9 @@ public: void setExportedBuffer(FrameBuffer *buffer); + void setIspIndex(int ispIndex) { ispOutputIndex_ = ispIndex; } + int getIspIndex() const { return ispOutputIndex_; } + int allocateBuffers(unsigned int count); int queueBuffer(FrameBuffer *buffer); void returnBuffer(FrameBuffer *buffer); @@ -181,6 +184,9 @@ private: * as the stream needs to maintain ownership of these buffers. */ std::vector<std::unique_ptr<FrameBuffer>> internalBuffers_; + + /* For output streams, the ISP branch for this stream (otherwise -1). */ + int ispOutputIndex_; }; /* diff --git a/src/libcamera/pipeline/rpi/pisp/meson.build b/src/libcamera/pipeline/rpi/pisp/meson.build index 178df94c..121f93ef 100644 --- a/src/libcamera/pipeline/rpi/pisp/meson.build +++ b/src/libcamera/pipeline/rpi/pisp/meson.build @@ -5,7 +5,7 @@ libcamera_internal_sources += files([ ]) librt = cc.find_library('rt', required : true) -libpisp_dep = dependency('libpisp', fallback : ['libpisp', 'libpisp_dep']) +libpisp_dep = dependency('libpisp', version : '>=1.6.0', fallback : ['libpisp', 'libpisp_dep']) libcamera_deps += [libpisp_dep, librt] diff --git a/src/libcamera/pipeline/rpi/pisp/pisp.cpp b/src/libcamera/pipeline/rpi/pisp/pisp.cpp index c9d89d58..23f4c14d 100644 --- a/src/libcamera/pipeline/rpi/pisp/pisp.cpp +++ b/src/libcamera/pipeline/rpi/pisp/pisp.cpp @@ -1524,6 +1524,7 @@ int PiSPCameraData::platformConfigure(const RPi::RPiCameraConfiguration *rpiConf beEnables |= PISP_BE_RGB_ENABLE_OUTPUT0; ispIndex = 0; } + stream->setIspIndex(ispIndex); format = outStreams[i].format; bool needs32BitConversion = adjustDeviceFormat(format); @@ -2306,6 +2307,24 @@ void PiSPCameraData::tryRunPipeline() Request *request = requestQueue_.front(); ASSERT(request->metadata().empty()); + /* Pass the plane offsets of any output buffers to the Back End ISP. */ + for (auto const &[stream, buffer] : request->buffers()) { + int ispIndex = static_cast<const RPi::Stream *>(stream)->getIspIndex(); + if (ispIndex >= 0) { + /* + * PiSP takes only 2 offsets; offset 3 (where required) is assumed + * to be the same as offset 2. + */ + unsigned int offset = buffer->planes()[0].offset; + unsigned int offset2 = 0; + if (buffer->planes().size() > 1) + offset2 = buffer->planes()[1].offset; + + pisp_be_output_format_extra extra{ 0, 0, { offset, offset2 } }; + be_->SetOutputFormatExtra(ispIndex, extra); + } + } + /* See if a new ScalerCrop value needs to be applied. */ applyScalerCrop(request->controls()); diff --git a/subprojects/libpisp.wrap b/subprojects/libpisp.wrap index b92de484..1a3c3b77 100644 --- a/subprojects/libpisp.wrap +++ b/subprojects/libpisp.wrap @@ -2,5 +2,5 @@ [wrap-git] url = https://github.com/raspberrypi/libpisp.git -revision = v1.5.0 +revision = v1.6.0 depth = 1
This allows camera images to be written to offset locations within buffers (which must have be allocated with sufficient space available). Firstly, the RPi::Stream class tracks associated "ispOutputIndex" for output streams, that is, whether it corresponds to ISP output branch 0 or branch 1. Secondly, we pass buffer offsets on to libpisp for each request. The libpisp version is moved on to 1.6.0 which is required for this change. Signed-off-by: David Plowman <david.plowman@raspberrypi.com> --- .../pipeline/rpi/common/rpi_stream.h | 10 ++++++++-- src/libcamera/pipeline/rpi/pisp/meson.build | 2 +- src/libcamera/pipeline/rpi/pisp/pisp.cpp | 19 +++++++++++++++++++ subprojects/libpisp.wrap | 2 +- 4 files changed, 29 insertions(+), 4 deletions(-)