Re: [PATCH v3 10/15] media: mt9m114: Adjust IFP selections and src format when src pixelfmt changes to/from RAW10

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

On Sun, Jun 29, 2025 at 10:56:20PM +0200, Hans de Goede wrote:
> Changing the IFP src pad pixelfmt to RAW10 means disabling the scaler,

s/src/source/
s/pixelfmt/format/ or "media bus code", as this isn't a pixel format

Same below.

> which means that the crop and compose rectangles must be reset to
> match the sink format size with no border.
> 
> And when changing the src pad pixelfmt back from RAW10 to another pixelfmt
> which require demosaicing the crop and compose rectangles must be reset
> to the sink format size minus a 4 pixels border all around it.
> 
> Also when changing the src pad pixelfmt back from RAW10 to another pixelfmt
> the colorspace, ycbcr_enc and quantization need to be updated too.
> 
> Add a new mt9m114_ifp_update_sel_and_src_fmt() helper which resets all
> these taking the bordersize for the new src pixelfmt into account and
> call this helper whenever the src pad pixelfmt changes to/from RAW10.
> 
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> ---
> Changes in v3:
> - This is a new patch in v3 of this patch-set, which comes from splitting
>   up "media: mt9m114: Fix scaler bypass mode" into multiple patches
> ---
>  drivers/media/i2c/mt9m114.c | 53 +++++++++++++++++++++++++++++++++----
>  1 file changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 020caae95a3d..ef27780384f2 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1820,6 +1820,40 @@ static int mt9m114_ifp_enum_frameintervals(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +/*
> + * Helper function to update IFP crop, compose rects and src format when

s/rects/rectangles/

> + * the pixel border size changes, which requires resetting these.
> + */
> +static void mt9m114_ifp_update_sel_and_src_fmt(struct v4l2_subdev_state *state)
> +{
> +	struct v4l2_mbus_framefmt *src_format, *sink_format;
> +	struct v4l2_rect *crop;
> +	unsigned int border;
> +
> +	sink_format = v4l2_subdev_state_get_format(state, 0);
> +	src_format = v4l2_subdev_state_get_format(state, 1);
> +	crop = v4l2_subdev_state_get_crop(state, 0);
> +	border = mt9m114_ifp_get_border(state);
> +
> +	crop->left = border;
> +	crop->top = border;
> +	crop->width = sink_format->width - 2 * border;
> +	crop->height = sink_format->height - 2 * border;
> +	*v4l2_subdev_state_get_compose(state, 0) = *crop;
> +
> +	src_format->width = crop->width;
> +	src_format->height = crop->height;

Please add a blank line here.

> +	if (src_format->code == MEDIA_BUS_FMT_SGRBG10_1X10) {
> +		src_format->colorspace = V4L2_COLORSPACE_RAW;
> +		src_format->ycbcr_enc = V4L2_YCBCR_ENC_601;
> +		src_format->quantization = V4L2_QUANTIZATION_FULL_RANGE;
> +	} else {
> +		src_format->colorspace = V4L2_COLORSPACE_SRGB;
> +		src_format->ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
> +		src_format->quantization = V4L2_QUANTIZATION_DEFAULT;
> +	}
> +}
> +
>  static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>  			       struct v4l2_subdev_state *state,
>  			       struct v4l2_subdev_format *fmt)
> @@ -1843,11 +1877,20 @@ static int mt9m114_ifp_set_fmt(struct v4l2_subdev *sd,
>  		/* Only the media bus code can be changed on the source pad. */
>  		info = mt9m114_format_info(sensor, 1, fmt->format.code);
>  
> -		format->code = info->code;
> -
> -		/* If the output format is RAW10, bypass the scaler. */
> -		if (format->code == MEDIA_BUS_FMT_SGRBG10_1X10)
> -			*format = *v4l2_subdev_state_get_format(state, 0);
> +		/*
> +		 * If the output format changes from/to RAW10 then the crop
> +		 * rectangle needs to be adjusted to add / remove the 4 pixel
> +		 * border used for demosaicing. And these changes then need to
> +		 * be propagated to the compose rectangle and src format size.
> +		 */
> +		if (format->code != info->code &&
> +		    (format->code == MEDIA_BUS_FMT_SGRBG10_1X10 ||
> +		     info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {

This would be shorter:

		if ((format->code == MEDIA_BUS_FMT_SGRBG10_1X10) !=
		    (info->code == MEDIA_BUS_FMT_SGRBG10_1X10)) {

Whether or not it's more readable may be a matter of personal preference
though.

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

> +			format->code = info->code;
> +			mt9m114_ifp_update_sel_and_src_fmt(state);
> +		} else {
> +			format->code = info->code;
> +		}
>  	}
>  
>  	fmt->format = *format;

-- 
Regards,

Laurent Pinchart