[PATCH 0/8] imx7/imx8mm media / csi patches

[PATCH 0/8] imx7/imx8mm media / csi patches

[Alexander Stein]

Hey everyone,

this is a set of patch for imx[7] media drivers based on next-20220203. With
this set I was able to capture video frames from a MIPI CSI-2 camera in my
TQMa8MQML + MBA8MX hardware. The actual camera used is a Vision Components
'VC MIPI IMX327 C' camera. IMX327 is compatible to IMX290. Patch 8 shows the
DT overlay I'm using, not suitable for merging.
Please ignore the FPGA part, this is mainly for power supply and GPIO reset
line. This is currently a custom driver I'm working on, but I do not want to
focus on in this series.

Please note I tested this only on this imx8 platform.

First thanks to Dorota for the patchset at [1] (patches 1-4) which is necessary
to capture correct images. I integrated Hans' review into it. I hope the
I didn't make a mistake and the original author is kept along. I used v4 for that
patchset, it is v1 again in this set. I hope this is not confusing.

Starting from patch 5 there are small fixes which allows me to configure my
media device.

Device configuration:
```
media-ctl -l "'imx290 2-001a':0->'csis-32e30000.mipi-csi':0 [1]"
media-ctl -V "'imx290 2-001a':0 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]"
media-ctl -V "'csi':0 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]"
v4l2-ctl -d0 --set-fmt-video width=1920,height=1080,pixelformat='RG10',field=none
media-ctl -p
```

The media-ctl topology is:
```
# media-ctl -p
Media controller API version 5.17.0

Media device information
------------------------
driver          imx7-csi
model           imx-media
serial          
bus info        platform:32e20000.csi
hw revision     0x0
driver version  5.17.0

Device topology
- entity 1: csi (2 pads, 2 links)
            type V4L2 subdev subtype Unknown flags 0
            device node name /dev/v4l-subdev0
        pad0: Sink
                [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
                <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
        pad1: Source
                [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
                -> "csi capture":0 [ENABLED,IMMUTABLE]

- entity 4: csi capture (1 pad, 1 link)
            type Node subtype V4L flags 0
            device node name /dev/video0
        pad0: Sink
                <- "csi":1 [ENABLED,IMMUTABLE]

- entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
             type V4L2 subdev subtype Unknown flags 0
             device node name /dev/v4l-subdev1
        pad0: Sink
                [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:709 ycbcr:601 quantization:lim-range]
                <- "imx290 2-001a":0 [ENABLED]
        pad1: Source
                [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:709 ycbcr:601 quantization:lim-range]
                -> "csi":0 [ENABLED,IMMUTABLE]

- entity 15: imx290 2-001a (1 pad, 1 link)
             type V4L2 subdev subtype Sensor flags 0
             device node name /dev/v4l-subdev2
        pad0: Source
                [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]
                -> "csis-32e30000.mipi-csi":0 [ENABLED]
```

Note: MIPI CSI settle times are not calculated correctly right now and need a
manual overwrite:
echo 13 > /sys/kernel/debug/32e30000.mipi-csi/ths_settle 
echo 2 > /sys/kernel/debug/32e30000.mipi-csi/tclk_settle

I ignored the settings for xfer, ycbcr and quantization for now. I do neither
know how they will affect me nor what it should be.
Also I did not focus on v4l2-compliance tool, this is a further task as well.
IMHO imx7-mipi-csis.c should also create an immutable link to the camera
sensor.

Regards,
Alexander

[1] https://patchwork.linuxtv.org/project/linux-media/patch/20211104113631.206899-2-dorota.czaplejewicz@puri.sm/

Alexander Stein (4):
  media: imx: imx7_mipi_csis: store colorspace in set_fmt as well
  media: imx: imx7_media-csi: Add support for additional Bayer patterns
  media: imx: utils: Add more Bayer formats
  [DNI] arm64: dts: tqma8mqml: add IMX327 MIPI-CSI overlay

Dorota Czaplejewicz (4):
  media: imx: Store the type of hardware implementation
  media: imx: Forward type of hardware implementation
  media: imx: Use dedicated format handler for i.MX7/8
  media: imx: Fail conversion if pixel format not supported

 arch/arm64/boot/dts/freescale/Makefile        |   4 +
 .../imx8mm-tqma8mqml-mba8mx-imx327.dts        |  95 +++++++++++++
 drivers/staging/media/imx/imx-ic-prpencvf.c   |   3 +-
 drivers/staging/media/imx/imx-media-capture.c |  20 ++-
 drivers/staging/media/imx/imx-media-csi.c     |   3 +-
 drivers/staging/media/imx/imx-media-utils.c   | 130 +++++++++++++++++-
 drivers/staging/media/imx/imx-media.h         |   6 +-
 drivers/staging/media/imx/imx7-media-csi.c    |  15 +-
 drivers/staging/media/imx/imx7-mipi-csis.c    |   1 +
 9 files changed, 262 insertions(+), 15 deletions(-)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-imx327.dts

-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] media: imx: Store the type of hardware implementation

[Alexander Stein]

From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>

The driver covers i.MX5/6, as well as i.MX7/8 hardware.
Those implementations differ, e.g. in the sizes of buffers they accept.

Some functionality should be abstracted, and storing type achieves that.

Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in comparison to original commit from Dorota:
* Applied the suggestion from Hans at [1].

[1] https://patchwork.linuxtv.org/project/linux-media/patch/20211104113631.206899-2-dorota.czaplejewicz@puri.sm/

 drivers/staging/media/imx/imx-ic-prpencvf.c   | 3 ++-
 drivers/staging/media/imx/imx-media-capture.c | 5 ++++-
 drivers/staging/media/imx/imx-media-csi.c     | 3 ++-
 drivers/staging/media/imx/imx-media.h         | 3 ++-
 drivers/staging/media/imx/imx7-media-csi.c    | 3 ++-
 5 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
index 9b81cfbcd777..caaaac1a515a 100644
--- a/drivers/staging/media/imx/imx-ic-prpencvf.c
+++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
@@ -1266,7 +1266,8 @@ static int prp_registered(struct v4l2_subdev *sd)
 
 	priv->vdev = imx_media_capture_device_init(ic_priv->ipu_dev,
 						   &ic_priv->sd,
-						   PRPENCVF_SRC_PAD, true);
+						   PRPENCVF_SRC_PAD, true,
+						   true);
 	if (IS_ERR(priv->vdev))
 		return PTR_ERR(priv->vdev);
 
diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index 93ba09236010..b61bf9f8ddf8 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -47,6 +47,7 @@ struct capture_priv {
 	struct v4l2_ctrl_handler ctrl_hdlr;	/* Controls inherited from subdevs */
 
 	bool legacy_api;			/* Use the legacy (pre-MC) API */
+	bool is_imx56;				/* Hardware is i.MX5/i.MX6 */
 };
 
 #define to_capture_priv(v) container_of(v, struct capture_priv, vdev)
@@ -957,7 +958,8 @@ EXPORT_SYMBOL_GPL(imx_media_capture_device_unregister);
 
 struct imx_media_video_dev *
 imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
-			      int pad, bool legacy_api)
+			      int pad, bool legacy_api,
+			      bool is_imx56)
 {
 	struct capture_priv *priv;
 	struct video_device *vfd;
@@ -972,6 +974,7 @@ imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
 	priv->src_sd_pad = pad;
 	priv->dev = dev;
 	priv->legacy_api = legacy_api;
+	priv->is_imx56 = is_imx56;
 
 	mutex_init(&priv->mutex);
 	INIT_LIST_HEAD(&priv->ready_q);
diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index bd7f156f2d52..c8f6add00dbb 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -1803,7 +1803,8 @@ static int csi_registered(struct v4l2_subdev *sd)
 	}
 
 	priv->vdev = imx_media_capture_device_init(priv->sd.dev, &priv->sd,
-						   CSI_SRC_PAD_IDMAC, true);
+						   CSI_SRC_PAD_IDMAC, true,
+						   true);
 	if (IS_ERR(priv->vdev)) {
 		ret = PTR_ERR(priv->vdev);
 		goto free_fim;
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index f263fc3adbb9..73e8e6e0d8e8 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -282,7 +282,8 @@ int imx_media_ic_unregister(struct v4l2_subdev *sd);
 /* imx-media-capture.c */
 struct imx_media_video_dev *
 imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
-			      int pad, bool legacy_api);
+			      int pad, bool legacy_api,
+			      bool is_imx56);
 void imx_media_capture_device_remove(struct imx_media_video_dev *vdev);
 int imx_media_capture_device_register(struct imx_media_video_dev *vdev,
 				      u32 link_flags);
diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 32311fc0e2a4..158d2a736c6d 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -1039,7 +1039,8 @@ static int imx7_csi_registered(struct v4l2_subdev *sd)
 	}
 
 	csi->vdev = imx_media_capture_device_init(csi->sd.dev, &csi->sd,
-						  IMX7_CSI_PAD_SRC, false);
+						  IMX7_CSI_PAD_SRC, false,
+						  false);
 	if (IS_ERR(csi->vdev))
 		return PTR_ERR(csi->vdev);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] media: imx: Store the type of hardware implementation

[Laurent Pinchart]

Hi Alexander and Dorota,

On Fri, Feb 04, 2022 at 01:15:07PM +0100, Alexander Stein wrote:
> From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> 
> The driver covers i.MX5/6, as well as i.MX7/8 hardware.
> Those implementations differ, e.g. in the sizes of buffers they accept.
> 
> Some functionality should be abstracted, and storing type achieves that.

I think that longer term (which ideally shouldn't be too far in the
future) we should decouple the i.MX5/6 and i.MX7/8 drivers (this naming
is actually not quite correct, there are i.MX6 SoCs that have a CSI
bridge, not an IPUv3). The platforms are completely different at the
hardware level, they shouldn't share the same code. That would allow us
to evolve the CSI bridge driver independently from the IPUv3 driver, and
move it from staging to drivers/media/.

I'm not against merging this if it can help short term, but please also
feel free to start decoupling the drivers, even if it results in some
duplicated code.

> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in comparison to original commit from Dorota:
> * Applied the suggestion from Hans at [1].
> 
> [1] https://patchwork.linuxtv.org/project/linux-media/patch/20211104113631.206899-2-dorota.czaplejewicz@puri.sm/
> 
>  drivers/staging/media/imx/imx-ic-prpencvf.c   | 3 ++-
>  drivers/staging/media/imx/imx-media-capture.c | 5 ++++-
>  drivers/staging/media/imx/imx-media-csi.c     | 3 ++-
>  drivers/staging/media/imx/imx-media.h         | 3 ++-
>  drivers/staging/media/imx/imx7-media-csi.c    | 3 ++-
>  5 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> index 9b81cfbcd777..caaaac1a515a 100644
> --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> @@ -1266,7 +1266,8 @@ static int prp_registered(struct v4l2_subdev *sd)
>  
>  	priv->vdev = imx_media_capture_device_init(ic_priv->ipu_dev,
>  						   &ic_priv->sd,
> -						   PRPENCVF_SRC_PAD, true);
> +						   PRPENCVF_SRC_PAD, true,
> +						   true);
>  	if (IS_ERR(priv->vdev))
>  		return PTR_ERR(priv->vdev);
>  
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index 93ba09236010..b61bf9f8ddf8 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -47,6 +47,7 @@ struct capture_priv {
>  	struct v4l2_ctrl_handler ctrl_hdlr;	/* Controls inherited from subdevs */
>  
>  	bool legacy_api;			/* Use the legacy (pre-MC) API */
> +	bool is_imx56;				/* Hardware is i.MX5/i.MX6 */

Can we create an enum type instead, to make the code more explicit ?

>  };
>  
>  #define to_capture_priv(v) container_of(v, struct capture_priv, vdev)
> @@ -957,7 +958,8 @@ EXPORT_SYMBOL_GPL(imx_media_capture_device_unregister);
>  
>  struct imx_media_video_dev *
>  imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> -			      int pad, bool legacy_api)
> +			      int pad, bool legacy_api,
> +			      bool is_imx56)
>  {
>  	struct capture_priv *priv;
>  	struct video_device *vfd;
> @@ -972,6 +974,7 @@ imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
>  	priv->src_sd_pad = pad;
>  	priv->dev = dev;
>  	priv->legacy_api = legacy_api;
> +	priv->is_imx56 = is_imx56;
>  
>  	mutex_init(&priv->mutex);
>  	INIT_LIST_HEAD(&priv->ready_q);
> diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> index bd7f156f2d52..c8f6add00dbb 100644
> --- a/drivers/staging/media/imx/imx-media-csi.c
> +++ b/drivers/staging/media/imx/imx-media-csi.c
> @@ -1803,7 +1803,8 @@ static int csi_registered(struct v4l2_subdev *sd)
>  	}
>  
>  	priv->vdev = imx_media_capture_device_init(priv->sd.dev, &priv->sd,
> -						   CSI_SRC_PAD_IDMAC, true);
> +						   CSI_SRC_PAD_IDMAC, true,
> +						   true);
>  	if (IS_ERR(priv->vdev)) {
>  		ret = PTR_ERR(priv->vdev);
>  		goto free_fim;
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index f263fc3adbb9..73e8e6e0d8e8 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -282,7 +282,8 @@ int imx_media_ic_unregister(struct v4l2_subdev *sd);
>  /* imx-media-capture.c */
>  struct imx_media_video_dev *
>  imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> -			      int pad, bool legacy_api);
> +			      int pad, bool legacy_api,
> +			      bool is_imx56);
>  void imx_media_capture_device_remove(struct imx_media_video_dev *vdev);
>  int imx_media_capture_device_register(struct imx_media_video_dev *vdev,
>  				      u32 link_flags);
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 32311fc0e2a4..158d2a736c6d 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1039,7 +1039,8 @@ static int imx7_csi_registered(struct v4l2_subdev *sd)
>  	}
>  
>  	csi->vdev = imx_media_capture_device_init(csi->sd.dev, &csi->sd,
> -						  IMX7_CSI_PAD_SRC, false);
> +						  IMX7_CSI_PAD_SRC, false,
> +						  false);
>  	if (IS_ERR(csi->vdev))
>  		return PTR_ERR(csi->vdev);
>  

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 1/8] media: imx: Store the type of hardware implementation

