Hi Steve,
Thank you for the patch.
On Sat, Apr 04, 2020 at 03:41:25PM -0700, Steve Longerbeam wrote:
> Add a PIXFMT_SEL_IPU selection flag, to select only the IPU-internal
> pixel formats, and move the single-entry IPU-internal pixel format
> arrays into pixel_formats[]. imx_media_find_ipu_format() and
> imx_media_enum_ipu_format() can now simply call find_format() and
> enum_format().
>
> The RGB32 format is both an IPU-internal format, and an in-memory format
> via idmac channels that is supported by the IPUv3 driver, so it appears
> twice in pixel_formats[], one with ipufmt=false for the in-memory format,
> and again with ipufmt=true for the IPU-internal format.
I like code sharing, but I think the big pixel_formats array is starting
to mix unrelated information together. The fact that the
V4L2_PIX_FMT_YUV32 entry only exists as a .ipufmt == true variant
bothers me. Down the line I'll propose splitting the i.MX7 code out to a
separate driver, without using imx-media-utils.c. We'll then have an
opportunity to refactor this for IPU-based systems only. Until then,
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Steve Longerbeam <slongerbeam@xxxxxxxxx>
> ---
> drivers/staging/media/imx/imx-media-utils.c | 118 +++++---------------
> drivers/staging/media/imx/imx-media.h | 1 +
> 2 files changed, 27 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index beaa920d7ac7..873fdcee7d37 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -53,7 +53,13 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> .cs = IPUV3_COLORSPACE_YUV,
> .bpp = 16,
> .planar = true,
> - },
> + }, {
> + .fourcc = V4L2_PIX_FMT_YUV32,
> + .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
> + .cs = IPUV3_COLORSPACE_YUV,
> + .bpp = 32,
> + .ipufmt = true,
> + },
> /*** RGB formats start here ***/
> {
> .fourcc = V4L2_PIX_FMT_RGB565,
> @@ -73,6 +79,11 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> .fourcc = V4L2_PIX_FMT_BGR24,
> .cs = IPUV3_COLORSPACE_RGB,
> .bpp = 24,
> + }, {
> + .fourcc = V4L2_PIX_FMT_XRGB32,
> + .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> + .cs = IPUV3_COLORSPACE_RGB,
> + .bpp = 32,
> }, {
> .fourcc = V4L2_PIX_FMT_XRGB32,
> .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> @@ -186,42 +197,24 @@ static const struct imx_media_pixfmt pixel_formats[] = {
> },
> };
>
> -static const struct imx_media_pixfmt ipu_yuv_formats[] = {
> - {
> - .fourcc = V4L2_PIX_FMT_YUV32,
> - .codes = {MEDIA_BUS_FMT_AYUV8_1X32},
> - .cs = IPUV3_COLORSPACE_YUV,
> - .bpp = 32,
> - .ipufmt = true,
> - },
> -};
> -
> -#define NUM_IPU_YUV_FORMATS ARRAY_SIZE(ipu_yuv_formats)
> -
> -static const struct imx_media_pixfmt ipu_rgb_formats[] = {
> - {
> - .fourcc = V4L2_PIX_FMT_XRGB32,
> - .codes = {MEDIA_BUS_FMT_ARGB8888_1X32},
> - .cs = IPUV3_COLORSPACE_RGB,
> - .bpp = 32,
> - .ipufmt = true,
> - },
> -};
> -
> -#define NUM_IPU_RGB_FORMATS ARRAY_SIZE(ipu_rgb_formats)
> -
> static const struct imx_media_pixfmt *find_format(u32 fourcc,
> u32 code,
> enum imx_pixfmt_sel fmt_sel,
> bool allow_non_mbus)
> {
> + bool sel_ipu = fmt_sel & PIXFMT_SEL_IPU;
> unsigned int i;
>
> + fmt_sel &= ~PIXFMT_SEL_IPU;
> +
> for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> const struct imx_media_pixfmt *fmt = &pixel_formats[i];
> enum imx_pixfmt_sel sel;
> unsigned int j;
>
> + if (sel_ipu != fmt->ipufmt)
> + continue;
> +
> sel = fmt->bayer ? PIXFMT_SEL_BAYER :
> ((fmt->cs == IPUV3_COLORSPACE_YUV) ?
> PIXFMT_SEL_YUV : PIXFMT_SEL_RGB);
> @@ -249,13 +242,19 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,
> enum imx_pixfmt_sel fmt_sel,
> bool allow_non_mbus)
> {
> + bool sel_ipu = fmt_sel & PIXFMT_SEL_IPU;
> unsigned int i;
>
> + fmt_sel &= ~PIXFMT_SEL_IPU;
> +
> for (i = 0; i < ARRAY_SIZE(pixel_formats); i++) {
> const struct imx_media_pixfmt *fmt = &pixel_formats[i];
> enum imx_pixfmt_sel sel;
> unsigned int j;
>
> + if (sel_ipu != fmt->ipufmt)
> + continue;
> +
> sel = fmt->bayer ? PIXFMT_SEL_BAYER :
> ((fmt->cs == IPUV3_COLORSPACE_YUV) ?
> PIXFMT_SEL_YUV : PIXFMT_SEL_RGB);
> @@ -317,79 +316,14 @@ EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);
> const struct imx_media_pixfmt *
> imx_media_find_ipu_format(u32 code, enum imx_pixfmt_sel fmt_sel)
> {
> - const struct imx_media_pixfmt *array, *fmt, *ret = NULL;
> - u32 array_size;
> - int i, j;
> -
> - fmt_sel &= ~PIXFMT_SEL_BAYER;
> -
> - switch (fmt_sel) {
> - case PIXFMT_SEL_YUV:
> - array_size = NUM_IPU_YUV_FORMATS;
> - array = ipu_yuv_formats;
> - break;
> - case PIXFMT_SEL_RGB:
> - array_size = NUM_IPU_RGB_FORMATS;
> - array = ipu_rgb_formats;
> - break;
> - case PIXFMT_SEL_YUV_RGB:
> - array_size = NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS;
> - array = ipu_yuv_formats;
> - break;
> - default:
> - return NULL;
> - }
> -
> - for (i = 0; i < array_size; i++) {
> - if (fmt_sel == PIXFMT_SEL_YUV_RGB && i >= NUM_IPU_YUV_FORMATS)
> - fmt = &ipu_rgb_formats[i - NUM_IPU_YUV_FORMATS];
> - else
> - fmt = &array[i];
> -
> - for (j = 0; code && fmt->codes[j]; j++) {
> - if (code == fmt->codes[j]) {
> - ret = fmt;
> - goto out;
> - }
> - }
> - }
> -
> -out:
> - return ret;
> + return find_format(0, code, fmt_sel | PIXFMT_SEL_IPU, false);
> }
> EXPORT_SYMBOL_GPL(imx_media_find_ipu_format);
>
> int imx_media_enum_ipu_format(u32 *code, u32 index,
> enum imx_pixfmt_sel fmt_sel)
> {
> - fmt_sel &= ~PIXFMT_SEL_BAYER;
> -
> - switch (fmt_sel) {
> - case PIXFMT_SEL_YUV:
> - if (index >= NUM_IPU_YUV_FORMATS)
> - return -EINVAL;
> - *code = ipu_yuv_formats[index].codes[0];
> - break;
> - case PIXFMT_SEL_RGB:
> - if (index >= NUM_IPU_RGB_FORMATS)
> - return -EINVAL;
> - *code = ipu_rgb_formats[index].codes[0];
> - break;
> - case PIXFMT_SEL_YUV_RGB:
> - if (index >= NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS)
> - return -EINVAL;
> - if (index >= NUM_IPU_YUV_FORMATS) {
> - index -= NUM_IPU_YUV_FORMATS;
> - *code = ipu_rgb_formats[index].codes[0];
> - } else {
> - *code = ipu_yuv_formats[index].codes[0];
> - }
> - break;
> - default:
> - return -EINVAL;
> - }
> -
> - return 0;
> + return enum_format(NULL, code, index, fmt_sel | PIXFMT_SEL_IPU, false);
> }
> EXPORT_SYMBOL_GPL(imx_media_enum_ipu_format);
>
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index ac7c521d8148..c61592750729 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -83,6 +83,7 @@ enum imx_pixfmt_sel {
> PIXFMT_SEL_YUV = BIT(0), /* select YUV formats */
> PIXFMT_SEL_RGB = BIT(1), /* select RGB formats */
> PIXFMT_SEL_BAYER = BIT(2), /* select BAYER formats */
> + PIXFMT_SEL_IPU = BIT(3), /* select IPU-internal formats */
> PIXFMT_SEL_YUV_RGB = PIXFMT_SEL_YUV | PIXFMT_SEL_RGB,
> PIXFMT_SEL_ANY = PIXFMT_SEL_YUV | PIXFMT_SEL_RGB | PIXFMT_SEL_BAYER,
> };
--
Regards,
Laurent Pinchart
