[PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Jacopo Mondi]

Currently the imx258 driver requires to have the 'rotation' device node
property specified in DTS with a fixed value of 180 degrees.

The "rotation" fwnode device property is intended to allow specify the
sensor's physical mounting rotation, so that it can be exposed through
the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
can decide how to compensate for that.

The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
a 180 degrees image rotation being produced by the sensor. But this
doesn't imply that the physical mounting rotation should match the
driver's implementation.

I took into the series Robert's patch that register device node properties and
on top of that register flips controls, in order to remove the hard requirement
of the 180 degrees rotation property presence.

Jacopo Mondi (2):
  media: imx258: Register H/V flip controls
  media: imx258: Remove mandatory 180 degrees rotation

Robert Mader (1):
  media: imx258: Parse and register properties

 drivers/media/i2c/imx258.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

--
2.39.0

[PATCH 1/3] media: imx258: Parse and register properties

[Jacopo Mondi]

From: Robert Mader <robert.mader@collabora.com>

Analogous to e.g. the imx219. This enables propagating
V4L2_CID_CAMERA_ORIENTATION and V4L2_CID_CAMERA_SENSOR_ROTATION
values.
The motivation is to allow libcamera detect these values from the
device tree and propagate them further to e.g. Pipewire.

While at it, reserve space for 3 additional controls even if
v4l2_ctrl_new_fwnode_properties() can only register 2 of
them, to fix the existing implementation which reserve space for 8
controls but actually registers 9.

Signed-off-by: Robert Mader <robert.mader@collabora.com>
Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

----
Changes in v2:
 - Reserve 11 instead of 10 controls
 - Change order of variable declaration
 - Slightly extend description
---
 drivers/media/i2c/imx258.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index eab5fc1ee2f7..3b560865b657 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -9,6 +9,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 #include <asm/unaligned.h>
 
 #define IMX258_REG_VALUE_08BIT		1
@@ -1148,6 +1149,7 @@ static const struct v4l2_subdev_internal_ops imx258_internal_ops = {
 static int imx258_init_controls(struct imx258 *imx258)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
+	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 vblank_def;
 	s64 vblank_min;
@@ -1156,7 +1158,7 @@ static int imx258_init_controls(struct imx258 *imx258)
 	int ret;
 
 	ctrl_hdlr = &imx258->ctrl_handler;
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
 	if (ret)
 		return ret;
 
@@ -1232,6 +1234,15 @@ static int imx258_init_controls(struct imx258 *imx258)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_device_parse(&client->dev, &props);
+	if (ret)
+		goto error;
+
+	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx258_ctrl_ops,
+					      &props);
+	if (ret)
+		goto error;
+
 	imx258->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.39.0

Re: [PATCH 1/3] media: imx258: Parse and register properties

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 17, 2023 at 11:06:01AM +0100, Jacopo Mondi wrote:
> From: Robert Mader <robert.mader@collabora.com>
> 
> Analogous to e.g. the imx219. This enables propagating
> V4L2_CID_CAMERA_ORIENTATION and V4L2_CID_CAMERA_SENSOR_ROTATION
> values.
> The motivation is to allow libcamera detect these values from the

s/detect/to detect/

> device tree and propagate them further to e.g. Pipewire.
> 
> While at it, reserve space for 3 additional controls even if
> v4l2_ctrl_new_fwnode_properties() can only register 2 of
> them, to fix the existing implementation which reserve space for 8
> controls but actually registers 9.
> 
> Signed-off-by: Robert Mader <robert.mader@collabora.com>
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ----
> Changes in v2:
>  - Reserve 11 instead of 10 controls
>  - Change order of variable declaration
>  - Slightly extend description
> ---
>  drivers/media/i2c/imx258.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index eab5fc1ee2f7..3b560865b657 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -9,6 +9,7 @@
>  #include <linux/pm_runtime.h>
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
> +#include <media/v4l2-fwnode.h>
>  #include <asm/unaligned.h>
>  
>  #define IMX258_REG_VALUE_08BIT		1
> @@ -1148,6 +1149,7 @@ static const struct v4l2_subdev_internal_ops imx258_internal_ops = {
>  static int imx258_init_controls(struct imx258 *imx258)
>  {
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> +	struct v4l2_fwnode_device_properties props;
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
>  	s64 vblank_def;
>  	s64 vblank_min;
> @@ -1156,7 +1158,7 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	int ret;
>  
>  	ctrl_hdlr = &imx258->ctrl_handler;
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 8);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
>  	if (ret)
>  		return ret;
>  
> @@ -1232,6 +1234,15 @@ static int imx258_init_controls(struct imx258 *imx258)
>  		goto error;
>  	}
>  
> +	ret = v4l2_fwnode_device_parse(&client->dev, &props);
> +	if (ret)
> +		goto error;
> +
> +	ret = v4l2_ctrl_new_fwnode_properties(ctrl_hdlr, &imx258_ctrl_ops,
> +					      &props);
> +	if (ret)
> +		goto error;
> +
>  	imx258->sd.ctrl_handler = ctrl_hdlr;
>  
>  	return 0;

-- 
Regards,

Laurent Pinchart

[PATCH 2/3] media: imx258: Register H/V flip controls

[Jacopo Mondi]

Register controls for V4L2_CID_HFLIP and V4L2_CID_VFLIP.

The controls are registered as read-only and enabled by default, as the
driver embeds a 180 degrees rotation in its programming sequences and
only supports that mode of operations.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx258.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 3b560865b657..2e0a4ea76589 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -1151,6 +1151,7 @@ static int imx258_init_controls(struct imx258 *imx258)
 	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
 	struct v4l2_fwnode_device_properties props;
 	struct v4l2_ctrl_handler *ctrl_hdlr;
+	struct v4l2_ctrl *vflip, *hflip;
 	s64 vblank_def;
 	s64 vblank_min;
 	s64 pixel_rate_min;
@@ -1158,7 +1159,7 @@ static int imx258_init_controls(struct imx258 *imx258)
 	int ret;
 
 	ctrl_hdlr = &imx258->ctrl_handler;
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
 	if (ret)
 		return ret;
 
@@ -1174,6 +1175,17 @@ static int imx258_init_controls(struct imx258 *imx258)
 	if (imx258->link_freq)
 		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
+	/* The driver only supports one bayer order and flips by default. */
+	hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
+				  V4L2_CID_HFLIP, 1, 1, 1, 1);
+	if (hflip)
+		hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
+	vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
+				  V4L2_CID_VFLIP, 1, 1, 1, 1);
+	if (vflip)
+		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
+
 	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
 	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
 	/* By default, PIXEL_RATE is read only */
-- 
2.39.0

