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@xxxxxxxxxxxxxxxx>
> > > > ---
> > > > 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
