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@xxxxxxx>
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