Re: [PATCH 2/3] media: imx258: Register H/V flip controls

[Sakari Ailus]

Hi Jacopo,

Thanks for the patch.

On Tue, Jan 17, 2023 at 11:06:02AM +0100, Jacopo Mondi wrote:
> Register controls for V4L2_CID_HFLIP and V4L2_CID_VFLIP.
> 
> The controls are registered as read-only and enabled by default, as the
> driver embeds a 180 degrees rotation in its programming sequences and
> only supports that mode of operations.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> ---
>  drivers/media/i2c/imx258.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 3b560865b657..2e0a4ea76589 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -1151,6 +1151,7 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
>  	struct v4l2_fwnode_device_properties props;
>  	struct v4l2_ctrl_handler *ctrl_hdlr;
> +	struct v4l2_ctrl *vflip, *hflip;
>  	s64 vblank_def;
>  	s64 vblank_min;
>  	s64 pixel_rate_min;
> @@ -1158,7 +1159,7 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	int ret;
>  
>  	ctrl_hdlr = &imx258->ctrl_handler;
> -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
>  	if (ret)
>  		return ret;
>  
> @@ -1174,6 +1175,17 @@ static int imx258_init_controls(struct imx258 *imx258)
>  	if (imx258->link_freq)
>  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> +	/* The driver only supports one bayer order and flips by default. */
> +	hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> +				  V4L2_CID_HFLIP, 1, 1, 1, 1);
> +	if (hflip)
> +		hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
> +	vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> +				  V4L2_CID_VFLIP, 1, 1, 1, 1);

This defaults the controls to 1, which suggests the image is upside down.

The rotation property has been used to tell the driver the sensor is upside
down, and this has also had the effect of reversing flip contron values so
if they're disabled, the image is upright.

See e.g. the CCS driver.

I'd do the same here.

> +	if (vflip)
> +		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> +
>  	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
>  	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
>  	/* By default, PIXEL_RATE is read only */

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] media: imx258: Register H/V flip controls

[Jacopo Mondi]

Hi Sakari

On Tue, Jan 17, 2023 at 10:17:13AM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the patch.
>
> On Tue, Jan 17, 2023 at 11:06:02AM +0100, Jacopo Mondi wrote:
> > Register controls for V4L2_CID_HFLIP and V4L2_CID_VFLIP.
> >
> > The controls are registered as read-only and enabled by default, as the
> > driver embeds a 180 degrees rotation in its programming sequences and
> > only supports that mode of operations.
> >
> > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > ---
> >  drivers/media/i2c/imx258.c | 14 +++++++++++++-
> >  1 file changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index 3b560865b657..2e0a4ea76589 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -1151,6 +1151,7 @@ static int imx258_init_controls(struct imx258 *imx258)
> >  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> >  	struct v4l2_fwnode_device_properties props;
> >  	struct v4l2_ctrl_handler *ctrl_hdlr;
> > +	struct v4l2_ctrl *vflip, *hflip;
> >  	s64 vblank_def;
> >  	s64 vblank_min;
> >  	s64 pixel_rate_min;
> > @@ -1158,7 +1159,7 @@ static int imx258_init_controls(struct imx258 *imx258)
> >  	int ret;
> >
> >  	ctrl_hdlr = &imx258->ctrl_handler;
> > -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
> >  	if (ret)
> >  		return ret;
> >
> > @@ -1174,6 +1175,17 @@ static int imx258_init_controls(struct imx258 *imx258)
> >  	if (imx258->link_freq)
> >  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> >
> > +	/* The driver only supports one bayer order and flips by default. */
> > +	hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > +				  V4L2_CID_HFLIP, 1, 1, 1, 1);
> > +	if (hflip)
> > +		hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> > +	vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > +				  V4L2_CID_VFLIP, 1, 1, 1, 1);
>
> This defaults the controls to 1, which suggests the image is upside down.
>
> The rotation property has been used to tell the driver the sensor is upside

Well the rotation property should tell userspace how the camera is
mounted, not to the driver how to compensate it ? There are several
reasons not to automatically compensate in the driver, mostly the fact
that the desired rotation to have the images in the "right"
orientation might require a transpose (as in example for 90 degrees or
270 degrees mounting rotations) which sensors cannot actually do.

I would rather inform userspace of the mounting rotation and the
sensor capabilities and have them decide how to compensate it.

> down, and this has also had the effect of reversing flip contron values so
> if they're disabled, the image is upright.

Ok, but why should the driver only accept a mounting rotation of 180
degrees ? If your sensor is mounted on a mobile device (as the
PinephonPro where this sensor is used) the phyisical mounting rotation
is actually 270 degrees.

Userspace will see that flips are not writable, the rotation to
compensate is 270 degrees, and will handle that as it like the most.

> See e.g. the CCS driver.
>
> I'd do the same here.
>
> > +	if (vflip)
> > +		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > +
> >  	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> >  	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> >  	/* By default, PIXEL_RATE is read only */
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 2/3] media: imx258: Register H/V flip controls

[Laurent Pinchart]

Hello,

On Tue, Jan 17, 2023 at 11:28:39AM +0100, Jacopo Mondi wrote:
> On Tue, Jan 17, 2023 at 10:17:13AM +0000, Sakari Ailus wrote:
> > On Tue, Jan 17, 2023 at 11:06:02AM +0100, Jacopo Mondi wrote:
> > > Register controls for V4L2_CID_HFLIP and V4L2_CID_VFLIP.
> > >
> > > The controls are registered as read-only and enabled by default, as the
> > > driver embeds a 180 degrees rotation in its programming sequences and
> > > only supports that mode of operations.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> > > ---
> > >  drivers/media/i2c/imx258.c | 14 +++++++++++++-
> > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > > index 3b560865b657..2e0a4ea76589 100644
> > > --- a/drivers/media/i2c/imx258.c
> > > +++ b/drivers/media/i2c/imx258.c
> > > @@ -1151,6 +1151,7 @@ static int imx258_init_controls(struct imx258 *imx258)
> > >  	struct i2c_client *client = v4l2_get_subdevdata(&imx258->sd);
> > >  	struct v4l2_fwnode_device_properties props;
> > >  	struct v4l2_ctrl_handler *ctrl_hdlr;
> > > +	struct v4l2_ctrl *vflip, *hflip;
> > >  	s64 vblank_def;
> > >  	s64 vblank_min;
> > >  	s64 pixel_rate_min;
> > > @@ -1158,7 +1159,7 @@ static int imx258_init_controls(struct imx258 *imx258)
> > >  	int ret;
> > >
> > >  	ctrl_hdlr = &imx258->ctrl_handler;
> > > -	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 11);
> > > +	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 13);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > @@ -1174,6 +1175,17 @@ static int imx258_init_controls(struct imx258 *imx258)
> > >  	if (imx258->link_freq)
> > >  		imx258->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > >
> > > +	/* The driver only supports one bayer order and flips by default. */
> > > +	hflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > > +				  V4L2_CID_HFLIP, 1, 1, 1, 1);
> > > +	if (hflip)
> > > +		hflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > > +	vflip = v4l2_ctrl_new_std(ctrl_hdlr, &imx258_ctrl_ops,
> > > +				  V4L2_CID_VFLIP, 1, 1, 1, 1);
> >
> > This defaults the controls to 1, which suggests the image is upside down.
> >
> > The rotation property has been used to tell the driver the sensor is upside
> 
> Well the rotation property should tell userspace how the camera is
> mounted, not to the driver how to compensate it ? There are several
> reasons not to automatically compensate in the driver, mostly the fact
> that the desired rotation to have the images in the "right"
> orientation might require a transpose (as in example for 90 degrees or
> 270 degrees mounting rotations) which sensors cannot actually do.
> 
> I would rather inform userspace of the mounting rotation and the
> sensor capabilities and have them decide how to compensate it.
> 
> > down, and this has also had the effect of reversing flip contron values so
> > if they're disabled, the image is upright.
> 
> Ok, but why should the driver only accept a mounting rotation of 180
> degrees ? If your sensor is mounted on a mobile device (as the
> PinephonPro where this sensor is used) the phyisical mounting rotation
> is actually 270 degrees.
> 
> Userspace will see that flips are not writable, the rotation to
> compensate is 270 degrees, and will handle that as it like the most.

