[libcamera-devel,00/12] raspberrypi: Report sensor orientation through DT
mbox series

Message ID 20200704004028.21153-1-laurent.pinchart@ideasonboard.com
Headers show
Series
  • raspberrypi: Report sensor orientation through DT
Related show

Message

Laurent Pinchart July 4, 2020, 12:40 a.m. UTC
Hi Dave,

This patch series reports sensor orientation through DT for the OV5647,
IMX219 and IMX477. The first 8 patches are backported from mainline,
while the last 4 patches are new. Patch 09/12 could possibly be skipped
for now, as we don't use the rotation property in the ov5647 DT overlay.

The patches are based on top of rpi-5.4.y. libcamera patches will follow
shortly.

Jacopo Mondi (8):
  media: dt-bindings: video-interfaces: Document 'orientation' property
  media: dt-bindings: video-interface: Replace 'rotation' description
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
  media: v4l2-ctrls: Add camera orientation and rotation
  media: v4l2-fwnode: Add helper to parse device properties
  media: v4l2-ctrls: Add helper to register properties
  media: i2c: imx219: Parse and register properties

Laurent Pinchart (4):
  media: i2c: ov5647: Parse and register properties
  media: i2c: imx477: Parse and register properties
  dt/dtoverlays: imx219: Set sensor rotation
  dt/dtoverlays: imx477: Set sensor rotation

 .../bindings/media/video-interfaces.txt       | 370 +++++++++++++++++-
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 151 +++++++
 arch/arm/boot/dts/overlays/imx219-overlay.dts |   2 +
 arch/arm/boot/dts/overlays/imx477-overlay.dts |   2 +
 drivers/media/i2c/imx219.c                    |  12 +-
 drivers/media/i2c/imx477.c                    |  12 +-
 drivers/media/i2c/ov5647.c                    |  13 +-
 drivers/media/v4l2-core/v4l2-ctrls.c          |  53 +++
 drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
 include/media/v4l2-ctrls.h                    |  26 ++
 include/media/v4l2-fwnode.h                   |  47 +++
 include/uapi/linux/v4l2-controls.h            |   7 +
 12 files changed, 731 insertions(+), 6 deletions(-)

Comments

Dave Stevenson July 6, 2020, 5:33 p.m. UTC | #1
Hi Laurent.

On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> This patch series reports sensor orientation through DT for the OV5647,
> IMX219 and IMX477. The first 8 patches are backported from mainline,
> while the last 4 patches are new. Patch 09/12 could possibly be skipped
> for now, as we don't use the rotation property in the ov5647 DT overlay.
>
> The patches are based on top of rpi-5.4.y. libcamera patches will follow
> shortly.

The patches look reasonable. I'd suggest that rotation really ought to
be a dtoverlay property rather than hard coded. Particularly with
IMX219 there is no defined "right way" of mounting it. IMX477 with the
tripod screw-hole does have a natural orientation.
Are you expecting those to be merged into our kernel tree? We normally
use Github pull requests rather than mailing lists.

  Dave

> Jacopo Mondi (8):
>   media: dt-bindings: video-interfaces: Document 'orientation' property
>   media: dt-bindings: video-interface: Replace 'rotation' description
>   media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION
>   media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
>   media: v4l2-ctrls: Add camera orientation and rotation
>   media: v4l2-fwnode: Add helper to parse device properties
>   media: v4l2-ctrls: Add helper to register properties
>   media: i2c: imx219: Parse and register properties
>
> Laurent Pinchart (4):
>   media: i2c: ov5647: Parse and register properties
>   media: i2c: imx477: Parse and register properties
>   dt/dtoverlays: imx219: Set sensor rotation
>   dt/dtoverlays: imx477: Set sensor rotation
>
>  .../bindings/media/video-interfaces.txt       | 370 +++++++++++++++++-
>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 151 +++++++
>  arch/arm/boot/dts/overlays/imx219-overlay.dts |   2 +
>  arch/arm/boot/dts/overlays/imx477-overlay.dts |   2 +
>  drivers/media/i2c/imx219.c                    |  12 +-
>  drivers/media/i2c/imx477.c                    |  12 +-
>  drivers/media/i2c/ov5647.c                    |  13 +-
>  drivers/media/v4l2-core/v4l2-ctrls.c          |  53 +++
>  drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
>  include/media/v4l2-ctrls.h                    |  26 ++
>  include/media/v4l2-fwnode.h                   |  47 +++
>  include/uapi/linux/v4l2-controls.h            |   7 +
>  12 files changed, 731 insertions(+), 6 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
>
Laurent Pinchart July 7, 2020, 4:47 p.m. UTC | #2
Hi Dave,

