On 4/5/20 10:30 AM, Steve Longerbeam wrote:
Hi Laurent, On 4/5/20 9:53 AM, Laurent Pinchart wrote:Hi Steve, Thank you for the patch. On Sat, Apr 04, 2020 at 03:41:23PM -0700, Steve Longerbeam wrote:From: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> The format lookup (and enumeration) functions take a boolean flag to tell if Bayer formats should be considered. This leads to hard to read lines such as return enum_format(fourcc, NULL, index, cs_sel, true, false); where the boolean parameters can easily be mixed. To make the code clearer, add a CS_SEL_BAYER flag that can be passed through the codespace_sel parameter of the lookup functions to replace the bool parameter. Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> [Instead of declaring CS_SEL_ANY as a bitfield containing only CS_SEL_YUV | CS_SEL_RGB, declare CS_SEL_ANY as all of the above (YUV, RGB, BAYER). A new enum is declared for the YUV | RGB selection as CS_SEL_YUV_RGB, and that is used by sub-devices that don't support BAYER and only allow selecting and enumerating YUV or RGB encodings. CS_SEL_ANY is now only used by the CSI sub-devices and the attached capture interfaces, since only those devices support BAYER formats.] Signed-off-by: Steve Longerbeam <slongerbeam@xxxxxxxxx> --- drivers/staging/media/imx/imx-ic-prp.c | 8 +-- drivers/staging/media/imx/imx-ic-prpencvf.c | 9 +-- drivers/staging/media/imx/imx-media-capture.c | 14 ++-- .../staging/media/imx/imx-media-csc-scaler.c | 2 +- drivers/staging/media/imx/imx-media-csi.c | 15 ++--drivers/staging/media/imx/imx-media-utils.c | 68 +++++++++----------drivers/staging/media/imx/imx-media.h | 16 ++--- drivers/staging/media/imx/imx7-media-csi.c | 12 ++-- 8 files changed, 70 insertions(+), 74 deletions(-)diff --git a/drivers/staging/media/imx/imx-ic-prp.c b/drivers/staging/media/imx/imx-ic-prp.cindex 2a4f77e83ed3..9c1f723972e4 100644 --- a/drivers/staging/media/imx/imx-ic-prp.c +++ b/drivers/staging/media/imx/imx-ic-prp.c@@ -107,7 +107,7 @@ static int prp_enum_mbus_code(struct v4l2_subdev *sd,switch (code->pad) { case PRP_SINK_PAD: ret = imx_media_enum_ipu_format(&code->code, code->index, - CS_SEL_ANY); + CS_SEL_YUV_RGB); break; case PRP_SRC_PAD_PRPENC: case PRP_SRC_PAD_PRPVF: @@ -180,10 +180,10 @@ static int prp_set_fmt(struct v4l2_subdev *sd, MIN_H, MAX_H, H_ALIGN, S_ALIGN); cc = imx_media_find_ipu_format(sdformat->format.code, - CS_SEL_ANY); + CS_SEL_YUV_RGB); if (!cc) { - imx_media_enum_ipu_format(&code, 0, CS_SEL_ANY); - cc = imx_media_find_ipu_format(code, CS_SEL_ANY); + imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV); + cc = imx_media_find_ipu_format(code, CS_SEL_YUV);Should these two be CS_SEL_YUV_RGB ?It could be, but this is selecting a default format if the requested format isn't supported, so it just asks for the first enumerated IPU-internal format (AYUV32) as a default.Which brings up an idea I've been considering to clean up these default format selection paths a bit. Aconst struct imx_media_pixfmt * imx_media_default_{pixel|mbus|ipu}_format(enum imx_pixfmt_sel fmt_sel) would return a default YUV/RGB/BAYER format depending on fmt_sel.sdformat->format.code = cc->codes[0]; }diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.cindex 09c4e3f33807..5a22cdc7378a 100644 --- a/drivers/staging/media/imx/imx-ic-prpencvf.c +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c@@ -850,7 +850,8 @@ static int prp_enum_mbus_code(struct v4l2_subdev *sd,if (code->pad >= PRPENCVF_NUM_PADS) return -EINVAL;- return imx_media_enum_ipu_format(&code->code, code->index, CS_SEL_ANY);+ return imx_media_enum_ipu_format(&code->code, code->index, + CS_SEL_YUV_RGB); } static int prp_get_fmt(struct v4l2_subdev *sd, @@ -885,12 +886,12 @@ static void prp_try_fmt(struct prp_priv *priv, { struct v4l2_mbus_framefmt *infmt;- *cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_ANY); + *cc = imx_media_find_ipu_format(sdformat->format.code, CS_SEL_YUV_RGB);if (!*cc) { u32 code; - imx_media_enum_ipu_format(&code, 0, CS_SEL_ANY); - *cc = imx_media_find_ipu_format(code, CS_SEL_ANY); + imx_media_enum_ipu_format(&code, 0, CS_SEL_YUV); + *cc = imx_media_find_ipu_format(code, CS_SEL_YUV);And here too ?Same as above.sdformat->format.code = (*cc)->codes[0]; }diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.cindex d37b776ff86d..d60b49ec4fa4 100644 --- a/drivers/staging/media/imx/imx-media-capture.c +++ b/drivers/staging/media/imx/imx-media-capture.c@@ -91,7 +91,7 @@ static int capture_enum_framesizes(struct file *file, void *fh,}; int ret;- cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY, true);+ cc = imx_media_find_format(fsize->pixel_format, CS_SEL_ANY); if (!cc) return -EINVAL;@@ -133,7 +133,7 @@ static int capture_enum_frameintervals(struct file *file, void *fh,}; int ret;- cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY, true);+ cc = imx_media_find_format(fival->pixel_format, CS_SEL_ANY); if (!cc) return -EINVAL;@@ -177,7 +177,7 @@ static int capture_enum_fmt_vid_cap(struct file *file, void *fh,return ret; } else { cc_src = imx_media_find_mbus_format(fmt_src.format.code, - CS_SEL_ANY, true); + CS_SEL_ANY); if (WARN_ON(!cc_src)) return -EINVAL;@@ -217,14 +217,14 @@ static int __capture_try_fmt_vid_cap(struct capture_priv *priv,CS_SEL_YUV : CS_SEL_RGB; fourcc = f->fmt.pix.pixelformat; - cc = imx_media_find_format(fourcc, cs_sel, false); + cc = imx_media_find_format(fourcc, cs_sel); if (!cc) { imx_media_enum_format(&fourcc, 0, cs_sel); - cc = imx_media_find_format(fourcc, cs_sel, false); + cc = imx_media_find_format(fourcc, cs_sel); } } else { cc_src = imx_media_find_mbus_format(fmt_src->format.code, - CS_SEL_ANY, true); + CS_SEL_ANY); if (WARN_ON(!cc_src)) return -EINVAL;@@ -790,7 +790,7 @@ int imx_media_capture_device_register(struct imx_media_video_dev *vdev)vdev->compose.width = fmt_src.format.width; vdev->compose.height = fmt_src.format.height; vdev->cc = imx_media_find_format(vdev->fmt.fmt.pix.pixelformat, - CS_SEL_ANY, false); + CS_SEL_ANY);Should this be CS_SEL_YUV_RGB too ?Not in this case. The capture interface could be attached to a CSI, which supports bayer, so the CSI may have selected a bayer format when it registered (see below re: imx_media_enum_mbus_format())
^^ sorry, re: imx_media_init_mbus_fmt().
, so bayer needs to be included in the search.v4l2_info(sd, "Registered %s as /dev/%s\n", vfd->name, video_device_node_name(vfd));diff --git a/drivers/staging/media/imx/imx-media-csc-scaler.c b/drivers/staging/media/imx/imx-media-csc-scaler.cindex 2cc77f6e84b6..3e1c88938e7d 100644 --- a/drivers/staging/media/imx/imx-media-csc-scaler.c +++ b/drivers/staging/media/imx/imx-media-csc-scaler.c@@ -164,7 +164,7 @@ static int ipu_csc_scaler_enum_fmt(struct file *file, void *fh,u32 fourcc; int ret; - ret = imx_media_enum_format(&fourcc, f->index, CS_SEL_ANY); + ret = imx_media_enum_format(&fourcc, f->index, CS_SEL_YUV_RGB); if (ret) return ret;diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.cindex e76a6a85baa3..298294b95293 100644 --- a/drivers/staging/media/imx/imx-media-csi.c +++ b/drivers/staging/media/imx/imx-media-csi.c@@ -1234,12 +1234,12 @@ static int csi_enum_mbus_code(struct v4l2_subdev *sd,mutex_lock(&priv->lock); infmt = __csi_get_fmt(priv, cfg, CSI_SINK_PAD, code->which); - incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY, true); + incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY); switch (code->pad) { case CSI_SINK_PAD: ret = imx_media_enum_mbus_format(&code->code, code->index, - CS_SEL_ANY, true); + CS_SEL_ANY); break; case CSI_SRC_PAD_DIRECT: case CSI_SRC_PAD_IDMAC: @@ -1433,8 +1433,7 @@ static void csi_try_fmt(struct csi_priv *priv, switch (sdformat->pad) { case CSI_SRC_PAD_DIRECT: case CSI_SRC_PAD_IDMAC: - incc = imx_media_find_mbus_format(infmt->code, - CS_SEL_ANY, true); + incc = imx_media_find_mbus_format(infmt->code, CS_SEL_ANY); sdformat->format.width = compose->width; sdformat->format.height = compose->height; @@ -1470,12 +1469,10 @@ static void csi_try_fmt(struct csi_priv *priv, MIN_H, MAX_H, H_ALIGN, S_ALIGN); *cc = imx_media_find_mbus_format(sdformat->format.code, - CS_SEL_ANY, true); + CS_SEL_ANY); if (!*cc) { - imx_media_enum_mbus_format(&code, 0, - CS_SEL_ANY, false); - *cc = imx_media_find_mbus_format(code, - CS_SEL_ANY, false); + imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY); + *cc = imx_media_find_mbus_format(code, CS_SEL_ANY);Same here.Same as above, CSI supports bayer so the default format search should technically include bayer. Although since this is selecting some default, maybe it shouldn't allow bayer as a default.sdformat->format.code = (*cc)->codes[0]; }diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.cindex cf0aba8d53ba..6a3b0b737e5f 100644 --- a/drivers/staging/media/imx/imx-media-utils.c +++ b/drivers/staging/media/imx/imx-media-utils.c@@ -213,8 +213,7 @@ static const struct imx_media_pixfmt ipu_rgb_formats[] = {static const struct imx_media_pixfmt *find_format(u32 fourcc, u32 code, enum codespace_sel cs_sel, - bool allow_non_mbus, - bool allow_bayer) + bool allow_non_mbus) { unsigned int i;@@ -223,12 +222,12 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,enum codespace_sel fmt_cs_sel; unsigned int j; - fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ? - CS_SEL_YUV : CS_SEL_RGB; + fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER : + ((fmt->cs == IPUV3_COLORSPACE_YUV) ? + CS_SEL_YUV : CS_SEL_RGB); - if ((cs_sel != CS_SEL_ANY && fmt_cs_sel != cs_sel) || - (!allow_non_mbus && !fmt->codes[0]) || - (!allow_bayer && fmt->bayer)) + if (!(fmt_cs_sel & cs_sel) || + (!allow_non_mbus && !fmt->codes[0])) continue; if (fourcc && fmt->fourcc == fourcc)@@ -248,8 +247,7 @@ static const struct imx_media_pixfmt *find_format(u32 fourcc,static int enum_format(u32 *fourcc, u32 *code, u32 index, enum codespace_sel cs_sel, - bool allow_non_mbus, - bool allow_bayer) + bool allow_non_mbus) { unsigned int i;@@ -258,12 +256,12 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,enum codespace_sel fmt_cs_sel; unsigned int j; - fmt_cs_sel = (fmt->cs == IPUV3_COLORSPACE_YUV) ? - CS_SEL_YUV : CS_SEL_RGB; + fmt_cs_sel = fmt->bayer ? CS_SEL_BAYER : + ((fmt->cs == IPUV3_COLORSPACE_YUV) ? + CS_SEL_YUV : CS_SEL_RGB); - if ((cs_sel != CS_SEL_ANY && fmt_cs_sel != cs_sel) || - (!allow_non_mbus && !fmt->codes[0]) || - (!allow_bayer && fmt->bayer)) + if (!(fmt_cs_sel & cs_sel) || + (!allow_non_mbus && !fmt->codes[0])) continue; if (fourcc && index == 0) {@@ -290,30 +288,28 @@ static int enum_format(u32 *fourcc, u32 *code, u32 index,} const struct imx_media_pixfmt *-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel, bool allow_bayer)+imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel) { - return find_format(fourcc, 0, cs_sel, true, allow_bayer); + return find_format(fourcc, 0, cs_sel, true); } EXPORT_SYMBOL_GPL(imx_media_find_format);int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel){ - return enum_format(fourcc, NULL, index, cs_sel, true, false); + return enum_format(fourcc, NULL, index, cs_sel, true); } EXPORT_SYMBOL_GPL(imx_media_enum_format); const struct imx_media_pixfmt * -imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel, - bool allow_bayer) +imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel) { - return find_format(0, code, cs_sel, false, allow_bayer); + return find_format(0, code, cs_sel, false); } EXPORT_SYMBOL_GPL(imx_media_find_mbus_format);-int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel,- bool allow_bayer)+int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel){ - return enum_format(NULL, code, index, cs_sel, false, allow_bayer); + return enum_format(NULL, code, index, cs_sel, false); } EXPORT_SYMBOL_GPL(imx_media_enum_mbus_format);@@ -324,6 +320,8 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)u32 array_size; int i, j; + cs_sel &= ~CS_SEL_BAYER; + switch (cs_sel) { case CS_SEL_YUV: array_size = NUM_IPU_YUV_FORMATS;@@ -333,7 +331,7 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)array_size = NUM_IPU_RGB_FORMATS; array = ipu_rgb_formats; break; - case CS_SEL_ANY: + case CS_SEL_YUV_RGB: array_size = NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS; array = ipu_yuv_formats; break;@@ -342,7 +340,7 @@ imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel)} for (i = 0; i < array_size; i++) { - if (cs_sel == CS_SEL_ANY && i >= NUM_IPU_YUV_FORMATS) + if (cs_sel == CS_SEL_YUV_RGB && i >= NUM_IPU_YUV_FORMATS) fmt = &ipu_rgb_formats[i - NUM_IPU_YUV_FORMATS]; else fmt = &array[i]; @@ -362,6 +360,8 @@ EXPORT_SYMBOL_GPL(imx_media_find_ipu_format);int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel){ + cs_sel &= ~CS_SEL_BAYER; + switch (cs_sel) { case CS_SEL_YUV: if (index >= NUM_IPU_YUV_FORMATS)@@ -373,7 +373,7 @@ int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel)return -EINVAL; *code = ipu_rgb_formats[index].codes[0]; break; - case CS_SEL_ANY: + case CS_SEL_YUV_RGB: if (index >= NUM_IPU_YUV_FORMATS + NUM_IPU_RGB_FORMATS) return -EINVAL; if (index >= NUM_IPU_YUV_FORMATS) {@@ -401,8 +401,8 @@ int imx_media_init_mbus_fmt(struct v4l2_mbus_framefmt *mbus,mbus->height = height; mbus->field = field; if (code == 0) - imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV, false); - lcc = imx_media_find_mbus_format(code, CS_SEL_ANY, false); + imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV); + lcc = imx_media_find_mbus_format(code, CS_SEL_ANY);And here.This is imx_media_enum_mbus_format()
^^ sorry, I meant imx_media_init_mbus_fmt().
, which is called by a subdev when it is registered to fill its mbus formats with some sane defaults. It's appropriate to use PIXFMT_SEL_ANY here since it is called by CSI.if (!lcc) { lcc = imx_media_find_ipu_format(code, CS_SEL_ANY); if (!lcc)@@ -473,7 +473,7 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,const struct imx_media_pixfmt *cc; bool is_rgb = false; - cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY, true); + cc = imx_media_find_mbus_format(tryfmt->code, CS_SEL_ANY); if (!cc) cc = imx_media_find_ipu_format(tryfmt->code, CS_SEL_ANY); if (cc && cc->cs == IPUV3_COLORSPACE_RGB)@@ -527,8 +527,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,if (!cc) { cc = imx_media_find_ipu_format(mbus->code, CS_SEL_ANY); if (!cc) - cc = imx_media_find_mbus_format(mbus->code, CS_SEL_ANY, - true); + cc = imx_media_find_mbus_format(mbus->code, + CS_SEL_ANY); if (!cc) return -EINVAL; }@@ -540,8 +540,8 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,if (cc->ipufmt && cc->cs == IPUV3_COLORSPACE_YUV) { u32 code; - imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV, false); - cc = imx_media_find_mbus_format(code, CS_SEL_YUV, false); + imx_media_enum_mbus_format(&code, 0, CS_SEL_YUV); + cc = imx_media_find_mbus_format(code, CS_SEL_YUV); } /* Round up width for minimum burst size */@@ -592,7 +592,7 @@ int imx_media_ipu_image_to_mbus_fmt(struct v4l2_mbus_framefmt *mbus,{ const struct imx_media_pixfmt *fmt;- fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY, true);+ fmt = imx_media_find_format(image->pix.pixelformat, CS_SEL_ANY); if (!fmt) return -EINVAL;diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.hindex 11861191324a..652673a703cd 100644 --- a/drivers/staging/media/imx/imx-media.h +++ b/drivers/staging/media/imx/imx-media.h @@ -150,20 +150,20 @@ struct imx_media_dev { }; enum codespace_sel { - CS_SEL_YUV = 0, - CS_SEL_RGB, - CS_SEL_ANY, + CS_SEL_YUV = BIT(0), + CS_SEL_RGB = BIT(1), + CS_SEL_BAYER = BIT(2), + CS_SEL_YUV_RGB = CS_SEL_YUV | CS_SEL_RGB, + CS_SEL_ANY = CS_SEL_YUV | CS_SEL_RGB | CS_SEL_BAYER, }; /* imx-media-utils.c */ const struct imx_media_pixfmt *-imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel, bool allow_bayer);+imx_media_find_format(u32 fourcc, enum codespace_sel cs_sel);int imx_media_enum_format(u32 *fourcc, u32 index, enum codespace_sel cs_sel);const struct imx_media_pixfmt * -imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel, - bool allow_bayer);-int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel,- bool allow_bayer); +imx_media_find_mbus_format(u32 code, enum codespace_sel cs_sel);+int imx_media_enum_mbus_format(u32 *code, u32 index, enum codespace_sel cs_sel);const struct imx_media_pixfmt * imx_media_find_ipu_format(u32 code, enum codespace_sel cs_sel);int imx_media_enum_ipu_format(u32 *code, u32 index, enum codespace_sel cs_sel); diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.cindex acbdffb77668..a469dc76a787 100644 --- a/drivers/staging/media/imx/imx7-media-csi.c +++ b/drivers/staging/media/imx/imx7-media-csi.c@@ -959,7 +959,7 @@ static int imx7_csi_enum_mbus_code(struct v4l2_subdev *sd,switch (code->pad) { case IMX7_CSI_PAD_SINK: ret = imx_media_enum_mbus_format(&code->code, code->index, - CS_SEL_ANY, true); + CS_SEL_ANY); break; case IMX7_CSI_PAD_SRC: if (code->index != 0) { @@ -1019,8 +1019,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi, switch (sdformat->pad) { case IMX7_CSI_PAD_SRC: - in_cc = imx_media_find_mbus_format(in_fmt->code, CS_SEL_ANY, - true); + in_cc = imx_media_find_mbus_format(in_fmt->code, CS_SEL_ANY); sdformat->format.width = in_fmt->width; sdformat->format.height = in_fmt->height;@@ -1035,11 +1034,10 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,break; case IMX7_CSI_PAD_SINK: *cc = imx_media_find_mbus_format(sdformat->format.code, - CS_SEL_ANY, true); + CS_SEL_ANY); if (!*cc) { - imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY, false); - *cc = imx_media_find_mbus_format(code, CS_SEL_ANY, - false); + imx_media_enum_mbus_format(&code, 0, CS_SEL_ANY); + *cc = imx_media_find_mbus_format(code, CS_SEL_ANY);And finally here.I *think* the i.MX7 CSI supports bayer, so this is allowing a bayer format as a default, but again maybe it should avoid selecting bayer in that case.Stevesdformat->format.code = (*cc)->codes[0]; }
