| Message ID | 20221205152149.1364185-4-dave.stevenson@raspberrypi.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips | expand |
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?
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 >
Stevenson,
Thanks for your explanation.
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
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,
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(+)