On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote:
> On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote:
> >
> > Hi Dave,
> >
> > This patch series reports sensor orientation through DT for the OV5647,
> > IMX219 and IMX477. The first 8 patches are backported from mainline,
> > while the last 4 patches are new. Patch 09/12 could possibly be skipped
> > for now, as we don't use the rotation property in the ov5647 DT overlay.
> >
> > The patches are based on top of rpi-5.4.y. libcamera patches will follow
> > shortly.
> 
> The patches look reasonable. I'd suggest that rotation really ought to
> be a dtoverlay property rather than hard coded. Particularly with
> IMX219 there is no defined "right way" of mounting it. IMX477 with the
> tripod screw-hole does have a natural orientation.

I agree, the rotation belongs to an overlay, as it will depend on how
the camera module is integrated in the final product. This patch series
moves the default rotation from the RPi IPA to the camera sensor
overlays to provide the same default value as currently used, but it's
really up to the user to decide on the correct value.

> Are you expecting those to be merged into our kernel tree? We normally
> use Github pull requests rather than mailing lists.

I wanted the patches to appear on the libcamera-devel mailing list in
case discussions were needed, and to provide a reference to the kernel
modifications required by the corresponding libcamera patches I've
posted. Should I send a pull request, or are there issues you think need
to be addressed first ?

> > Jacopo Mondi (8):
> >   media: dt-bindings: video-interfaces: Document 'orientation' property
> >   media: dt-bindings: video-interface: Replace 'rotation' description
> >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION
> >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
> >   media: v4l2-ctrls: Add camera orientation and rotation
> >   media: v4l2-fwnode: Add helper to parse device properties
> >   media: v4l2-ctrls: Add helper to register properties
> >   media: i2c: imx219: Parse and register properties
> >
> > Laurent Pinchart (4):
> >   media: i2c: ov5647: Parse and register properties
> >   media: i2c: imx477: Parse and register properties
> >   dt/dtoverlays: imx219: Set sensor rotation
> >   dt/dtoverlays: imx477: Set sensor rotation
> >
> >  .../bindings/media/video-interfaces.txt       | 370 +++++++++++++++++-
> >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 151 +++++++
> >  arch/arm/boot/dts/overlays/imx219-overlay.dts |   2 +
> >  arch/arm/boot/dts/overlays/imx477-overlay.dts |   2 +
> >  drivers/media/i2c/imx219.c                    |  12 +-
> >  drivers/media/i2c/imx477.c                    |  12 +-
> >  drivers/media/i2c/ov5647.c                    |  13 +-
> >  drivers/media/v4l2-core/v4l2-ctrls.c          |  53 +++
> >  drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
> >  include/media/v4l2-ctrls.h                    |  26 ++
> >  include/media/v4l2-fwnode.h                   |  47 +++
> >  include/uapi/linux/v4l2-controls.h            |   7 +
> >  12 files changed, 731 insertions(+), 6 deletions(-)
Dave Stevenson July 7, 2020, 9:13 p.m. UTC | #3
On Tue, 7 Jul 2020 at 17:48, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote:
> > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote:
> > >
> > > Hi Dave,
> > >
> > > This patch series reports sensor orientation through DT for the OV5647,
> > > IMX219 and IMX477. The first 8 patches are backported from mainline,
> > > while the last 4 patches are new. Patch 09/12 could possibly be skipped
> > > for now, as we don't use the rotation property in the ov5647 DT overlay.
> > >
> > > The patches are based on top of rpi-5.4.y. libcamera patches will follow
> > > shortly.
> >
> > The patches look reasonable. I'd suggest that rotation really ought to
> > be a dtoverlay property rather than hard coded. Particularly with
> > IMX219 there is no defined "right way" of mounting it. IMX477 with the
> > tripod screw-hole does have a natural orientation.
>
> I agree, the rotation belongs to an overlay, as it will depend on how
> the camera module is integrated in the final product. This patch series
> moves the default rotation from the RPi IPA to the camera sensor
> overlays to provide the same default value as currently used, but it's
> really up to the user to decide on the correct value.
>
> > Are you expecting those to be merged into our kernel tree? We normally
> > use Github pull requests rather than mailing lists.
>
> I wanted the patches to appear on the libcamera-devel mailing list in
> case discussions were needed, and to provide a reference to the kernel
> modifications required by the corresponding libcamera patches I've
> posted. Should I send a pull request, or are there issues you think need
> to be addressed first ?

