Re: [PATCH v12 04/12] media: rkisp1: Support devices lacking dual crop

[Laurent Pinchart]

Hi Paul,

Thank you for the patch.

On Fri, Feb 16, 2024 at 06:54:50PM +0900, Paul Elder wrote:
> Some versions of the ISP supported by the rkisp1 driver, such as the ISP
> in the i.MX8MP, lack the dual crop registers and don't support cropping
> at the resizer input. They instead rely on cropping in the Image
> Stabilization module, at the output of the ISP, to modify the resizer
> input size and implement digital zoom.
> 
> Add a dual crop feature flag to distinguish the versions of the ISP that
> support dual crop from those that don't, and make sure that the sink
> crop is set to the sink format when dual crop is not supported.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Signed-off-by: Paul Elder <paul.elder@xxxxxxxxxxxxxxxx>
> Tested-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> Tested-by: Adam Ford <aford173@xxxxxxxxx>
> Reviewed-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> Changes in v12:
> - Remove mention of moving resizer input crop to image stabilizer from
>   commit message
> - Make sure the sink crop is set to the sink format when dual crop is
>   not supported
> ---
>  .../media/platform/rockchip/rkisp1/rkisp1-common.h    |  2 ++
>  drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c   |  6 ++++--
>  .../media/platform/rockchip/rkisp1/rkisp1-resizer.c   | 11 +++++++----
>  3 files changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> index f7c251f79aa9..219d4a2547aa 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-common.h
> @@ -112,6 +112,7 @@ enum rkisp1_isp_pad {
>   * @RKISP1_FEATURE_MIPI_CSI2: The ISP has an internal MIPI CSI-2 receiver
>   * @RKISP1_FEATURE_MAIN_STRIDE: The ISP supports configurable stride on the main path
>   * @RKISP1_FEATURE_SELF_PATH: The ISP has a self path
> + * @RKISP1_FEATURE_DUAL_CROP: The ISP has the dual crop block at the resizer input
>   *
>   * The ISP features are stored in a bitmask in &rkisp1_info.features and allow
>   * the driver to implement support for features present in some ISP versions
> @@ -121,6 +122,7 @@ enum rkisp1_feature {
>  	RKISP1_FEATURE_MIPI_CSI2 = BIT(0),
>  	RKISP1_FEATURE_MAIN_STRIDE = BIT(1),
>  	RKISP1_FEATURE_SELF_PATH = BIT(2),
> +	RKISP1_FEATURE_DUAL_CROP = BIT(3),
>  };
>  
>  #define rkisp1_has_feature(rkisp1, feature) \
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> index f48a21985b18..2e40c376cab5 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-dev.c
> @@ -507,7 +507,8 @@ static const struct rkisp1_info px30_isp_info = {
>  	.isr_size = ARRAY_SIZE(px30_isp_isrs),
>  	.isp_ver = RKISP1_V12,
>  	.features = RKISP1_FEATURE_MIPI_CSI2
> -		  | RKISP1_FEATURE_SELF_PATH,
> +		  | RKISP1_FEATURE_SELF_PATH
> +		  | RKISP1_FEATURE_DUAL_CROP,
>  };
>  
>  static const char * const rk3399_isp_clks[] = {
> @@ -527,7 +528,8 @@ static const struct rkisp1_info rk3399_isp_info = {
>  	.isr_size = ARRAY_SIZE(rk3399_isp_isrs),
>  	.isp_ver = RKISP1_V10,
>  	.features = RKISP1_FEATURE_MIPI_CSI2
> -		  | RKISP1_FEATURE_SELF_PATH,
> +		  | RKISP1_FEATURE_SELF_PATH
> +		  | RKISP1_FEATURE_DUAL_CROP,
>  };
>  
>  static const struct of_device_id rkisp1_of_match[] = {
> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> index dd77a31e6014..755d319b568e 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> @@ -447,8 +447,9 @@ static void rkisp1_rsz_set_sink_crop(struct rkisp1_resizer *rsz,
>  	/* Not crop for MP bayer raw data */

	/* Not crop for MP bayer raw data, or for devices lacking dual crop. */

Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>

I can handle this when applying.

>  	mbus_info = rkisp1_mbus_info_get_by_code(sink_fmt->code);
>  
> -	if (rsz->id == RKISP1_MAINPATH &&
> -	    mbus_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) {
> +	if ((rsz->id == RKISP1_MAINPATH &&
> +	     mbus_info->pixel_enc == V4L2_PIXEL_ENC_BAYER) ||
> +	    !rkisp1_has_feature(rsz->rkisp1, DUAL_CROP)) {
>  		sink_crop->left = 0;
>  		sink_crop->top = 0;
>  		sink_crop->width = sink_fmt->width;
> @@ -635,7 +636,8 @@ static int rkisp1_rsz_s_stream(struct v4l2_subdev *sd, int enable)
>  	struct v4l2_subdev_state *sd_state;
>  
>  	if (!enable) {
> -		rkisp1_dcrop_disable(rsz, RKISP1_SHADOW_REGS_ASYNC);
> +		if (rkisp1_has_feature(rkisp1, DUAL_CROP))
> +			rkisp1_dcrop_disable(rsz, RKISP1_SHADOW_REGS_ASYNC);
>  		rkisp1_rsz_disable(rsz, RKISP1_SHADOW_REGS_ASYNC);
>  		return 0;
>  	}
> @@ -646,7 +648,8 @@ static int rkisp1_rsz_s_stream(struct v4l2_subdev *sd, int enable)
>  	sd_state = v4l2_subdev_lock_and_get_active_state(sd);
>  
>  	rkisp1_rsz_config(rsz, sd_state, when);
> -	rkisp1_dcrop_config(rsz, sd_state);
> +	if (rkisp1_has_feature(rkisp1, DUAL_CROP))
> +		rkisp1_dcrop_config(rsz, sd_state);
>  
>  	v4l2_subdev_unlock_state(sd_state);
>  

-- 
Regards,

Laurent Pinchart