[Alexander Stein]

Hi Laurent,

Am Samstag, 5. Februar 2022, 04:21:34 CET schrieb Laurent Pinchart:
> Hi Alexander and Dorota,
> 
> On Fri, Feb 04, 2022 at 01:15:07PM +0100, Alexander Stein wrote:
> > From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > 
> > The driver covers i.MX5/6, as well as i.MX7/8 hardware.
> > Those implementations differ, e.g. in the sizes of buffers they accept.
> > 
> > Some functionality should be abstracted, and storing type achieves that.
> 
> I think that longer term (which ideally shouldn't be too far in the
> future) we should decouple the i.MX5/6 and i.MX7/8 drivers (this naming
> is actually not quite correct, there are i.MX6 SoCs that have a CSI
> bridge, not an IPUv3). The platforms are completely different at the
> hardware level, they shouldn't share the same code. That would allow us
> to evolve the CSI bridge driver independently from the IPUv3 driver, and
> move it from staging to drivers/media/.

That sounds reasonable. Although I'm not sure where to start. Split it for 
i.MX6 in the first place (CSI bridge vs. IPUv3)? Or start splitting across 
i.MX generation? I've yet to have broad knowledge about the internals to be 
able to come up with a good decision.

> I'm not against merging this if it can help short term, but please also
> feel free to start decoupling the drivers, even if it results in some
> duplicated code.

Thanks for willing to accept this short term patches. I'm hesitating to 
decoupling for now as I haven't fully grasped all the details and small 
similarities and differences.

> > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in comparison to original commit from Dorota:
> > * Applied the suggestion from Hans at [1].
> > 
> > [1]
> > https://patchwork.linuxtv.org/project/linux-media/patch/20211104113631.20
> > 6899-2-dorota.czaplejewicz@puri.sm/> 
> >  drivers/staging/media/imx/imx-ic-prpencvf.c   | 3 ++-
> >  drivers/staging/media/imx/imx-media-capture.c | 5 ++++-
> >  drivers/staging/media/imx/imx-media-csi.c     | 3 ++-
> >  drivers/staging/media/imx/imx-media.h         | 3 ++-
> >  drivers/staging/media/imx/imx7-media-csi.c    | 3 ++-
> >  5 files changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c
> > b/drivers/staging/media/imx/imx-ic-prpencvf.c index
> > 9b81cfbcd777..caaaac1a515a 100644
> > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > @@ -1266,7 +1266,8 @@ static int prp_registered(struct v4l2_subdev *sd)
> > 
> >  	priv->vdev = imx_media_capture_device_init(ic_priv->ipu_dev,
> >  	
> >  						   &ic_priv-
>sd,
> > 
> > -						   
PRPENCVF_SRC_PAD, true);
> > +						   
PRPENCVF_SRC_PAD, true,
> > +						   true);
> > 
> >  	if (IS_ERR(priv->vdev))
> >  	
> >  		return PTR_ERR(priv->vdev);
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-capture.c
> > b/drivers/staging/media/imx/imx-media-capture.c index
> > 93ba09236010..b61bf9f8ddf8 100644
> > --- a/drivers/staging/media/imx/imx-media-capture.c
> > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > @@ -47,6 +47,7 @@ struct capture_priv {
> > 
> >  	struct v4l2_ctrl_handler ctrl_hdlr;	/* Controls inherited 
from subdevs
> >  	*/
> >  	
> >  	bool legacy_api;			/* Use the legacy (pre-
MC) API */
> > 
> > +	bool is_imx56;				/* Hardware 
is i.MX5/i.MX6 */
> 
> Can we create an enum type instead, to make the code more explicit ?

I don't mind. So I will pick up the original patches from Dorota at [1] 
instead which already had an enum.

Best regards,
Alexander

[1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/
20211104113631.206899-2-dorota.czaplejewicz@puri.sm/

> >  };
> >  
> >  #define to_capture_priv(v) container_of(v, struct capture_priv, vdev)
> > 
> > @@ -957,7 +958,8 @@
> > EXPORT_SYMBOL_GPL(imx_media_capture_device_unregister);
> > 
> >  struct imx_media_video_dev *
> >  imx_media_capture_device_init(struct device *dev, struct v4l2_subdev
> >  *src_sd,> 
> > -			      int pad, bool legacy_api)
> > +			      int pad, bool legacy_api,
> > +			      bool is_imx56)
> > 
> >  {
> >  
> >  	struct capture_priv *priv;
> >  	struct video_device *vfd;
> > 
> > @@ -972,6 +974,7 @@ imx_media_capture_device_init(struct device *dev,
> > struct v4l2_subdev *src_sd,> 
> >  	priv->src_sd_pad = pad;
> >  	priv->dev = dev;
> >  	priv->legacy_api = legacy_api;
> > 
> > +	priv->is_imx56 = is_imx56;
> > 
> >  	mutex_init(&priv->mutex);
> >  	INIT_LIST_HEAD(&priv->ready_q);
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-csi.c
> > b/drivers/staging/media/imx/imx-media-csi.c index
> > bd7f156f2d52..c8f6add00dbb 100644
> > --- a/drivers/staging/media/imx/imx-media-csi.c
> > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > @@ -1803,7 +1803,8 @@ static int csi_registered(struct v4l2_subdev *sd)
> > 
> >  	}
> >  	
> >  	priv->vdev = imx_media_capture_device_init(priv->sd.dev, &priv->sd,
> > 
> > -						   
CSI_SRC_PAD_IDMAC, true);
> > +						   
CSI_SRC_PAD_IDMAC, true,
> > +						   true);
> > 
> >  	if (IS_ERR(priv->vdev)) {
> >  	
> >  		ret = PTR_ERR(priv->vdev);
> >  		goto free_fim;
> > 
> > diff --git a/drivers/staging/media/imx/imx-media.h
> > b/drivers/staging/media/imx/imx-media.h index f263fc3adbb9..73e8e6e0d8e8
> > 100644
> > --- a/drivers/staging/media/imx/imx-media.h
> > +++ b/drivers/staging/media/imx/imx-media.h
> > @@ -282,7 +282,8 @@ int imx_media_ic_unregister(struct v4l2_subdev *sd);
> > 
> >  /* imx-media-capture.c */
> >  struct imx_media_video_dev *
> >  imx_media_capture_device_init(struct device *dev, struct v4l2_subdev
> >  *src_sd,> 
> > -			      int pad, bool legacy_api);
> > +			      int pad, bool legacy_api,
> > +			      bool is_imx56);
> > 
> >  void imx_media_capture_device_remove(struct imx_media_video_dev *vdev);
> >  int imx_media_capture_device_register(struct imx_media_video_dev *vdev,
> >  
> >  				      u32 link_flags);
> > 
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> > b/drivers/staging/media/imx/imx7-media-csi.c index
> > 32311fc0e2a4..158d2a736c6d 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -1039,7 +1039,8 @@ static int imx7_csi_registered(struct v4l2_subdev
> > *sd)> 
> >  	}
> >  	
> >  	csi->vdev = imx_media_capture_device_init(csi->sd.dev, &csi->sd,
> > 
> > -						  
IMX7_CSI_PAD_SRC, false);
> > +						  
IMX7_CSI_PAD_SRC, false,
> > +						  false);
> > 
> >  	if (IS_ERR(csi->vdev))
> >  	
> >  		return PTR_ERR(csi->vdev);





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 1/8] media: imx: Store the type of hardware implementation

[Laurent Pinchart]

Hi Alexander,

On Mon, Feb 07, 2022 at 10:22:25AM +0100, Alexander Stein wrote:
> Am Samstag, 5. Februar 2022, 04:21:34 CET schrieb Laurent Pinchart:
> > On Fri, Feb 04, 2022 at 01:15:07PM +0100, Alexander Stein wrote:
> > > From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > > 
> > > The driver covers i.MX5/6, as well as i.MX7/8 hardware.
> > > Those implementations differ, e.g. in the sizes of buffers they accept.
> > > 
> > > Some functionality should be abstracted, and storing type achieves that.
> > 
> > I think that longer term (which ideally shouldn't be too far in the
> > future) we should decouple the i.MX5/6 and i.MX7/8 drivers (this naming
> > is actually not quite correct, there are i.MX6 SoCs that have a CSI
> > bridge, not an IPUv3). The platforms are completely different at the
> > hardware level, they shouldn't share the same code. That would allow us
> > to evolve the CSI bridge driver independently from the IPUv3 driver, and
> > move it from staging to drivers/media/.
> 
> That sounds reasonable. Although I'm not sure where to start. Split it for 
> i.MX6 in the first place (CSI bridge vs. IPUv3)? Or start splitting across 
> i.MX generation? I've yet to have broad knowledge about the internals to be 
> able to come up with a good decision.

There are only two camera interfaces supported in this directory at this
point, IPUv3 and CSI bridge (the latter implemented in imx7-media-csi).
There's no need to split across i.MX generations.

> > I'm not against merging this if it can help short term, but please also
> > feel free to start decoupling the drivers, even if it results in some
> > duplicated code.
> 
> Thanks for willing to accept this short term patches. I'm hesitating to 
> decoupling for now as I haven't fully grasped all the details and small 
> similarities and differences.

I started giving it a try, but it will take some more work.

> > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > Changes in comparison to original commit from Dorota:
> > > * Applied the suggestion from Hans at [1].
> > > 
> > > [1] https://patchwork.linuxtv.org/project/linux-media/patch/20211104113631.206899-2-dorota.czaplejewicz@puri.sm/
> > >  drivers/staging/media/imx/imx-ic-prpencvf.c   | 3 ++-
> > >  drivers/staging/media/imx/imx-media-capture.c | 5 ++++-
> > >  drivers/staging/media/imx/imx-media-csi.c     | 3 ++-
> > >  drivers/staging/media/imx/imx-media.h         | 3 ++-
> > >  drivers/staging/media/imx/imx7-media-csi.c    | 3 ++-
> > >  5 files changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/staging/media/imx/imx-ic-prpencvf.c b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > > index 9b81cfbcd777..caaaac1a515a 100644
> > > --- a/drivers/staging/media/imx/imx-ic-prpencvf.c
> > > +++ b/drivers/staging/media/imx/imx-ic-prpencvf.c
> > > @@ -1266,7 +1266,8 @@ static int prp_registered(struct v4l2_subdev *sd)
> > > 
> > >  	priv->vdev = imx_media_capture_device_init(ic_priv->ipu_dev,
> > >  						   &ic_priv->sd,
> > > -						   PRPENCVF_SRC_PAD, true);
> > > +						   PRPENCVF_SRC_PAD, true,
> > > +						   true);
> > >  	if (IS_ERR(priv->vdev))
> > >  		return PTR_ERR(priv->vdev);
> > > 
> > > diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> > > index 93ba09236010..b61bf9f8ddf8 100644
> > > --- a/drivers/staging/media/imx/imx-media-capture.c
> > > +++ b/drivers/staging/media/imx/imx-media-capture.c
> > > @@ -47,6 +47,7 @@ struct capture_priv {
> > > 
> > >  	struct v4l2_ctrl_handler ctrl_hdlr;	/* Controls inherited from subdevs */
> > >  	
> > >  	bool legacy_api;			/* Use the legacy (pre-MC) API */
> > > +	bool is_imx56;				/* Hardware is i.MX5/i.MX6 */
> > 
> > Can we create an enum type instead, to make the code more explicit ?
> 
> I don't mind. So I will pick up the original patches from Dorota at [1] 
> instead which already had an enum.
> 
> [1] https://patchwork.kernel.org/project/linux-arm-kernel/patch/
> 20211104113631.206899-2-dorota.czaplejewicz@puri.sm/
> 
> > >  };
> > >  
> > >  #define to_capture_priv(v) container_of(v, struct capture_priv, vdev)
> > > @@ -957,7 +958,8 @@
> > > EXPORT_SYMBOL_GPL(imx_media_capture_device_unregister);
> > >  struct imx_media_video_dev *
> > >  imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> > > -			      int pad, bool legacy_api)
> > > +			      int pad, bool legacy_api,
> > > +			      bool is_imx56)
> > >  {
> > >  	struct capture_priv *priv;
> > >  	struct video_device *vfd;
> > > @@ -972,6 +974,7 @@ imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> > >  	priv->src_sd_pad = pad;
> > >  	priv->dev = dev;
> > >  	priv->legacy_api = legacy_api;
> > > +	priv->is_imx56 = is_imx56;
> > > 
> > >  	mutex_init(&priv->mutex);
> > >  	INIT_LIST_HEAD(&priv->ready_q);
> > > diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
> > > index bd7f156f2d52..c8f6add00dbb 100644
> > > --- a/drivers/staging/media/imx/imx-media-csi.c
> > > +++ b/drivers/staging/media/imx/imx-media-csi.c
> > > @@ -1803,7 +1803,8 @@ static int csi_registered(struct v4l2_subdev *sd)
> > >  	}
> > >  	
> > >  	priv->vdev = imx_media_capture_device_init(priv->sd.dev, &priv->sd,
> > > -						   CSI_SRC_PAD_IDMAC, true);
> > > +						   CSI_SRC_PAD_IDMAC, true,
> > > +						   true);
> > >  	if (IS_ERR(priv->vdev)) {
> > >  		ret = PTR_ERR(priv->vdev);
> > >  		goto free_fim;
> > > diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> > > index f263fc3adbb9..73e8e6e0d8e8 100644
> > > --- a/drivers/staging/media/imx/imx-media.h
> > > +++ b/drivers/staging/media/imx/imx-media.h
> > > @@ -282,7 +282,8 @@ int imx_media_ic_unregister(struct v4l2_subdev *sd);
> > >  /* imx-media-capture.c */
> > >  struct imx_media_video_dev *
> > >  imx_media_capture_device_init(struct device *dev, struct v4l2_subdev *src_sd,
> > > -			      int pad, bool legacy_api);
> > > +			      int pad, bool legacy_api,
> > > +			      bool is_imx56);
> > >  void imx_media_capture_device_remove(struct imx_media_video_dev *vdev);
> > >  int imx_media_capture_device_register(struct imx_media_video_dev *vdev,
> > >  				      u32 link_flags);
> > > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > > index 32311fc0e2a4..158d2a736c6d 100644
> > > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > > @@ -1039,7 +1039,8 @@ static int imx7_csi_registered(struct v4l2_subdev *sd)
> > >  	}
> > >  	
> > >  	csi->vdev = imx_media_capture_device_init(csi->sd.dev, &csi->sd,
> > > -						  IMX7_CSI_PAD_SRC, false);
> > > +						  IMX7_CSI_PAD_SRC, false,
> > > +						  false);
> > >  	if (IS_ERR(csi->vdev))
> > >  		return PTR_ERR(csi->vdev);
> > > 

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] media: imx: Forward type of hardware implementation

[Alexander Stein]

From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>

Pass down the hardware type so imx_media_mbus_fmt_to_pix_fmt can do
the actual switch.

Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in comparison to original commit from Dorota:
* is_imx56 is used instead of enum

 drivers/staging/media/imx/imx-media-capture.c | 15 +++++++++------
 drivers/staging/media/imx/imx-media-utils.c   |  3 ++-
 drivers/staging/media/imx/imx-media.h         |  3 ++-
 3 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
index b61bf9f8ddf8..8aad6d6d350e 100644
--- a/drivers/staging/media/imx/imx-media-capture.c
+++ b/drivers/staging/media/imx/imx-media-capture.c
@@ -139,7 +139,8 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
 }
 
 static const struct imx_media_pixfmt *
-__capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose)
+__capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose,
+		  bool is_imx56)
 {
 	struct v4l2_mbus_framefmt fmt_src;
 	const struct imx_media_pixfmt *cc;
@@ -171,7 +172,7 @@ __capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose)
 	}
 
 	v4l2_fill_mbus_format(&fmt_src, pixfmt, 0);
-	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc);
+	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc, is_imx56);
 
 	if (compose) {
 		compose->width = fmt_src.width;
@@ -184,7 +185,9 @@ __capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose)
 static int capture_try_fmt_vid_cap(struct file *file, void *fh,
 				   struct v4l2_format *f)
 {
-	__capture_try_fmt(&f->fmt.pix, NULL);
+	struct capture_priv *priv = video_drvdata(file);
+
+	__capture_try_fmt(&f->fmt.pix, NULL, priv->is_imx56);
 	return 0;
 }
 
@@ -199,7 +202,7 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
 		return -EBUSY;
 	}
 
-	cc = __capture_try_fmt(&f->fmt.pix, &priv->vdev.compose);
+	cc = __capture_try_fmt(&f->fmt.pix, &priv->vdev.compose, priv->is_imx56);
 
 	priv->vdev.cc = cc;
 	priv->vdev.fmt = f->fmt.pix;
@@ -418,7 +421,7 @@ __capture_legacy_try_fmt(struct capture_priv *priv,
 		}
 	}
 
-	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src->format, cc);
+	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src->format, cc, priv->is_imx56);
 
 	return cc;
 }
@@ -889,7 +892,7 @@ static int capture_init_format(struct capture_priv *priv)
 		fmt_src.format.height = IMX_MEDIA_DEF_PIX_HEIGHT;
 	}
 
-	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt, &fmt_src.format, NULL);
+	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt, &fmt_src.format, NULL, priv->is_imx56);
 	vdev->compose.width = fmt_src.format.width;
 	vdev->compose.height = fmt_src.format.height;
 
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 94bc866ca28c..0daa6aad45f4 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -518,7 +518,8 @@ EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
 
 int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  const struct v4l2_mbus_framefmt *mbus,
-				  const struct imx_media_pixfmt *cc)
+				  const struct imx_media_pixfmt *cc,
+				  bool is_imx56)
 {
 	u32 width;
 	u32 stride;
diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
index 73e8e6e0d8e8..2be1bc97955c 100644
--- a/drivers/staging/media/imx/imx-media.h
+++ b/drivers/staging/media/imx/imx-media.h
@@ -198,7 +198,8 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
 			       bool ic_route);
 int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 				  const struct v4l2_mbus_framefmt *mbus,
-				  const struct imx_media_pixfmt *cc);
+				  const struct imx_media_pixfmt *cc,
+				  bool is_imx56);
 void imx_media_grp_id_to_sd_name(char *sd_name, int sz,
 				 u32 grp_id, int ipu_id);
 struct v4l2_subdev *
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] media: imx: Forward type of hardware implementation

[Laurent Pinchart]

Hi Alexander and Dorota,

Thank you for the patch.

On Fri, Feb 04, 2022 at 01:15:08PM +0100, Alexander Stein wrote:
> From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> 
> Pass down the hardware type so imx_media_mbus_fmt_to_pix_fmt can do
> the actual switch.
> 
> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Turning the bool into an enum as mentioned in the review of 1/8,

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

> ---
> Changes in comparison to original commit from Dorota:
> * is_imx56 is used instead of enum
> 
>  drivers/staging/media/imx/imx-media-capture.c | 15 +++++++++------
>  drivers/staging/media/imx/imx-media-utils.c   |  3 ++-
>  drivers/staging/media/imx/imx-media.h         |  3 ++-
>  3 files changed, 13 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-capture.c b/drivers/staging/media/imx/imx-media-capture.c
> index b61bf9f8ddf8..8aad6d6d350e 100644
> --- a/drivers/staging/media/imx/imx-media-capture.c
> +++ b/drivers/staging/media/imx/imx-media-capture.c
> @@ -139,7 +139,8 @@ static int capture_g_fmt_vid_cap(struct file *file, void *fh,
>  }
>  
>  static const struct imx_media_pixfmt *
> -__capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose)
> +__capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose,
> +		  bool is_imx56)
>  {
>  	struct v4l2_mbus_framefmt fmt_src;
>  	const struct imx_media_pixfmt *cc;
> @@ -171,7 +172,7 @@ __capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose)
>  	}
>  
>  	v4l2_fill_mbus_format(&fmt_src, pixfmt, 0);
> -	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc);
> +	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src, cc, is_imx56);
>  
>  	if (compose) {
>  		compose->width = fmt_src.width;
> @@ -184,7 +185,9 @@ __capture_try_fmt(struct v4l2_pix_format *pixfmt, struct v4l2_rect *compose)
>  static int capture_try_fmt_vid_cap(struct file *file, void *fh,
>  				   struct v4l2_format *f)
>  {
> -	__capture_try_fmt(&f->fmt.pix, NULL);
> +	struct capture_priv *priv = video_drvdata(file);
> +
> +	__capture_try_fmt(&f->fmt.pix, NULL, priv->is_imx56);
>  	return 0;
>  }
>  
> @@ -199,7 +202,7 @@ static int capture_s_fmt_vid_cap(struct file *file, void *fh,
>  		return -EBUSY;
>  	}
>  
> -	cc = __capture_try_fmt(&f->fmt.pix, &priv->vdev.compose);
> +	cc = __capture_try_fmt(&f->fmt.pix, &priv->vdev.compose, priv->is_imx56);
>  
>  	priv->vdev.cc = cc;
>  	priv->vdev.fmt = f->fmt.pix;
> @@ -418,7 +421,7 @@ __capture_legacy_try_fmt(struct capture_priv *priv,
>  		}
>  	}
>  
> -	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src->format, cc);
> +	imx_media_mbus_fmt_to_pix_fmt(pixfmt, &fmt_src->format, cc, priv->is_imx56);
>  
>  	return cc;
>  }
> @@ -889,7 +892,7 @@ static int capture_init_format(struct capture_priv *priv)
>  		fmt_src.format.height = IMX_MEDIA_DEF_PIX_HEIGHT;
>  	}
>  
> -	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt, &fmt_src.format, NULL);
> +	imx_media_mbus_fmt_to_pix_fmt(&vdev->fmt, &fmt_src.format, NULL, priv->is_imx56);
>  	vdev->compose.width = fmt_src.format.width;
>  	vdev->compose.height = fmt_src.format.height;
>  
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 94bc866ca28c..0daa6aad45f4 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -518,7 +518,8 @@ EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>  
>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  				  const struct v4l2_mbus_framefmt *mbus,
> -				  const struct imx_media_pixfmt *cc)
> +				  const struct imx_media_pixfmt *cc,
> +				  bool is_imx56)
>  {
>  	u32 width;
>  	u32 stride;
> diff --git a/drivers/staging/media/imx/imx-media.h b/drivers/staging/media/imx/imx-media.h
> index 73e8e6e0d8e8..2be1bc97955c 100644
> --- a/drivers/staging/media/imx/imx-media.h
> +++ b/drivers/staging/media/imx/imx-media.h
> @@ -198,7 +198,8 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>  			       bool ic_route);
>  int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  				  const struct v4l2_mbus_framefmt *mbus,
> -				  const struct imx_media_pixfmt *cc);
> +				  const struct imx_media_pixfmt *cc,
> +				  bool is_imx56);
>  void imx_media_grp_id_to_sd_name(char *sd_name, int sz,
>  				 u32 grp_id, int ipu_id);
>  struct v4l2_subdev *

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] media: imx: Use dedicated format handler for i.MX7/8