It'd be nice if rotation was a dt overlay parameter before creating
the PR, but otherwise it looks fine to me.

Where patches are backports of patches that have been merged to
mainline, we normally add a line
"Commit <full hash> upstream"
to the commit text, just to make Dom's job easier when he pulls stuff
forward to newer kernels.

  Dave

> > > Jacopo Mondi (8):
> > >   media: dt-bindings: video-interfaces: Document 'orientation' property
> > >   media: dt-bindings: video-interface: Replace 'rotation' description
> > >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION
> > >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
> > >   media: v4l2-ctrls: Add camera orientation and rotation
> > >   media: v4l2-fwnode: Add helper to parse device properties
> > >   media: v4l2-ctrls: Add helper to register properties
> > >   media: i2c: imx219: Parse and register properties
> > >
> > > Laurent Pinchart (4):
> > >   media: i2c: ov5647: Parse and register properties
> > >   media: i2c: imx477: Parse and register properties
> > >   dt/dtoverlays: imx219: Set sensor rotation
> > >   dt/dtoverlays: imx477: Set sensor rotation
> > >
> > >  .../bindings/media/video-interfaces.txt       | 370 +++++++++++++++++-
> > >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 151 +++++++
> > >  arch/arm/boot/dts/overlays/imx219-overlay.dts |   2 +
> > >  arch/arm/boot/dts/overlays/imx477-overlay.dts |   2 +
> > >  drivers/media/i2c/imx219.c                    |  12 +-
> > >  drivers/media/i2c/imx477.c                    |  12 +-
> > >  drivers/media/i2c/ov5647.c                    |  13 +-
> > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  53 +++
> > >  drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
> > >  include/media/v4l2-ctrls.h                    |  26 ++
> > >  include/media/v4l2-fwnode.h                   |  47 +++
> > >  include/uapi/linux/v4l2-controls.h            |   7 +
> > >  12 files changed, 731 insertions(+), 6 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart
Laurent Pinchart July 7, 2020, 9:40 p.m. UTC | #4
Hi Dave,

