diff mbox series

[v2,3/5] media: i2c: imx319: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips

Message ID 20221205152149.1364185-4-dave.stevenson@raspberrypi.com (mailing list archive)
State Accepted
Delegated to: Sakari Ailus
Headers
Series Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips |

Commit Message

Dave Stevenson Dec. 5, 2022, 3:21 p.m. UTC 
  The driver changes the Bayer order based on the flips, but
does not define the control correctly with the
V4L2_CTRL_FLAG_MODIFY_LAYOUT flag.

Add the V4L2_CTRL_FLAG_MODIFY_LAYOUT flag.

Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
---
 drivers/media/i2c/imx319.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Cao, Bingbu Dec. 6, 2022, 3:42 a.m. UTC | #1 
Stevenson, 

Thanks for your patch.

I am wondering how V4L2_CTRL_FLAG_MODIFY_LAYOUT flag was used in current
v4l2 ctrl framework, literally it means the v4l2 ctrl will change the buffer
layout. From my understanding, such as 90 degrees rotation apparently change
the layout. But I am not sure this is also the case for vflip/hflip, user can
notice the bayer order update from get_fmt.

Sakari, what do you think?
Dave Stevenson Dec. 6, 2022, 11:39 a.m. UTC | #2 
Hi

On Tue, 6 Dec 2022 at 03:42, Cao, Bingbu <bingbu.cao@intel.com> wrote:
>
> Stevenson,
>
> Thanks for your patch.
>
> I am wondering how V4L2_CTRL_FLAG_MODIFY_LAYOUT flag was used in current
> v4l2 ctrl framework, literally it means the v4l2 ctrl will change the buffer
> layout. From my understanding, such as 90 degrees rotation apparently change
> the layout. But I am not sure this is also the case for vflip/hflip, user can
> notice the bayer order update from get_fmt.

Documentation at [1]
"3.5.1. Interactions between formats, controls and buffers
...
The set of information needed to interpret the content of a buffer
(e.g. the pixel format, the line stride, the tiling orientation or the
rotation) is collectively referred to in the rest of this section as
the buffer layout."
pixel_format is part of the buffer layout.

V4L2_CTRL_FLAG_MODIFY_LAYOUT is telling the userspace that it must
call get_fmt after changing the control in order to find out how the
format has changed. Without it there is no obligation to call get_fmt,
and userspace can legitimately expect the format/layout to be the
same.
Not all sensors change the Bayer order with flips (OnSemi sensors in
particular tend not to), so you can't make assumptions over the
behaviour.

> Sakari, what do you think?

Seeing as Sakari has accepted the patches and created a pull request
to Mauro including them suggests that this is indeed the correct thing
to do.

There is now a unified behaviour across all sensor drivers that change
Bayer order due to flips.
libcamera is relying on correct behaviour in order to be able to work
out the native Bayer order of the sensor, and that is why I was
checking that all mainline drivers were doing the right thing.

Thanks
  Dave

[1] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/buffer.html#interactions-between-formats-controls-and-buffers

> ________________________
> BRs,
> VTG - Linux&Chrome IPU SW
> Bingbu Cao
>
> > -----Original Message-----
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Sent: Monday, December 5, 2022 23:22
> > To: Rui Miguel Silva <rmfrfs@gmail.com>; Sakari Ailus
> > <sakari.ailus@linux.intel.com>; Cao, Bingbu <bingbu.cao@intel.com>; Qiu,
> > Tian Shu <tian.shu.qiu@intel.com>; Su, Jimmy <jimmy.su@intel.com>; linux-
> > media@vger.kernel.org
> > Cc: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Subject: [PATCH v2 3/5] media: i2c: imx319: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT
> > on flips
> >
> > The driver changes the Bayer order based on the flips, but does not define
> > the control correctly with the V4L2_CTRL_FLAG_MODIFY_LAYOUT flag.
> >
> > Add the V4L2_CTRL_FLAG_MODIFY_LAYOUT flag.
> >
> > Signed-off-by: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > ---
> >  drivers/media/i2c/imx319.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c index
> > 245a18fb40ad..45b1b61b2880 100644
> > --- a/drivers/media/i2c/imx319.c
> > +++ b/drivers/media/i2c/imx319.c
> > @@ -2328,8 +2328,12 @@ static int imx319_init_controls(struct imx319 *imx319)
> >
> >       imx319->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
> >                                         V4L2_CID_HFLIP, 0, 1, 1, 0);
> > +     if (imx319->hflip)
> > +             imx319->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >       imx319->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
> >                                         V4L2_CID_VFLIP, 0, 1, 1, 0);
> > +     if (imx319->vflip)
> > +             imx319->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> >
> >       v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
> >                         IMX319_ANA_GAIN_MIN, IMX319_ANA_GAIN_MAX,
> > --
> > 2.34.1
>
Cao, Bingbu Dec. 6, 2022, 11:52 a.m. UTC | #3 
Stevenson,

Thanks for your explanation.

Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
diff mbox series

Patch

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 245a18fb40ad..45b1b61b2880 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2328,8 +2328,12 @@  static int imx319_init_controls(struct imx319 *imx319)
 
 	imx319->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
 					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (imx319->hflip)
+		imx319->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	imx319->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops,
 					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (imx319->vflip)
+		imx319->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &imx319_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
 			  IMX319_ANA_GAIN_MIN, IMX319_ANA_GAIN_MAX,