[libcamera-devel] [PATCH 3/3] libcamera: ipu3: Adjust comment on ImgU configuration

[Laurent Pinchart]

Hi Jacopo,

On Tue, Sep 29, 2020 at 04:56:38PM +0200, Jacopo Mondi wrote:
> On Tue, Sep 29, 2020 at 07:39:28AM +0200, Niklas Söderlund wrote:
> > On 2020-09-24 16:51:43 +0200, Jacopo Mondi wrote:
> > > Be more precise in commenting why the ImgU shall not be configured
> > > if only the RAW stream is requested.
> > >
> > > As an example, if the ImgU gets unecessary configured:
> > > cam -srole=viewfinder -c2 -C -> WORKS
> > > cam -srole=stillraw -c2 -C -> WORKS
> > > cam -srole=viewfinder -c2 -C -> Failed to queue buffer 0: Invalid argument
> > >
> > > Since commit 1bd2e981a209 ("libcamera: ipu3: Fix RAW+YUV capture")
> >
> > Unless you are _very_ lucky the commit hash will not be the same when
> > this patch is merged :-)
> 
> You're very right!
> 
> > > the ImgU configuration procedure also correctly implements the
> > > assumption that at least one of the YUV output is being operated. If
> > > that's not the case, as in the RAW-only capture use case, the code
> > > tries to access a non-existing configuration. One more reason to
> > > exit early if no YUV stream is requested.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > With Laurent's comment addressed and the commit hash removed (or proof
> > of guaranteed hash collision ;-P)
> >
> > Reviewed-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> 
> Thanks, with this fixed I'll now push the series

If you push the first patch first then you can update this one with the
right commit ID before pushing it.

> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index 477f85a45755..2f90ad2e2780 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -459,9 +459,9 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
> > >  		return ret;
> > >
> > >  	/*
> > > -	 * If the ImgU gets configured with proper IF, BDS and GDC sizes, it
> > > -	 * is then expected that frames are dequeued from its main output
> > > -	 * otherwise the system stalls.
> > > +	 * If the ImgU gets configured it is then expected that buffers are
> > > +	 * queued to its outputs, otherwise the next capture session
> > > +	 * that uses the ImgU fails at queueing buffers at its input.
> > >  	 *
> > >  	 * If no ImgU configuration has been computed, it means only a RAW
> > >  	 * stream has been requested: return here to skip the ImgU configuration

-- 
Regards,

Laurent Pinchart