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

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Thu, Sep 24, 2020 at 04:51:43PM +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")
> 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>
> ---
>  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.

"it is then expected" sounds like we expect this, while I believe you
mean the driver expects (or seems to expect) this. I'd phrase the
comment as

	 * If the ImgU gets configured, its driver seems to expect that
	 * buffers will be queued to its outputs, as otherwise the next
	 * capture session that uses the ImgU fails when queueing
	 * buffers to its input.

Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>


>  	 *
>  	 * 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