This sounds good to me. The alternative would be to avoid exposing the
flip controls to userspace, and add 180 to the rotation reported in DT
before passing it to userspace. I don't think that's the right option,
drivers shouldn't try to hide this.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> > See e.g. the CCS driver.
> >
> > I'd do the same here.
> >
> > > +	if (vflip)
> > > +		vflip->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> > > +
> > >  	pixel_rate_max = link_freq_to_pixel_rate(link_freq_menu_items[0]);
> > >  	pixel_rate_min = link_freq_to_pixel_rate(link_freq_menu_items[1]);
> > >  	/* By default, PIXEL_RATE is read only */

-- 
Regards,

Laurent Pinchart

[PATCH 3/3] media: imx258: Remove mandatory 180 degrees rotation

[Jacopo Mondi]

The "rotation" fwnode device property is intended to allow specify the
sensor's physical mounting rotation, so that it can be exposed through
the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
can decide how to compensate for that.

The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
a 180 degrees image rotation being produced by the sensor. But this
doesn't imply that the physical mounting rotation should match the
driver's implementation.

Now that the driver registers V4L2_CID_CAMERA_SENSOR_ROTATION
and V4L2_CID_HFLIP/VFLIP correctly, userspace has all the required
information to handle the rotation correctly, hence it is not necessary
to require the 'rotation' property to be fixed to 180 degrees in DTS.

Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
---
 drivers/media/i2c/imx258.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
index 2e0a4ea76589..85d73b186111 100644
--- a/drivers/media/i2c/imx258.c
+++ b/drivers/media/i2c/imx258.c
@@ -1299,14 +1299,6 @@ static int imx258_probe(struct i2c_client *client)
 		return -EINVAL;
 	}
 
-	/*
-	 * Check that the device is mounted upside down. The driver only
-	 * supports a single pixel order right now.
-	 */
-	ret = device_property_read_u32(&client->dev, "rotation", &val);
-	if (ret || val != 180)
-		return -EINVAL;
-
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
 
-- 
2.39.0

Re: [PATCH 3/3] media: imx258: Remove mandatory 180 degrees rotation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Jan 17, 2023 at 11:06:03AM +0100, Jacopo Mondi wrote:
> The "rotation" fwnode device property is intended to allow specify the

s/specify/specifying/

> sensor's physical mounting rotation, so that it can be exposed through
> the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> can decide how to compensate for that.
> 
> The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> a 180 degrees image rotation being produced by the sensor. But this
> doesn't imply that the physical mounting rotation should match the
> driver's implementation.
> 
> Now that the driver registers V4L2_CID_CAMERA_SENSOR_ROTATION
> and V4L2_CID_HFLIP/VFLIP correctly, userspace has all the required
> information to handle the rotation correctly, hence it is not necessary
> to require the 'rotation' property to be fixed to 180 degrees in DTS.
> 
> Signed-off-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/i2c/imx258.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> index 2e0a4ea76589..85d73b186111 100644
> --- a/drivers/media/i2c/imx258.c
> +++ b/drivers/media/i2c/imx258.c
> @@ -1299,14 +1299,6 @@ static int imx258_probe(struct i2c_client *client)
>  		return -EINVAL;
>  	}
>  
> -	/*
> -	 * Check that the device is mounted upside down. The driver only
> -	 * supports a single pixel order right now.
> -	 */
> -	ret = device_property_read_u32(&client->dev, "rotation", &val);
> -	if (ret || val != 180)
> -		return -EINVAL;
> -
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&imx258->sd, client, &imx258_subdev_ops);
>  

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Sakari Ailus]

Hi Jacopo,

On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> Currently the imx258 driver requires to have the 'rotation' device node
> property specified in DTS with a fixed value of 180 degrees.
> 
> The "rotation" fwnode device property is intended to allow specify the
> sensor's physical mounting rotation, so that it can be exposed through
> the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> can decide how to compensate for that.
> 
> The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> a 180 degrees image rotation being produced by the sensor. But this
> doesn't imply that the physical mounting rotation should match the
> driver's implementation.
> 
> I took into the series Robert's patch that register device node properties and
> on top of that register flips controls, in order to remove the hard requirement
> of the 180 degrees rotation property presence.

Reconsidering these patches after the flipping vs. rotation discussion,
they seem fine. The only thing I'd like to see, after removing the rotation
property check, would be to add support for the actual flipping controls.
I'm pretty sure they can be found in the same registers as on CCS --- the
rest of the registers look very much like that. Would you like to send a
patch? :-)

-- 
Regards,

Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Dave Stevenson]

Hi Jacopo and Sakari

On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Jacopo,
>
> On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > Currently the imx258 driver requires to have the 'rotation' device node
> > property specified in DTS with a fixed value of 180 degrees.
> >
> > The "rotation" fwnode device property is intended to allow specify the
> > sensor's physical mounting rotation, so that it can be exposed through
> > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > can decide how to compensate for that.
> >
> > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > a 180 degrees image rotation being produced by the sensor. But this
> > doesn't imply that the physical mounting rotation should match the
> > driver's implementation.
> >
> > I took into the series Robert's patch that register device node properties and
> > on top of that register flips controls, in order to remove the hard requirement
> > of the 180 degrees rotation property presence.
>
> Reconsidering these patches after the flipping vs. rotation discussion,
> they seem fine. The only thing I'd like to see, after removing the rotation
> property check, would be to add support for the actual flipping controls.
> I'm pretty sure they can be found in the same registers as on CCS --- the
> rest of the registers look very much like that. Would you like to send a
> patch? :-)