[Alexander Stein]

From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>

This splits out a format handler which takes into account
the capabilities of the i.MX7/8 video device,
as opposed to the default handler compatible with both i.MX5/6 and i.MX7/8.

Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
Changes in comparison to original commit from Dorota:
* is_imx56 is used instead of enum

 drivers/staging/media/imx/imx-media-utils.c | 56 +++++++++++++++++++--
 1 file changed, 52 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 0daa6aad45f4..32aaa2e81bea 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -516,10 +516,9 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
 }
 EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
 
-int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
-				  const struct v4l2_mbus_framefmt *mbus,
-				  const struct imx_media_pixfmt *cc,
-				  bool is_imx56)
+static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
+					   const struct v4l2_mbus_framefmt *mbus,
+					   const struct imx_media_pixfmt *cc)
 {
 	u32 width;
 	u32 stride;
@@ -568,6 +567,55 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 
 	return 0;
 }
+
+static int imx78_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
+					   const struct v4l2_mbus_framefmt *mbus,
+					   const struct imx_media_pixfmt *cc)
+{
+	int ret;
+
+	if (!cc)
+		cc = imx_media_find_mbus_format(mbus->code, PIXFMT_SEL_ANY);
+
+	if (!cc)
+		return -EINVAL;
+	/*
+	 * The hardware can handle line lengths divisible by 4 pixels
+	 * as long as the whole buffer size ends up divisible by 8 bytes.
+	 * If not, use the value of 8 pixels recommended in the datasheet.
+	 */
+	ret = v4l2_fill_pixfmt(pix, cc->fourcc,
+			       round_up(mbus->width, 4), mbus->height);
+	if (ret)
+		return ret;
+
+	/* Only 8bits-per-pixel formats may need to get aligned to 8 pixels,
+	 * because both 10-bit and 16-bit pixels occupy 2 bytes.
+	 * In those, 4-pixel aligmnent is equal to 8-byte alignment.
+	 */
+	if (pix->sizeimage % 8 != 0)
+		ret = v4l2_fill_pixfmt(pix, cc->fourcc,
+				       round_up(mbus->width, 8), mbus->height);
+
+	pix->colorspace = mbus->colorspace;
+	pix->xfer_func = mbus->xfer_func;
+	pix->ycbcr_enc = mbus->ycbcr_enc;
+	pix->quantization = mbus->quantization;
+	pix->field = mbus->field;
+
+	return ret;
+}
+
+int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
+				  const struct v4l2_mbus_framefmt *mbus,
+				  const struct imx_media_pixfmt *cc,
+				  bool is_imx56)
+{
+	if (is_imx56)
+		return imx56_media_mbus_fmt_to_pix_fmt(pix, mbus, cc);
+	else
+		return imx78_media_mbus_fmt_to_pix_fmt(pix, mbus, cc);
+}
 EXPORT_SYMBOL_GPL(imx_media_mbus_fmt_to_pix_fmt);
 
 void imx_media_free_dma_buf(struct device *dev,
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] media: imx: Use dedicated format handler for i.MX7/8

[Laurent Pinchart]

Hi Alexander and Dorota,

Thank you for the patch.

On Fri, Feb 04, 2022 at 01:15:09PM +0100, Alexander Stein wrote:
> From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> 
> This splits out a format handler which takes into account
> the capabilities of the i.MX7/8 video device,
> as opposed to the default handler compatible with both i.MX5/6 and i.MX7/8.
> 
> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> Changes in comparison to original commit from Dorota:
> * is_imx56 is used instead of enum
> 
>  drivers/staging/media/imx/imx-media-utils.c | 56 +++++++++++++++++++--
>  1 file changed, 52 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 0daa6aad45f4..32aaa2e81bea 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -516,10 +516,9 @@ void imx_media_try_colorimetry(struct v4l2_mbus_framefmt *tryfmt,
>  }
>  EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
>  
> -int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> -				  const struct v4l2_mbus_framefmt *mbus,
> -				  const struct imx_media_pixfmt *cc,
> -				  bool is_imx56)
> +static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> +					   const struct v4l2_mbus_framefmt *mbus,
> +					   const struct imx_media_pixfmt *cc)
>  {
>  	u32 width;
>  	u32 stride;
> @@ -568,6 +567,55 @@ int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  
>  	return 0;
>  }
> +
> +static int imx78_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> +					   const struct v4l2_mbus_framefmt *mbus,
> +					   const struct imx_media_pixfmt *cc)
> +{
> +	int ret;
> +
> +	if (!cc)
> +		cc = imx_media_find_mbus_format(mbus->code, PIXFMT_SEL_ANY);
> +
> +	if (!cc)
> +		return -EINVAL;
> +	/*
> +	 * The hardware can handle line lengths divisible by 4 pixels
> +	 * as long as the whole buffer size ends up divisible by 8 bytes.
> +	 * If not, use the value of 8 pixels recommended in the datasheet.
> +	 */
> +	ret = v4l2_fill_pixfmt(pix, cc->fourcc,
> +			       round_up(mbus->width, 4), mbus->height);
> +	if (ret)
> +		return ret;
> +
> +	/* Only 8bits-per-pixel formats may need to get aligned to 8 pixels,
> +	 * because both 10-bit and 16-bit pixels occupy 2 bytes.
> +	 * In those, 4-pixel aligmnent is equal to 8-byte alignment.
> +	 */
> +	if (pix->sizeimage % 8 != 0)
> +		ret = v4l2_fill_pixfmt(pix, cc->fourcc,
> +				       round_up(mbus->width, 8), mbus->height);

I think you could simplify this by using cc->bpp to figure out the
alignment instead of calling v4l2_fill_pixfmt() twice.

> +
> +	pix->colorspace = mbus->colorspace;
> +	pix->xfer_func = mbus->xfer_func;
> +	pix->ycbcr_enc = mbus->ycbcr_enc;
> +	pix->quantization = mbus->quantization;
> +	pix->field = mbus->field;

Should v4l2_fill_pixfmt() be updated to handle colorspace and field too
instead of doing it manually here ?

> +
> +	return ret;
> +}
> +
> +int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> +				  const struct v4l2_mbus_framefmt *mbus,
> +				  const struct imx_media_pixfmt *cc,
> +				  bool is_imx56)
> +{
> +	if (is_imx56)
> +		return imx56_media_mbus_fmt_to_pix_fmt(pix, mbus, cc);
> +	else
> +		return imx78_media_mbus_fmt_to_pix_fmt(pix, mbus, cc);
> +}
>  EXPORT_SYMBOL_GPL(imx_media_mbus_fmt_to_pix_fmt);
>  
>  void imx_media_free_dma_buf(struct device *dev,

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 3/8] media: imx: Use dedicated format handler for i.MX7/8

[Alexander Stein]

Hi Laurent,

Am Samstag, 5. Februar 2022, 05:04:50 CET schrieb Laurent Pinchart:
> Hi Alexander and Dorota,
> 
> Thank you for the patch.
> 
> On Fri, Feb 04, 2022 at 01:15:09PM +0100, Alexander Stein wrote:
> > From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > 
> > This splits out a format handler which takes into account
> > the capabilities of the i.MX7/8 video device,
> > as opposed to the default handler compatible with both i.MX5/6 and
> > i.MX7/8.
> > 
> > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > Changes in comparison to original commit from Dorota:
> > * is_imx56 is used instead of enum
> > 
> >  drivers/staging/media/imx/imx-media-utils.c | 56 +++++++++++++++++++--
> >  1 file changed, 52 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-utils.c
> > b/drivers/staging/media/imx/imx-media-utils.c index
> > 0daa6aad45f4..32aaa2e81bea 100644
> > --- a/drivers/staging/media/imx/imx-media-utils.c
> > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > @@ -516,10 +516,9 @@ void imx_media_try_colorimetry(struct
> > v4l2_mbus_framefmt *tryfmt,> 
> >  }
> >  EXPORT_SYMBOL_GPL(imx_media_try_colorimetry);
> > 
> > -int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> > -				  const struct v4l2_mbus_framefmt 
*mbus,
> > -				  const struct imx_media_pixfmt 
*cc,
> > -				  bool is_imx56)
> > +static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> > +					   const struct 
v4l2_mbus_framefmt *mbus,
> > +					   const struct 
imx_media_pixfmt *cc)
> > 
> >  {
> >  
> >  	u32 width;
> >  	u32 stride;
> > 
> > @@ -568,6 +567,55 @@ int imx_media_mbus_fmt_to_pix_fmt(struct
> > v4l2_pix_format *pix,> 
> >  	return 0;
> >  
> >  }
> > 
> > +
> > +static int imx78_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> > +					   const struct 
v4l2_mbus_framefmt *mbus,
> > +					   const struct 
imx_media_pixfmt *cc)
> > +{
> > +	int ret;
> > +
> > +	if (!cc)
> > +		cc = imx_media_find_mbus_format(mbus->code, 
PIXFMT_SEL_ANY);
> > +
> > +	if (!cc)
> > +		return -EINVAL;
> > +	/*
> > +	 * The hardware can handle line lengths divisible by 4 pixels
> > +	 * as long as the whole buffer size ends up divisible by 8 bytes.
> > +	 * If not, use the value of 8 pixels recommended in the datasheet.
> > +	 */
> > +	ret = v4l2_fill_pixfmt(pix, cc->fourcc,
> > +			       round_up(mbus->width, 4), mbus-
>height);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Only 8bits-per-pixel formats may need to get aligned to 8 
pixels,
> > +	 * because both 10-bit and 16-bit pixels occupy 2 bytes.
> > +	 * In those, 4-pixel aligmnent is equal to 8-byte alignment.
> > +	 */
> > +	if (pix->sizeimage % 8 != 0)
> > +		ret = v4l2_fill_pixfmt(pix, cc->fourcc,
> > +				       round_up(mbus->width, 8), 
mbus->height);
> 
> I think you could simplify this by using cc->bpp to figure out the
> alignment instead of calling v4l2_fill_pixfmt() twice.

Yeah, this should be possible. I can't test it (yet) though, I don't have a 8-
Bit sensor. Will do in the next version.

> > +	pix->colorspace = mbus->colorspace;
> > +	pix->xfer_func = mbus->xfer_func;
> > +	pix->ycbcr_enc = mbus->ycbcr_enc;
> > +	pix->quantization = mbus->quantization;
> > +	pix->field = mbus->field;
> 
> Should v4l2_fill_pixfmt() be updated to handle colorspace and field too
> instead of doing it manually here ?

I'm not so sure which parts of v4l2_pix_format shall be set into 
v4l2_fill_pixfmt(). Unfortunately there is no API description in the header, 
git log doesn't help much either. In the end this should be a separate patch.

Best regards,
Alexander

> > +
> > +	return ret;
> > +}
> > +
> > +int imx_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> > +				  const struct v4l2_mbus_framefmt 
*mbus,
> > +				  const struct imx_media_pixfmt 
*cc,
> > +				  bool is_imx56)
> > +{
> > +	if (is_imx56)
> > +		return imx56_media_mbus_fmt_to_pix_fmt(pix, mbus, cc);
> > +	else
> > +		return imx78_media_mbus_fmt_to_pix_fmt(pix, mbus, cc);
> > +}
> > 
> >  EXPORT_SYMBOL_GPL(imx_media_mbus_fmt_to_pix_fmt);
> >  
> >  void imx_media_free_dma_buf(struct device *dev,





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] media: imx: Fail conversion if pixel format not supported

[Alexander Stein]

From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>

imx_media_find_mbus_format has NULL as a valid return value,
therefore the caller should take it into account.

Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/staging/media/imx/imx-media-utils.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 32aaa2e81bea..e0a256a08c3b 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
 		cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
 	}
 
+	if (!cc)
+		return -EINVAL;
+
 	/* Round up width for minimum burst size */
 	width = round_up(mbus->width, 8);
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] media: imx: Fail conversion if pixel format not supported

[Laurent Pinchart]

Hi Alexander and Dorota,

Thank you for the patch.

