Re: [PATCH v7 1/2] media: mt9m114: Allow set_selection while streaming

Re: [PATCH v7 1/2] media: mt9m114: Allow set_selection while streaming

[kernel test robot]

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20250710151340.496218-2-mathis.foerst@mt.com>
References: <20250710151340.496218-2-mathis.foerst@mt.com>
TO: Mathis Foerst <mathis.foerst@mt.com>
TO: linux-kernel@vger.kernel.org
CC: Mathis Foerst <mathis.foerst@mt.com>
CC: Sakari Ailus <sakari.ailus@linux.intel.com>
CC: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
CC: Mauro Carvalho Chehab <mchehab@kernel.org>
CC: linux-media@vger.kernel.org
CC: manuel.traut@mt.com
CC: mathis.foerst@zuehlke.com

Hi Mathis,

kernel test robot noticed the following build warnings:

[auto build test WARNING on a8598c7de1bcd94461ca54c972efa9b4ea501fb9]

url:    https://github.com/intel-lab-lkp/linux/commits/Mathis-Foerst/media-mt9m114-Allow-set_selection-while-streaming/20250710-231657
base:   a8598c7de1bcd94461ca54c972efa9b4ea501fb9
patch link:    https://lore.kernel.org/r/20250710151340.496218-2-mathis.foerst%40mt.com
patch subject: [PATCH v7 1/2] media: mt9m114: Allow set_selection while streaming
:::::: branch date: 3 days ago
:::::: commit date: 3 days ago
config: x86_64-randconfig-161-20250713 (https://download.01.org/0day-ci/archive/20250714/202507140702.0iwcjn1a-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <error27@gmail.com>
| Closes: https://lore.kernel.org/r/202507140702.0iwcjn1a-lkp@intel.com/

smatch warnings:
drivers/media/i2c/mt9m114.c:1334 mt9m114_pa_set_selection() warn: curly braces intended?
drivers/media/i2c/mt9m114.c:1339 mt9m114_pa_set_selection() warn: inconsistent indenting

vim +1334 drivers/media/i2c/mt9m114.c

24d756e914fc34 Laurent Pinchart 2023-09-20  1282  
24d756e914fc34 Laurent Pinchart 2023-09-20  1283  static int mt9m114_pa_set_selection(struct v4l2_subdev *sd,
24d756e914fc34 Laurent Pinchart 2023-09-20  1284  				    struct v4l2_subdev_state *state,
24d756e914fc34 Laurent Pinchart 2023-09-20  1285  				    struct v4l2_subdev_selection *sel)
24d756e914fc34 Laurent Pinchart 2023-09-20  1286  {
24d756e914fc34 Laurent Pinchart 2023-09-20  1287  	struct mt9m114 *sensor = pa_to_mt9m114(sd);
24d756e914fc34 Laurent Pinchart 2023-09-20  1288  	struct v4l2_mbus_framefmt *format;
24d756e914fc34 Laurent Pinchart 2023-09-20  1289  	struct v4l2_rect *crop;
979c26f67a0dab Mathis Foerst    2025-07-10  1290  	int ret = 0;
24d756e914fc34 Laurent Pinchart 2023-09-20  1291  
24d756e914fc34 Laurent Pinchart 2023-09-20  1292  	if (sel->target != V4L2_SEL_TGT_CROP)
24d756e914fc34 Laurent Pinchart 2023-09-20  1293  		return -EINVAL;
24d756e914fc34 Laurent Pinchart 2023-09-20  1294  
bc0e8d91feec72 Sakari Ailus     2023-10-13  1295  	crop = v4l2_subdev_state_get_crop(state, sel->pad);
bc0e8d91feec72 Sakari Ailus     2023-10-13  1296  	format = v4l2_subdev_state_get_format(state, sel->pad);
24d756e914fc34 Laurent Pinchart 2023-09-20  1297  
24d756e914fc34 Laurent Pinchart 2023-09-20  1298  	/*
24d756e914fc34 Laurent Pinchart 2023-09-20  1299  	 * Clamp the crop rectangle. The vertical coordinates must be even, and
24d756e914fc34 Laurent Pinchart 2023-09-20  1300  	 * the horizontal coordinates must be a multiple of 4.
24d756e914fc34 Laurent Pinchart 2023-09-20  1301  	 *
24d756e914fc34 Laurent Pinchart 2023-09-20  1302  	 * FIXME: The horizontal coordinates must be a multiple of 8 when
24d756e914fc34 Laurent Pinchart 2023-09-20  1303  	 * binning, but binning is configured after setting the selection, so
24d756e914fc34 Laurent Pinchart 2023-09-20  1304  	 * we can't know tell here if it will be used.
24d756e914fc34 Laurent Pinchart 2023-09-20  1305  	 */
979c26f67a0dab Mathis Foerst    2025-07-10  1306  	sel->r.left = ALIGN(sel->r.left, 4);
979c26f67a0dab Mathis Foerst    2025-07-10  1307  	sel->r.top = ALIGN(sel->r.top, 2);
979c26f67a0dab Mathis Foerst    2025-07-10  1308  	sel->r.width = clamp_t(unsigned int, ALIGN(sel->r.width, 4),
24d756e914fc34 Laurent Pinchart 2023-09-20  1309  			       MT9M114_PIXEL_ARRAY_MIN_OUTPUT_WIDTH,
979c26f67a0dab Mathis Foerst    2025-07-10  1310  			       MT9M114_PIXEL_ARRAY_WIDTH - sel->r.left);
979c26f67a0dab Mathis Foerst    2025-07-10  1311  	sel->r.height = clamp_t(unsigned int, ALIGN(sel->r.height, 2),
24d756e914fc34 Laurent Pinchart 2023-09-20  1312  				MT9M114_PIXEL_ARRAY_MIN_OUTPUT_HEIGHT,
979c26f67a0dab Mathis Foerst    2025-07-10  1313  				MT9M114_PIXEL_ARRAY_HEIGHT - sel->r.top);
979c26f67a0dab Mathis Foerst    2025-07-10  1314  
979c26f67a0dab Mathis Foerst    2025-07-10  1315  	/* Changing the selection size is not allowed in streaming state */
979c26f67a0dab Mathis Foerst    2025-07-10  1316  	if (sensor->streaming &&
979c26f67a0dab Mathis Foerst    2025-07-10  1317  	    (sel->r.height != crop->height || sel->r.width != crop->width))
979c26f67a0dab Mathis Foerst    2025-07-10  1318  		return -EBUSY;
24d756e914fc34 Laurent Pinchart 2023-09-20  1319  
979c26f67a0dab Mathis Foerst    2025-07-10  1320  	*crop = sel->r;
24d756e914fc34 Laurent Pinchart 2023-09-20  1321  
24d756e914fc34 Laurent Pinchart 2023-09-20  1322  	/* Reset the format. */
24d756e914fc34 Laurent Pinchart 2023-09-20  1323  	format->width = crop->width;
24d756e914fc34 Laurent Pinchart 2023-09-20  1324  	format->height = crop->height;
24d756e914fc34 Laurent Pinchart 2023-09-20  1325  
979c26f67a0dab Mathis Foerst    2025-07-10  1326  	if (sel->which != V4L2_SUBDEV_FORMAT_ACTIVE)
979c26f67a0dab Mathis Foerst    2025-07-10  1327  		return ret;
979c26f67a0dab Mathis Foerst    2025-07-10  1328  
24d756e914fc34 Laurent Pinchart 2023-09-20  1329  	mt9m114_pa_ctrl_update_blanking(sensor, format);
24d756e914fc34 Laurent Pinchart 2023-09-20  1330  
979c26f67a0dab Mathis Foerst    2025-07-10  1331  	/* Apply values immediately if streaming */
979c26f67a0dab Mathis Foerst    2025-07-10  1332  	if (sensor->streaming)
979c26f67a0dab Mathis Foerst    2025-07-10  1333  		ret = mt9m114_configure_pa(sensor, state);
979c26f67a0dab Mathis Foerst    2025-07-10 @1334  		if (ret)
979c26f67a0dab Mathis Foerst    2025-07-10  1335  			return ret;
979c26f67a0dab Mathis Foerst    2025-07-10  1336  		/* Changing the cropping config requires a CONFIG_CHANGE */
979c26f67a0dab Mathis Foerst    2025-07-10  1337  		ret = mt9m114_set_state(sensor,
979c26f67a0dab Mathis Foerst    2025-07-10  1338  					MT9M114_SYS_STATE_ENTER_CONFIG_CHANGE);
979c26f67a0dab Mathis Foerst    2025-07-10 @1339  	return ret;
24d756e914fc34 Laurent Pinchart 2023-09-20  1340  }
24d756e914fc34 Laurent Pinchart 2023-09-20  1341  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

[PATCH v7 0/2] MT9M114 driver bugfix and improvements

[Mathis Foerst]

Hi,

this patch series contains the following bugfix and improvements
for the MT9M114 camera driver:

Changelog:

v6 -> v7:
- Remove already picked patches 1, 2, 3 and 6.
- Remove patch 4 as suggested in review.
- Fix formatting issues
- Return -EBUSY from mt9m114_pa_set_selection if the cropping size changes
  while in streaming state.

v5 -> v6:
- Add 'Reviewed-By:' tags. Sorry for forgetting this in the previous versions

v4 -> v5:
- Apply reformatings and small refactorings as suggested in review comments
- Split PATCH 4 into two parts: One for applying HFLIP / VFLIP while 
  streaming, one for applying set_selection while streaming.
- Add condition to apply set_selection immediately only if the size of the
  cropping rectangle does not change in PATCH 5
- Use device_property_read_u32 instead of of_property_read_u32 in PATCH 7

v3 -> v4:
- Rename DT binding from "onnn,slew-rate" to "slew-rate" in PATCH 1 and 6 as
  requested in the review comment.

v2 -> v3:
- Dropped PATCH 2 ("media: mt9m114: Add get_mbus_config").
  Based on the comments, this issure won't be fixed in the MT9M114
  driver but in "imx-media-csi.c" in a separate patch.
- Renumbered patches accordingly.
- Fix the incomplete renaming of the DT property from 'pad-slew-rate'
  to 'onnn,slew-rate' in PATCH 1 and 6.
- Fix checkpatch formatting suggestions in PATCH 2 and 6.

v1 -> v2:
- Fix the subjects of the patches
- Dropped PATCH 1 ("Add bypass-pll DT-binding") as it can be automatically
  detected if the PLL should be bypassed.
- Renumbered patches accordingly
- Switch to uint32, add default value and clarify documentation in PATCH 1
- Add 'Fixes' and 'Cc' tags as suggested in PATCH 6

Link to v1 discussion:
https://lore.kernel.org/linux-media/20250226153929.274562-1-mathis.foerst@mt.com/
Link to v2 discussion:
https://lore.kernel.org/linux-media/20250304103647.34235-1-mathis.foerst@mt.com/
Link to v3 discussion:
https://lore.kernel.org/linux-media/20250305101453.708270-1-mathis.foerst@mt.com/
Link to v4 discussion:
https://lore.kernel.org/linux-media/20250307093140.370061-1-mathis.foerst@mt.com/

Bugfixes:
- Fix a deadlock when using the V4L2 pad-ops get/set_frame_interval

New Features:
- Bypass the internal PLL if EXTCLK matches the configured link_frequency
- Make the slew-rate of the output pads configurable via DT
- Allow to change the cropping configuration while the sensor is in streaming 
  state

Thanks,
Mathis

Mathis Foerst (2):
  media: mt9m114: Allow set_selection while streaming
  media: mt9m114: Set pad-slew-rate

 drivers/media/i2c/mt9m114.c | 68 ++++++++++++++++++++++++++++---------
 1 file changed, 52 insertions(+), 16 deletions(-)


base-commit: a8598c7de1bcd94461ca54c972efa9b4ea501fb9
-- 
2.34.1

[PATCH v7 1/2] media: mt9m114: Allow set_selection while streaming

[Mathis Foerst]

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.
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@mt.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 */
+	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 = {
-- 
2.34.1

Re: [PATCH v7 1/2] media: mt9m114: Allow set_selection while streaming

[Laurent Pinchart]

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@mt.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@ideasonboard.com>

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

-- 
Regards,

Laurent Pinchart