Yes it is register 0x0101, bits 0 (H) & 1 (V).

Do watch out as there are register errors in the driver. Currently
Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
That means that when you initially implement flips the Bayer order
won't change, but you change the field of view by one line.
The start and end values also break the requirements listed in the
datasheets for STA/END values being multiples of X (table 4-2 of the
datasheet). Correcting that will change the Bayer order when inverted.
Does that count as a regression to userspace? I hope not. Memory says
that if you don't correct Y_ADD_END then some of the binned modes
misbehave.

I have been through this loop before as Soho Enterprise [1] make an
IMX258 board for the Pi. I haven't upstreamed the patches [2] though
(sorry).

  Dave

[1] https://soho-enterprise.com/
[2] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c

> --
> Regards,
>
> Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Laurent Pinchart]

Hi Dave,

On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> On Thu, 26 Jan 2023 at 14:52, Sakari Ailus wrote:
> > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > Currently the imx258 driver requires to have the 'rotation' device node
> > > property specified in DTS with a fixed value of 180 degrees.
> > >
> > > The "rotation" fwnode device property is intended to allow specify the
> > > sensor's physical mounting rotation, so that it can be exposed through
> > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > can decide how to compensate for that.
> > >
> > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > a 180 degrees image rotation being produced by the sensor. But this
> > > doesn't imply that the physical mounting rotation should match the
> > > driver's implementation.
> > >
> > > I took into the series Robert's patch that register device node properties and
> > > on top of that register flips controls, in order to remove the hard requirement
> > > of the 180 degrees rotation property presence.
> >
> > Reconsidering these patches after the flipping vs. rotation discussion,
> > they seem fine. The only thing I'd like to see, after removing the rotation
> > property check, would be to add support for the actual flipping controls.
> > I'm pretty sure they can be found in the same registers as on CCS --- the
> > rest of the registers look very much like that. Would you like to send a
> > patch? :-)
> 
> Yes it is register 0x0101, bits 0 (H) & 1 (V).
> 
> Do watch out as there are register errors in the driver. Currently
> Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
> That means that when you initially implement flips the Bayer order
> won't change, but you change the field of view by one line.
> The start and end values also break the requirements listed in the
> datasheets for STA/END values being multiples of X (table 4-2 of the
> datasheet). Correcting that will change the Bayer order when inverted.
> Does that count as a regression to userspace? I hope not. Memory says
> that if you don't correct Y_ADD_END then some of the binned modes
> misbehave.

As long as the driver reports the correct bayer pattern, it should be
fine.

Interactions between formats and flip controls is something we still
need to clarify. I plan to have a follow-up discussion on this with
Jacopo and Sakari after sending documentation patches for the
interactions between rotation and flips. If you would like to join the
fun, please let me know.

Also, if you have any comment on the rotation & flip discussion notes,
and especially if there's anything that doesn't seem right to you,
feedback would be appreciated. If everything is good, you can just ack
the documentation patches when I'll post them :-)

> I have been through this loop before as Soho Enterprise [1] make an
> IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> (sorry).

Thanks for sharing the branch.

> [1] https://soho-enterprise.com/
> [2] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Dave Stevenson]

Hi Laurent

On Thu, 26 Jan 2023 at 16:01, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus wrote:
> > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > > Currently the imx258 driver requires to have the 'rotation' device node
> > > > property specified in DTS with a fixed value of 180 degrees.
> > > >
> > > > The "rotation" fwnode device property is intended to allow specify the
> > > > sensor's physical mounting rotation, so that it can be exposed through
> > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > > can decide how to compensate for that.
> > > >
> > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > > a 180 degrees image rotation being produced by the sensor. But this
> > > > doesn't imply that the physical mounting rotation should match the
> > > > driver's implementation.
> > > >
> > > > I took into the series Robert's patch that register device node properties and
> > > > on top of that register flips controls, in order to remove the hard requirement
> > > > of the 180 degrees rotation property presence.
> > >
> > > Reconsidering these patches after the flipping vs. rotation discussion,
> > > they seem fine. The only thing I'd like to see, after removing the rotation
> > > property check, would be to add support for the actual flipping controls.
> > > I'm pretty sure they can be found in the same registers as on CCS --- the
> > > rest of the registers look very much like that. Would you like to send a
> > > patch? :-)
> >
> > Yes it is register 0x0101, bits 0 (H) & 1 (V).
> >
> > Do watch out as there are register errors in the driver. Currently
> > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
> > That means that when you initially implement flips the Bayer order
> > won't change, but you change the field of view by one line.
> > The start and end values also break the requirements listed in the
> > datasheets for STA/END values being multiples of X (table 4-2 of the
> > datasheet). Correcting that will change the Bayer order when inverted.
> > Does that count as a regression to userspace? I hope not. Memory says
> > that if you don't correct Y_ADD_END then some of the binned modes
> > misbehave.
>
> As long as the driver reports the correct bayer pattern, it should be
> fine.

It does report the correct Bayer order so I would hope so too, however
I know the hard coding that can go on in client apps!

> Interactions between formats and flip controls is something we still
> need to clarify. I plan to have a follow-up discussion on this with
> Jacopo and Sakari after sending documentation patches for the
> interactions between rotation and flips. If you would like to join the
> fun, please let me know.

Feel free to send me an invite for future discussions, however I'm
currently assigned to other tasks and have been told to leave image
sensors alone :-(

> Also, if you have any comment on the rotation & flip discussion notes,
> and especially if there's anything that doesn't seem right to you,
> feedback would be appreciated. If everything is good, you can just ack
> the documentation patches when I'll post them :-)

I have the luxury of being able to largely ignore the existing
clients, but the proposals as currently described seem like they
should work for all parties. I'll respond to your docs when they're
posted.

> > I have been through this loop before as Soho Enterprise [1] make an
> > IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> > (sorry).
>
> Thanks for sharing the branch.

No problem. I am trying to persuade management here that it is worth
the effort to upstream patches, but it's tough going sometimes.
I do always try to ensure that our downstream patches follow the rules
and have SoB etc, so others are at liberty to lift them if they wish.

  Dave

> > [1] https://soho-enterprise.com/
> > [2] https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Sakari Ailus]

Hi Dave,

