[libcamera-devel,v3,16/22] libcamera: ipu3: Fill stride and frameSize at config validation

8637 diff mbox series

Fill the stride and frameSize fields of the StreamConfiguration at
configuration validation time instead of at camera configuration time.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
New in v3
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

Hi Paul,

Thank you for the patch.

On Sat, Jul 04, 2020 at 10:31:34PM +0900, Paul Elder wrote:
> Fill the stride and frameSize fields of the StreamConfiguration at
> configuration validation time instead of at camera configuration time.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> New in v3
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 12 ++++--------
>  1 file changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 00559ce..f116c93 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -278,6 +278,10 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  				<< cfg.toString();
>  			status = Adjusted;
>  		}
> +
> +		const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> +		cfg.stride = info.stride(cfg.size.width, 0);

I think you need more than this. For raw formats, the CIO2 has a 64
bytes alignment requirement. For other formats, it seems that there's no
other special alignment, but the kernel code is difficult to follow, so
I'm not 100% sure.

> +		cfg.frameSize = info.frameSize(cfg.size);

And here, we can't compute the frameSize based on cfg.size, it has to be
computed based on the stride that takes the extra alignment into
account. I'm tempted to to propose dropping
PixelFormatInfo::frameSize(), as all we need is

		cfg.frameSize = cfg.stride * cfg.size.height;

here, and designed a proper helper to calculate the frame size will need
to take multi-planar formats into account, which we don't support yet.
On the other hand, having a PixelFormatInfo::frameSize() function, even
if not perfect (for instance taking stride and height of the first
plane and considering that the stride of the other planes can be deduced
by the stride of the first plane), and enforcing its usage in pipeline
handlers, will mean that when it will be time to support multi-planar
formats properly all the code that needs to be changed will be easily
identifiable.

>  	}
>  
>  	return status;
> @@ -495,21 +499,13 @@ int PipelineHandlerIPU3::configure(Camera *camera, CameraConfiguration *c)
>  			if (ret)
>  				return ret;
>  
> -			cfg.stride = outputFormat.planes[0].bpl;
>  			outActive = true;
>  		} else if (stream == vfStream) {
>  			ret = imgu->configureViewfinder(cfg, &outputFormat);
>  			if (ret)
>  				return ret;
>  
> -			cfg.stride = outputFormat.planes[0].bpl;
>  			vfActive = true;
> -		} else {
> -			/*
> -			 * The RAW stream is configured as part of the CIO2 and
> -			 * no configuration is needed for the ImgU.
> -			 */
> -			cfg.stride = cio2Format.planes[0].bpl;
>  		}
>  	}
>