media: imx283: Report correct V4L2_SEL_TGT_CROP

[PATCH] media: imx283: Report correct V4L2_SEL_TGT_CROP

Posted by Stefan Klug 3 weeks, 4 days ago

The target crop rectangle is initialized with the crop of the default
sensor mode. This is incorrect when a different sensor mode gets
selected. Fix that by updating the crop rectangle when changing the
sensor mode.

Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
---
 drivers/media/i2c/imx283.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
index 3174d5ffd2d7..c8863c9e0ccf 100644
--- a/drivers/media/i2c/imx283.c
+++ b/drivers/media/i2c/imx283.c
@@ -1123,6 +1123,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_state *sd_state,
 				 struct v4l2_subdev_format *fmt)
 {
+	struct v4l2_rect *crop;
 	struct v4l2_mbus_framefmt *format;
 	const struct imx283_mode *mode;
 	struct imx283 *imx283 = to_imx283(sd);
@@ -1149,6 +1150,9 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
 
 	*format = fmt->format;
 
+	crop = v4l2_subdev_state_get_crop(sd_state, IMAGE_PAD);
+	*crop = mode->crop;
+
 	return 0;
 }
 
-- 
2.43.0

Re: [PATCH] media: imx283: Report correct V4L2_SEL_TGT_CROP

Posted by Umang Jain 3 weeks, 3 days ago

Hi Stefan

On 30/10/24 10:04 pm, Stefan Klug wrote:
> The target crop rectangle is initialized with the crop of the default
> sensor mode. This is incorrect when a different sensor mode gets
> selected. Fix that by updating the crop rectangle when changing the
> sensor mode.
>
> Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> ---
>   drivers/media/i2c/imx283.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> index 3174d5ffd2d7..c8863c9e0ccf 100644
> --- a/drivers/media/i2c/imx283.c
> +++ b/drivers/media/i2c/imx283.c
> @@ -1123,6 +1123,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>   				 struct v4l2_subdev_state *sd_state,
>   				 struct v4l2_subdev_format *fmt)
>   {
> +	struct v4l2_rect *crop;
>   	struct v4l2_mbus_framefmt *format;
>   	const struct imx283_mode *mode;
>   	struct imx283 *imx283 = to_imx283(sd);
> @@ -1149,6 +1150,9 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
>   
>   	*format = fmt->format;
>   
> +	crop = v4l2_subdev_state_get_crop(sd_state, IMAGE_PAD);
> +	*crop = mode->crop;
> +

One thing to note, is the crop for binning modes.

Do you need to report

     mode->crop.width / mode->hbin_ratio
     mode->crop.height / mode->vbin_ratio

for those modes?

>   	return 0;
>   }
>

Re: [PATCH] media: imx283: Report correct V4L2_SEL_TGT_CROP

Posted by Stefan Klug 3 weeks, 3 days ago

Hi Umang,

On Thu, Oct 31, 2024 at 11:07:00AM +0530, Umang Jain wrote:
> Hi Stefan
> 
> On 30/10/24 10:04 pm, Stefan Klug wrote:
> > The target crop rectangle is initialized with the crop of the default
> > sensor mode. This is incorrect when a different sensor mode gets
> > selected. Fix that by updating the crop rectangle when changing the
> > sensor mode.
> > 
> > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > ---
> >   drivers/media/i2c/imx283.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> > index 3174d5ffd2d7..c8863c9e0ccf 100644
> > --- a/drivers/media/i2c/imx283.c
> > +++ b/drivers/media/i2c/imx283.c
> > @@ -1123,6 +1123,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
> >   				 struct v4l2_subdev_state *sd_state,
> >   				 struct v4l2_subdev_format *fmt)
> >   {
> > +	struct v4l2_rect *crop;
> >   	struct v4l2_mbus_framefmt *format;
> >   	const struct imx283_mode *mode;
> >   	struct imx283 *imx283 = to_imx283(sd);
> > @@ -1149,6 +1150,9 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
> >   	*format = fmt->format;
> > +	crop = v4l2_subdev_state_get_crop(sd_state, IMAGE_PAD);
> > +	*crop = mode->crop;
> > +
> 
> One thing to note, is the crop for binning modes.
> 
> Do you need to report
> 
>     mode->crop.width / mode->hbin_ratio
>     mode->crop.height / mode->vbin_ratio
> 
> for those modes?

Good point. I was naively assuming that it has the same semantics as we
use for ScalerCrop in libcamera where it is explicitly stated that the
coordinates are in sensor pixels without binning. That has the added
advantage that we can deduce the binning factor from TGT_CROP and the
actual output size. However I couldn't find a precise specification for
that in the linux docs. 

Maybe Sakari or Laurent have a definiteve answer there?

Best regards,
Stefan

> 
> >   	return 0;
> >   }
>

Re: [PATCH] media: imx283: Report correct V4L2_SEL_TGT_CROP

Posted by Laurent Pinchart 3 weeks, 3 days ago

Hello,

On Thu, Oct 31, 2024 at 10:13:13AM +0100, Stefan Klug wrote:
> On Thu, Oct 31, 2024 at 11:07:00AM +0530, Umang Jain wrote:
> > On 30/10/24 10:04 pm, Stefan Klug wrote:
> > > The target crop rectangle is initialized with the crop of the default
> > > sensor mode. This is incorrect when a different sensor mode gets
> > > selected. Fix that by updating the crop rectangle when changing the
> > > sensor mode.
> > > 
> > > Signed-off-by: Stefan Klug <stefan.klug@ideasonboard.com>
> > > ---
> > >   drivers/media/i2c/imx283.c | 4 ++++
> > >   1 file changed, 4 insertions(+)
> > > 
> > > diff --git a/drivers/media/i2c/imx283.c b/drivers/media/i2c/imx283.c
> > > index 3174d5ffd2d7..c8863c9e0ccf 100644
> > > --- a/drivers/media/i2c/imx283.c
> > > +++ b/drivers/media/i2c/imx283.c
> > > @@ -1123,6 +1123,7 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
> > >   				 struct v4l2_subdev_state *sd_state,
> > >   				 struct v4l2_subdev_format *fmt)
> > >   {
> > > +	struct v4l2_rect *crop;
> > >   	struct v4l2_mbus_framefmt *format;
> > >   	const struct imx283_mode *mode;
> > >   	struct imx283 *imx283 = to_imx283(sd);
> > > @@ -1149,6 +1150,9 @@ static int imx283_set_pad_format(struct v4l2_subdev *sd,
> > >   	*format = fmt->format;
> > > +	crop = v4l2_subdev_state_get_crop(sd_state, IMAGE_PAD);
> > > +	*crop = mode->crop;
> > > +
> > 
> > One thing to note, is the crop for binning modes.
> > 
> > Do you need to report
> > 
> >     mode->crop.width / mode->hbin_ratio
> >     mode->crop.height / mode->vbin_ratio
> > 
> > for those modes?
> 
> Good point. I was naively assuming that it has the same semantics as we
> use for ScalerCrop in libcamera where it is explicitly stated that the
> coordinates are in sensor pixels without binning. That has the added
> advantage that we can deduce the binning factor from TGT_CROP and the
> actual output size. However I couldn't find a precise specification for
> that in the linux docs. 
> 
> Maybe Sakari or Laurent have a definiteve answer there?

This is not standardized in V4L2, and different drivers implement
different semantics. There's an ongoing effort to fix this, see
https://lore.kernel.org/r/20241011075535.588140-1-sakari.ailus@linux.intel.com.
Reviews are appreciated :-)

> > >   	return 0;
> > >   }

-- 
Regards,

Laurent Pinchart