[PATCH v1 0/2] media: i2c: imx219: Miscellaneous fixes

[PATCH v1 0/2] media: i2c: imx219: Miscellaneous fixes

[Laurent Pinchart]

Hello,

This small series fixes two issues introduced by the "[PATCH v2 0/7]
media: i2c: imx219: Use subdev active state" series, merged in the stage
tree and scheduled for upstream merge in v6.6. Please see individual
patches for details.

Laurent Pinchart (2):
  media: i2c: imx219: Fix a typo referring to a wrong variable
  media: i2c: imx219: Fix crop rectangle setting when changing format

 drivers/media/i2c/imx219.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)


base-commit: a5f4ec0a9e69c1d2cafd55dd9810c69d9d269b8e
-- 
Regards,

Laurent Pinchart

[PATCH v1 1/2] media: i2c: imx219: Fix a typo referring to a wrong variable

[Laurent Pinchart]

The imx219_init_cfg() function has stopped operating on the try format
since commit 7e700847b1fe ("media: i2c: imx219: Switch from open to
init_cfg"), but a comment in the function wasn't updated. Fix it.

While at it, improve spelling in a second comment in the function.

Fixes: 7e700847b1fe ("media: i2c: imx219: Switch from open to init_cfg")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index a1136fdfbed2..6f88e002c8d8 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -691,12 +691,12 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
 	struct v4l2_mbus_framefmt *format;
 	struct v4l2_rect *crop;
 
-	/* Initialize try_fmt */
+	/* Initialize the format. */
 	format = v4l2_subdev_get_pad_format(sd, state, 0);
 	imx219_update_pad_format(imx219, &supported_modes[0], format,
 				 MEDIA_BUS_FMT_SRGGB10_1X10);
 
-	/* Initialize crop rectangle. */
+	/* Initialize the crop rectangle. */
 	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
 	crop->top = IMX219_PIXEL_ARRAY_TOP;
 	crop->left = IMX219_PIXEL_ARRAY_LEFT;
-- 
Regards,

Laurent Pinchart

[PATCH v1 2/2] media: i2c: imx219: Fix crop rectangle setting when changing format

[Laurent Pinchart]

When moving the imx219 driver to the subdev active state, commit
e8a5b1df000e ("media: i2c: imx219: Use subdev active state") used the
pad crop rectangle stored in the subdev state to report the crop
rectangle of the active mode. That crop rectangle was however not set in
the state when setting the format, which resulted in reporting an
incorrect crop rectangle to userspace. Fix it.

Fixes: e8a5b1df000e ("media: i2c: imx219: Use subdev active state")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/i2c/imx219.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
index 6f88e002c8d8..ec53abe2e84e 100644
--- a/drivers/media/i2c/imx219.c
+++ b/drivers/media/i2c/imx219.c
@@ -750,6 +750,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 	const struct imx219_mode *mode;
 	int exposure_max, exposure_def, hblank;
 	struct v4l2_mbus_framefmt *format;
+	struct v4l2_rect *crop;
 
 	mode = v4l2_find_nearest_size(supported_modes,
 				      ARRAY_SIZE(supported_modes),
@@ -757,10 +758,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 				      fmt->format.width, fmt->format.height);
 
 	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
-	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
 
-	if (imx219->mode == mode && format->code == fmt->format.code)
-		return 0;
+	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
+	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
+
+	*format = fmt->format;
+	*crop = mode->crop;
 
 	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
 		imx219->mode = mode;
@@ -788,8 +791,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
 					 hblank);
 	}
 
-	*format = fmt->format;
-
 	return 0;
 }
 
-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 1/2] media: i2c: imx219: Fix a typo referring to a wrong variable

[Jacopo Mondi]

Hi Laurent