On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> 
> imx_media_find_mbus_format has NULL as a valid return value,
> therefore the caller should take it into account.
> 
> Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/staging/media/imx/imx-media-utils.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index 32aaa2e81bea..e0a256a08c3b 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
>  		cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);

The code passed to the function comes from the previous line:

	imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);

As far as I can tell, this is guaranteed to return a code that will
result in imx_media_find_mbus_format() returning a non-NULL pointer.

>  	}
>  
> +	if (!cc)
> +		return -EINVAL;
> +
>  	/* Round up width for minimum burst size */
>  	width = round_up(mbus->width, 8);
>  

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] media: imx: Fail conversion if pixel format not supported

[Dorota Czaplejewicz]

[-- Attachment #1.1: Type: text/plain, Size: 2271 bytes --]

Hi Laurent,

On Sat, 5 Feb 2022 06:07:37 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Alexander and Dorota,
> 
> Thank you for the patch.
> 
> On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> > From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > 
> > imx_media_find_mbus_format has NULL as a valid return value,
> > therefore the caller should take it into account.
> > 
> > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> >  drivers/staging/media/imx/imx-media-utils.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> > index 32aaa2e81bea..e0a256a08c3b 100644
> > --- a/drivers/staging/media/imx/imx-media-utils.c
> > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct v4l2_pix_format *pix,
> >  		cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);  
> 
> The code passed to the function comes from the previous line:
> 
> 	imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> 
> As far as I can tell, this is guaranteed to return a code that will
> result in imx_media_find_mbus_format() returning a non-NULL pointer.
> 
While I am not well-versed in the implicit code style of the kernel, I decided to leave it in because it makes the code more locally legible. With a check here, even a non-functional one, there's no need to understand the internals of `imx_media_find_mbus_format` that are only implicit. That makes the code less surprising when interested only in the outer function.

The other advantage of a check is becoming robust against future changes to `imx_media_find_mbus_format` itself.

I don't have a strong preference about keeping or leaving this patch, but if this check was there in the first place, I wouldn't have spent time trying to figure out whether there's a bug here.

Cheers,
Dorota

> >  	}
> >  
> > +	if (!cc)
> > +		return -EINVAL;
> > +
> >  	/* Round up width for minimum burst size */
> >  	width = round_up(mbus->width, 8);
> >    
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 4/8] media: imx: Fail conversion if pixel format not supported

[Alexander Stein]

Hi Laurent and Dorota,

Am Samstag, 5. Februar 2022, 08:51:51 CET schrieb Dorota Czaplejewicz:
> * PGP Signed by an unknown key
> 
> Hi Laurent,
> 
> On Sat, 5 Feb 2022 06:07:37 +0200
> 
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Alexander and Dorota,
> > 
> > Thank you for the patch.
> > 
> > On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> > > From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > > 
> > > imx_media_find_mbus_format has NULL as a valid return value,
> > > therefore the caller should take it into account.
> > > 
> > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > ---
> > > 
> > >  drivers/staging/media/imx/imx-media-utils.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/staging/media/imx/imx-media-utils.c
> > > b/drivers/staging/media/imx/imx-media-utils.c index
> > > 32aaa2e81bea..e0a256a08c3b 100644
> > > --- a/drivers/staging/media/imx/imx-media-utils.c
> > > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > > @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct
> > > v4l2_pix_format *pix,> > 
> > >  		cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
> > 
> > The code passed to the function comes from the previous line:
> > 	imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> > 
> > As far as I can tell, this is guaranteed to return a code that will
> > result in imx_media_find_mbus_format() returning a non-NULL pointer.
> 
> While I am not well-versed in the implicit code style of the kernel, I
> decided to leave it in because it makes the code more locally legible. With
> a check here, even a non-functional one, there's no need to understand the
> internals of `imx_media_find_mbus_format` that are only implicit. That
> makes the code less surprising when interested only in the outer function.
> 
> The other advantage of a check is becoming robust against future changes to
> `imx_media_find_mbus_format` itself.
> 
> I don't have a strong preference about keeping or leaving this patch, but if
> this check was there in the first place, I wouldn't have spent time trying
> to figure out whether there's a bug here.

Laurent, thanks for your feedback.
I'm on Dorota's side here. While you are right that the code from 
  imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
is guaranteed to return something. I don't lnek this implicit assumption over 
2 function calls. Better be safe than sorry. For that reason code should be 
initialized as well. Will create a patch for that.

Regards,
Alexander




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 4/8] media: imx: Fail conversion if pixel format not supported

[Laurent Pinchart]

Hello,

On Mon, Feb 07, 2022 at 10:52:00AM +0100, Alexander Stein wrote:
> Am Samstag, 5. Februar 2022, 08:51:51 CET schrieb Dorota Czaplejewicz:
> > On Sat, 5 Feb 2022 06:07:37 +0200 Laurent Pinchart wrote:
> > > On Fri, Feb 04, 2022 at 01:15:10PM +0100, Alexander Stein wrote:
> > > > From: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > > > 
> > > > imx_media_find_mbus_format has NULL as a valid return value,
> > > > therefore the caller should take it into account.
> > > > 
> > > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.sm>
> > > > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > > > ---
> > > > 
> > > >  drivers/staging/media/imx/imx-media-utils.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/staging/media/imx/imx-media-utils.c
> > > > b/drivers/staging/media/imx/imx-media-utils.c index
> > > > 32aaa2e81bea..e0a256a08c3b 100644
> > > > --- a/drivers/staging/media/imx/imx-media-utils.c
> > > > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > > > @@ -544,6 +544,9 @@ static int imx56_media_mbus_fmt_to_pix_fmt(struct
> > > > v4l2_pix_format *pix,> > 
> > > >  		cc = imx_media_find_mbus_format(code, PIXFMT_SEL_YUV);
> > > 
> > > The code passed to the function comes from the previous line:
> > > 	imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> > > 
> > > As far as I can tell, this is guaranteed to return a code that will
> > > result in imx_media_find_mbus_format() returning a non-NULL pointer.
> > 
> > While I am not well-versed in the implicit code style of the kernel, I
> > decided to leave it in because it makes the code more locally legible. With
> > a check here, even a non-functional one, there's no need to understand the
> > internals of `imx_media_find_mbus_format` that are only implicit. That
> > makes the code less surprising when interested only in the outer function.
> > 
> > The other advantage of a check is becoming robust against future changes to
> > `imx_media_find_mbus_format` itself.
> > 
> > I don't have a strong preference about keeping or leaving this patch, but if
> > this check was there in the first place, I wouldn't have spent time trying
> > to figure out whether there's a bug here.
> 
> Laurent, thanks for your feedback.
> I'm on Dorota's side here. While you are right that the code from 
>   imx_media_enum_mbus_formats(&code, 0, PIXFMT_SEL_YUV);
> is guaranteed to return something. I don't lnek this implicit assumption over 
> 2 function calls. Better be safe than sorry. For that reason code should be 
> initialized as well. Will create a patch for that.

I'm fine with hardening the code even if the issue can't occur at the
moment.

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] media: imx: imx7_mipi_csis: store colorspace in set_fmt as well

[Alexander Stein]

Without this the default (SMPTE 170M) from init_cfg stays unchanged.
Even after configuring 'srgb' colorspace (or 'raw')
$ media-ctl -V "'csis-32e30000.mipi-csi':0 [colorspace:srgb]"
the colorspace does not change at all:
$ media-ctl --get-v4l2 "'csis-32e30000.mipi-csi':0"
  [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:smpte170m xfer:709
   ycbcr:601 quantization:lim-range]

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index a22d0e6b3d44..7b0e57efcf82 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -1062,6 +1062,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
 	fmt->code = csis_fmt->code;
 	fmt->width = sdformat->format.width;
 	fmt->height = sdformat->format.height;
+	fmt->colorspace = sdformat->format.colorspace;
 
 	sdformat->format = *fmt;
 
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] media: imx: imx7_mipi_csis: store colorspace in set_fmt as well

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Fri, Feb 04, 2022 at 01:15:11PM +0100, Alexander Stein wrote:
> Without this the default (SMPTE 170M) from init_cfg stays unchanged.
> Even after configuring 'srgb' colorspace (or 'raw')
> $ media-ctl -V "'csis-32e30000.mipi-csi':0 [colorspace:srgb]"
> the colorspace does not change at all:
> $ media-ctl --get-v4l2 "'csis-32e30000.mipi-csi':0"
>   [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:smpte170m xfer:709
>    ycbcr:601 quantization:lim-range]
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index a22d0e6b3d44..7b0e57efcf82 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -1062,6 +1062,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
>  	fmt->code = csis_fmt->code;
>  	fmt->width = sdformat->format.width;
>  	fmt->height = sdformat->format.height;
> +	fmt->colorspace = sdformat->format.colorspace;

Looks good, but shouldn't you also store the other colorspace-related
fields (ycbcr_enc, quantization and xfer_func) ?

>  	sdformat->format = *fmt;

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 5/8] media: imx: imx7_mipi_csis: store colorspace in set_fmt as well

[Alexander Stein]

Hi Laurent,

Am Samstag, 5. Februar 2022, 04:26:31 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> Thank you for the patch.
> 
> On Fri, Feb 04, 2022 at 01:15:11PM +0100, Alexander Stein wrote:
> > Without this the default (SMPTE 170M) from init_cfg stays unchanged.
> > Even after configuring 'srgb' colorspace (or 'raw')
> > $ media-ctl -V "'csis-32e30000.mipi-csi':0 [colorspace:srgb]"
> > the colorspace does not change at all:
> > $ media-ctl --get-v4l2 "'csis-32e30000.mipi-csi':0"
> > 
> >   [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:smpte170m xfer:709
> >   
> >    ycbcr:601 quantization:lim-range]
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > 
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c
> > b/drivers/staging/media/imx/imx7-mipi-csis.c index
> > a22d0e6b3d44..7b0e57efcf82 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -1062,6 +1062,7 @@ static int mipi_csis_set_fmt(struct v4l2_subdev *sd,
> > 
> >  	fmt->code = csis_fmt->code;
> >  	fmt->width = sdformat->format.width;
> >  	fmt->height = sdformat->format.height;
> > 
> > +	fmt->colorspace = sdformat->format.colorspace;
> 
> Looks good, but shouldn't you also store the other colorspace-related
> fields (ycbcr_enc, quantization and xfer_func) ?

Thanks for that feedback. Sounds reasonable, will do that. 

Regards,
Alexander




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] media: imx: imx7_media-csi: Add support for additional Bayer patterns

[Alexander Stein]

imx7_csi_configure() allows configuring these Bayer patterns when
starting a stream. So allow these in link_validate() as well.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
I'm wondering if V4L2_PIX_FMT_SBGGR16 (and their variants) is correct in this
function. imx7_csi_configure() does not list MEDIA_BUS_FMT_Sxxxx16_1X16.
Also I can't find a proper a proper setting in CSI_CR18 of CSI Bridge in
IMX8M Mini RM for RAW16. The feature list names a  "16-bit data port for
Bayer data input", but is it actually supported? I do not know anything about
the MIPI CSI data formats though. Maybe someone else can clarify this.

 drivers/staging/media/imx/imx7-media-csi.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index 158d2a736c6d..7e737221f187 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -1004,6 +1004,18 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
 	case V4L2_PIX_FMT_SGBRG8:
 	case V4L2_PIX_FMT_SGRBG8:
 	case V4L2_PIX_FMT_SRGGB8:
+	case V4L2_PIX_FMT_SBGGR10:
+	case V4L2_PIX_FMT_SGBRG10:
+	case V4L2_PIX_FMT_SGRBG10:
+	case V4L2_PIX_FMT_SRGGB10:
+	case V4L2_PIX_FMT_SBGGR12:
+	case V4L2_PIX_FMT_SGBRG12:
+	case V4L2_PIX_FMT_SGRBG12:
+	case V4L2_PIX_FMT_SRGGB12:
+	case V4L2_PIX_FMT_SBGGR14:
+	case V4L2_PIX_FMT_SGBRG14:
+	case V4L2_PIX_FMT_SGRBG14:
+	case V4L2_PIX_FMT_SRGGB14:
 	case V4L2_PIX_FMT_SBGGR16:
 	case V4L2_PIX_FMT_SGBRG16:
 	case V4L2_PIX_FMT_SGRBG16:
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] media: imx: imx7_media-csi: Add support for additional Bayer patterns

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Fri, Feb 04, 2022 at 01:15:12PM +0100, Alexander Stein wrote:
> imx7_csi_configure() allows configuring these Bayer patterns when
> starting a stream. So allow these in link_validate() as well.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

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