On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> Hi Jacopo and Sakari
> 
> On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Jacopo,
> >
> > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > Currently the imx258 driver requires to have the 'rotation' device node
> > > property specified in DTS with a fixed value of 180 degrees.
> > >
> > > The "rotation" fwnode device property is intended to allow specify the
> > > sensor's physical mounting rotation, so that it can be exposed through
> > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > can decide how to compensate for that.
> > >
> > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > a 180 degrees image rotation being produced by the sensor. But this
> > > doesn't imply that the physical mounting rotation should match the
> > > driver's implementation.
> > >
> > > I took into the series Robert's patch that register device node properties and
> > > on top of that register flips controls, in order to remove the hard requirement
> > > of the 180 degrees rotation property presence.
> >
> > Reconsidering these patches after the flipping vs. rotation discussion,
> > they seem fine. The only thing I'd like to see, after removing the rotation
> > property check, would be to add support for the actual flipping controls.
> > I'm pretty sure they can be found in the same registers as on CCS --- the
> > rest of the registers look very much like that. Would you like to send a
> > patch? :-)
> 
> Yes it is register 0x0101, bits 0 (H) & 1 (V).
> 
> Do watch out as there are register errors in the driver. Currently
> Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.

Yes, this is the problem with register list based drivers. Well spotted.

I remember one driver for a Toshiba sensor using value of 5 for a register
the range of which was 2--10, but only even values were allowed. It worked
nonetheless... oh well.

I wonder if this sensor would work better with the CCS driver 

> That means that when you initially implement flips the Bayer order
> won't change, but you change the field of view by one line.
> The start and end values also break the requirements listed in the
> datasheets for STA/END values being multiples of X (table 4-2 of the
> datasheet). Correcting that will change the Bayer order when inverted.
> Does that count as a regression to userspace? I hope not. Memory says
> that if you don't correct Y_ADD_END then some of the binned modes
> misbehave.

Most sensors also require even values for the ?_ADDR_START registers (and
odd for the _ADDR_END registers). Using an invalid value sometimes might
work, too, but only testing will tell.

> 
> I have been through this loop before as Soho Enterprise [1] make an
> IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> (sorry).

It'd be nice if both worked with the same driver.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Dave Stevenson]

Hi Sakari

On Thu, 26 Jan 2023 at 16:02, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
>
> Hi Dave,
>
> On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> > Hi Jacopo and Sakari
> >
> > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Jacopo,
> > >
> > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > > Currently the imx258 driver requires to have the 'rotation' device node
> > > > property specified in DTS with a fixed value of 180 degrees.
> > > >
> > > > The "rotation" fwnode device property is intended to allow specify the
> > > > sensor's physical mounting rotation, so that it can be exposed through
> > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > > can decide how to compensate for that.
> > > >
> > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > > a 180 degrees image rotation being produced by the sensor. But this
> > > > doesn't imply that the physical mounting rotation should match the
> > > > driver's implementation.
> > > >
> > > > I took into the series Robert's patch that register device node properties and
> > > > on top of that register flips controls, in order to remove the hard requirement
> > > > of the 180 degrees rotation property presence.
> > >
> > > Reconsidering these patches after the flipping vs. rotation discussion,
> > > they seem fine. The only thing I'd like to see, after removing the rotation
> > > property check, would be to add support for the actual flipping controls.
> > > I'm pretty sure they can be found in the same registers as on CCS --- the
> > > rest of the registers look very much like that. Would you like to send a
> > > patch? :-)
> >
> > Yes it is register 0x0101, bits 0 (H) & 1 (V).
> >
> > Do watch out as there are register errors in the driver. Currently
> > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
>
> Yes, this is the problem with register list based drivers. Well spotted.

It fell out when I flipped the image and the Bayer order didn't change.

> I remember one driver for a Toshiba sensor using value of 5 for a register
> the range of which was 2--10, but only even values were allowed. It worked
> nonetheless... oh well.
>
> I wonder if this sensor would work better with the CCS driver

That's a question I'll leave to others to investigate :-)

> > That means that when you initially implement flips the Bayer order
> > won't change, but you change the field of view by one line.
> > The start and end values also break the requirements listed in the
> > datasheets for STA/END values being multiples of X (table 4-2 of the
> > datasheet). Correcting that will change the Bayer order when inverted.
> > Does that count as a regression to userspace? I hope not. Memory says
> > that if you don't correct Y_ADD_END then some of the binned modes
> > misbehave.
>
> Most sensors also require even values for the ?_ADDR_START registers (and
> odd for the _ADDR_END registers). Using an invalid value sometimes might
> work, too, but only testing will tell.
>
> >
> > I have been through this loop before as Soho Enterprise [1] make an
> > IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> > (sorry).
>
> It'd be nice if both worked with the same driver.

All my patches on that branch should work for existing users, other
than potentially the change of Bayer order if they're making
assumptions.

I have been in touch with Sony about IMX258, however they couldn't
release some information as it was confidential to some company called
Intel ;-)
The PDAF or non-PDAF version information is in the OTP, but they can't
release the details with regard to where.

  Dave

> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Jacopo Mondi]

Hello

On Thu, Jan 26, 2023 at 06:02:07PM +0200, Sakari Ailus wrote:
> Hi Dave,
>
> On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> > Hi Jacopo and Sakari
> >
> > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Jacopo,
> > >
> > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > > Currently the imx258 driver requires to have the 'rotation' device node
> > > > property specified in DTS with a fixed value of 180 degrees.
> > > >
> > > > The "rotation" fwnode device property is intended to allow specify the
> > > > sensor's physical mounting rotation, so that it can be exposed through
> > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > > can decide how to compensate for that.
> > > >
> > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > > a 180 degrees image rotation being produced by the sensor. But this
> > > > doesn't imply that the physical mounting rotation should match the
> > > > driver's implementation.
> > > >
> > > > I took into the series Robert's patch that register device node properties and
> > > > on top of that register flips controls, in order to remove the hard requirement
> > > > of the 180 degrees rotation property presence.
> > >
> > > Reconsidering these patches after the flipping vs. rotation discussion,
> > > they seem fine. The only thing I'd like to see, after removing the rotation
> > > property check, would be to add support for the actual flipping controls.
> > > I'm pretty sure they can be found in the same registers as on CCS --- the
> > > rest of the registers look very much like that. Would you like to send a
> > > patch? :-)
> >
> > Yes it is register 0x0101, bits 0 (H) & 1 (V).
> >
> > Do watch out as there are register errors in the driver. Currently
> > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
>
> Yes, this is the problem with register list based drivers. Well spotted.
>
> I remember one driver for a Toshiba sensor using value of 5 for a register
> the range of which was 2--10, but only even values were allowed. It worked
> nonetheless... oh well.
>
> I wonder if this sensor would work better with the CCS driver
>
> > That means that when you initially implement flips the Bayer order
> > won't change, but you change the field of view by one line.
> > The start and end values also break the requirements listed in the
> > datasheets for STA/END values being multiples of X (table 4-2 of the
> > datasheet). Correcting that will change the Bayer order when inverted.
> > Does that count as a regression to userspace? I hope not. Memory says
> > that if you don't correct Y_ADD_END then some of the binned modes
> > misbehave.
>
> Most sensors also require even values for the ?_ADDR_START registers (and
> odd for the _ADDR_END registers). Using an invalid value sometimes might
> work, too, but only testing will tell.
>
> >
> > I have been through this loop before as Soho Enterprise [1] make an
> > IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> > (sorry).
>
> It'd be nice if both worked with the same driver.
>