On Mon, Aug 14, 2023 at 10:34:34PM +0300, Laurent Pinchart wrote:
> The imx219_init_cfg() function has stopped operating on the try format
> since commit 7e700847b1fe ("media: i2c: imx219: Switch from open to
> init_cfg"), but a comment in the function wasn't updated. Fix it.
>
> While at it, improve spelling in a second comment in the function.
>
> Fixes: 7e700847b1fe ("media: i2c: imx219: Switch from open to init_cfg")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>


> ---
>  drivers/media/i2c/imx219.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index a1136fdfbed2..6f88e002c8d8 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -691,12 +691,12 @@ static int imx219_init_cfg(struct v4l2_subdev *sd,
>  	struct v4l2_mbus_framefmt *format;
>  	struct v4l2_rect *crop;
>
> -	/* Initialize try_fmt */
> +	/* Initialize the format. */
>  	format = v4l2_subdev_get_pad_format(sd, state, 0);
>  	imx219_update_pad_format(imx219, &supported_modes[0], format,
>  				 MEDIA_BUS_FMT_SRGGB10_1X10);
>
> -	/* Initialize crop rectangle. */
> +	/* Initialize the crop rectangle. */
>  	crop = v4l2_subdev_get_pad_crop(sd, state, 0);
>  	crop->top = IMX219_PIXEL_ARRAY_TOP;
>  	crop->left = IMX219_PIXEL_ARRAY_LEFT;
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v1 2/2] media: i2c: imx219: Fix crop rectangle setting when changing format

[Jacopo Mondi]

Hi Laurent

On Mon, Aug 14, 2023 at 10:34:35PM +0300, Laurent Pinchart wrote:
> When moving the imx219 driver to the subdev active state, commit
> e8a5b1df000e ("media: i2c: imx219: Use subdev active state") used the
> pad crop rectangle stored in the subdev state to report the crop
> rectangle of the active mode. That crop rectangle was however not set in
> the state when setting the format, which resulted in reporting an
> incorrect crop rectangle to userspace. Fix it.
>
> Fixes: e8a5b1df000e ("media: i2c: imx219: Use subdev active state")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/i2c/imx219.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> index 6f88e002c8d8..ec53abe2e84e 100644
> --- a/drivers/media/i2c/imx219.c
> +++ b/drivers/media/i2c/imx219.c
> @@ -750,6 +750,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  	const struct imx219_mode *mode;
>  	int exposure_max, exposure_def, hblank;
>  	struct v4l2_mbus_framefmt *format;
> +	struct v4l2_rect *crop;
>
>  	mode = v4l2_find_nearest_size(supported_modes,
>  				      ARRAY_SIZE(supported_modes),
> @@ -757,10 +758,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  				      fmt->format.width, fmt->format.height);
>
>  	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> -	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
>
> -	if (imx219->mode == mode && format->code == fmt->format.code)
> -		return 0;

Has this check been lost ?

> +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> +
> +	*format = fmt->format;
> +	*crop = mode->crop;
>
>  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
>  		imx219->mode = mode;
> @@ -788,8 +791,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
>  					 hblank);
>  	}
>
> -	*format = fmt->format;
> -
>  	return 0;
>  }
>
> --
> Regards,
>
> Laurent Pinchart
>

Re: [PATCH v1 2/2] media: i2c: imx219: Fix crop rectangle setting when changing format

[Laurent Pinchart]

Hi Jacopo,

On Mon, Aug 28, 2023 at 02:19:54PM +0200, Jacopo Mondi wrote:
> On Mon, Aug 14, 2023 at 10:34:35PM +0300, Laurent Pinchart wrote:
> > When moving the imx219 driver to the subdev active state, commit
> > e8a5b1df000e ("media: i2c: imx219: Use subdev active state") used the
> > pad crop rectangle stored in the subdev state to report the crop
> > rectangle of the active mode. That crop rectangle was however not set in
> > the state when setting the format, which resulted in reporting an
> > incorrect crop rectangle to userspace. Fix it.
> >
> > Fixes: e8a5b1df000e ("media: i2c: imx219: Use subdev active state")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx219.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > index 6f88e002c8d8..ec53abe2e84e 100644
> > --- a/drivers/media/i2c/imx219.c
> > +++ b/drivers/media/i2c/imx219.c
> > @@ -750,6 +750,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  	const struct imx219_mode *mode;
> >  	int exposure_max, exposure_def, hblank;
> >  	struct v4l2_mbus_framefmt *format;
> > +	struct v4l2_rect *crop;
> >
> >  	mode = v4l2_find_nearest_size(supported_modes,
> >  				      ARRAY_SIZE(supported_modes),
> > @@ -757,10 +758,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  				      fmt->format.width, fmt->format.height);
> >
> >  	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> > -	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> >
> > -	if (imx219->mode == mode && format->code == fmt->format.code)
> > -		return 0;
> 
> Has this check been lost ?

It has. Is it an issue ?

> > +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > +
> > +	*format = fmt->format;
> > +	*crop = mode->crop;
> >
> >  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> >  		imx219->mode = mode;
> > @@ -788,8 +791,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> >  					 hblank);
> >  	}
> >
> > -	*format = fmt->format;
> > -
> >  	return 0;
> >  }
> >

-- 
Regards,

Laurent Pinchart

Re: [PATCH v1 2/2] media: i2c: imx219: Fix crop rectangle setting when changing format

[Jacopo Mondi]

Hi Laurent

