Re: [PATCH v2 09/12] media: mt9m114: Fix scaler bypass mode

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

Thank you for the patch.

On Sat, May 31, 2025 at 06:31:44PM +0200, Hans de Goede wrote:
> As indicated by the comment in mt9m114_ifp_set_fmt():
> 
> 	/* If the output format is RAW10, bypass the scaler. */
> 	if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
> 		...
> 
> The intend of the driver is that the scalar is bypassed when the ISP
> source/output pad's pixel-format is set to MEDIA_BUS_FMT_SGRBG10_1X10.
> 
> This patch makes 2 changes which are required to get this to work properly:
> 
> 1. Set the MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE bit in
>    the MT9M114_CAM_OUTPUT_FORMAT register.

Does that bit actually make a difference ? It's documented as only being
effective when MT9M114_CAM_OUTPUT_FORMAT_BT656_ENABLE is also set, and
the driver never sets that.

> 
> 2. Disable cropping/composing by setting crop and compose selections on
>    the ISP sink/input format to the format widthxheight @ 0x0.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v2:
> - When bypassing the scalar make ifp_get_selection() / ifp_set_selection()
>   fill sel->r with a rectangle of (0,0)/wxh and return 0 instead of
>   returning -EINVAL
> ---
>  drivers/media/i2c/mt9m114.c | 35 ++++++++++++++++++++++++++++++-----
>  1 file changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 7a1451006cfe..d954f2be8f0d 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -467,7 +467,8 @@ static const struct mt9m114_format_info mt9m114_format_infos[] = {
>  		/* Keep the format compatible with the IFP sink pad last. */
>  		.code = MEDIA_BUS_FMT_SGRBG10_1X10,
>  		.output_format = MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_RAWR10
> -			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER,
> +			| MT9M114_CAM_OUTPUT_FORMAT_FORMAT_BAYER
> +			| MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE,
>  		.flags = MT9M114_FMT_FLAG_PARALLEL | MT9M114_FMT_FLAG_CSI2,
>  	}
>  };
> @@ -850,6 +851,7 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>  	const struct v4l2_mbus_framefmt *format;
>  	const struct v4l2_rect *crop;
>  	const struct v4l2_rect *compose;
> +	unsigned int border;
>  	u64 output_format;
>  	int ret = 0;
>  
> @@ -869,10 +871,12 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>  	 * by demosaicing that are taken into account in the crop rectangle but
>  	 * not in the hardware.
>  	 */

The comment should be updated.

> +	border = (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) ? 0 : 4;

No need for parentheses.

> +
>  	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_XOFFSET,
> -		  crop->left - 4, &ret);
> +		  crop->left - border, &ret);
>  	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_YOFFSET,
> -		  crop->top - 4, &ret);
> +		  crop->top - border, &ret);
>  	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_WIDTH,
>  		  crop->width, &ret);
>  	cci_write(sensor->regmap, MT9M114_CAM_CROP_WINDOW_HEIGHT,
> @@ -911,7 +915,8 @@ static int mt9m114_configure_ifp(struct mt9m114 *sensor,
>  			   MT9M114_CAM_OUTPUT_FORMAT_BAYER_FORMAT_MASK |
>  			   MT9M114_CAM_OUTPUT_FORMAT_FORMAT_MASK |
>  			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_BYTES |
> -			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE);
> +			   MT9M114_CAM_OUTPUT_FORMAT_SWAP_RED_BLUE |
> +			   MT9M114_CAM_OUTPUT_FORMAT_BT656_CROP_SCALE_DISABLE);
>  	output_format |= info->output_format;
>  
>  	cci_write(sensor->regmap, MT9M114_CAM_OUTPUT_FORMAT,
> @@ -1810,6 +1815,7 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>  {
>  	struct mt9m114 *sensor = ifp_to_mt9m114(sd);
>  	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_rect *crop;
>  
>  	format = v4l2_subdev_state_get_format(state, fmt->pad);
>  
> @@ -1830,8 +1836,15 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>  		format->code = info->code;
>  
>  		/* If the output format is RAW10, bypass the scaler. */
> -		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
> +		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
>  			*format = *v4l2_subdev_state_get_format(state, 0);
> +			crop = v4l2_subdev_state_get_crop(state, 0);
> +			crop->left = 0;
> +			crop->top = 0;
> +			crop->width = format->width;
> +			crop->height = format->height;
> +			*v4l2_subdev_state_get_compose(state, 0) = *crop;
> +		}

This one is annoying. Normally changing a format or selection rectangle
in a subdev should only propagate the configuration downstream. Here
you're modiying selection rectangles upstream of the source pad. I think
we'll need to live with it, I don't really see a way around that. It's a
non-standard behaviour though, and would require sensor-specific
userspace to handle this right.

There's however one issue. When switching back from a raw output to a
processed output, the crop and compose rectangles will remain the same,
and will have values that are not valid for processed output as they
won't have the 4 pixels border subtracted. I think you'll need to update
them, but only when the source format actually changes. Setting the
source format without modifying the media bus code should not result in
the selection rectangles being reset.

All this needs to be explained in a comment in the code.

>  	}
>  
>  	fmt->format = *format;
> @@ -1851,6 +1864,12 @@ static int mt9m114_ifp_get_selection(struct v4l2_subdev *sd,
>  	if (sel->pad != 0)
>  		return -EINVAL;
>  
> +	/* Crop and compose cannot be changed when bypassing the scaler */

s/scaler/scaler./

Same below.

> +	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> +		return 0;
> +	}

Instead of special-casing this, can you use a similar approach as in
mt9m114_configure_ifp() and replace the 4 and 8 below with border and
boarder * 2 (with an update to the comment too) ?

> +
>  	switch (sel->target) {
>  	case V4L2_SEL_TGT_CROP:
>  		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> @@ -1911,6 +1930,12 @@ static int mt9m114_ifp_set_selection(struct v4l2_subdev *sd,
>  	if (sel->pad != 0)
>  		return -EINVAL;
>  
> +	/* Crop and compose cannot be changed when bypassing the scaler */
> +	if (v4l2_subdev_state_get_format(state, 1)->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
> +		sel->r = *v4l2_subdev_state_get_crop(state, 0);
> +		return 0;
> +	}

Let's add a source_format variable to avoid duplicating the
v4l2_subdev_state_get_format(), call, as we use the source format at the
end of the function.

> +
>  	format = v4l2_subdev_state_get_format(state, 0);
>  	crop = v4l2_subdev_state_get_crop(state, 0);
>  	compose = v4l2_subdev_state_get_compose(state, 0);

-- 
Regards,

Laurent Pinchart