Re: [PATCH v1 2/7] staging: media: starfive: Add raw pad for ISP

Dan Carpenter
2024-03-06T14:23:40Z

On Wed, Mar 06, 2024 at 01:33:29AM -0800, Changhuang Liang wrote:
> Add raw pad for ISP, it supported the conversion of RAW10 into RAW12.
>

To be honest, I don't understand what "it supported the conversion of
RAW10 into RAW12" means. I don't think that this patch has any impact
on user space but I'm not 100% sure.

A lot of this patch is just reformating stuff and it would be easier to
review if the reformating were separated into a separate patch.

patch 2: Clean pad selection in isp_try_format()

The code to select isp_dev->formats[] is overly complicated. We can
just use the "pad" as the index. This will making adding new pads
easier in future patches. No functional change.

patch 3: Add raw pad for ISP

> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
> .../staging/media/starfive/camss/stf-isp.c | 26 ++++++++++++-------
> .../staging/media/starfive/camss/stf-isp.h | 1 +
> 2 files changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/staging/media/starfive/camss/stf-isp.c b/drivers/staging/media/starfive/camss/stf-isp.c
> index a5573abe0d7b..6bab0ac23120 100644
> --- a/drivers/staging/media/starfive/camss/stf-isp.c
> +++ b/drivers/staging/media/starfive/camss/stf-isp.c
> @@ -10,9 +10,6 @@
>
> #include "stf-camss.h"
>
> -#define SINK_FORMATS_INDEX 0
> -#define SOURCE_FORMATS_INDEX 1
> -

This is cleanup. patch 2.

> static int isp_set_selection(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *state,
> struct v4l2_subdev_selection *sel);
> @@ -28,9 +25,17 @@ static const struct stf_isp_format isp_formats_source[] = {
> { MEDIA_BUS_FMT_YUYV8_1_5X8, 8 },
> };
>
> +static const struct stf_isp_format isp_formats_source_raw[] = {
> + { MEDIA_BUS_FMT_SRGGB12_1X12, 12 },
> + { MEDIA_BUS_FMT_SGRBG12_1X12, 12 },
> + { MEDIA_BUS_FMT_SGBRG12_1X12, 12 },
> + { MEDIA_BUS_FMT_SBGGR12_1X12, 12 },
> +};
> +

patch 3.

> static const struct stf_isp_format_table isp_formats_st7110[] = {
> { isp_formats_sink, ARRAY_SIZE(isp_formats_sink) },
> { isp_formats_source, ARRAY_SIZE(isp_formats_source) },
> + { isp_formats_source_raw, ARRAY_SIZE(isp_formats_source_raw) },
> };

patch 3.

>
> static const struct stf_isp_format *
> @@ -113,10 +118,7 @@ static void isp_try_format(struct stf_isp_dev *isp_dev,
> return;
> }
>
> - if (pad == STF_ISP_PAD_SINK)
> - formats = &isp_dev->formats[SINK_FORMATS_INDEX];
> - else if (pad == STF_ISP_PAD_SRC)
> - formats = &isp_dev->formats[SOURCE_FORMATS_INDEX];
> + formats = &isp_dev->formats[pad];

patch 2.

>
> fmt->width = clamp_t(u32, fmt->width, STFCAMSS_FRAME_MIN_WIDTH,
> STFCAMSS_FRAME_MAX_WIDTH);
> @@ -142,7 +144,7 @@ static int isp_enum_mbus_code(struct v4l2_subdev *sd,
> if (code->index >= ARRAY_SIZE(isp_formats_sink))
> return -EINVAL;
>
> - formats = &isp_dev->formats[SINK_FORMATS_INDEX];
> + formats = &isp_dev->formats[code->pad];

patch 2.

> code->code = formats->fmts[code->index].code;
> } else {
> struct v4l2_mbus_framefmt *sink_fmt;
> @@ -281,8 +283,11 @@ static int isp_set_selection(struct v4l2_subdev *sd,
> crop.target = V4L2_SEL_TGT_CROP;
> crop.r = *rect;
> isp_set_selection(sd, state, &crop);
> +
> + crop.pad = STF_ISP_PAD_SRC_RAW;
> + isp_set_selection(sd, state, &crop);

patch 3.

> } else if (sel->target == V4L2_SEL_TGT_CROP &&
> - sel->pad == STF_ISP_PAD_SRC) {
> + (sel->pad == STF_ISP_PAD_SRC || sel->pad == STF_ISP_PAD_SRC_RAW)) {

patch 3.

> struct v4l2_subdev_format fmt = { 0 };
>
> rect = v4l2_subdev_state_get_crop(state, sel->pad);
> @@ -294,7 +299,7 @@ static int isp_set_selection(struct v4l2_subdev *sd,
>
> /* Reset source pad format width and height */
> fmt.which = sel->which;
> - fmt.pad = STF_ISP_PAD_SRC;
> + fmt.pad = sel->pad;

patch 2.

> fmt.format.width = rect->width;
> fmt.format.height = rect->height;
> isp_set_format(sd, state, &fmt);
> @@ -361,6 +366,7 @@ int stf_isp_register(struct stf_isp_dev *isp_dev, struct v4l2_device *v4l2_dev)
>
> pads[STF_ISP_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> pads[STF_ISP_PAD_SRC].flags = MEDIA_PAD_FL_SOURCE;
> + pads[STF_ISP_PAD_SRC_RAW].flags = MEDIA_PAD_FL_SOURCE;

patch 3.

>
> sd->entity.function = MEDIA_ENT_F_PROC_VIDEO_ISP;
> sd->entity.ops = &isp_media_ops;
> diff --git a/drivers/staging/media/starfive/camss/stf-isp.h b/drivers/staging/media/starfive/camss/stf-isp.h
> index 07d6c2758253..4fc5cfac115c 100644
> --- a/drivers/staging/media/starfive/camss/stf-isp.h
> +++ b/drivers/staging/media/starfive/camss/stf-isp.h
> @@ -393,6 +393,7 @@
> enum stf_isp_pad_id {
> STF_ISP_PAD_SINK = 0,
> STF_ISP_PAD_SRC,
> + STF_ISP_PAD_SRC_RAW,

patch 3.

> STF_ISP_PAD_MAX
> };

regards,
dan carpenter