Received-SPF: pass (google.com: domain of linux-kernel+bounces-94037-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 6 Mar 2024 17:23:34 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Changhuang Liang <changhuang.liang@starfivetech.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Hans Verkuil <hverkuil-cisco@xs4all.nl>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	Jack Zhu <jack.zhu@starfivetech.com>, linux-media@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-staging@lists.linux.dev
Subject: Re: [PATCH v1 2/7] staging: media: starfive: Add raw pad for ISP
Message-ID: <4d54e516-448a-47ce-abcd-e705896935a7@moroto.mountain>
References: <20240306093334.9321-1-changhuang.liang@starfivetech.com>
 <20240306093334.9321-3-changhuang.liang@starfivetech.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240306093334.9321-3-changhuang.liang@starfivetech.com>

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