On Mon, Aug 28, 2023 at 10:15:06PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Aug 28, 2023 at 02:19:54PM +0200, Jacopo Mondi wrote:
> > On Mon, Aug 14, 2023 at 10:34:35PM +0300, Laurent Pinchart wrote:
> > > When moving the imx219 driver to the subdev active state, commit
> > > e8a5b1df000e ("media: i2c: imx219: Use subdev active state") used the
> > > pad crop rectangle stored in the subdev state to report the crop
> > > rectangle of the active mode. That crop rectangle was however not set in
> > > the state when setting the format, which resulted in reporting an
> > > incorrect crop rectangle to userspace. Fix it.
> > >
> > > Fixes: e8a5b1df000e ("media: i2c: imx219: Use subdev active state")
> > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx219.c | 11 ++++++-----
> > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > index 6f88e002c8d8..ec53abe2e84e 100644
> > > --- a/drivers/media/i2c/imx219.c
> > > +++ b/drivers/media/i2c/imx219.c
> > > @@ -750,6 +750,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  	const struct imx219_mode *mode;
> > >  	int exposure_max, exposure_def, hblank;
> > >  	struct v4l2_mbus_framefmt *format;
> > > +	struct v4l2_rect *crop;
> > >
> > >  	mode = v4l2_find_nearest_size(supported_modes,
> > >  				      ARRAY_SIZE(supported_modes),
> > > @@ -757,10 +758,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  				      fmt->format.width, fmt->format.height);
> > >
> > >  	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> > > -	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > >
> > > -	if (imx219->mode == mode && format->code == fmt->format.code)
> > > -		return 0;
> >
> > Has this check been lost ?
>
> It has. Is it an issue ?
>

well, the check ensure we exit earlier if the sensor configuration
doesn't change.. as the newly introduced crop rectangle comes from the
mode as well, I'm missing why it should now be dropped...

> > > +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > +
> > > +	*format = fmt->format;
> > > +	*crop = mode->crop;
> > >
> > >  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > >  		imx219->mode = mode;
> > > @@ -788,8 +791,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > >  					 hblank);
> > >  	}
> > >
> > > -	*format = fmt->format;
> > > -
> > >  	return 0;
> > >  }
> > >
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH v1 2/2] media: i2c: imx219: Fix crop rectangle setting when changing format

[Laurent Pinchart]

Hi Jacopo,

On Tue, Aug 29, 2023 at 08:31:27AM +0200, Jacopo Mondi wrote:
> On Mon, Aug 28, 2023 at 10:15:06PM +0300, Laurent Pinchart wrote:
> > On Mon, Aug 28, 2023 at 02:19:54PM +0200, Jacopo Mondi wrote:
> > > On Mon, Aug 14, 2023 at 10:34:35PM +0300, Laurent Pinchart wrote:
> > > > When moving the imx219 driver to the subdev active state, commit
> > > > e8a5b1df000e ("media: i2c: imx219: Use subdev active state") used the
> > > > pad crop rectangle stored in the subdev state to report the crop
> > > > rectangle of the active mode. That crop rectangle was however not set in
> > > > the state when setting the format, which resulted in reporting an
> > > > incorrect crop rectangle to userspace. Fix it.
> > > >
> > > > Fixes: e8a5b1df000e ("media: i2c: imx219: Use subdev active state")
> > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > ---
> > > >  drivers/media/i2c/imx219.c | 11 ++++++-----
> > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx219.c b/drivers/media/i2c/imx219.c
> > > > index 6f88e002c8d8..ec53abe2e84e 100644
> > > > --- a/drivers/media/i2c/imx219.c
> > > > +++ b/drivers/media/i2c/imx219.c
> > > > @@ -750,6 +750,7 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > > >  	const struct imx219_mode *mode;
> > > >  	int exposure_max, exposure_def, hblank;
> > > >  	struct v4l2_mbus_framefmt *format;
> > > > +	struct v4l2_rect *crop;
> > > >
> > > >  	mode = v4l2_find_nearest_size(supported_modes,
> > > >  				      ARRAY_SIZE(supported_modes),
> > > > @@ -757,10 +758,12 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > > >  				      fmt->format.width, fmt->format.height);
> > > >
> > > >  	imx219_update_pad_format(imx219, mode, &fmt->format, fmt->format.code);
> > > > -	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > >
> > > > -	if (imx219->mode == mode && format->code == fmt->format.code)
> > > > -		return 0;
> > >
> > > Has this check been lost ?
> >
> > It has. Is it an issue ?
> 
> well, the check ensure we exit earlier if the sensor configuration
> doesn't change.. as the newly introduced crop rectangle comes from the
> mode as well, I'm missing why it should now be dropped...

This is a minor optimization, and most drivers don't have similar code.
I would like to eventually improve consistency between camera sensor
drivers, so I thought I would drop the check here.

You're right that it should at least be mentioned in the commit message,
and likely split to a separate patch.

> > > > +	format = v4l2_subdev_get_pad_format(sd, sd_state, 0);
> > > > +	crop = v4l2_subdev_get_pad_crop(sd, sd_state, 0);
> > > > +
> > > > +	*format = fmt->format;
> > > > +	*crop = mode->crop;
> > > >
> > > >  	if (fmt->which == V4L2_SUBDEV_FORMAT_ACTIVE) {
> > > >  		imx219->mode = mode;
> > > > @@ -788,8 +791,6 @@ static int imx219_set_pad_format(struct v4l2_subdev *sd,
> > > >  					 hblank);
> > > >  	}
> > > >
> > > > -	*format = fmt->format;
> > > > -
> > > >  	return 0;
> > > >  }
> > > >

-- 
Regards,

Laurent Pinchart