There are a lot of interesting changes in here that would be worth
upstreaming
https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c

I would prefer if we can get these three easy patches of minr in and
then start shoveling the good stuff from the rpi repo ?

Otherwise I can plumb flip support in with the current wrong totals
which, if I understand you right, doesn't require changing the bayer
patter order ?


> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Dave Stevenson]

Hi Jacopo

On Thu, 26 Jan 2023 at 17:31, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hello
>
> On Thu, Jan 26, 2023 at 06:02:07PM +0200, Sakari Ailus wrote:
> > Hi Dave,
> >
> > On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> > > Hi Jacopo and Sakari
> > >
> > > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Jacopo,
> > > >
> > > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > > > Currently the imx258 driver requires to have the 'rotation' device node
> > > > > property specified in DTS with a fixed value of 180 degrees.
> > > > >
> > > > > The "rotation" fwnode device property is intended to allow specify the
> > > > > sensor's physical mounting rotation, so that it can be exposed through
> > > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > > > can decide how to compensate for that.
> > > > >
> > > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > > > a 180 degrees image rotation being produced by the sensor. But this
> > > > > doesn't imply that the physical mounting rotation should match the
> > > > > driver's implementation.
> > > > >
> > > > > I took into the series Robert's patch that register device node properties and
> > > > > on top of that register flips controls, in order to remove the hard requirement
> > > > > of the 180 degrees rotation property presence.
> > > >
> > > > Reconsidering these patches after the flipping vs. rotation discussion,
> > > > they seem fine. The only thing I'd like to see, after removing the rotation
> > > > property check, would be to add support for the actual flipping controls.
> > > > I'm pretty sure they can be found in the same registers as on CCS --- the
> > > > rest of the registers look very much like that. Would you like to send a
> > > > patch? :-)
> > >
> > > Yes it is register 0x0101, bits 0 (H) & 1 (V).
> > >
> > > Do watch out as there are register errors in the driver. Currently
> > > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
> >
> > Yes, this is the problem with register list based drivers. Well spotted.
> >
> > I remember one driver for a Toshiba sensor using value of 5 for a register
> > the range of which was 2--10, but only even values were allowed. It worked
> > nonetheless... oh well.
> >
> > I wonder if this sensor would work better with the CCS driver
> >
> > > That means that when you initially implement flips the Bayer order
> > > won't change, but you change the field of view by one line.
> > > The start and end values also break the requirements listed in the
> > > datasheets for STA/END values being multiples of X (table 4-2 of the
> > > datasheet). Correcting that will change the Bayer order when inverted.
> > > Does that count as a regression to userspace? I hope not. Memory says
> > > that if you don't correct Y_ADD_END then some of the binned modes
> > > misbehave.
> >
> > Most sensors also require even values for the ?_ADDR_START registers (and
> > odd for the _ADDR_END registers). Using an invalid value sometimes might
> > work, too, but only testing will tell.
> >
> > >
> > > I have been through this loop before as Soho Enterprise [1] make an
> > > IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> > > (sorry).
> >
> > It'd be nice if both worked with the same driver.
> >
>
> There are a lot of interesting changes in here that would be worth
> upstreaming
> https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c

I do feel a little guilty for not upstreaming these patches myself,
but I'm currently not being given any time to do so. Perhaps I'll take
an afternoon off and blitz a load of patches (imx258, imx290, and
ov7251 for a start).

Obviously that branch is based on 5.15. I was going to say that the
rpi-6.2.y branch may be a better source, however it looks like all the
commits got squashed :-(

> I would prefer if we can get these three easy patches of minr in and
> then start shoveling the good stuff from the rpi repo ?
>
> Otherwise I can plumb flip support in with the current wrong totals
> which, if I understand you right, doesn't require changing the bayer
> patter order ?

Memory says VFLIP won't currently change Bayer order, but I think HFLIP will.
If it is permitted to merge the current read-only FLIP patches, then
I'd suggest doing that initially.

Out of interest, do you have a user of imx258, or is this just as clean ups?
It may be possible to get a sample from Soho Enterprises if you
explain your involvement in libcamera. They're generally very
approachable.

  Dave

>
> > --
> > Kind regards,
> >
> > Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Jacopo Mondi]

Hi David

