Re: [PATCH] media: rkisp1: resizer: Stop manual allocation of v4l2_subdev_state

From: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linux-media@vger.kernel.org
Cc: Dafna Hirschfeld <dafna@fastmail.com>,
	Paul Elder <paul.elder@ideasonboard.com>,
	Kieran Bingham <kieran.bingham@ideasonboard.com>,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH] media: rkisp1: resizer: Stop manual allocation of v4l2_subdev_state
Date: Tue, 5 Dec 2023 11:32:34 +0200	[thread overview]
Message-ID: <88681389-6969-4ac0-93f8-a3fe1ae1a1fc@ideasonboard.com> (raw)
In-Reply-To: <20231126020948.2700-1-laurent.pinchart@ideasonboard.com>

On 26/11/2023 04:09, Laurent Pinchart wrote:
> Supported media bus codes on the resizer sink pad are identical to the
> ISP source pad. The .enum_mbus_code() handler thus delegates the
> enumeration to the ISP's operation. This is problematic for two
> reasons:
> 
> - Format enumeration on the ISP source pad is dependent on the format
>    configured on the ISP sink pad for the same subdev state (TRY or
>    ACTIVE), while format enumeration on the resizer sink pad should
>    return all formats supported by the resizer subdev, regardless of the
>    ISP configuration.
> 
> - Delegating the operation involves creating a fake v4l2_subdev_state on
>    the stack to pass to the ISP .enum_mbus_code() handler. This gets in
>    the way of evolution of both the ISP enumeration handler and, more
>    generally, the V4L2 subdev state infrastructure.
> 
> Fix those two issues by implementing format enumeration manually for the
> resizer.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
> Paul, Kieran, would you be able to test this for regressions ?
> ---
>   .../platform/rockchip/rkisp1/rkisp1-resizer.c | 38 ++++++++++++-------
>   1 file changed, 24 insertions(+), 14 deletions(-)

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

I tested on my i.MX8MP board, although my kernel branch has quite a bit 
of non-upstream patches to get everything working. But I don't think 
those affect this patch.

  Tomi

> diff --git a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> index f7af360dcb28..a8e377701302 100644
> --- a/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> +++ b/drivers/media/platform/rockchip/rkisp1/rkisp1-resizer.c
> @@ -330,12 +330,8 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>   {
>   	struct rkisp1_resizer *rsz =
>   		container_of(sd, struct rkisp1_resizer, sd);
> -	struct v4l2_subdev_pad_config dummy_cfg;
> -	struct v4l2_subdev_state pad_state = {
> -		.pads = &dummy_cfg
> -	};
> -	u32 pad = code->pad;
> -	int ret;
> +	unsigned int index = code->index;
> +	unsigned int i;
>   
>   	if (code->pad == RKISP1_RSZ_PAD_SRC) {
>   		/* supported mbus codes on the src are the same as in the capture */
> @@ -355,15 +351,29 @@ static int rkisp1_rsz_enum_mbus_code(struct v4l2_subdev *sd,
>   		return 0;
>   	}
>   
> -	/* supported mbus codes on the sink pad are the same as isp src pad */
> -	code->pad = RKISP1_ISP_PAD_SOURCE_VIDEO;
> -	ret = v4l2_subdev_call(&rsz->rkisp1->isp.sd, pad, enum_mbus_code,
> -			       &pad_state, code);
> +	/*
> +	 * Supported mbus codes on the sink pad are the same as on the ISP
> +	 * source pad.
> +	 */
> +	for (i = 0; ; i++) {
> +		const struct rkisp1_mbus_info *fmt =
> +			rkisp1_mbus_info_get_by_index(i);
>   
> -	/* restore pad */
> -	code->pad = pad;
> -	code->flags = 0;
> -	return ret;
> +		if (!fmt)
> +			break;
> +
> +		if (!(fmt->direction & RKISP1_ISP_SD_SRC))
> +			continue;
> +
> +		if (!index) {
> +			code->code = fmt->mbus_code;
> +			return 0;
> +		}
> +
> +		index--;
> +	}
> +
> +	return -EINVAL;
>   }
>   
>   static int rkisp1_rsz_init_state(struct v4l2_subdev *sd,