On Tue, Jul 07, 2020 at 10:13:04PM +0100, Dave Stevenson wrote:
> On Tue, 7 Jul 2020 at 17:48, Laurent Pinchart wrote:
> > On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote:
> > > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote:
> > > >
> > > > Hi Dave,
> > > >
> > > > This patch series reports sensor orientation through DT for the OV5647,
> > > > IMX219 and IMX477. The first 8 patches are backported from mainline,
> > > > while the last 4 patches are new. Patch 09/12 could possibly be skipped
> > > > for now, as we don't use the rotation property in the ov5647 DT overlay.
> > > >
> > > > The patches are based on top of rpi-5.4.y. libcamera patches will follow
> > > > shortly.
> > >
> > > The patches look reasonable. I'd suggest that rotation really ought to
> > > be a dtoverlay property rather than hard coded. Particularly with
> > > IMX219 there is no defined "right way" of mounting it. IMX477 with the
> > > tripod screw-hole does have a natural orientation.
> >
> > I agree, the rotation belongs to an overlay, as it will depend on how
> > the camera module is integrated in the final product. This patch series
> > moves the default rotation from the RPi IPA to the camera sensor
> > overlays to provide the same default value as currently used, but it's
> > really up to the user to decide on the correct value.
> >
> > > Are you expecting those to be merged into our kernel tree? We normally
> > > use Github pull requests rather than mailing lists.
> >
> > I wanted the patches to appear on the libcamera-devel mailing list in
> > case discussions were needed, and to provide a reference to the kernel
> > modifications required by the corresponding libcamera patches I've
> > posted. Should I send a pull request, or are there issues you think need
> > to be addressed first ?
> 
> It'd be nice if rotation was a dt overlay parameter before creating
> the PR, but otherwise it looks fine to me.

I wasn't aware of the DT parameters mechanism. I'll adapt the patches
accordingly, and will also reference upstream commit for backported
patches.

> Where patches are backports of patches that have been merged to
> mainline, we normally add a line
> "Commit <full hash> upstream"
> to the commit text, just to make Dom's job easier when he pulls stuff
> forward to newer kernels.
> 
> > > > Jacopo Mondi (8):
> > > >   media: dt-bindings: video-interfaces: Document 'orientation' property
> > > >   media: dt-bindings: video-interface: Replace 'rotation' description
> > > >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION
> > > >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
> > > >   media: v4l2-ctrls: Add camera orientation and rotation
> > > >   media: v4l2-fwnode: Add helper to parse device properties
> > > >   media: v4l2-ctrls: Add helper to register properties
> > > >   media: i2c: imx219: Parse and register properties
> > > >
> > > > Laurent Pinchart (4):
> > > >   media: i2c: ov5647: Parse and register properties
> > > >   media: i2c: imx477: Parse and register properties
> > > >   dt/dtoverlays: imx219: Set sensor rotation
> > > >   dt/dtoverlays: imx477: Set sensor rotation
> > > >
> > > >  .../bindings/media/video-interfaces.txt       | 370 +++++++++++++++++-
> > > >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 151 +++++++
> > > >  arch/arm/boot/dts/overlays/imx219-overlay.dts |   2 +
> > > >  arch/arm/boot/dts/overlays/imx477-overlay.dts |   2 +
> > > >  drivers/media/i2c/imx219.c                    |  12 +-
> > > >  drivers/media/i2c/imx477.c                    |  12 +-
> > > >  drivers/media/i2c/ov5647.c                    |  13 +-
> > > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  53 +++
> > > >  drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
> > > >  include/media/v4l2-ctrls.h                    |  26 ++
> > > >  include/media/v4l2-fwnode.h                   |  47 +++
> > > >  include/uapi/linux/v4l2-controls.h            |   7 +
> > > >  12 files changed, 731 insertions(+), 6 deletions(-)
Dave Stevenson July 8, 2020, 10:32 a.m. UTC | #5
On Tue, 7 Jul 2020 at 22:40, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Dave,
>
> On Tue, Jul 07, 2020 at 10:13:04PM +0100, Dave Stevenson wrote:
> > On Tue, 7 Jul 2020 at 17:48, Laurent Pinchart wrote:
> > > On Mon, Jul 06, 2020 at 06:33:41PM +0100, Dave Stevenson wrote:
> > > > On Sat, 4 Jul 2020 at 01:40, Laurent Pinchart wrote:
> > > > >
> > > > > Hi Dave,
> > > > >
> > > > > This patch series reports sensor orientation through DT for the OV5647,
> > > > > IMX219 and IMX477. The first 8 patches are backported from mainline,
> > > > > while the last 4 patches are new. Patch 09/12 could possibly be skipped
> > > > > for now, as we don't use the rotation property in the ov5647 DT overlay.
> > > > >
> > > > > The patches are based on top of rpi-5.4.y. libcamera patches will follow
> > > > > shortly.
> > > >
> > > > The patches look reasonable. I'd suggest that rotation really ought to
> > > > be a dtoverlay property rather than hard coded. Particularly with
> > > > IMX219 there is no defined "right way" of mounting it. IMX477 with the
> > > > tripod screw-hole does have a natural orientation.
> > >
> > > I agree, the rotation belongs to an overlay, as it will depend on how
> > > the camera module is integrated in the final product. This patch series
> > > moves the default rotation from the RPi IPA to the camera sensor
> > > overlays to provide the same default value as currently used, but it's
> > > really up to the user to decide on the correct value.
> > >
> > > > Are you expecting those to be merged into our kernel tree? We normally
> > > > use Github pull requests rather than mailing lists.
> > >
> > > I wanted the patches to appear on the libcamera-devel mailing list in
> > > case discussions were needed, and to provide a reference to the kernel
> > > modifications required by the corresponding libcamera patches I've
> > > posted. Should I send a pull request, or are there issues you think need
> > > to be addressed first ?
> >
> > It'd be nice if rotation was a dt overlay parameter before creating
> > the PR, but otherwise it looks fine to me.
>
> I wasn't aware of the DT parameters mechanism. I'll adapt the patches
> accordingly, and will also reference upstream commit for backported
> patches.

