linux-kernel - Re: [PATCH v4 4/6] 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: <20250423183900.GF2675@pendragon.ideasonboard.com>
Date: Wed, 23 Apr 2025 21:39:00 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Mathis Foerst <mathis.foerst@...com>
Cc: linux-kernel@...r.kernel.org,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-media@...r.kernel.org, devicetree@...r.kernel.org,
	manuel.traut@...com, mathis.foerst@...hlke.com
Subject: Re: [PATCH v4 4/6] media: mt9m114: Allow set_selection while
 streaming

Hi Mathis,

Thank you for the patch.

On Fri, Mar 07, 2025 at 10:31:38AM +0100, 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.
> The same holds for changes to the V4L2 controls HFLIP & VFLIP.

Could you please split this in two patches, one for flip, abd one for
crop ? From flipping I think it's just a driver bug that I forgot to
issue a CONFIG_CHANGE, while for cropping it was by design. The commit
message for the crop patch can explain why this has to change, while the
commit message for the flip patch can just explains it's fixing a bug.
This will also reflect all the changes in the commit subjects, while
here the subject only mentions set_selection and hides the flip change.

> 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.
> 
> Call mt9m114_configure_pa() in mt9m114_pa_set_selection() if the sensor is
> in streaming state and issue a CONFIG_CHANGE to apply the changes
> immediately.
> Issue a CONFIG_CHANGE when the V4L2 controls HFLIP or VFLIP are set if the
> sensor is in streaming state to apply the change immediately.
> 
> Signed-off-by: Mathis Foerst <mathis.foerst@...com>
> ---
>  drivers/media/i2c/mt9m114.c | 32 +++++++++++++++++++++++++++++---
>  1 file changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/i2c/mt9m114.c b/drivers/media/i2c/mt9m114.c
> index 9a49dab65180..65b9124e464f 100644
> --- a/drivers/media/i2c/mt9m114.c
> +++ b/drivers/media/i2c/mt9m114.c
> @@ -1098,6 +1098,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = cci_update_bits(sensor->regmap,
>  				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  				      mask, ctrl->val ? mask : 0, NULL);
> +		if (ret)
> +			return ret;

Add a blank line.

But it's a bug, you'll leak a PM reference count. You need to break
instead.

> +		if (sensor->streaming) {
> +			// Changing the flip config while streaming requires a CONFIG_CHANGE

C-style comments please, and reflow at 80 columns. Or possibly better,
you could copy the code from mt9m114_ifp_s_ctrl() for consistency:


		/*
		 * A Config-Change needs to be issued for the change to take
		 * effect. If we're not streaming ignore this, the change will
		 * be applied when the stream is started.
		 */
		if (ret || !sensor->streaming)
			break;

		ret = mt9m114_set_state(sensor,
					MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);

Same comments for V4L2_CID_VFLIP.

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

As this can be an expensive operation, I think the hflip and vflip
controls should be put in a cluster, to be able to change them both in
one go.

> +		}
>  		break;
>  
>  	case V4L2_CID_VFLIP:
> @@ -1105,6 +1112,13 @@ static int mt9m114_pa_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ret = cci_update_bits(sensor->regmap,
>  				      MT9M114_CAM_SENSOR_CONTROL_READ_MODE,
>  				      mask, ctrl->val ? mask : 0, NULL);
> +		if (ret)
> +			return ret;
> +		if (sensor->streaming) {
> +			// Changing the flip config while streaming requires a CONFIG_CHANGE
> +			ret = mt9m114_set_state(sensor,
> +						MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> +		}
>  		break;
>  
>  	default:
> @@ -1286,6 +1300,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;
> @@ -1316,10 +1331,21 @@ static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
>  	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 */

Changing the crop rectangle modifies the output size of the PA. It will
not match the size the IFP expects at its input anymore. Could you
please explain how this will work and why it won't cause issues ?

> +	if (sensor->streaming) {
> +		ret = mt9m114_configure_pa(sensor, state);
> +		if (ret)
> +			return ret;
> +		// Changing the cropping config requires a CONFIG_CHANGE
> +		ret = mt9m114_set_state(sensor,	MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
> +	}
> +
> +	return ret;
>  }
>  
>  static const struct v4l2_subdev_pad_ops mt9m114_pa_pad_ops = {

-- 
Regards,

Laurent Pinchart