Stevenson, Thanks for your explanation. Reviewed-by: Bingbu Cao <bingbu.cao@xxxxxxxxx> ________________________ BRs, Bingbu Cao > -----Original Message----- > From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > Sent: Tuesday, December 6, 2022 19:40 > To: Cao, Bingbu <bingbu.cao@xxxxxxxxx> > Cc: Rui Miguel Silva <rmfrfs@xxxxxxxxx>; Sakari Ailus > <sakari.ailus@xxxxxxxxxxxxxxx>; Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>; Su, > Jimmy <jimmy.su@xxxxxxxxx>; linux-media@xxxxxxxxxxxxxxx > Subject: Re: [PATCH v2 3/5] media: i2c: imx319: Set > V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips > > Hi > > On Tue, 6 Dec 2022 at 03:42, Cao, Bingbu <bingbu.cao@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> > > > Sent: Monday, December 5, 2022 23:22 > > > To: Rui Miguel Silva <rmfrfs@xxxxxxxxx>; Sakari Ailus > > > <sakari.ailus@xxxxxxxxxxxxxxx>; Cao, Bingbu <bingbu.cao@xxxxxxxxx>; > > > Qiu, Tian Shu <tian.shu.qiu@xxxxxxxxx>; Su, Jimmy > > > <jimmy.su@xxxxxxxxx>; linux- media@xxxxxxxxxxxxxxx > > > Cc: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx> > > > 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@xxxxxxxxxxxxxxx> > > > --- > > > 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 > >