> ---
> I'm wondering if V4L2_PIX_FMT_SBGGR16 (and their variants) is correct in this
> function. imx7_csi_configure() does not list MEDIA_BUS_FMT_Sxxxx16_1X16.
> Also I can't find a proper a proper setting in CSI_CR18 of CSI Bridge in
> IMX8M Mini RM for RAW16. The feature list names a  "16-bit data port for
> Bayer data input", but is it actually supported? I do not know anything about
> the MIPI CSI data formats though. Maybe someone else can clarify this.

The CSI bridge has a 16-bit input. The MIPI_DATA_FORMAT field in CR18
maps to the CSI-2 DT value, and it's not clear if it's only used to
filter on the CSI-2 DT, or if it does more than that. If we're lucky,
it's the former and we can just use 0x2e.

>  drivers/staging/media/imx/imx7-media-csi.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index 158d2a736c6d..7e737221f187 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1004,6 +1004,18 @@ static int imx7_csi_pad_link_validate(struct v4l2_subdev *sd,
>  	case V4L2_PIX_FMT_SGBRG8:
>  	case V4L2_PIX_FMT_SGRBG8:
>  	case V4L2_PIX_FMT_SRGGB8:
> +	case V4L2_PIX_FMT_SBGGR10:
> +	case V4L2_PIX_FMT_SGBRG10:
> +	case V4L2_PIX_FMT_SGRBG10:
> +	case V4L2_PIX_FMT_SRGGB10:
> +	case V4L2_PIX_FMT_SBGGR12:
> +	case V4L2_PIX_FMT_SGBRG12:
> +	case V4L2_PIX_FMT_SGRBG12:
> +	case V4L2_PIX_FMT_SRGGB12:
> +	case V4L2_PIX_FMT_SBGGR14:
> +	case V4L2_PIX_FMT_SGBRG14:
> +	case V4L2_PIX_FMT_SGRBG14:
> +	case V4L2_PIX_FMT_SRGGB14:
>  	case V4L2_PIX_FMT_SBGGR16:
>  	case V4L2_PIX_FMT_SGBRG16:
>  	case V4L2_PIX_FMT_SGRBG16:

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 6/8] media: imx: imx7_media-csi: Add support for additional Bayer patterns

[Alexander Stein]

Hi Laurent,

Am Samstag, 5. Februar 2022, 04:40:14 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> Thank you for the patch.
> 
> On Fri, Feb 04, 2022 at 01:15:12PM +0100, Alexander Stein wrote:
> > imx7_csi_configure() allows configuring these Bayer patterns when
> > starting a stream. So allow these in link_validate() as well.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> > I'm wondering if V4L2_PIX_FMT_SBGGR16 (and their variants) is correct in
> > this function. imx7_csi_configure() does not list
> > MEDIA_BUS_FMT_Sxxxx16_1X16. Also I can't find a proper a proper setting
> > in CSI_CR18 of CSI Bridge in IMX8M Mini RM for RAW16. The feature list
> > names a  "16-bit data port for Bayer data input", but is it actually
> > supported? I do not know anything about the MIPI CSI data formats though.
> > Maybe someone else can clarify this.
> The CSI bridge has a 16-bit input. The MIPI_DATA_FORMAT field in CR18
> maps to the CSI-2 DT value, and it's not clear if it's only used to
> filter on the CSI-2 DT, or if it does more than that. If we're lucky,
> it's the former and we can just use 0x2e.

Well, 0x2e is not listed as a valid value for MIPI_DATA_FORMAT in both i.MX7D 
RM Rev1 and i.MX8M Mini RM Rev 3. So I'm not so sure about that. Maybe this 
holds true for other devices as well. I can not test RAW16 anyway, so I just 
wanted to raise a possible issue here.

Regards,
Alexander

> 
> >  drivers/staging/media/imx/imx7-media-csi.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c
> > b/drivers/staging/media/imx/imx7-media-csi.c index
> > 158d2a736c6d..7e737221f187 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -1004,6 +1004,18 @@ static int imx7_csi_pad_link_validate(struct
> > v4l2_subdev *sd,> 
> >  	case V4L2_PIX_FMT_SGBRG8:
> >  	case V4L2_PIX_FMT_SGRBG8:
> > 
> >  	case V4L2_PIX_FMT_SRGGB8:
> > +	case V4L2_PIX_FMT_SBGGR10:
> > +	case V4L2_PIX_FMT_SGBRG10:
> > +	case V4L2_PIX_FMT_SGRBG10:
> > +	case V4L2_PIX_FMT_SRGGB10:
> > +	case V4L2_PIX_FMT_SBGGR12:
> > +	case V4L2_PIX_FMT_SGBRG12:
> > +	case V4L2_PIX_FMT_SGRBG12:
> > +	case V4L2_PIX_FMT_SRGGB12:
> > +	case V4L2_PIX_FMT_SBGGR14:
> > +	case V4L2_PIX_FMT_SGBRG14:
> > +	case V4L2_PIX_FMT_SGRBG14:
> > 
> > +	case V4L2_PIX_FMT_SRGGB14:
> >  	case V4L2_PIX_FMT_SBGGR16:
> >  	case V4L2_PIX_FMT_SGBRG16:
> >  	case V4L2_PIX_FMT_SGRBG16:





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] media: imx: utils: Add more Bayer formats

[Alexander Stein]

Without this the ioctl VIDIOC_ENUM_FMT will not list the 10/12/14-Bit
Bayer formats. This in return results in
"v4l2-ctl --set-fmt-video pixelformat='RG10'" failing to set the
pixelformat as it is not enumerated as supported format.

Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
This adds the following formats to `v4l2-ctl --list-formats`
[...]
        [18]: 'BG10' (10-bit Bayer BGBG/GRGR)
        [19]: 'GB10' (10-bit Bayer GBGB/RGRG)
        [20]: 'BA10' (10-bit Bayer GRGR/BGBG)
        [21]: 'RG10' (10-bit Bayer RGRG/GBGB)
        [22]: 'BG12' (12-bit Bayer BGBG/GRGR)
        [23]: 'GB12' (12-bit Bayer GBGB/RGRG)
        [24]: 'BA12' (12-bit Bayer GRGR/BGBG)
        [25]: 'RG12' (12-bit Bayer RGRG/GBGB)
        [26]: 'BG14' (14-bit Bayer BGBG/GRGR)
        [27]: 'GB14' (14-bit Bayer GBGB/RGRG)
        [28]: 'GR14' (14-bit Bayer GRGR/BGBG)
        [29]: 'RG14' (14-bit Bayer RGRG/GBGB)
[...]

 drivers/staging/media/imx/imx-media-utils.c | 72 +++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index e0a256a08c3b..ea56c82d3b32 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -130,6 +130,78 @@ static const struct imx_media_pixfmt pixel_formats[] = {
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 8,
 		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SBGGR10,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 10,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SGBRG10,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 10,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SGRBG10,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 10,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SRGGB10,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 10,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SBGGR12,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 12,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SGBRG12,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 12,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SGRBG12,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 12,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SRGGB12,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 12,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SBGGR14,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR14_1X14),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 14,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SGBRG14,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG14_1X14),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 14,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SGRBG14,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG14_1X14),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 14,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_SRGGB14,
