| Message ID | 20211017102904.756408-4-dorota.czaplejewicz@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [PATCHv2,1/4] media: imx: Remove unused functions | expand |
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
On Sun, Oct 17, 2021 at 01:08:37PM +0200, Dorota Czaplejewicz wrote: > +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); Do we need a if (!cc) NULL check after this assignment? > + } > + > + /* > + * 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. > + */ > + divisor = 4 << (mbus->height % 2); > + > + width = round_up(mbus->width, divisor); > + > + if (cc->planar) > + stride = round_up(width, 16); > + else > + stride = round_up((width * cc->bpp) >> 3, divisor); > + > + 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; > + > + return 0; > +} regards, dan carpenter
Hi, On Wed, 3 Nov 2021 13:41:00 +0300 Dan Carpenter <dan.carpenter@oracle.com> wrote: > On Sun, Oct 17, 2021 at 01:08:37PM +0200, Dorota Czaplejewicz wrote: > > +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); > > Do we need a if (!cc) NULL check after this assignment? In v3 of this series, this statement is only present in the IMX56 code path, which is unmodified compared to the original. However, there's a missing check in the IMX78. I'm not sure if those can fail in practice, but for the sake of correctness, I rolled out an updated v4 series, where both checks are present. Message-id: 20211104113631.206899-1-dorota.czaplejewicz@puri.sm Cheers, Dorota > > > + } > > + > > + /* > > + * 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. > > + */ > > + divisor = 4 << (mbus->height % 2); > > + > > + width = round_up(mbus->width, divisor); > > + > > + if (cc->planar) > > + stride = round_up(width, 16); > > + else > > + stride = round_up((width * cc->bpp) >> 3, divisor); > > + > > + 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; > > + > > + return 0; > > +} > > regards, > dan carpenter
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); + } + + /* + * 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. + */ + divisor = 4 << (mbus->height % 2); + + width = round_up(mbus->width, divisor); + + if (cc->planar) + stride = round_up(width, 16); + else + stride = round_up((width * cc->bpp) >> 3, divisor); + + 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; + + return 0; +} + +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) { + switch (type) { + case DEVICE_TYPE_IMX56: + return imx56_media_mbus_fmt_to_pix_fmt(pix, mbus, cc); + case DEVICE_TYPE_IMX78: + return imx78_media_mbus_fmt_to_pix_fmt(pix, mbus, cc); + } + return -EINVAL; +} EXPORT_SYMBOL_GPL(imx_media_mbus_fmt_to_pix_fmt); void imx_media_free_dma_buf(struct device *dev,
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> --- drivers/staging/media/imx/imx-media-utils.c | 78 +++++++++++++++++++-- 1 file changed, 74 insertions(+), 4 deletions(-)