Thanks. If you're stuck then create the PR and I'll either add any
additional changes, or make comment as to what is needed.

Phil will be wanting an update to
https://github.com/raspberrypi/linux/blob/rpi-5.4.y/arch/arm/boot/dts/overlays/README
as well so that the help text reflects any parameters.

> > Where patches are backports of patches that have been merged to
> > mainline, we normally add a line
> > "Commit <full hash> upstream"
> > to the commit text, just to make Dom's job easier when he pulls stuff
> > forward to newer kernels.
> >
> > > > > Jacopo Mondi (8):
> > > > >   media: dt-bindings: video-interfaces: Document 'orientation' property
> > > > >   media: dt-bindings: video-interface: Replace 'rotation' description
> > > > >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_ORIENTATION
> > > > >   media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
> > > > >   media: v4l2-ctrls: Add camera orientation and rotation
> > > > >   media: v4l2-fwnode: Add helper to parse device properties
> > > > >   media: v4l2-ctrls: Add helper to register properties
> > > > >   media: i2c: imx219: Parse and register properties
> > > > >
> > > > > Laurent Pinchart (4):
> > > > >   media: i2c: ov5647: Parse and register properties
> > > > >   media: i2c: imx477: Parse and register properties
> > > > >   dt/dtoverlays: imx219: Set sensor rotation
> > > > >   dt/dtoverlays: imx477: Set sensor rotation
> > > > >
> > > > >  .../bindings/media/video-interfaces.txt       | 370 +++++++++++++++++-
> > > > >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 151 +++++++
> > > > >  arch/arm/boot/dts/overlays/imx219-overlay.dts |   2 +
> > > > >  arch/arm/boot/dts/overlays/imx477-overlay.dts |   2 +
> > > > >  drivers/media/i2c/imx219.c                    |  12 +-
> > > > >  drivers/media/i2c/imx477.c                    |  12 +-
> > > > >  drivers/media/i2c/ov5647.c                    |  13 +-
> > > > >  drivers/media/v4l2-core/v4l2-ctrls.c          |  53 +++
> > > > >  drivers/media/v4l2-core/v4l2-fwnode.c         |  42 ++
> > > > >  include/media/v4l2-ctrls.h                    |  26 ++
> > > > >  include/media/v4l2-fwnode.h                   |  47 +++
> > > > >  include/uapi/linux/v4l2-controls.h            |   7 +
> > > > >  12 files changed, 731 insertions(+), 6 deletions(-)
>
> --
> Regards,
>
> Laurent Pinchart