+		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB14_1X14),
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 14,
+		.bayer  = true,
 	}, {
 		.fourcc = V4L2_PIX_FMT_SBGGR16,
 		.codes  = IMX_BUS_FMTS(
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] media: imx: utils: Add more Bayer formats

[Laurent Pinchart]

Hi Alexander,

Thank you for the patch.

On Fri, Feb 04, 2022 at 01:15:13PM +0100, Alexander Stein wrote:
> Without this the ioctl VIDIOC_ENUM_FMT will not list the 10/12/14-Bit
> Bayer formats. This in return results in
> "v4l2-ctl --set-fmt-video pixelformat='RG10'" failing to set the
> pixelformat as it is not enumerated as supported format.
> 
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>

Do IPUv3-based SoCs support 10-, 12- and 14-bit formats ? If so,

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

> ---
> This adds the following formats to `v4l2-ctl --list-formats`
> [...]
>         [18]: 'BG10' (10-bit Bayer BGBG/GRGR)
>         [19]: 'GB10' (10-bit Bayer GBGB/RGRG)
>         [20]: 'BA10' (10-bit Bayer GRGR/BGBG)
>         [21]: 'RG10' (10-bit Bayer RGRG/GBGB)
>         [22]: 'BG12' (12-bit Bayer BGBG/GRGR)
>         [23]: 'GB12' (12-bit Bayer GBGB/RGRG)
>         [24]: 'BA12' (12-bit Bayer GRGR/BGBG)
>         [25]: 'RG12' (12-bit Bayer RGRG/GBGB)
>         [26]: 'BG14' (14-bit Bayer BGBG/GRGR)
>         [27]: 'GB14' (14-bit Bayer GBGB/RGRG)
>         [28]: 'GR14' (14-bit Bayer GRGR/BGBG)
>         [29]: 'RG14' (14-bit Bayer RGRG/GBGB)
> [...]
> 
>  drivers/staging/media/imx/imx-media-utils.c | 72 +++++++++++++++++++++
>  1 file changed, 72 insertions(+)
> 
> diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
> index e0a256a08c3b..ea56c82d3b32 100644
> --- a/drivers/staging/media/imx/imx-media-utils.c
> +++ b/drivers/staging/media/imx/imx-media-utils.c
> @@ -130,6 +130,78 @@ static const struct imx_media_pixfmt pixel_formats[] = {
>  		.cs     = IPUV3_COLORSPACE_RGB,
>  		.bpp    = 8,
>  		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SBGGR10,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 10,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SGBRG10,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 10,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SGRBG10,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 10,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SRGGB10,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 10,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SBGGR12,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 12,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SGBRG12,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 12,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SGRBG12,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 12,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SRGGB12,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 12,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SBGGR14,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR14_1X14),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 14,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SGBRG14,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG14_1X14),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 14,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SGRBG14,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG14_1X14),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 14,
> +		.bayer  = true,
> +	}, {
> +		.fourcc = V4L2_PIX_FMT_SRGGB14,
> +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB14_1X14),
> +		.cs     = IPUV3_COLORSPACE_RGB,
> +		.bpp    = 14,
> +		.bayer  = true,
>  	}, {
>  		.fourcc = V4L2_PIX_FMT_SBGGR16,
>  		.codes  = IMX_BUS_FMTS(

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 7/8] media: imx: utils: Add more Bayer formats

[Alexander Stein]

Hi Laurent,

Am Samstag, 5. Februar 2022, 04:31:36 CET schrieb Laurent Pinchart:
> Hi Alexander,
> 
> Thank you for the patch.
> 
> On Fri, Feb 04, 2022 at 01:15:13PM +0100, Alexander Stein wrote:
> > Without this the ioctl VIDIOC_ENUM_FMT will not list the 10/12/14-Bit
> > Bayer formats. This in return results in
> > "v4l2-ctl --set-fmt-video pixelformat='RG10'" failing to set the
> > pixelformat as it is not enumerated as supported format.
> > 
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> 
> Do IPUv3-based SoCs support 10-, 12- and 14-bit formats ? If so,
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'm no expert regarding IPUv3. But according to i.MX6D/Q RM Rev4 apparently 
there is difference between Bayer format or "Generic data" 
(CSI0_SENS_DATA_FORMAT: b011). Data width is set independently in 
CSI0_DATA_WIDTH where 10-, 12- and 14-bit is supported (even the odd ones in 
bewteen).
I don't know if I got the right place, but [1] seems to do the mapping for 
IPUv3 what imx7_csi_configure() is doing for CSI on imx7/8. The 
MEDIA_BUS_FMT_Sxxxx14_1X14 formats are not added to the list yet though, but 
that is a different matter.
Unless someone objects I'm going forwards with this patch.

Regards,
Alexander

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/
drivers/gpu/ipu-v3/ipu-csi.c#n302

> > ---
> > This adds the following formats to `v4l2-ctl --list-formats`
> > [...]
> > 
> >         [18]: 'BG10' (10-bit Bayer BGBG/GRGR)
> >         [19]: 'GB10' (10-bit Bayer GBGB/RGRG)
> >         [20]: 'BA10' (10-bit Bayer GRGR/BGBG)
> >         [21]: 'RG10' (10-bit Bayer RGRG/GBGB)
> >         [22]: 'BG12' (12-bit Bayer BGBG/GRGR)
> >         [23]: 'GB12' (12-bit Bayer GBGB/RGRG)
> >         [24]: 'BA12' (12-bit Bayer GRGR/BGBG)
> >         [25]: 'RG12' (12-bit Bayer RGRG/GBGB)
> >         [26]: 'BG14' (14-bit Bayer BGBG/GRGR)
> >         [27]: 'GB14' (14-bit Bayer GBGB/RGRG)
> >         [28]: 'GR14' (14-bit Bayer GRGR/BGBG)
> >         [29]: 'RG14' (14-bit Bayer RGRG/GBGB)
> > 
> > [...]
> > 
> >  drivers/staging/media/imx/imx-media-utils.c | 72 +++++++++++++++++++++
> >  1 file changed, 72 insertions(+)
> > 
> > diff --git a/drivers/staging/media/imx/imx-media-utils.c
> > b/drivers/staging/media/imx/imx-media-utils.c index
> > e0a256a08c3b..ea56c82d3b32 100644
> > --- a/drivers/staging/media/imx/imx-media-utils.c
> > +++ b/drivers/staging/media/imx/imx-media-utils.c
> > @@ -130,6 +130,78 @@ static const struct imx_media_pixfmt pixel_formats[]
> > = {> 
> >  		.cs     = IPUV3_COLORSPACE_RGB,
> >  		.bpp    = 8,
> >  		.bayer  = true,
> > 
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SBGGR10,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR10_1X10),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 10,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SGBRG10,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG10_1X10),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 10,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SGRBG10,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG10_1X10),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 10,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SRGGB10,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB10_1X10),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 10,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SBGGR12,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR12_1X12),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 12,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SGBRG12,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG12_1X12),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 12,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SGRBG12,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG12_1X12),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 12,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SRGGB12,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB12_1X12),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 12,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SBGGR14,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SBGGR14_1X14),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 14,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SGBRG14,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGBRG14_1X14),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 14,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SGRBG14,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SGRBG14_1X14),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 14,
> > +		.bayer  = true,
> > +	}, {
> > +		.fourcc = V4L2_PIX_FMT_SRGGB14,
> > +		.codes  = IMX_BUS_FMTS(MEDIA_BUS_FMT_SRGGB14_1X14),
> > +		.cs     = IPUV3_COLORSPACE_RGB,
> > +		.bpp    = 14,
> > +		.bayer  = true,
> > 
> >  	}, {
> >  	
> >  		.fourcc = V4L2_PIX_FMT_SBGGR16,
> >  		.codes  = IMX_BUS_FMTS(





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] [DNI] arm64: dts: tqma8mqml: add IMX327 MIPI-CSI overlay

[Alexander Stein]

---
 arch/arm64/boot/dts/freescale/Makefile        |  4 +
 .../imx8mm-tqma8mqml-mba8mx-imx327.dts        | 95 +++++++++++++++++++
 2 files changed, 99 insertions(+)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-imx327.dts

diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 852615febf9a..5ef8ff05452e 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -57,6 +57,10 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mm-icore-mx8mm-edimm2.2.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mm-kontron-n801x-s.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mm-nitrogen-r2.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mm-tqma8mqml-mba8mx.dtb
+
+tqma8mqml-mba8mx-imx327-dtbs += imx8mm-tqma8mqml-mba8mx.dtb imx8mm-tqma8mqml-mba8mx-imx327.dtbo
+dtb-$(CONFIG_ARCH_MXC) += tqma8mqml-mba8mx-imx327.dtb
+
 dtb-$(CONFIG_ARCH_MXC) += imx8mm-var-som-symphony.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mm-venice-gw71xx-0x.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mm-venice-gw72xx-0x.dtb
diff --git a/arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-imx327.dts b/arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-imx327.dts
new file mode 100644
index 000000000000..3f1223d4d73b
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-imx327.dts
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: (GPL-2.0-or-later OR MIT)
+/*
+ * Copyright 2021 TQ-Systems GmbH
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	compatible = "tq,imx8mm-tqma8mqml-mba8mx", "tq,imx8mm-tqma8mqml", "fsl,imx8mm";
+
+	sensor_clk: sensor-clk {
+		compatible = "fixed-clock";
+		#clock-cells = <0>;
+		clock-frequency = <37125000>;
+	};
+};
+
+&csi {
+       status = "okay";
+};
+
+&i2c3 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	vc_fpga: fpga@10 {
+		reg = <0x10>;
+		compatible = "vc,fpga";
+
+		vc_fpga_reg: regulator {
+			regulator-name = "CAM_VCC";
+		};
+
+		vc_fpga_reset: reset {
+			#reset-cells = <0>;
+		};
+
+		vc_fpga_gpio: gpio-chip {
+			gpio-controller;
+			#gpio-cells = <2>;
+		};
+	};
+
+	sony_imx327: camera@1a {
+		#address-cells = <0x1>;
+		#size-cells = <0x0>;
+		reg = <0x1a>;
+
+		compatible = "sony,imx327", "sony,imx290";
+
+		clock-names = "xclk";
+		clocks = <&sensor_clk>;
+		clock-frequency = <37125000>;
+
+		vdd-supply = <&vc_fpga_reg>;
+
+		vdda-supply = <&vc_fpga_reg>;
+		vddd-supply = <&vc_fpga_reg>;
+		vdddo-supply = <&vc_fpga_reg>;
+
+		reset-gpios = <&vc_fpga_gpio 0 GPIO_ACTIVE_HIGH>;
+
+		port@0 {
+			reg = <0>;
+
+			sony_imx327_ep0: endpoint {
+				remote-endpoint = <&imx8mm_mipi_csi_in>;
+				data-lanes = <1 2>;
+				clock-lanes = <0>;
+				clock-noncontinuous = <1>;
+				link-frequencies = /bits/ 64 <445500000 297000000>;
+			};
+		};
+	};
+};
+
+&mipi_csi {
+       status = "okay";
+
+       ports {
+               #address-cells = <1>;
+               #size-cells = <0>;
+
+               port@0 {
+                       reg = <0>;
+                       imx8mm_mipi_csi_in: endpoint {
+                               remote-endpoint = <&sony_imx327_ep0>;
+                               data-lanes = <1 2>;
+                       };
+               };
+       };
+};
\ No newline at end of file
-- 
2.25.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] imx7/imx8mm media / csi patches

[Laurent Pinchart]

Hi Alexander,

On Fri, Feb 04, 2022 at 01:15:06PM +0100, Alexander Stein wrote:
> Hey everyone,
> 
> this is a set of patch for imx[7] media drivers based on next-20220203. With
> this set I was able to capture video frames from a MIPI CSI-2 camera in my
> TQMa8MQML + MBA8MX hardware. The actual camera used is a Vision Components
> 'VC MIPI IMX327 C' camera. IMX327 is compatible to IMX290. Patch 8 shows the
> DT overlay I'm using, not suitable for merging.

You may be interested in https://gitlab.com/ideasonboard/nxp/linux/-/commits/pinchartl/v5.17/sensors/

> Please ignore the FPGA part, this is mainly for power supply and GPIO reset
> line. This is currently a custom driver I'm working on, but I do not want to
> focus on in this series.
> 
> Please note I tested this only on this imx8 platform.
> 
> First thanks to Dorota for the patchset at [1] (patches 1-4) which is necessary
> to capture correct images. I integrated Hans' review into it. I hope the
> I didn't make a mistake and the original author is kept along. I used v4 for that
> patchset, it is v1 again in this set. I hope this is not confusing.
> 
> Starting from patch 5 there are small fixes which allows me to configure my
> media device.
> 
> Device configuration:
> ```
> media-ctl -l "'imx290 2-001a':0->'csis-32e30000.mipi-csi':0 [1]"
> media-ctl -V "'imx290 2-001a':0 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]"
> media-ctl -V "'csi':0 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]"
> v4l2-ctl -d0 --set-fmt-video width=1920,height=1080,pixelformat='RG10',field=none
> media-ctl -p
> ```
> 
> The media-ctl topology is:
> ```
> # media-ctl -p
> Media controller API version 5.17.0
> 
> Media device information
> ------------------------
> driver          imx7-csi
> model           imx-media
> serial          
> bus info        platform:32e20000.csi
> hw revision     0x0
> driver version  5.17.0
> 
> Device topology
> - entity 1: csi (2 pads, 2 links)
>             type V4L2 subdev subtype Unknown flags 0
>             device node name /dev/v4l-subdev0
>         pad0: Sink
>                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
>                 <- "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
>         pad1: Source
>                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:none ycbcr:601 quantization:full-range]
>                 -> "csi capture":0 [ENABLED,IMMUTABLE]
> 
> - entity 4: csi capture (1 pad, 1 link)
>             type Node subtype V4L flags 0
>             device node name /dev/video0
>         pad0: Sink
>                 <- "csi":1 [ENABLED,IMMUTABLE]
> 
> - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
>              type V4L2 subdev subtype Unknown flags 0
>              device node name /dev/v4l-subdev1
>         pad0: Sink
>                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:709 ycbcr:601 quantization:lim-range]
>                 <- "imx290 2-001a":0 [ENABLED]
>         pad1: Source
>                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw xfer:709 ycbcr:601 quantization:lim-range]
>                 -> "csi":0 [ENABLED,IMMUTABLE]
> 
> - entity 15: imx290 2-001a (1 pad, 1 link)
>              type V4L2 subdev subtype Sensor flags 0
>              device node name /dev/v4l-subdev2
>         pad0: Source
>                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]
>                 -> "csis-32e30000.mipi-csi":0 [ENABLED]
> ```
> 
> Note: MIPI CSI settle times are not calculated correctly right now and need a
> manual overwrite:
> echo 13 > /sys/kernel/debug/32e30000.mipi-csi/ths_settle 
> echo 2 > /sys/kernel/debug/32e30000.mipi-csi/tclk_settle

The reference manual isn't very prolix on tclk_settle :-S That's
something I'd love to figure out one day.

For ths_settle, is the issue on the CSIS driver side, or the sensor
driver side ?

> I ignored the settings for xfer, ycbcr and quantization for now. I do neither
> know how they will affect me nor what it should be.
> Also I did not focus on v4l2-compliance tool, this is a further task as well.
> IMHO imx7-mipi-csis.c should also create an immutable link to the camera
> sensor.

That makes sense, but note that, while CSI-2 is meant to be a
point-to-point bus, there are boards designed with multiple sensors
connected to the same CSI-2 RX without any hardware multiplexer. They
keep one of the two sensors in reset at all times and are lucky that the
signal reflections don't mess things up.

> [1] https://patchwork.linuxtv.org/project/linux-media/patch/20211104113631.206899-2-dorota.czaplejewicz@puri.sm/
> 
> Alexander Stein (4):
>   media: imx: imx7_mipi_csis: store colorspace in set_fmt as well
>   media: imx: imx7_media-csi: Add support for additional Bayer patterns
>   media: imx: utils: Add more Bayer formats
>   [DNI] arm64: dts: tqma8mqml: add IMX327 MIPI-CSI overlay
> 
> Dorota Czaplejewicz (4):
>   media: imx: Store the type of hardware implementation
>   media: imx: Forward type of hardware implementation
>   media: imx: Use dedicated format handler for i.MX7/8
>   media: imx: Fail conversion if pixel format not supported
> 
>  arch/arm64/boot/dts/freescale/Makefile        |   4 +
>  .../imx8mm-tqma8mqml-mba8mx-imx327.dts        |  95 +++++++++++++
>  drivers/staging/media/imx/imx-ic-prpencvf.c   |   3 +-
>  drivers/staging/media/imx/imx-media-capture.c |  20 ++-
>  drivers/staging/media/imx/imx-media-csi.c     |   3 +-
>  drivers/staging/media/imx/imx-media-utils.c   | 130 +++++++++++++++++-
>  drivers/staging/media/imx/imx-media.h         |   6 +-
>  drivers/staging/media/imx/imx7-media-csi.c    |  15 +-
>  drivers/staging/media/imx/imx7-mipi-csis.c    |   1 +
>  9 files changed, 262 insertions(+), 15 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/freescale/imx8mm-tqma8mqml-mba8mx-imx327.dts

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 0/8] imx7/imx8mm media / csi patches

[Alexander Stein]

Hi Laurent,

Am Samstag, 5. Februar 2022, 04:16:54 CET schrieb Laurent Pinchart:
> On Fri, Feb 04, 2022 at 01:15:06PM +0100, Alexander Stein wrote:
> > Hey everyone,
> > 
> > this is a set of patch for imx[7] media drivers based on next-20220203.
> > With this set I was able to capture video frames from a MIPI CSI-2 camera
> > in my TQMa8MQML + MBA8MX hardware. The actual camera used is a Vision
> > Components 'VC MIPI IMX327 C' camera. IMX327 is compatible to IMX290.
> > Patch 8 shows the DT overlay I'm using, not suitable for merging.
> 
> You may be interested in
> https://gitlab.com/ideasonboard/nxp/linux/-/commits/pinchartl/v5.17/sensors
> /

Thanks for your feedback. Working on imx290 driver for proper imx327 support 
is on my todo. For the records, there are also patches at [1].

> > Please ignore the FPGA part, this is mainly for power supply and GPIO
> > reset
> > line. This is currently a custom driver I'm working on, but I do not want
> > to focus on in this series.
> > 
> > Please note I tested this only on this imx8 platform.
> > 
> > First thanks to Dorota for the patchset at [1] (patches 1-4) which is
> > necessary to capture correct images. I integrated Hans' review into it. I
> > hope the I didn't make a mistake and the original author is kept along. I
> > used v4 for that patchset, it is v1 again in this set. I hope this is not
> > confusing.
> > 
> > Starting from patch 5 there are small fixes which allows me to configure
> > my
> > media device.
> > 
> > Device configuration:
> > ```
> > media-ctl -l "'imx290 2-001a':0->'csis-32e30000.mipi-csi':0 [1]"
> > media-ctl -V "'imx290 2-001a':0 [fmt:SRGGB10_1X10/1920x1080 field:none
> > colorspace:raw]" media-ctl -V "'csi':0 [fmt:SRGGB10_1X10/1920x1080
> > field:none colorspace:raw]" v4l2-ctl -d0 --set-fmt-video
> > width=1920,height=1080,pixelformat='RG10',field=none media-ctl -p
> > ```
> > 
> > The media-ctl topology is:
> > ```
> > # media-ctl -p
> > Media controller API version 5.17.0
> > 
> > Media device information
> > ------------------------
> > driver          imx7-csi
> > model           imx-media
> > serial
> > bus info        platform:32e20000.csi
> > hw revision     0x0
> > driver version  5.17.0
> > 
> > Device topology
> > - entity 1: csi (2 pads, 2 links)
> > 
> >             type V4L2 subdev subtype Unknown flags 0
> >             device node name /dev/v4l-subdev0
> >         
> >         pad0: Sink
> >         
> >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> >                 xfer:none ycbcr:601 quantization:full-range] <-
> >                 "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
> >         
> >         pad1: Source
> >         
> >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> >                 xfer:none ycbcr:601 quantization:full-range] -> "csi
> >                 capture":0 [ENABLED,IMMUTABLE]
> > 
> > - entity 4: csi capture (1 pad, 1 link)
> > 
> >             type Node subtype V4L flags 0
> >             device node name /dev/video0
> >         
> >         pad0: Sink
> >         
> >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > 
> > - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
> > 
> >              type V4L2 subdev subtype Unknown flags 0
> >              device node name /dev/v4l-subdev1
> >         
> >         pad0: Sink
> >         
> >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> >                 xfer:709 ycbcr:601 quantization:lim-range] <- "imx290
> >                 2-001a":0 [ENABLED]
> >         
> >         pad1: Source
> >         
> >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> >                 xfer:709 ycbcr:601 quantization:lim-range] -> "csi":0
> >                 [ENABLED,IMMUTABLE]
> > 
> > - entity 15: imx290 2-001a (1 pad, 1 link)
> > 
> >              type V4L2 subdev subtype Sensor flags 0
> >              device node name /dev/v4l-subdev2
> >         
> >         pad0: Source
> >         
> >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]
> >                 -> "csis-32e30000.mipi-csi":0 [ENABLED]
> > 
> > ```
> > 
> > Note: MIPI CSI settle times are not calculated correctly right now and
> > need a manual overwrite:
> > echo 13 > /sys/kernel/debug/32e30000.mipi-csi/ths_settle
> > echo 2 > /sys/kernel/debug/32e30000.mipi-csi/tclk_settle
> 
> The reference manual isn't very prolix on tclk_settle :-S That's
> something I'd love to figure out one day.
> 
> For ths_settle, is the issue on the CSIS driver side, or the sensor
> driver side ?

I can't answer that yet, but during my work on a fslc linux-5.10 I noticed 
that the link frequency and/or some other clock was wrong, so the calculated 
value didn't match.
Anyway I get the impression that the final values might also be affected by 
the actual hardware. But I do not know any details regarding this.

> > I ignored the settings for xfer, ycbcr and quantization for now. I do
> > neither know how they will affect me nor what it should be.
> > Also I did not focus on v4l2-compliance tool, this is a further task as
> > well. IMHO imx7-mipi-csis.c should also create an immutable link to the
> > camera sensor.
> 
> That makes sense, but note that, while CSI-2 is meant to be a
> point-to-point bus, there are boards designed with multiple sensors
> connected to the same CSI-2 RX without any hardware multiplexer. They
> keep one of the two sensors in reset at all times and are lucky that the
> signal reflections don't mess things up.

Still, even if only one device is actually powered, it is a immutable link 
from v4l2 perspective. You don't switch the sensors on the fly, or do you 
really do that? In the end immutable links are only a minor issue (to me).

Regards,
Alexander

[1] https://github.com/raspberrypi/linux/commits/rpi-5.10.y/drivers/media/i2c/
imx290.c




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: (EXT) Re: [PATCH 0/8] imx7/imx8mm media / csi patches

[Laurent Pinchart]

Hi Alexander,

On Mon, Feb 07, 2022 at 09:59:48AM +0100, Alexander Stein wrote:
> Am Samstag, 5. Februar 2022, 04:16:54 CET schrieb Laurent Pinchart:
> > On Fri, Feb 04, 2022 at 01:15:06PM +0100, Alexander Stein wrote:
> > > Hey everyone,
> > > 
> > > this is a set of patch for imx[7] media drivers based on next-20220203.
> > > With this set I was able to capture video frames from a MIPI CSI-2 camera
> > > in my TQMa8MQML + MBA8MX hardware. The actual camera used is a Vision
> > > Components 'VC MIPI IMX327 C' camera. IMX327 is compatible to IMX290.
> > > Patch 8 shows the DT overlay I'm using, not suitable for merging.
> > 
> > You may be interested in
> > https://gitlab.com/ideasonboard/nxp/linux/-/commits/pinchartl/v5.17/sensors
> > /
> 
> Thanks for your feedback. Working on imx290 driver for proper imx327 support 
> is on my todo. For the records, there are also patches at [1].

There's also a driver for the Vision Components FPGA controller in my
branch. If you would like me to review yours, please CC me when posting
it.

> > > Please ignore the FPGA part, this is mainly for power supply and GPIO
> > > reset
> > > line. This is currently a custom driver I'm working on, but I do not want
> > > to focus on in this series.
> > > 
> > > Please note I tested this only on this imx8 platform.
> > > 
> > > First thanks to Dorota for the patchset at [1] (patches 1-4) which is
> > > necessary to capture correct images. I integrated Hans' review into it. I
> > > hope the I didn't make a mistake and the original author is kept along. I
> > > used v4 for that patchset, it is v1 again in this set. I hope this is not
> > > confusing.
> > > 
> > > Starting from patch 5 there are small fixes which allows me to configure my
> > > media device.
> > > 
> > > Device configuration:
> > > ```
> > > media-ctl -l "'imx290 2-001a':0->'csis-32e30000.mipi-csi':0 [1]"
> > > media-ctl -V "'imx290 2-001a':0 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]"
> > > media-ctl -V "'csi':0 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]"
> > > v4l2-ctl -d0 --set-fmt-video width=1920,height=1080,pixelformat='RG10',field=none
> > > media-ctl -p
> > > ```
> > > 
> > > The media-ctl topology is:
> > > ```
> > > # media-ctl -p
> > > Media controller API version 5.17.0
> > > 
> > > Media device information
> > > ------------------------
> > > driver          imx7-csi
> > > model           imx-media
> > > serial
> > > bus info        platform:32e20000.csi
> > > hw revision     0x0
> > > driver version  5.17.0
> > > 
> > > Device topology
> > > - entity 1: csi (2 pads, 2 links)
> > > 
> > >             type V4L2 subdev subtype Unknown flags 0
> > >             device node name /dev/v4l-subdev0
> > >         
> > >         pad0: Sink
> > >         
> > >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> > >                 xfer:none ycbcr:601 quantization:full-range] <-
> > >                 "csis-32e30000.mipi-csi":1 [ENABLED,IMMUTABLE]
> > >         
> > >         pad1: Source
> > >         
> > >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> > >                 xfer:none ycbcr:601 quantization:full-range] -> "csi
> > >                 capture":0 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 4: csi capture (1 pad, 1 link)
> > > 
> > >             type Node subtype V4L flags 0
> > >             device node name /dev/video0
> > >         
> > >         pad0: Sink
> > >         
> > >                 <- "csi":1 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 10: csis-32e30000.mipi-csi (2 pads, 2 links)
> > > 
> > >              type V4L2 subdev subtype Unknown flags 0
> > >              device node name /dev/v4l-subdev1
> > >         
> > >         pad0: Sink
> > >         
> > >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> > >                 xfer:709 ycbcr:601 quantization:lim-range] <- "imx290
> > >                 2-001a":0 [ENABLED]
> > >         
> > >         pad1: Source
> > >         
> > >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw
> > >                 xfer:709 ycbcr:601 quantization:lim-range] -> "csi":0
> > >                 [ENABLED,IMMUTABLE]
> > > 
> > > - entity 15: imx290 2-001a (1 pad, 1 link)
> > > 
> > >              type V4L2 subdev subtype Sensor flags 0
> > >              device node name /dev/v4l-subdev2
> > >         
> > >         pad0: Source
> > >         
> > >                 [fmt:SRGGB10_1X10/1920x1080 field:none colorspace:raw]
> > >                 -> "csis-32e30000.mipi-csi":0 [ENABLED]
> > > 
> > > ```
> > > 
> > > Note: MIPI CSI settle times are not calculated correctly right now and
> > > need a manual overwrite:
> > > echo 13 > /sys/kernel/debug/32e30000.mipi-csi/ths_settle
> > > echo 2 > /sys/kernel/debug/32e30000.mipi-csi/tclk_settle
> > 
> > The reference manual isn't very prolix on tclk_settle :-S That's
> > something I'd love to figure out one day.
> > 
> > For ths_settle, is the issue on the CSIS driver side, or the sensor
> > driver side ?
> 
> I can't answer that yet, but during my work on a fslc linux-5.10 I noticed 
> that the link frequency and/or some other clock was wrong, so the calculated 
> value didn't match.
> Anyway I get the impression that the final values might also be affected by 
> the actual hardware. But I do not know any details regarding this.
> 
> > > I ignored the settings for xfer, ycbcr and quantization for now. I do
> > > neither know how they will affect me nor what it should be.
> > > Also I did not focus on v4l2-compliance tool, this is a further task as
> > > well. IMHO imx7-mipi-csis.c should also create an immutable link to the
> > > camera sensor.
> > 
> > That makes sense, but note that, while CSI-2 is meant to be a
> > point-to-point bus, there are boards designed with multiple sensors
> > connected to the same CSI-2 RX without any hardware multiplexer. They
> > keep one of the two sensors in reset at all times and are lucky that the
> > signal reflections don't mess things up.
> 
> Still, even if only one device is actually powered, it is a immutable link 
> from v4l2 perspective. You don't switch the sensors on the fly, or do you 
> really do that? In the end immutable links are only a minor issue (to me).

The sensor couldn't be switched while streaming, but you could control
which sensor to use before starting streaming by enabling and disabling
links. An immutable link can never be disabled.

We could create the link as immutable if only once sensor is connected,
which is the usual case. However, I wonder if it wouldn't make things
more difficult for userspace that would need to avoid trying to disable
the link in that case.

> [1] https://github.com/raspberrypi/linux/commits/rpi-5.10.y/drivers/media/i2c/
> imx290.c

-- 
Regards,

Laurent Pinchart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel