Re: [PATCHv2 4/4] media: imx: Use dedicated format handler for i.MX7/8

[Philipp Zabel <p.zabel@pengutronix.de>]

Hi Dorota,

On Sun, 2021-10-17 at 13:08 +0200, Dorota Czaplejewicz wrote:
> This splits out a format handler which takes into account
> the capabilities of the i.MX7/8 video device,
> as opposed to the default handler compatible with both i.MX5/6 and i.MX7/8.
> 
> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>

Thank you, I agree this looks better than moving
imx_media_mbus_fmt_to_pix_fmt around. Comments below.

> ---
>  drivers/staging/media/imx/imx-media-utils.c | 78 +++++++++++++++++++--
>  1 file changed, 74 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index e124dd722107..938db2e2ddb1 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -516,10 +516,9 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>  }
>  EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>  
> -int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> -				  const struct v4l2_mbus_framefmt *mbus,
> -				  const struct imx_media_pixfmt *cc,
> -				  enum imx_device_type type)
> +static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> +					   const struct v4l2_mbus_framefmt *mbus,
> +					   const struct imx_media_pixfmt *cc)
>  {
>  	u32 width;
>  	u32 stride;
> @@ -568,6 +567,77 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  
>  	return 0;
>  }
> +
> +static int imx78_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> +					   const struct v4l2_mbus_framefmt *mbus,
> +					   const struct imx_media_pixfmt *cc)
> +{
> +	u32 width;
> +	u32 stride;
> +	u8 divisor;
> +
> +	if (!cc) {
> +		cc = imx_media_find_ipu_format(mbus->code,
> +					       PIXFMT_SEL_YUV_RGB);
> +		if (!cc)
> +			cc = imx_media_find_mbus_format(mbus->code,
> +							PIXFMT_SEL_ANY);
> +		if (!cc)
> +			return -EINVAL;
> +	}
> +
> +	/*
> +	 * TODO: the IPU currently does not support the AYUV32 format,
> +	 * so until it does convert to a supported YUV format.
> +	 */
> +	if (cc->ipufmt && cc->cs == IPUV3_COLORSPACE_YUV) {
> +		u32 code;
> +
> +		imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> +		cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
> +	}

This whole part seems too convoluted for the non-IPU CSI on i.MX7/8.
Certainly the TODO comment does not apply. There is no IPU on i.MX7/8,
and the CSI does not support AYUV32 if I read the datasheet correctly.

imx_media_find_ipu_format() just returns the two IPU internal 32-bit
formats, MEDIA_BUS_FMT_AYUV8_1X32/V4L2_PIX_FMT_YUV32 and
MEDIA_BUS_FMT_ARGB8888_1X32/V4L2_PIX_FMT_XRGB32, both of which can never
be set according to imx7_csi_pad_link_validate(). So I think this could
be reduced to just:

	if (!cc)
		cc = imx_media_find_mbus_format(mbus->code, PIXFMT_SEL_ANY);                                                                                                        

After the removal of imx_media_mbus_fmt_to_ipu_image(), the only place
where imx_media_find_mbus_format() is called with cc == NULL is
in capture_init_format(), with mbus format code MEDIA_BUS_FMT_UYVY8_2X8.

> +
> +	/*
> +	 * The hardware can handle line lengths divisible by 4 bytes,
> +	 * as long as the number of lines is even.
> +	 * Otherwise, use the value of 8 bytes recommended in the datasheet.
> +	 */

The comment talks about byte alignment but then the code goes on to
align the width in pixels, which may be confusing to the reader.

> +	divisor = 4 << (mbus->height % 2);

Since this is not used to actually divide anything, I'd avoid calling
this divisor. I'd suggest "align(ment)" or something similar.

Also I'd expect that for 16-bit formats like YUYV, 4 pixel alignment
should be ok for any height.

> +
> +	width = round_up(mbus->width, divisor);
> +
> +	if (cc->planar)
> +		stride = round_up(width, 16);
> +	else
> +		stride = round_up((width * cc->bpp) >> 3, divisor);

This is probably incorrect, and the driver doesn't use bytesperline
anyway. I think  the following should probably just use
v4l2_fill_pixfmt() to set width, height, pixelformat, bytesperline, and
sizeimage:

> +
> +	pix->width = width;
> +	pix->height = mbus->height;
> +	pix->pixelformat = cc->fourcc;
> +	pix->colorspace = mbus->colorspace;
> +	pix->xfer_func = mbus->xfer_func;
> +	pix->ycbcr_enc = mbus->ycbcr_enc;
> +	pix->quantization = mbus->quantization;
> +	pix->field = mbus->field;
> +	pix->bytesperline = stride;
> +	pix->sizeimage = cc->planar ? ((stride * pix->height * cc->bpp) >> 3) :
> +			 stride * pix->height;

regards
Philipp