linux-kernel - Re: [PATCH v7 1/2] media: mt9m114: Allow set

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250710223858.GB16271@pendragon.ideasonboard.com>
Date: Fri, 11 Jul 2025 01:38:58 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mathis Foerst <mathis.foerst@...com>
Cc: linux-kernel@...r.kernel.org,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, manuel.traut@...com,
	mathis.foerst@...hlke.com
Subject: Re: [PATCH v7 1/2] media: mt9m114: Allow set_selection while
 streaming

Hi Mathis,

Thank you for the patch.

On Thu, Jul 10, 2025 at 05:13:39PM +0200, Mathis Foerst wrote:
> The current implementation does not apply changes to the crop-
> configuration of the sensor immediately if the sensor is in
> streaming state. The user has to stop and restart the stream for
> the changes to be applied.

Nitpicking, you need a blank line between two different paragraphs.

> This can be undesirable e.g. in a calibration usecase where the user
> wants to see the impact of his changes in a live video stream.
> Under the condition that the width & height of the cropped image area
> does not change, the changed cropping configuration can be applied to
> the pixel-array immediately without disturbing the IFP.
> 
> Call mt9m114_configure_pa() in mt9m114_pa_set_selection() if the sensor is
> in streaming state and the size of the cropping rectangle didn't change,
> issue a CONFIG_CHANGE to apply the changes immediately.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@...com>
> ---
>  drivers/media/i2c/mt9m114.c | 42 +++++++++++++++++++++++++------------
>  1 file changed, 29 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 3f540ca40f3c..5f4474d36653 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1287,6 +1287,7 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
>  	struct mt9m114 *sensor = pa_to_mt9m114(sd);
>  	struct v4l2_mbus_framefmt *format;
>  	struct v4l2_rect *crop;
> +	int ret = 0;
>  
>  	if (sel->target != V4L2_SEL_TGT_CROP)
>  		return -EINVAL;
> @@ -1302,25 +1303,40 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
>  	 * binning, but binning is configured after setting the selection, so
>  	 * we can't know tell here if it will be used.
>  	 */
> -	crop->left = ALIGN(sel->r.left, 4);
> -	crop->top = ALIGN(sel->r.top, 2);
> -	crop->width = clamp_t(unsigned int, ALIGN(sel->r.width, 4),
> -			      MT9M114_PIXEL_ARRAY_MIN_OUTPUT_WIDTH,
> -			      MT9M114_PIXEL_ARRAY_WIDTH - crop->left);
> -	crop->height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
> -			       MT9M114_PIXEL_ARRAY_MIN_OUTPUT_HEIGHT,
> -			       MT9M114_PIXEL_ARRAY_HEIGHT - crop->top);
> -
> -	sel->r = *crop;
> +	sel->r.left = ALIGN(sel->r.left, 4);
> +	sel->r.top = ALIGN(sel->r.top, 2);
> +	sel->r.width = clamp_t(unsigned int, ALIGN(sel->r.width, 4),
> +			       MT9M114_PIXEL_ARRAY_MIN_OUTPUT_WIDTH,
> +			       MT9M114_PIXEL_ARRAY_WIDTH - sel->r.left);
> +	sel->r.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
> +				MT9M114_PIXEL_ARRAY_MIN_OUTPUT_HEIGHT,
> +				MT9M114_PIXEL_ARRAY_HEIGHT - sel->r.top);
> +
> +	/* Changing the selection size is not allowed in streaming state */
> +	if (sensor->streaming &&
> +	    (sel->r.height != crop->height || sel->r.width != crop->width))
> +		return -EBUSY;
> +
> +	*crop = sel->r;
>  
>  	/* Reset the format. */
>  	format->width = crop->width;
>  	format->height = crop->height;
>  
> -	if (sel->which == V4L2_SUBDEV_FORMAT_ACTIVE)
> -		mt9m114_pa_ctrl_update_blanking(sensor, format);
> +	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
> +		return ret;
>  
> -	return 0;
> +	mt9m114_pa_ctrl_update_blanking(sensor, format);
> +
> +	/* Apply values immediately if streaming */

s/streaming/streaming./

> +	if (sensor->streaming)
> +		ret = mt9m114_configure_pa(sensor, state);
> +		if (ret)
> +			return ret;
> +		/* Changing the cropping config requires a CONFIG_CHANGE */

s/CONFIG_CHANGE/CONFIG_CHANGE./

> +		ret = mt9m114_set_state(sensor,
> +					MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);

Misleading indentation, this call is done unconditionally, even when not
streaming. You're forgotten the curly braces.

If any of those calls fail, the rectangle in the "crop" variable will
have been updated but not applied to the hardware. We could restore the
value, or delay storing it in "crop" until the end of the function. It's
probably not really worth it though, so with the above issues addressed,

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

> +	return ret;
>  }
>  
>  static const struct v4l2_subdev_pad_ops mt9m114_pa_pad_ops = {

-- 
Regards,

Laurent Pinchart