All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips
@ 2022-12-05 15:21 Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 1/5] media: i2c: ov2680: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips Dave Stevenson
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Dave Stevenson @ 2022-12-05 15:21 UTC (permalink / raw)
  To: Rui Miguel Silva, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Jimmy Su, linux-media
  Cc: Dave Stevenson

Hi.

I was doing a basic sweep of drivers and noted that these 5 drivers change the
Bayer order based on HFLIP and VFLIP. However they don't set the
V4L2_CTRL_FLAG_MODIFY_LAYOUT flag on the controls, so userspace has no notion
that it needs to check for a changed format.

Add the flag to all the controls.

  Dave

Changes from v1:
- Corrected typo in imx355
- Moved setting the flags for ov2680 at Rui's request.


 drivers/media/i2c/imx208.c  | 4 ++++
 drivers/media/i2c/imx319.c  | 4 ++++
 drivers/media/i2c/imx355.c  | 4 ++++
 drivers/media/i2c/ov08d10.c | 5 +++++
 drivers/media/i2c/ov2680.c  | 2 ++
 5 files changed, 19 insertions(+)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/5] media: i2c: ov2680: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips Dave Stevenson
@ 2022-12-05 15:21 ` Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 2/5] media: i2c: imx208: " Dave Stevenson
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Dave Stevenson @ 2022-12-05 15:21 UTC (permalink / raw)
  To: Rui Miguel Silva, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Jimmy Su, linux-media
  Cc: Dave Stevenson

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>
Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
---
 drivers/media/i2c/ov2680.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/ov2680.c b/drivers/media/i2c/ov2680.c
index de66d3395a4d..54153bf66bdd 100644
--- a/drivers/media/i2c/ov2680.c
+++ b/drivers/media/i2c/ov2680.c
@@ -967,6 +967,8 @@ static int ov2680_v4l2_register(struct ov2680_dev *sensor)
 
 	ctrls->gain->flags |= V4L2_CTRL_FLAG_VOLATILE;
 	ctrls->exposure->flags |= V4L2_CTRL_FLAG_VOLATILE;
+	ctrls->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+	ctrls->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	v4l2_ctrl_auto_cluster(2, &ctrls->auto_gain, 0, true);
 	v4l2_ctrl_auto_cluster(2, &ctrls->auto_exp, 1, true);
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 2/5] media: i2c: imx208: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 1/5] media: i2c: ov2680: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips Dave Stevenson
@ 2022-12-05 15:21 ` Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 3/5] media: i2c: imx319: " Dave Stevenson
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Dave Stevenson @ 2022-12-05 15:21 UTC (permalink / raw)
  To: Rui Miguel Silva, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Jimmy Su, linux-media
  Cc: Dave Stevenson

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/imx208.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/imx208.c b/drivers/media/i2c/imx208.c
index a0e17bb9d4ca..64c70ebf9869 100644
--- a/drivers/media/i2c/imx208.c
+++ b/drivers/media/i2c/imx208.c
@@ -937,8 +937,12 @@ static int imx208_init_controls(struct imx208 *imx208)
 
 	imx208->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
 					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (imx208->hflip)
+		imx208->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	imx208->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops,
 					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (imx208->vflip)
+		imx208->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &imx208_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
 			  IMX208_ANA_GAIN_MIN, IMX208_ANA_GAIN_MAX,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 3/5] media: i2c: imx319: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 1/5] media: i2c: ov2680: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 2/5] media: i2c: imx208: " Dave Stevenson
@ 2022-12-05 15:21 ` Dave Stevenson
  2022-12-06  3:42   ` Cao, Bingbu
  2022-12-05 15:21 ` [PATCH v2 4/5] media: i2c: imx355: " Dave Stevenson
  2022-12-05 15:21 ` [PATCH v2 5/5] media: i2c: ov08d10: " Dave Stevenson
  4 siblings, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2022-12-05 15:21 UTC (permalink / raw)
  To: Rui Miguel Silva, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Jimmy Su, linux-media
  Cc: Dave Stevenson

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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 4/5] media: i2c: imx355: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips Dave Stevenson
                   ` (2 preceding siblings ...)
  2022-12-05 15:21 ` [PATCH v2 3/5] media: i2c: imx319: " Dave Stevenson
@ 2022-12-05 15:21 ` Dave Stevenson
  2022-12-06 11:52   ` Cao, Bingbu
  2022-12-05 15:21 ` [PATCH v2 5/5] media: i2c: ov08d10: " Dave Stevenson
  4 siblings, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2022-12-05 15:21 UTC (permalink / raw)
  To: Rui Miguel Silva, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Jimmy Su, linux-media
  Cc: Dave Stevenson

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/imx355.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c
index b46178681c05..25d4dbb6041e 100644
--- a/drivers/media/i2c/imx355.c
+++ b/drivers/media/i2c/imx355.c
@@ -1617,8 +1617,12 @@ static int imx355_init_controls(struct imx355 *imx355)
 
 	imx355->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops,
 					  V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (imx355->hflip)
+		imx355->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	imx355->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops,
 					  V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (imx355->vflip)
+		imx355->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
 			  IMX355_ANA_GAIN_MIN, IMX355_ANA_GAIN_MAX,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH v2 5/5] media: i2c: ov08d10: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips Dave Stevenson
                   ` (3 preceding siblings ...)
  2022-12-05 15:21 ` [PATCH v2 4/5] media: i2c: imx355: " Dave Stevenson
@ 2022-12-05 15:21 ` Dave Stevenson
  4 siblings, 0 replies; 10+ messages in thread