On Thu, Jan 26, 2023 at 05:58:30PM +0000, Dave Stevenson wrote:
> Hi Jacopo
>
> On Thu, 26 Jan 2023 at 17:31, Jacopo Mondi
> <jacopo.mondi@ideasonboard.com> wrote:
> >
> > Hello
> >
> > On Thu, Jan 26, 2023 at 06:02:07PM +0200, Sakari Ailus wrote:
> > > Hi Dave,
> > >
> > > On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> > > > Hi Jacopo and Sakari
> > > >
> > > > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Jacopo,
> > > > >
> > > > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > > > > Currently the imx258 driver requires to have the 'rotation' device node
> > > > > > property specified in DTS with a fixed value of 180 degrees.
> > > > > >
> > > > > > The "rotation" fwnode device property is intended to allow specify the
> > > > > > sensor's physical mounting rotation, so that it can be exposed through
> > > > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > > > > can decide how to compensate for that.
> > > > > >
> > > > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > > > > a 180 degrees image rotation being produced by the sensor. But this
> > > > > > doesn't imply that the physical mounting rotation should match the
> > > > > > driver's implementation.
> > > > > >
> > > > > > I took into the series Robert's patch that register device node properties and
> > > > > > on top of that register flips controls, in order to remove the hard requirement
> > > > > > of the 180 degrees rotation property presence.
> > > > >
> > > > > Reconsidering these patches after the flipping vs. rotation discussion,
> > > > > they seem fine. The only thing I'd like to see, after removing the rotation
> > > > > property check, would be to add support for the actual flipping controls.
> > > > > I'm pretty sure they can be found in the same registers as on CCS --- the
> > > > > rest of the registers look very much like that. Would you like to send a
> > > > > patch? :-)
> > > >
> > > > Yes it is register 0x0101, bits 0 (H) & 1 (V).
> > > >
> > > > Do watch out as there are register errors in the driver. Currently
> > > > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
> > >
> > > Yes, this is the problem with register list based drivers. Well spotted.
> > >
> > > I remember one driver for a Toshiba sensor using value of 5 for a register
> > > the range of which was 2--10, but only even values were allowed. It worked
> > > nonetheless... oh well.
> > >
> > > I wonder if this sensor would work better with the CCS driver
> > >
> > > > That means that when you initially implement flips the Bayer order
> > > > won't change, but you change the field of view by one line.
> > > > The start and end values also break the requirements listed in the
> > > > datasheets for STA/END values being multiples of X (table 4-2 of the
> > > > datasheet). Correcting that will change the Bayer order when inverted.
> > > > Does that count as a regression to userspace? I hope not. Memory says
> > > > that if you don't correct Y_ADD_END then some of the binned modes
> > > > misbehave.
> > >
> > > Most sensors also require even values for the ?_ADDR_START registers (and
> > > odd for the _ADDR_END registers). Using an invalid value sometimes might
> > > work, too, but only testing will tell.
> > >
> > > >
> > > > I have been through this loop before as Soho Enterprise [1] make an
> > > > IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> > > > (sorry).
> > >
> > > It'd be nice if both worked with the same driver.
> > >
> >
> > There are a lot of interesting changes in here that would be worth
> > upstreaming
> > https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c
>
> I do feel a little guilty for not upstreaming these patches myself,
> but I'm currently not being given any time to do so. Perhaps I'll take
> an afternoon off and blitz a load of patches (imx258, imx290, and
> ov7251 for a start).
>
> Obviously that branch is based on 5.15. I was going to say that the
> rpi-6.2.y branch may be a better source, however it looks like all the
> commits got squashed :-(
>
> > I would prefer if we can get these three easy patches of minr in and
> > then start shoveling the good stuff from the rpi repo ?
> >
> > Otherwise I can plumb flip support in with the current wrong totals
> > which, if I understand you right, doesn't require changing the bayer
> > patter order ?
>
> Memory says VFLIP won't currently change Bayer order, but I think HFLIP will.
> If it is permitted to merge the current read-only FLIP patches, then
> I'd suggest doing that initially.
>
> Out of interest, do you have a user of imx258, or is this just as clean ups?
> It may be possible to get a sample from Soho Enterprises if you
> explain your involvement in libcamera. They're generally very
> approachable.
>

Thanks for the hint.

I'm using the imx258 on the PinephonePro, so I have a testing device.

I don't see a tuning file for the sensor in libcamera, does Soho
Enterprise ever published one ?

>   Dave
>
> >
> > > --
> > > Kind regards,
> > >
> > > Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Dave Stevenson]

Hi Jacopo

On Thu, 26 Jan 2023 at 20:03, Jacopo Mondi
<jacopo.mondi@ideasonboard.com> wrote:
>
> Hi David
>
> On Thu, Jan 26, 2023 at 05:58:30PM +0000, Dave Stevenson wrote:
> > Hi Jacopo
> >
> > On Thu, 26 Jan 2023 at 17:31, Jacopo Mondi
> > <jacopo.mondi@ideasonboard.com> wrote:
> > >
> > > Hello
> > >
> > > On Thu, Jan 26, 2023 at 06:02:07PM +0200, Sakari Ailus wrote:
> > > > Hi Dave,
> > > >
> > > > On Thu, Jan 26, 2023 at 03:48:04PM +0000, Dave Stevenson wrote:
> > > > > Hi Jacopo and Sakari
> > > > >
> > > > > On Thu, 26 Jan 2023 at 14:52, Sakari Ailus <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> > > > > > > Currently the imx258 driver requires to have the 'rotation' device node
> > > > > > > property specified in DTS with a fixed value of 180 degrees.
> > > > > > >
> > > > > > > The "rotation" fwnode device property is intended to allow specify the
> > > > > > > sensor's physical mounting rotation, so that it can be exposed through
> > > > > > > the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> > > > > > > can decide how to compensate for that.
> > > > > > >
> > > > > > > The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> > > > > > > a 180 degrees image rotation being produced by the sensor. But this
> > > > > > > doesn't imply that the physical mounting rotation should match the
> > > > > > > driver's implementation.
> > > > > > >
> > > > > > > I took into the series Robert's patch that register device node properties and
> > > > > > > on top of that register flips controls, in order to remove the hard requirement
> > > > > > > of the 180 degrees rotation property presence.
> > > > > >
> > > > > > Reconsidering these patches after the flipping vs. rotation discussion,
> > > > > > they seem fine. The only thing I'd like to see, after removing the rotation
> > > > > > property check, would be to add support for the actual flipping controls.
> > > > > > I'm pretty sure they can be found in the same registers as on CCS --- the
> > > > > > rest of the registers look very much like that. Would you like to send a
> > > > > > patch? :-)
> > > > >
> > > > > Yes it is register 0x0101, bits 0 (H) & 1 (V).
> > > > >
> > > > > Do watch out as there are register errors in the driver. Currently
> > > > > Y_ADD_STA is set to 0, and Y_ADD_END to 3118, giving 3119 lines total.
> > > >
> > > > Yes, this is the problem with register list based drivers. Well spotted.
> > > >
> > > > I remember one driver for a Toshiba sensor using value of 5 for a register
> > > > the range of which was 2--10, but only even values were allowed. It worked
> > > > nonetheless... oh well.
> > > >
> > > > I wonder if this sensor would work better with the CCS driver
> > > >
> > > > > That means that when you initially implement flips the Bayer order
> > > > > won't change, but you change the field of view by one line.
> > > > > The start and end values also break the requirements listed in the
> > > > > datasheets for STA/END values being multiples of X (table 4-2 of the
> > > > > datasheet). Correcting that will change the Bayer order when inverted.
> > > > > Does that count as a regression to userspace? I hope not. Memory says
> > > > > that if you don't correct Y_ADD_END then some of the binned modes
> > > > > misbehave.
> > > >
> > > > Most sensors also require even values for the ?_ADDR_START registers (and
> > > > odd for the _ADDR_END registers). Using an invalid value sometimes might
> > > > work, too, but only testing will tell.
> > > >
> > > > >
> > > > > I have been through this loop before as Soho Enterprise [1] make an
> > > > > IMX258 board for the Pi. I haven't upstreamed the patches [2] though
> > > > > (sorry).
> > > >
> > > > It'd be nice if both worked with the same driver.
> > > >
> > >
> > > There are a lot of interesting changes in here that would be worth
> > > upstreaming
> > > https://github.com/raspberrypi/linux/commits/rpi-5.15.y/drivers/media/i2c/imx258.c
> >
> > I do feel a little guilty for not upstreaming these patches myself,
> > but I'm currently not being given any time to do so. Perhaps I'll take
> > an afternoon off and blitz a load of patches (imx258, imx290, and
> > ov7251 for a start).
> >
> > Obviously that branch is based on 5.15. I was going to say that the
> > rpi-6.2.y branch may be a better source, however it looks like all the
> > commits got squashed :-(
> >
> > > I would prefer if we can get these three easy patches of minr in and
> > > then start shoveling the good stuff from the rpi repo ?
> > >
> > > Otherwise I can plumb flip support in with the current wrong totals
> > > which, if I understand you right, doesn't require changing the bayer
> > > patter order ?
> >
> > Memory says VFLIP won't currently change Bayer order, but I think HFLIP will.
> > If it is permitted to merge the current read-only FLIP patches, then
> > I'd suggest doing that initially.
> >
> > Out of interest, do you have a user of imx258, or is this just as clean ups?
> > It may be possible to get a sample from Soho Enterprises if you
> > explain your involvement in libcamera. They're generally very
> > approachable.
> >
>
> Thanks for the hint.
>
> I'm using the imx258 on the PinephonePro, so I have a testing device.
>
> I don't see a tuning file for the sensor in libcamera, does Soho
> Enterprise ever published one ?

I don't recall having seen a final tuning file for imx258, but it
would be for the Pi IPA anyway which would make it less applicable for
the PinephonePro.
They've been making a number of modules, and I need to encourage them
to upstream their drivers and tunings. Their main guy is ex-Sony, so
he's fairly hot on image quality and tuning, but has turned to me for
a moderate amount of the kernel side detail.

It looks like https://forums.raspberrypi.com/viewtopic.php?t=331819
was our forum thread discussing imx258. The incorrect register
settings were giving white lines on the edges of the images,
particularly on the binned modes.
I have no information on HDR or PDAF for imx258, although I do know
both are different from IMX708. Also note that PDAF is optional in the
module, so shielded pixel correction needs to be correctly set for the
module.

Feel free to shout if you need me to test any driver changes, although
I'll keep an eye out for any imx258 patches anyway.

  Dave

> >   Dave
> >
> > >
> > > > --
> > > > Kind regards,
> > > >
> > > > Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Jacopo Mondi]

