| Message ID | 20201009122101.73858-7-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, On 09/10/2020 13:20, Jacopo Mondi wrote: > Report for each request the pipeline depth describing the number of > processing steps the frames went through. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index f7ade2a6d5f3..413c2b8b1f07 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> > @@ -841,6 +842,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > return; > > /* Mark the request as complete. */ > + request->metadata().set(controls::draft::PipelineDepth, 3); > pipe_->completeRequest(camera_, request); > } > > @@ -866,6 +868,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > if (request->findBuffer(&rawStream_)) { > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > if (isComplete) { > + request->metadata().set(controls::draft::PipelineDepth, 2); Will it be obvious to readers what the '2' stages are? Maybe later we'll somehow automatically handle it. I think hardcoding these values currently deserves an explanation of why it's only 2 (or 3 above) perhaps. But - eitherway - maybe that's overkill too. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > pipe_->completeRequest(camera_, request); > return; > } >
Hi Kieran, On Fri, Oct 09, 2020 at 01:36:57PM +0100, Kieran Bingham wrote: > Hi Jacopo, > > On 09/10/2020 13:20, Jacopo Mondi wrote: > > Report for each request the pipeline depth describing the number of > > processing steps the frames went through. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > index f7ade2a6d5f3..413c2b8b1f07 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> > > @@ -841,6 +842,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > return; > > > > /* Mark the request as complete. */ > > + request->metadata().set(controls::draft::PipelineDepth, 3); > > pipe_->completeRequest(camera_, request); > > } > > > > @@ -866,6 +868,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > if (request->findBuffer(&rawStream_)) { > > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > > if (isComplete) { > > + request->metadata().set(controls::draft::PipelineDepth, 2); > > Will it be obvious to readers what the '2' stages are? Maybe later we'll > somehow automatically handle it. > > I think hardcoding these values currently deserves an explanation of why > it's only 2 (or 3 above) perhaps. > > But - eitherway - maybe that's overkill too. Please don't forget this is pipeline code, not application facing code. I expect reading the proeprty documentation and noticing RAW=2, YUV=3 is pretty clear to someone poking with platform code. I can add a comment to previous patch that add the maximum pipeline depth though. > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > Thanks j > > pipe_->completeRequest(camera_, request); > > return; > > } > > > > -- > Regards > -- > Kieran
Hi Jacopo, Thank you for the patch. On Fri, Oct 09, 2020 at 02:51:12PM +0200, Jacopo Mondi wrote: > On Fri, Oct 09, 2020 at 01:36:57PM +0100, Kieran Bingham wrote: > > On 09/10/2020 13:20, Jacopo Mondi wrote: > > > Report for each request the pipeline depth describing the number of > > > processing steps the frames went through. > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > --- > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +++ > > > 1 file changed, 3 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > index f7ade2a6d5f3..413c2b8b1f07 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> > > > @@ -841,6 +842,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > > return; > > > > > > /* Mark the request as complete. */ > > > + request->metadata().set(controls::draft::PipelineDepth, 3); > > > pipe_->completeRequest(camera_, request); > > > } > > > > > > @@ -866,6 +868,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > > if (request->findBuffer(&rawStream_)) { > > > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > > > if (isComplete) { > > > + request->metadata().set(controls::draft::PipelineDepth, 2); > > > > Will it be obvious to readers what the '2' stages are? Maybe later we'll > > somehow automatically handle it. > > > > I think hardcoding these values currently deserves an explanation of why > > it's only 2 (or 3 above) perhaps. > > > > But - eitherway - maybe that's overkill too. > > Please don't forget this is pipeline code, not application facing > code. I expect reading the proeprty documentation and noticing RAW=2, > YUV=3 is pretty clear to someone poking with platform code. > > I can add a comment to previous patch that add the maximum pipeline > depth though. > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Starting the discussion to prepare for a non-draft implementation of this, down the line, would you envision this being a stream-level metadata ? > > > > > pipe_->completeRequest(camera_, request); > > > return; > > > }
Hi Laurent, On Mon, Oct 12, 2020 at 04:07:52AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Oct 09, 2020 at 02:51:12PM +0200, Jacopo Mondi wrote: > > On Fri, Oct 09, 2020 at 01:36:57PM +0100, Kieran Bingham wrote: > > > On 09/10/2020 13:20, Jacopo Mondi wrote: > > > > Report for each request the pipeline depth describing the number of > > > > processing steps the frames went through. > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > --- > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +++ > > > > 1 file changed, 3 insertions(+) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > index f7ade2a6d5f3..413c2b8b1f07 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> > > > > @@ -841,6 +842,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > > > return; > > > > > > > > /* Mark the request as complete. */ > > > > + request->metadata().set(controls::draft::PipelineDepth, 3); > > > > pipe_->completeRequest(camera_, request); > > > > } > > > > > > > > @@ -866,6 +868,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > > > if (request->findBuffer(&rawStream_)) { > > > > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > > > > if (isComplete) { > > > > + request->metadata().set(controls::draft::PipelineDepth, 2); > > > > > > Will it be obvious to readers what the '2' stages are? Maybe later we'll > > > somehow automatically handle it. > > > > > > I think hardcoding these values currently deserves an explanation of why > > > it's only 2 (or 3 above) perhaps. > > > > > > But - eitherway - maybe that's overkill too. > > > > Please don't forget this is pipeline code, not application facing > > code. I expect reading the proeprty documentation and noticing RAW=2, > > YUV=3 is pretty clear to someone poking with platform code. > > > > I can add a comment to previous patch that add the maximum pipeline > > depth though. > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Starting the discussion to prepare for a non-draft implementation of > this, down the line, would you envision this being a stream-level > metadata ? > To really answer this question I should have a better understanding of how this property is used by application. Strictly speaking, yes, each stream has its own number of processing steps applied, but maybe what really matters is the total applied to a request ? > > > > > > > pipe_->completeRequest(camera_, request); > > > > return; > > > > } > > -- > Regards, > > Laurent Pinchart
On Tue, Oct 13, 2020 at 10:57:08PM +0200, Jacopo Mondi wrote: > Hi Laurent, > > On Mon, Oct 12, 2020 at 04:07:52AM +0300, Laurent Pinchart wrote: > > Hi Jacopo, > > > > Thank you for the patch. > > > > On Fri, Oct 09, 2020 at 02:51:12PM +0200, Jacopo Mondi wrote: > > > On Fri, Oct 09, 2020 at 01:36:57PM +0100, Kieran Bingham wrote: > > > > On 09/10/2020 13:20, Jacopo Mondi wrote: > > > > > Report for each request the pipeline depth describing the number of > > > > > processing steps the frames went through. > > > > > > > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > --- > > > > > src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +++ > > > > > 1 file changed, 3 insertions(+) > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > > > index f7ade2a6d5f3..413c2b8b1f07 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> > > > > > @@ -841,6 +842,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) > > > > > return; > > > > > > > > > > /* Mark the request as complete. */ > > > > > + request->metadata().set(controls::draft::PipelineDepth, 3); > > > > > pipe_->completeRequest(camera_, request); > > > > > } > > > > > > > > > > @@ -866,6 +868,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) > > > > > if (request->findBuffer(&rawStream_)) { > > > > > bool isComplete = pipe_->completeBuffer(camera_, request, buffer); > > > > > if (isComplete) { > > > > > + request->metadata().set(controls::draft::PipelineDepth, 2); > > > > > > > > Will it be obvious to readers what the '2' stages are? Maybe later we'll > > > > somehow automatically handle it. > > > > > > > > I think hardcoding these values currently deserves an explanation of why > > > > it's only 2 (or 3 above) perhaps. > > > > > > > > But - eitherway - maybe that's overkill too. > > > > > > Please don't forget this is pipeline code, not application facing > > > code. I expect reading the proeprty documentation and noticing RAW=2, > > > YUV=3 is pretty clear to someone poking with platform code. > > > > > > I can add a comment to previous patch that add the maximum pipeline > > > depth though. > > > > > > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > > > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > Starting the discussion to prepare for a non-draft implementation of > > this, down the line, would you envision this being a stream-level > > metadata ? > > > > To really answer this question I should have a better understanding of > how this property is used by application. > > Strictly speaking, yes, each stream has its own number of processing > steps applied, but maybe what really matters is the total applied to a > request ? > I meant "maximum", not "total", sorry > > > > > > > > > pipe_->completeRequest(camera_, request); > > > > > return; > > > > > } > > > > -- > > Regards, > > > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index f7ade2a6d5f3..413c2b8b1f07 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> @@ -841,6 +842,7 @@ void IPU3CameraData::imguOutputBufferReady(FrameBuffer *buffer) return; /* Mark the request as complete. */ + request->metadata().set(controls::draft::PipelineDepth, 3); pipe_->completeRequest(camera_, request); } @@ -866,6 +868,7 @@ void IPU3CameraData::cio2BufferReady(FrameBuffer *buffer) if (request->findBuffer(&rawStream_)) { bool isComplete = pipe_->completeBuffer(camera_, request, buffer); if (isComplete) { + request->metadata().set(controls::draft::PipelineDepth, 2); pipe_->completeRequest(camera_, request); return; }
Report for each request the pipeline depth describing the number of processing steps the frames went through. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 3 +++ 1 file changed, 3 insertions(+)