From: Dave Stevenson @ 2022-12-05 15:21 UTC (permalink / raw)
  To: Rui Miguel Silva, Sakari Ailus, Bingbu Cao, Tianshu Qiu,
	Jimmy Su, linux-media
  Cc: Dave Stevenson

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/ov08d10.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/media/i2c/ov08d10.c b/drivers/media/i2c/ov08d10.c
index c1703596c3dc..a39e086a51c5 100644
--- a/drivers/media/i2c/ov08d10.c
+++ b/drivers/media/i2c/ov08d10.c
@@ -990,8 +990,13 @@ static int ov08d10_init_controls(struct ov08d10 *ov08d10)
 
 	ov08d10->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov08d10_ctrl_ops,
 					   V4L2_CID_HFLIP, 0, 1, 1, 0);
+	if (ov08d10->hflip)
+		ov08d10->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	ov08d10->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &ov08d10_ctrl_ops,
 					   V4L2_CID_VFLIP, 0, 1, 1, 0);
+	if (ov08d10->vflip)
+		ov08d10->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
+
 	if (ctrl_hdlr->error)
 		return ctrl_hdlr->error;
 
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* RE: [PATCH v2 3/5] media: i2c: imx319: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 ` [PATCH v2 3/5] media: i2c: imx319: " Dave Stevenson
@ 2022-12-06  3:42   ` Cao, Bingbu
  2022-12-06 11:39     ` Dave Stevenson
  0 siblings, 1 reply; 10+ messages in thread
From: Cao, Bingbu @ 2022-12-06  3:42 UTC (permalink / raw)
  To: Dave Stevenson, Rui Miguel Silva, Sakari Ailus, Qiu, Tian Shu,
	Su, Jimmy, linux-media

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?

________________________
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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 3/5] media: i2c: imx319: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-06  3:42   ` Cao, Bingbu
@ 2022-12-06 11:39     ` Dave Stevenson
  2022-12-06 11:52       ` Cao, Bingbu
  0 siblings, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2022-12-06 11:39 UTC (permalink / raw)
  To: Cao, Bingbu
  Cc: Rui Miguel Silva, Sakari Ailus, Qiu, Tian Shu, Su, Jimmy, linux-media

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
>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v2 3/5] media: i2c: imx319: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-06 11:39     ` Dave Stevenson
@ 2022-12-06 11:52       ` Cao, Bingbu
  0 siblings, 0 replies; 10+ messages in thread
From: Cao, Bingbu @ 2022-12-06 11:52 UTC (permalink / raw)
  To: Dave Stevenson
  Cc: Rui Miguel Silva, Sakari Ailus, Qiu, Tian Shu, Su, Jimmy, linux-media

Stevenson,

Thanks for your explanation.

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

________________________
BRs,  
Bingbu Cao 

> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Sent: Tuesday, December 6, 2022 19:40
> To: Cao, Bingbu <bingbu.cao@intel.com>
> Cc: Rui Miguel Silva <rmfrfs@gmail.com>; Sakari Ailus
> <sakari.ailus@linux.intel.com>; Qiu, Tian Shu <tian.shu.qiu@intel.com>; Su,
> Jimmy <jimmy.su@intel.com>; linux-media@vger.kernel.org
> 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@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
> >

^ permalink raw reply	[flat|nested] 10+ messages in thread

* RE: [PATCH v2 4/5] media: i2c: imx355: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips
  2022-12-05 15:21 ` [PATCH v2 4/5] media: i2c: imx355: " Dave Stevenson
@ 2022-12-06 11:52   ` Cao, Bingbu
  0 siblings, 0 replies; 10+ messages in thread
From: Cao, Bingbu @ 2022-12-06 11:52 UTC (permalink / raw)
  To: Dave Stevenson, Rui Miguel Silva, Sakari Ailus, Qiu, Tian Shu,
	Su, Jimmy, linux-media

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

________________________
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 4/5] media: i2c: imx355: 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/imx355.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/media/i2c/imx355.c b/drivers/media/i2c/imx355.c index
> b46178681c05..25d4dbb6041e 100644
> --- a/drivers/media/i2c/imx355.c
> +++ b/drivers/media/i2c/imx355.c
> @@ -1617,8 +1617,12 @@ static int imx355_init_controls(struct imx355 *imx355)
> 
>  	imx355->hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops,
>  					  V4L2_CID_HFLIP, 0, 1, 1, 0);
> +	if (imx355->hflip)
> +		imx355->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
>  	imx355->vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops,
>  					  V4L2_CID_VFLIP, 0, 1, 1, 0);
> +	if (imx355->vflip)
> +		imx355->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
> 
>  	v4l2_ctrl_new_std(ctrl_hdlr, &imx355_ctrl_ops, V4L2_CID_ANALOGUE_GAIN,
>  			  IMX355_ANA_GAIN_MIN, IMX355_ANA_GAIN_MAX,
> --
> 2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-12-06 11:54 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-05 15:21 [PATCH V2 0/5] Ensure sensor drivers set V4L2_CTRL_FLAG_MODIFY_LAYOUT for flips Dave Stevenson
2022-12-05 15:21 ` [PATCH v2 1/5] media: i2c: ov2680: Set V4L2_CTRL_FLAG_MODIFY_LAYOUT on flips Dave Stevenson
2022-12-05 15:21 ` [PATCH v2 2/5] media: i2c: imx208: " Dave Stevenson
2022-12-05 15:21 ` [PATCH v2 3/5] media: i2c: imx319: " Dave Stevenson
2022-12-06  3:42   ` Cao, Bingbu
2022-12-06 11:39     ` Dave Stevenson
2022-12-06 11:52       ` Cao, Bingbu
2022-12-05 15:21 ` [PATCH v2 4/5] media: i2c: imx355: " Dave Stevenson
2022-12-06 11:52   ` Cao, Bingbu
2022-12-05 15:21 ` [PATCH v2 5/5] media: i2c: ov08d10: " Dave Stevenson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.