| Message ID | 20200911162039.61933-7-jacopo@jmondi.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | Jacopo Mondi |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thanks for your work. On 2020-09-11 18:20:39 +0200, Jacopo Mondi wrote: > Report for each request the pipeline depth describing the number of > processing steps the frames went into. > > This change shows a limitation in the current implementation as the > number of processing steps a frame has been run through is a property > of each Stream. Currently we report the maximum depth among all the > returned frames. Is this something we should address (at lest on how we want this to be handled) before adding this control to the IPU3 pipeline? I'm a bit worried about adding it without a plan. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9ce329a83f5d..26ea26430f8e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -12,6 +12,7 @@ > #include <vector> > > #include <libcamera/camera.h> > +#include <libcamera/control_ids.h> > #include <libcamera/formats.h> > #include <libcamera/property_ids.h> > #include <libcamera/request.h> > @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > +static const ControlInfoMap IPU3Controls = { > + { &controls::DraftPipelineDepth, ControlInfo(2, 3) } > +}; > + > class IPU3CameraData : public CameraData > { > public: > @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras() > data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes, > { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) }); > > + data->controlInfo_ = IPU3Controls; > + > /** > * \todo Dynamically assign ImgU and output devices to each > * stream and camera; as of now, limit support to two cameras > @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > return; > > /* Mark the request as complete. */ > + request->controls().set(controls::DraftPipelineDepth, 3); > pipe_->completeRequest(camera_, request); > } > > @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > if (request->findBuffer(&rawStream_)) { > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > if (isComplete) { > + request->controls().set(controls::DraftPipelineDepth, 2); > pipe_->completeRequest(camera_, request); > return; > } > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, On Sun, Sep 13, 2020 at 01:46:37PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your work. > > On 2020-09-11 18:20:39 +0200, Jacopo Mondi wrote: > > Report for each request the pipeline depth describing the number of > > processing steps the frames went into. > > > > This change shows a limitation in the current implementation as the > > number of processing steps a frame has been run through is a property > > of each Stream. Currently we report the maximum depth among all the > > returned frames. > > Is this something we should address (at lest on how we want this to be > handled) before adding this control to the IPU3 pipeline? I'm a bit > worried about adding it without a plan. > mmm, I might have been pessimistic. For the sake of reporting the draft control to android I think this is fine for now, as Android as well represents this with a metadata relatively to the request and not to each Stream. Thanks j > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 9ce329a83f5d..26ea26430f8e 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -12,6 +12,7 @@ > > #include <vector> > > > > #include <libcamera/camera.h> > > +#include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > #include <libcamera/property_ids.h> > > #include <libcamera/request.h> > > @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > > > +static const ControlInfoMap IPU3Controls = { > > + { &controls::DraftPipelineDepth, ControlInfo(2, 3) } > > +}; > > + > > class IPU3CameraData : public CameraData > > { > > public: > > @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras() > > data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes, > > { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) }); > > > > + data->controlInfo_ = IPU3Controls; > > + > > /** > > * \todo Dynamically assign ImgU and output devices to each > > * stream and camera; as of now, limit support to two cameras > > @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > return; > > > > /* Mark the request as complete. */ > > + request->controls().set(controls::DraftPipelineDepth, 3); > > pipe_->completeRequest(camera_, request); > > } > > > > @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > if (request->findBuffer(&rawStream_)) { > > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > > if (isComplete) { > > + request->controls().set(controls::DraftPipelineDepth, 2); > > pipe_->completeRequest(camera_, request); > > return; > > } > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel > > -- > Regards, > Niklas Söderlund
Hi Jacopo, On 11/09/2020 17:20, Jacopo Mondi wrote: > Report for each request the pipeline depth describing the number of > processing steps the frames went into. > > This change shows a limitation in the current implementation as the > number of processing steps a frame has been run through is a property > of each Stream. Currently we report the maximum depth among all the > returned frames. So do we need 'stream properties' ? > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index 9ce329a83f5d..26ea26430f8e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -12,6 +12,7 @@ > #include <vector> > > #include <libcamera/camera.h> > +#include <libcamera/control_ids.h> > #include <libcamera/formats.h> > #include <libcamera/property_ids.h> > #include <libcamera/request.h> > @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > +static const ControlInfoMap IPU3Controls = { > + { &controls::DraftPipelineDepth, ControlInfo(2, 3) } > +}; > + I presume we're going to need to expose controls that are read from the V4L2 device too. Things like (given my recent discussion with David) the DigitalGain perhaps, or other things which are controls of the ISP in particular ? This might need to be updated at load/runtime in that case, so I don't think the static will be suitable - but that's a future problem. I'm actually not worried about that here right now - though perhaps we should add some sort of a \todo. > class IPU3CameraData : public CameraData > { > public: > @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras() > data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes, > { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) }); > > + data->controlInfo_ = IPU3Controls; > + > /** > * \todo Dynamically assign ImgU and output devices to each > * stream and camera; as of now, limit support to two cameras > @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > return; > > /* Mark the request as complete. */ > + request->controls().set(controls::DraftPipelineDepth, 3); > pipe_->completeRequest(camera_, request); > } > > @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > if (request->findBuffer(&rawStream_)) { > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > if (isComplete) { > + request->controls().set(controls::DraftPipelineDepth, 2); > pipe_->completeRequest(camera_, request); > return; > } > Otherwise, this patch is actually simple enough that I think I can give this already: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Although, I expect there to be some minimal changes in here as this series develops anyway, but nothing that I foresee being troublesome
Hi Kieran, On Wed, Oct 07, 2020 at 07:41:56PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 11/09/2020 17:20, Jacopo Mondi wrote: > > Report for each request the pipeline depth describing the number of > > processing steps the frames went into. > > > > This change shows a limitation in the current implementation as the > > number of processing steps a frame has been run through is a property > > of each Stream. Currently we report the maximum depth among all the > > returned frames. > > So do we need 'stream properties' ? > As well as we might need 'stream controls'. > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++ > > 1 file changed, 9 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index 9ce329a83f5d..26ea26430f8e 100644 > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > @@ -12,6 +12,7 @@ > > #include <vector> > > > > #include <libcamera/camera.h> > > +#include <libcamera/control_ids.h> > > #include <libcamera/formats.h> > > #include <libcamera/property_ids.h> > > #include <libcamera/request.h> > > @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; > > static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; > > static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; > > > > +static const ControlInfoMap IPU3Controls = { > > + { &controls::DraftPipelineDepth, ControlInfo(2, 3) } > > +}; > > + > > I presume we're going to need to expose controls that are read from the > V4L2 device too. Things like (given my recent discussion with David) the > DigitalGain perhaps, or other things which are controls of the ISP in > particular ? > To support manual control of the sensor we'll have to register a ControlInfo for each sensor control, yes. This opens another question: metadata vs controls. This is here just to be able to report a metadata, not a control which can be set by the application. So this is not needed and the below part is wrong > This might need to be updated at load/runtime in that case, so I don't > think the static will be suitable - but that's a future problem. More than the static, it's the const the problem here. The ControlInfoMap will have to be augmented at runtime yes, but for controls, not metadata as I'm doing here. > > I'm actually not worried about that here right now - though perhaps we > should add some sort of a \todo. > > > > class IPU3CameraData : public CameraData > > { > > public: > > @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras() > > data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes, > > { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) }); > > > > + data->controlInfo_ = IPU3Controls; > > + > > /** > > * \todo Dynamically assign ImgU and output devices to each > > * stream and camera; as of now, limit support to two cameras > > @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > return; > > > > /* Mark the request as complete. */ > > + request->controls().set(controls::DraftPipelineDepth, 3); This should be request->metadata() > > pipe_->completeRequest(camera_, request); > > } > > > > @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > if (request->findBuffer(&rawStream_)) { > > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > > if (isComplete) { > > + request->controls().set(controls::DraftPipelineDepth, 2); Same here. I had this in a fixup in my branch, I noticed this when I tried wiring this up to Android HAL. > > pipe_->completeRequest(camera_, request); > > return; > > } > > > > Otherwise, this patch is actually simple enough that I think I can give > this already: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Although, I expect there to be some minimal changes in here as this > series develops anyway, but nothing that I foresee being troublesome Yeah, I think I'll drop all tags in next series, as I think I've mixed up quire a few things in this first attempt. Thanks j > > -- > Regards > -- > Kieran
Hi Jacopo, On 08/10/2020 11:37, Jacopo Mondi wrote: > Hi Kieran, > > On Wed, Oct 07, 2020 at 07:41:56PM +0100, Kieran Bingham wrote: >> Hi Jacopo, >> >> On 11/09/2020 17:20, Jacopo Mondi wrote: >>> Report for each request the pipeline depth describing the number of >>> processing steps the frames went into. >>> >>> This change shows a limitation in the current implementation as the >>> number of processing steps a frame has been run through is a property >>> of each Stream. Currently we report the maximum depth among all the >>> returned frames. >> >> So do we need 'stream properties' ? >> > > As well as we might need 'stream controls'. > >> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> >>> --- >>> src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++ >>> 1 file changed, 9 insertions(+) >>> >>> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >>> index 9ce329a83f5d..26ea26430f8e 100644 >>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>> @@ -12,6 +12,7 @@ >>> #include <vector> >>> >>> #include <libcamera/camera.h> >>> +#include <libcamera/control_ids.h> >>> #include <libcamera/formats.h> >>> #include <libcamera/property_ids.h> >>> #include <libcamera/request.h> >>> @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; >>> static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; >>> static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; >>> >>> +static const ControlInfoMap IPU3Controls = { >>> + { &controls::DraftPipelineDepth, ControlInfo(2, 3) } >>> +}; >>> + >> >> I presume we're going to need to expose controls that are read from the >> V4L2 device too. Things like (given my recent discussion with David) the >> DigitalGain perhaps, or other things which are controls of the ISP in >> particular ? >> > > To support manual control of the sensor we'll have to register a ControlInfo > for each sensor control, yes. > > This opens another question: metadata vs controls. > This is here just to be able to report a metadata, not a control which > can be set by the application. So this is not needed and the below > part is wrong > >> This might need to be updated at load/runtime in that case, so I don't >> think the static will be suitable - but that's a future problem. > > More than the static, it's the const the problem here. > The ControlInfoMap will have to be augmented at runtime yes, but for > controls, not metadata as I'm doing here. > >> >> I'm actually not worried about that here right now - though perhaps we >> should add some sort of a \todo. >> >> >>> class IPU3CameraData : public CameraData >>> { >>> public: >>> @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras() >>> data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes, >>> { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) }); >>> >>> + data->controlInfo_ = IPU3Controls; >>> + >>> /** >>> * \todo Dynamically assign ImgU and output devices to each >>> * stream and camera; as of now, limit support to two cameras >>> @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) >>> return; >>> >>> /* Mark the request as complete. */ >>> + request->controls().set(controls::DraftPipelineDepth, 3); > > This should be request->metadata() Oh - yes of course - the app can't 'set' the depth ;-) > >>> pipe_->completeRequest(camera_, request); >>> } >>> >>> @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) >>> if (request->findBuffer(&rawStream_)) { >>> bool isComplete = pipe_->completeBuffer(camera_, request, buffer); >>> if (isComplete) { >>> + request->controls().set(controls::DraftPipelineDepth, 2); > > Same here. > > I had this in a fixup in my branch, I noticed this when I tried wiring > this up to Android HAL. > >>> pipe_->completeRequest(camera_, request); >>> return; >>> } >>> >> >> Otherwise, this patch is actually simple enough that I think I can give >> this already: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> Although, I expect there to be some minimal changes in here as this >> series develops anyway, but nothing that I foresee being troublesome > > Yeah, I think I'll drop all tags in next series, as I think I've mixed > up quire a few things in this first attempt. That's fine too! Look forward to the updates. Kieran > > Thanks > j > >> >> -- >> Regards >> -- >> Kieran
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9ce329a83f5d..26ea26430f8e 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -12,6 +12,7 @@ #include <vector> #include <libcamera/camera.h> +#include <libcamera/control_ids.h> #include <libcamera/formats.h> #include <libcamera/property_ids.h> #include <libcamera/request.h> @@ -41,6 +42,10 @@ static constexpr unsigned int IMGU_OUTPUT_HEIGHT_ALIGN = 4; static constexpr unsigned int IMGU_OUTPUT_WIDTH_MARGIN = 64; static constexpr unsigned int IMGU_OUTPUT_HEIGHT_MARGIN = 32; +static const ControlInfoMap IPU3Controls = { + { &controls::DraftPipelineDepth, ControlInfo(2, 3) } +}; + class IPU3CameraData : public CameraData { public: @@ -786,6 +791,8 @@ int PipelineHandlerIPU3::registerCameras() data->properties_.set(properties::DraftAvailableColorCorrectionAberrationModes, { static_cast<int32_t>(properties::DRAFT_COLOR_CORRECTION_ABERRATION_OFF) }); + data->controlInfo_ = IPU3Controls; + /** * \todo Dynamically assign ImgU and output devices to each * stream and camera; as of now, limit support to two cameras @@ -848,6 +855,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) return; /* Mark the request as complete. */ + request->controls().set(controls::DraftPipelineDepth, 3); pipe_->completeRequest(camera_, request); } @@ -873,6 +881,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) if (request->findBuffer(&rawStream_)) { bool isComplete = pipe_->completeBuffer(camera_, request, buffer); if (isComplete) { + request->controls().set(controls::DraftPipelineDepth, 2); pipe_->completeRequest(camera_, request); return; }
Report for each request the pipeline depth describing the number of processing steps the frames went into. This change shows a limitation in the current implementation as the number of processing steps a frame has been run through is a property of each Stream. Currently we report the maximum depth among all the returned frames. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 9 +++++++++ 1 file changed, 9 insertions(+)