Hi Sakari

   I don't see this patch being collected for 6.3 while I recall that
based on the discussion we concluded we can have these in and the
iterate on top ?

Thanks
  j

On Tue, Jan 17, 2023 at 11:06:00AM +0100, Jacopo Mondi wrote:
> Currently the imx258 driver requires to have the 'rotation' device node
> property specified in DTS with a fixed value of 180 degrees.
>
> The "rotation" fwnode device property is intended to allow specify the
> sensor's physical mounting rotation, so that it can be exposed through
> the read-only V4L2_CID_CAMERA_SENSOR_ROTATION control and applications
> can decide how to compensate for that.
>
> The imx258 driver has read-only VFLIP and HFLIP enabled, resulting in
> a 180 degrees image rotation being produced by the sensor. But this
> doesn't imply that the physical mounting rotation should match the
> driver's implementation.
>
> I took into the series Robert's patch that register device node properties and
> on top of that register flips controls, in order to remove the hard requirement
> of the 180 degrees rotation property presence.
>
> Jacopo Mondi (2):
>   media: imx258: Register H/V flip controls
>   media: imx258: Remove mandatory 180 degrees rotation
>
> Robert Mader (1):
>   media: imx258: Parse and register properties
>
>  drivers/media/i2c/imx258.c | 33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> --
> 2.39.0
>

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Sakari Ailus]

Hi Jacopo,

On Mon, Feb 27, 2023 at 06:11:47PM +0100, Jacopo Mondi wrote:
> Hi Sakari
> 
>    I don't see this patch being collected for 6.3 while I recall that
> based on the discussion we concluded we can have these in and the
> iterate on top ?

I know... I'll take it to my tree early in the next cycle (once we have
rc1).

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Robert Mader]

Just gentle reminder to pick up this series this time if not done so 
already :)

On 27.02.23 23:09, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Feb 27, 2023 at 06:11:47PM +0100, Jacopo Mondi wrote:
>> Hi Sakari
>>
>>     I don't see this patch being collected for 6.3 while I recall that
>> based on the discussion we concluded we can have these in and the
>> iterate on top ?
> I know... I'll take it to my tree early in the next cycle (once we have
> rc1).
>
-- 
Robert Mader
Consultant Software Developer

Collabora Ltd.
Platinum Building, St John's Innovation Park, Cambridge CB4 0DS, UK
Registered in England & Wales, no. 5513718

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Jacopo Mondi]

Hi Sakari,
  as Robert noted, this doesn't seem to be part of the pull request
for 6.4 ? Is it intentional ?

Thanks
  j

On Tue, Feb 28, 2023 at 12:09:24AM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Mon, Feb 27, 2023 at 06:11:47PM +0100, Jacopo Mondi wrote:
> > Hi Sakari
> >
> >    I don't see this patch being collected for 6.3 while I recall that
> > based on the discussion we concluded we can have these in and the
> > iterate on top ?
>
> I know... I'll take it to my tree early in the next cycle (once we have
> rc1).
>
> --
> Kind regards,
>
> Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Sakari Ailus]

Hi Jacopo,

On Wed, Mar 22, 2023 at 01:27:30PM +0100, Jacopo Mondi wrote:
> Hi Sakari,
>   as Robert noted, this doesn't seem to be part of the pull request
> for 6.4 ? Is it intentional ?

Uh, yes and no. These definitely should go to 6.4 but I wanted to address
the CCS driver at the same time. I'll try to post CCS patches next week but
if these don't make it, I'll just merge these nonetheless.

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 0/3] media: imx258: Remove rotation=<80 requirement

[Jacopo Mondi]

Thank you ;)

If you post CCS patches I'll try to have a look if it speeds things up

On Wed, Mar 22, 2023 at 02:30:39PM +0200, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Wed, Mar 22, 2023 at 01:27:30PM +0100, Jacopo Mondi wrote:
> > Hi Sakari,
> >   as Robert noted, this doesn't seem to be part of the pull request
> > for 6.4 ? Is it intentional ?
>
> Uh, yes and no. These definitely should go to 6.4 but I wanted to address
> the CCS driver at the same time. I'll try to post CCS patches next week but
> if these don't make it, I'll just merge these nonetheless.
>
> --
> Kind regards,
>
> Sakari Ailus