[PATCH 0/7] media: imx: Miscellaneous fixes for i.MX7

[PATCH 0/7] media: imx: Miscellaneous fixes for i.MX7

[Laurent Pinchart]

Hello,

This patch series contains miscellaneous fixes I developed while trying
to capture from a CSI-2 sensor on the i.MX7.

Patches 1/7, 2/7 and 6/7 are small fixes or enhancements, please see
individual commit messages. Patches 3/7, 4/7 and 5/7 add support for 10-
and 12-bit greyscale formats (both on the i.MX7 CSI-2 receiver side and
the CSI side). Patch 7/7 finally fixes a recent issue with video field
handling.

Laurent Pinchart (7):
  media: imx: imx7_mipi_csis: Power off the source when stopping
    streaming
  media: imx: imx7_mipi_csis: Print the RESOL_CH0 register
  media: imx: imx7_mipi_csis: Add greyscale formats support
  media: imx: imx6-media-csi: Replace Y16 with Y10 and Y12
  media: imx: imx7-media-csi: Add Y10 and Y12 formats support
  media: imx: imx7-media-csi: Remove unneeded register read
  media: imx: imx7-media-csi: Fix video field handling

 drivers/staging/media/imx/imx-media-csi.c   |  3 ++-
 drivers/staging/media/imx/imx-media-utils.c | 13 ++++++++-----
 drivers/staging/media/imx/imx7-media-csi.c  | 14 +++++++++++++-
 drivers/staging/media/imx/imx7-mipi-csis.c  | 15 ++++++++++++++-
 4 files changed, 37 insertions(+), 8 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH 1/7] media: imx: imx7_mipi_csis: Power off the source when stopping streaming

[Laurent Pinchart]

The .s_stream() implementation incorrectly powers on the source when
stopping the stream. Power it off instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 73d8354e618c..66407b228f8e 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -577,7 +577,7 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 		state->flags |= ST_STREAMING;
 	} else {
 		v4l2_subdev_call(state->src_sd, video, s_stream, 0);
-		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
+		ret = v4l2_subdev_call(state->src_sd, core, s_power, 0);
 		mipi_csis_stop_stream(state);
 		state->flags &= ~ST_STREAMING;
 		if (state->debug)
-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/7] media: imx: imx7_mipi_csis: Power off the source when stopping streaming

[Fabio Estevam]

Hi Laurent,

On Thu, Oct 24, 2019 at 2:41 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> The .s_stream() implementation incorrectly powers on the source when
> stopping the stream. Power it off instead.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Should this one have a Fixes tag?

Thanks

Re: [PATCH 1/7] media: imx: imx7_mipi_csis: Power off the source when stopping streaming

[Rui Miguel Silva]

Hi Laurent,
On Thu 24 Oct 2019 at 18:43, Fabio Estevam wrote:
> Hi Laurent,
>
> On Thu, Oct 24, 2019 at 2:41 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> The .s_stream() implementation incorrectly powers on the source when
>> stopping the stream. Power it off instead.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Should this one have a Fixes tag?

let me help you on this one:
Fixes: 7807063b862b ("media: staging/imx7: add MIPI CSI-2 receiver subdev for i.MX7")

---
Cheers,
	Rui


>
> Thanks

[PATCH 2/7] media: imx: imx7_mipi_csis: Print the RESOL_CH0 register

[Laurent Pinchart]

Add the RESOL_CH0 register to the list of registers printed through the
debugfs debug infrastructure for the driver, as it can be useful to
verify proper configuration of the CSI-2 receiver.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.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 66407b228f8e..a0ba3ed00cd8 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -303,6 +303,7 @@ static int mipi_csis_dump_regs(struct csi_state *state)
 		{ 0x20, "DPHYSTS" },
 		{ 0x10, "INTMSK" },
 		{ 0x40, "CONFIG_CH0" },
+		{ 0x44, "RESOL_CH0" },
 		{ 0xC0, "DBG_CONFIG" },
 		{ 0x38, "DPHYSLAVE_L" },
 		{ 0x3C, "DPHYSLAVE_H" },
-- 
Regards,

Laurent Pinchart

[PATCH 3/7] media: imx: imx7_mipi_csis: Add greyscale formats support

[Laurent Pinchart]

Add support for the 8-, 10- and 12-bit greyscale media bus formats, and
map them to the CSI-2 RAW8, RAW10 and RAW12 formats respectively.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index a0ba3ed00cd8..8ffafbb6b34b 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -282,6 +282,18 @@ static const struct csis_pix_format mipi_csis_formats[] = {
 		.code = MEDIA_BUS_FMT_YUYV8_2X8,
 		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_YCBCR422_8BIT,
 		.data_alignment = 16,
+	}, {
+		.code = MEDIA_BUS_FMT_Y8_1X8,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW8,
+		.data_alignment = 8,
+	}, {
+		.code = MEDIA_BUS_FMT_Y10_1X10,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW10,
+		.data_alignment = 16,
+	}, {
+		.code = MEDIA_BUS_FMT_Y12_1X12,
+		.fmt_reg = MIPI_CSIS_ISPCFG_FMT_RAW12,
+		.data_alignment = 16,
 	}
 };
 
-- 
Regards,

Laurent Pinchart

[PATCH 4/7] media: imx: imx6-media-csi: Replace Y16 with Y10 and Y12

[Laurent Pinchart]

The driver doesn't really support V4L2_PIX_FMT_Y16, as there's no 16-bit
greyscale media bus code defined by the kernel. It (ab)uses the format
to capture 10-bit and 12-bit greyscale formats. Fix it to properly
support MEDIA_BUS_FMT_Y10_1X10 and MEDIA_BUS_FMT_Y12_1X12 instead.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx-media-csi.c   |  3 ++-
 drivers/staging/media/imx/imx-media-utils.c | 13 ++++++++-----
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/media/imx/imx-media-csi.c b/drivers/staging/media/imx/imx-media-csi.c
index 367e39f5b382..29582ceb2164 100644
--- a/drivers/staging/media/imx/imx-media-csi.c
+++ b/drivers/staging/media/imx/imx-media-csi.c
@@ -457,7 +457,8 @@ static int csi_idmac_setup_channel(struct csi_priv *priv)
 	case V4L2_PIX_FMT_SGBRG16:
 	case V4L2_PIX_FMT_SGRBG16:
 	case V4L2_PIX_FMT_SRGGB16:
-	case V4L2_PIX_FMT_Y16:
+	case V4L2_PIX_FMT_Y10:
+	case V4L2_PIX_FMT_Y12:
 		burst_size = 8;
 		passthrough_bits = 16;
 		break;
diff --git a/drivers/staging/media/imx/imx-media-utils.c b/drivers/staging/media/imx/imx-media-utils.c
index 4cc6a7462ae2..a1c313e35bc0 100644
--- a/drivers/staging/media/imx/imx-media-utils.c
+++ b/drivers/staging/media/imx/imx-media-utils.c
@@ -166,11 +166,14 @@ static const struct imx_media_pixfmt rgb_formats[] = {
 		.bpp    = 8,
 		.bayer  = true,
 	}, {
-		.fourcc = V4L2_PIX_FMT_Y16,
-		.codes = {
-			MEDIA_BUS_FMT_Y10_1X10,
-			MEDIA_BUS_FMT_Y12_1X12,
-		},
+		.fourcc = V4L2_PIX_FMT_Y10,
+		.codes = {MEDIA_BUS_FMT_Y10_1X10},
+		.cs     = IPUV3_COLORSPACE_RGB,
+		.bpp    = 16,
+		.bayer  = true,
+	}, {
+		.fourcc = V4L2_PIX_FMT_Y12,
+		.codes = {MEDIA_BUS_FMT_Y12_1X12},
 		.cs     = IPUV3_COLORSPACE_RGB,
 		.bpp    = 16,
 		.bayer  = true,
-- 
Regards,

Laurent Pinchart

[PATCH 5/7] media: imx: imx7-media-csi: Add Y10 and Y12 formats support

[Laurent Pinchart]

Support capturing the 10- and 12-bit greyscale formats.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index bfd6b5fbf484..e4c9bcc045f7 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -804,6 +804,14 @@ static int imx7_csi_configure(struct imx7_csi *csi)
 	case V4L2_PIX_FMT_YUYV:
 		cr18 |= BIT_MIPI_DATA_FORMAT_YUV422_8B;
 		break;
+	case V4L2_PIX_FMT_Y10:
+		cr18 |= BIT_MIPI_DATA_FORMAT_RAW10;
+		cr1 |= BIT_PIXEL_BIT;
+		break;
+	case V4L2_PIX_FMT_Y12:
+		cr18 |= BIT_MIPI_DATA_FORMAT_RAW12;
+		cr1 |= BIT_PIXEL_BIT;
+		break;
 	case V4L2_PIX_FMT_SBGGR8:
 		cr18 |= BIT_MIPI_DATA_FORMAT_RAW8;
 		break;
-- 
Regards,

Laurent Pinchart

[PATCH 6/7] media: imx: imx7-media-csi: Remove unneeded register read

[Laurent Pinchart]

The imx7_csi_dma_reflash() function starts by reading the unrelated
register CSI_CSICR18 to then overwrite the read value with a read from
register CSI_CSICR3. This is likely due to a bad copy&paste, and is not
needed. Remove the extraneous read from register CSI_CSICR18.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index e4c9bcc045f7..ac07f55a63e3 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -292,7 +292,7 @@ static void imx7_csi_hw_disable(struct imx7_csi *csi)
 
 static void imx7_csi_dma_reflash(struct imx7_csi *csi)
 {
-	u32 cr3 = imx7_csi_reg_read(csi, CSI_CSICR18);
+	u32 cr3;
 
 	cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
 	cr3 |= BIT_DMA_REFLASH_RFF;
-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] media: imx: imx7-media-csi: Remove unneeded register read

[Steve Longerbeam]

Hi Laurent,

This patch also looks like it needs a Fixes tag.

Steve


On 10/23/19 5:41 PM, Laurent Pinchart wrote:
> The imx7_csi_dma_reflash() function starts by reading the unrelated
> register CSI_CSICR18 to then overwrite the read value with a read from
> register CSI_CSICR3. This is likely due to a bad copy&paste, and is not
> needed. Remove the extraneous read from register CSI_CSICR18.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/staging/media/imx/imx7-media-csi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index e4c9bcc045f7..ac07f55a63e3 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -292,7 +292,7 @@ static void imx7_csi_hw_disable(struct imx7_csi *csi)
>   
>   static void imx7_csi_dma_reflash(struct imx7_csi *csi)
>   {
> -	u32 cr3 = imx7_csi_reg_read(csi, CSI_CSICR18);
> +	u32 cr3;
>   
>   	cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
>   	cr3 |= BIT_DMA_REFLASH_RFF;

Re: [PATCH 6/7] media: imx: imx7-media-csi: Remove unneeded register read

[Steve Longerbeam]

On 10/24/19 10:57 AM, Steve Longerbeam wrote:
> Hi Laurent,
>
> This patch also looks like it needs a Fixes tag.

Fixes: 9e5fa4e1e5b5b ("media: imx7-media-csi: Use u32 for storing 
register reads")


>
>
>
> On 10/23/19 5:41 PM, Laurent Pinchart wrote:
>> The imx7_csi_dma_reflash() function starts by reading the unrelated
>> register CSI_CSICR18 to then overwrite the read value with a read from
>> register CSI_CSICR3. This is likely due to a bad copy&paste, and is not
>> needed. Remove the extraneous read from register CSI_CSICR18.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/staging/media/imx/imx7-media-csi.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c 
>> b/drivers/staging/media/imx/imx7-media-csi.c
>> index e4c9bcc045f7..ac07f55a63e3 100644
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -292,7 +292,7 @@ static void imx7_csi_hw_disable(struct imx7_csi 
>> *csi)
>>     static void imx7_csi_dma_reflash(struct imx7_csi *csi)
>>   {
>> -    u32 cr3 = imx7_csi_reg_read(csi, CSI_CSICR18);
>> +    u32 cr3;
>>         cr3 = imx7_csi_reg_read(csi, CSI_CSICR3);
>>       cr3 |= BIT_DMA_REFLASH_RFF;
>

[PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Laurent Pinchart]

Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
source pads") reworked the way that formats are set on the sink pad of
the CSI subdevice, and accidentally removed video field handling.
Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
supported, with the only two supported value being V4L2_FIELD_NONE and
V4L2_FIELD_ALTERNATE.

Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
index ac07f55a63e3..0db6473caf13 100644
--- a/drivers/staging/media/imx/imx7-media-csi.c
+++ b/drivers/staging/media/imx/imx7-media-csi.c
@@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
 		sdformat->format.width = in_fmt->width;
 		sdformat->format.height = in_fmt->height;
 		sdformat->format.code = in_fmt->code;
+		sdformat->format.field = in_fmt->field;
 		*cc = in_cc;
 
 		sdformat->format.colorspace = in_fmt->colorspace;
@@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
 							 false);
 			sdformat->format.code = (*cc)->codes[0];
 		}
+
+		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
+			sdformat->format.field = V4L2_FIELD_NONE;
 		break;
 	default:
 		return -EINVAL;
-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Steve Longerbeam]

Hi Laurent,

On 10/23/19 5:41 PM, Laurent Pinchart wrote:
> Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
> source pads") reworked the way that formats are set on the sink pad of
> the CSI subdevice, and accidentally removed video field handling.
> Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
> supported, with the only two supported value being V4L2_FIELD_NONE and
> V4L2_FIELD_ALTERNATE.

I think you mean the only two supported field values being 
V4L2_FIELD_NONE and V4L2_FIELD_INTERLACED.

For this driver to support ALTERNATE, it would have to detect the 
captured field type and place that type in the returned vb2 buf->field, 
which it is not doing.

Steve

>
> Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index ac07f55a63e3..0db6473caf13 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>   		sdformat->format.width = in_fmt->width;
>   		sdformat->format.height = in_fmt->height;
>   		sdformat->format.code = in_fmt->code;
> +		sdformat->format.field = in_fmt->field;
>   		*cc = in_cc;
>   
>   		sdformat->format.colorspace = in_fmt->colorspace;
> @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>   							 false);
>   			sdformat->format.code = (*cc)->codes[0];
>   		}
> +
> +		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>   		break;
>   	default:
>   		return -EINVAL;

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Steve Longerbeam]

On 10/23/19 8:25 PM, Steve Longerbeam wrote:
> Hi Laurent,
>
> On 10/23/19 5:41 PM, Laurent Pinchart wrote:
>> Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
>> source pads") reworked the way that formats are set on the sink pad of
>> the CSI subdevice, and accidentally removed video field handling.
>> Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
>> supported, with the only two supported value being V4L2_FIELD_NONE and
>> V4L2_FIELD_ALTERNATE.
>
> I think you mean the only two supported field values being 
> V4L2_FIELD_NONE and V4L2_FIELD_INTERLACED.
>
> For this driver to support ALTERNATE, it would have to detect the 
> captured field type and place that type in the returned vb2 
> buf->field, which it is not doing.

So the code change is correct, only the commit description is wrong.

Steve

>
>
>
>>
>> Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and 
>> source pads")
>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> ---
>>   drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c 
>> b/drivers/staging/media/imx/imx7-media-csi.c
>> index ac07f55a63e3..0db6473caf13 100644
>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>> @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>           sdformat->format.width = in_fmt->width;
>>           sdformat->format.height = in_fmt->height;
>>           sdformat->format.code = in_fmt->code;
>> +        sdformat->format.field = in_fmt->field;
>>           *cc = in_cc;
>>             sdformat->format.colorspace = in_fmt->colorspace;
>> @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>                                false);
>>               sdformat->format.code = (*cc)->codes[0];
>>           }
>> +
>> +        if (sdformat->format.field != V4L2_FIELD_INTERLACED)
>> +            sdformat->format.field = V4L2_FIELD_NONE;
>>           break;
>>       default:
>>           return -EINVAL;
>

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Laurent Pinchart]

Hi Steve,

On Wed, Oct 23, 2019 at 08:25:53PM -0700, Steve Longerbeam wrote:
> On 10/23/19 5:41 PM, Laurent Pinchart wrote:
> > Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
> > source pads") reworked the way that formats are set on the sink pad of
> > the CSI subdevice, and accidentally removed video field handling.
> > Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
> > supported, with the only two supported value being V4L2_FIELD_NONE and
> > V4L2_FIELD_ALTERNATE.
> 
> I think you mean the only two supported field values being 
> V4L2_FIELD_NONE and V4L2_FIELD_INTERLACED.
> 
> For this driver to support ALTERNATE, it would have to detect the 
> captured field type and place that type in the returned vb2 buf->field, 
> which it is not doing.

Yes, that's what I meant, sorry. I'll fix the commit message.

> > Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index ac07f55a63e3..0db6473caf13 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> >   		sdformat->format.width = in_fmt->width;
> >   		sdformat->format.height = in_fmt->height;
> >   		sdformat->format.code = in_fmt->code;
> > +		sdformat->format.field = in_fmt->field;
> >   		*cc = in_cc;
> >   
> >   		sdformat->format.colorspace = in_fmt->colorspace;
> > @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> >   							 false);
> >   			sdformat->format.code = (*cc)->codes[0];
> >   		}
> > +
> > +		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
> > +			sdformat->format.field = V4L2_FIELD_NONE;
> >   		break;
> >   	default:
> >   		return -EINVAL;
> 

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Steve Longerbeam]

Hi Laurent,

On 10/23/19 5:41 PM, Laurent Pinchart wrote:
> Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
> source pads") reworked the way that formats are set on the sink pad of
> the CSI subdevice, and accidentally removed video field handling.

Well, let me restate the problem. This driver never did have correct 
field handling (as you demonstrate in this patch, the driver never 
set/propagated the field type to its source pad). So it's not accurate 
to say the patch is a fix. A better description is that it adds 
rudimentary field handling.

Steve

> Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
> supported, with the only two supported value being V4L2_FIELD_NONE and
> V4L2_FIELD_ALTERNATE.
>
> Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>   drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
>   1 file changed, 4 insertions(+)
>
> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> index ac07f55a63e3..0db6473caf13 100644
> --- a/drivers/staging/media/imx/imx7-media-csi.c
> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>   		sdformat->format.width = in_fmt->width;
>   		sdformat->format.height = in_fmt->height;
>   		sdformat->format.code = in_fmt->code;
> +		sdformat->format.field = in_fmt->field;
>   		*cc = in_cc;
>   
>   		sdformat->format.colorspace = in_fmt->colorspace;
> @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>   							 false);
>   			sdformat->format.code = (*cc)->codes[0];
>   		}
> +
> +		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
> +			sdformat->format.field = V4L2_FIELD_NONE;
>   		break;
>   	default:
>   		return -EINVAL;

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Laurent Pinchart]

Hi Steve,

On Thu, Oct 24, 2019 at 08:11:20AM -0700, Steve Longerbeam wrote:
> On 10/23/19 5:41 PM, Laurent Pinchart wrote:
> > Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
> > source pads") reworked the way that formats are set on the sink pad of
> > the CSI subdevice, and accidentally removed video field handling.
> 
> Well, let me restate the problem. This driver never did have correct 
> field handling (as you demonstrate in this patch, the driver never 
> set/propagated the field type to its source pad). So it's not accurate 
> to say the patch is a fix. A better description is that it adds 
> rudimentary field handling.

Didn't it ? The above commit removed a call to
imx_media_fill_default_mbus_fields() from imx7_csi_try_fmt(), and that
function had rudimentary field handling. The Fixes: tag isn't there to
blame you :-)

> > Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
> > supported, with the only two supported value being V4L2_FIELD_NONE and
> > V4L2_FIELD_ALTERNATE.
> >
> > Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >   drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> >
> > diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> > index ac07f55a63e3..0db6473caf13 100644
> > --- a/drivers/staging/media/imx/imx7-media-csi.c
> > +++ b/drivers/staging/media/imx/imx7-media-csi.c
> > @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> >   		sdformat->format.width = in_fmt->width;
> >   		sdformat->format.height = in_fmt->height;
> >   		sdformat->format.code = in_fmt->code;
> > +		sdformat->format.field = in_fmt->field;
> >   		*cc = in_cc;
> >   
> >   		sdformat->format.colorspace = in_fmt->colorspace;
> > @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> >   							 false);
> >   			sdformat->format.code = (*cc)->codes[0];
> >   		}
> > +
> > +		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
> > +			sdformat->format.field = V4L2_FIELD_NONE;
> >   		break;
> >   	default:
> >   		return -EINVAL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Steve Longerbeam]

Hi Laurent,

On 3/9/20 1:52 PM, Laurent Pinchart wrote:
> Hi Steve,
>
> On Thu, Oct 24, 2019 at 08:11:20AM -0700, Steve Longerbeam wrote:
>> On 10/23/19 5:41 PM, Laurent Pinchart wrote:
>>> Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
>>> source pads") reworked the way that formats are set on the sink pad of
>>> the CSI subdevice, and accidentally removed video field handling.
>> Well, let me restate the problem. This driver never did have correct
>> field handling (as you demonstrate in this patch, the driver never
>> set/propagated the field type to its source pad). So it's not accurate
>> to say the patch is a fix. A better description is that it adds
>> rudimentary field handling.
> Didn't it ? The above commit removed a call to
> imx_media_fill_default_mbus_fields() from imx7_csi_try_fmt(), and that
> function had rudimentary field handling. The Fixes: tag isn't there to
> blame you :-)

Sorry, you're right, 4791bd7d6adc should have placed some sane field 
value back at sink pad in imx7_csi_try_fmt(). But the problem is that at 
the time, I didn't know what a sane field value for imx7 would be (your 
patch resolved that question, by stating only NONE and INTERLACED are 
supported). But I must stand by my statement that field was never 
propagated to source. I guess I was waiting for someone to implement 
basic field handling, instead of possibly doing the wrong thing myself. 
I should have clarified at the time that there needed to be basic field 
handling added.

Steve

>
>>> Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
>>> supported, with the only two supported value being V4L2_FIELD_NONE and
>>> V4L2_FIELD_ALTERNATE.
>>>
>>> Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>>> ---
>>>    drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
>>>    1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
>>> index ac07f55a63e3..0db6473caf13 100644
>>> --- a/drivers/staging/media/imx/imx7-media-csi.c
>>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
>>> @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>    		sdformat->format.width = in_fmt->width;
>>>    		sdformat->format.height = in_fmt->height;
>>>    		sdformat->format.code = in_fmt->code;
>>> +		sdformat->format.field = in_fmt->field;
>>>    		*cc = in_cc;
>>>    
>>>    		sdformat->format.colorspace = in_fmt->colorspace;
>>> @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
>>>    							 false);
>>>    			sdformat->format.code = (*cc)->codes[0];
>>>    		}
>>> +
>>> +		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
>>> +			sdformat->format.field = V4L2_FIELD_NONE;
>>>    		break;
>>>    	default:
>>>    		return -EINVAL;

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Laurent Pinchart]

Hi Steve,

On Mon, Mar 09, 2020 at 05:00:02PM -0700, Steve Longerbeam wrote:
> On 3/9/20 1:52 PM, Laurent Pinchart wrote:
> > On Thu, Oct 24, 2019 at 08:11:20AM -0700, Steve Longerbeam wrote:
> >> On 10/23/19 5:41 PM, Laurent Pinchart wrote:
> >>> Commit 4791bd7d6adc ("media: imx: Try colorimetry at both sink and
> >>> source pads") reworked the way that formats are set on the sink pad of
> >>> the CSI subdevice, and accidentally removed video field handling.
> >
> >> Well, let me restate the problem. This driver never did have correct
> >> field handling (as you demonstrate in this patch, the driver never
> >> set/propagated the field type to its source pad). So it's not accurate
> >> to say the patch is a fix. A better description is that it adds
> >> rudimentary field handling.
> >
> > Didn't it ? The above commit removed a call to
> > imx_media_fill_default_mbus_fields() from imx7_csi_try_fmt(), and that
> > function had rudimentary field handling. The Fixes: tag isn't there to
> > blame you :-)
> 
> Sorry, you're right, 4791bd7d6adc should have placed some sane field 
> value back at sink pad in imx7_csi_try_fmt(). But the problem is that at 
> the time, I didn't know what a sane field value for imx7 would be (your 
> patch resolved that question, by stating only NONE and INTERLACED are 
> supported). But I must stand by my statement that field was never 
> propagated to source. I guess I was waiting for someone to implement 
> basic field handling, instead of possibly doing the wrong thing myself. 
> I should have clarified at the time that there needed to be basic field 
> handling added.

We all know there's a bit of work needed on this driver :-) Hence the
staging status. I'll post a v2 of this series, and additional patches
will follow. I will likely not address interlaced support though, as I
have no system to test that on, but you can expect improvements for the
rest of the code, especially the parts used with CSI-2 sensors.

> >>> Restore it by defaulting to V4L2_FIELD_NONE if the field value isn't
> >>> supported, with the only two supported value being V4L2_FIELD_NONE and
> >>> V4L2_FIELD_ALTERNATE.
> >>>
> >>> Fixes: 4791bd7d6adc ("media: imx: Try colorimetry at both sink and source pads")
> >>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> ---
> >>>    drivers/staging/media/imx/imx7-media-csi.c | 4 ++++
> >>>    1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/staging/media/imx/imx7-media-csi.c b/drivers/staging/media/imx/imx7-media-csi.c
> >>> index ac07f55a63e3..0db6473caf13 100644
> >>> --- a/drivers/staging/media/imx/imx7-media-csi.c
> >>> +++ b/drivers/staging/media/imx/imx7-media-csi.c
> >>> @@ -1017,6 +1017,7 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> >>>    		sdformat->format.width = in_fmt->width;
> >>>    		sdformat->format.height = in_fmt->height;
> >>>    		sdformat->format.code = in_fmt->code;
> >>> +		sdformat->format.field = in_fmt->field;
> >>>    		*cc = in_cc;
> >>>    
> >>>    		sdformat->format.colorspace = in_fmt->colorspace;
> >>> @@ -1031,6 +1032,9 @@ static int imx7_csi_try_fmt(struct imx7_csi *csi,
> >>>    							 false);
> >>>    			sdformat->format.code = (*cc)->codes[0];
> >>>    		}
> >>> +
> >>> +		if (sdformat->format.field != V4L2_FIELD_INTERLACED)
> >>> +			sdformat->format.field = V4L2_FIELD_NONE;
> >>>    		break;
> >>>    	default:
> >>>    		return -EINVAL;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Laurent Pinchart]

Hi Steve and Rui,

I've spent more time on the i.MX7 support in the i.MX media staging
driver today, and I've reached a point where I'm not comfortable moving
forward without your ack.

I found format handling to be very broken, the driver enumerates formats
that are not supported by the device, and doesn't properly handle the
supported formats. While trying to fix that, I found out that the common
i.MX6 and i.MX7 helpers (imx-media-capture.c and imx-media-utils.c) get
in the way, as i.MX6 and i.MX7 format support are very entangled. I
would like to split the two in order to clean up the i.MX7 code, which
would also give an opportunity to later clean the i.MX6 code if desired.

Before moving in that time-consuming direction, I want to make sure this
will be accepted, as I don't want to spend days of work for nothing. If
you want to discuss this in real time, I'm available in the #v4l channel
on Freenode (nickname pinchartl).

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Steve Longerbeam]

Hi Laurent,


On 3/10/20 8:49 AM, Laurent Pinchart wrote:
> Hi Steve and Rui,
>
> I've spent more time on the i.MX7 support in the i.MX media staging
> driver today, and I've reached a point where I'm not comfortable moving
> forward without your ack.
>
> I found format handling to be very broken, the driver enumerates formats
> that are not supported by the device, and doesn't properly handle the
> supported formats. While trying to fix that, I found out that the common
> i.MX6 and i.MX7 helpers (imx-media-capture.c and imx-media-utils.c) get
> in the way, as i.MX6 and i.MX7 format support are very entangled. I
> would like to split the two in order to clean up the i.MX7 code, which
> would also give an opportunity to later clean the i.MX6 code if desired.
>
> Before moving in that time-consuming direction, I want to make sure this
> will be accepted, as I don't want to spend days of work for nothing. If
> you want to discuss this in real time, I'm available in the #v4l channel
> on Freenode (nickname pinchartl).

I'm on vacation returning March 16, maybe we can chat on irc when I 
return. I'm in California (PDT).

Steve

Re: [PATCH 7/7] media: imx: imx7-media-csi: Fix video field handling

[Laurent Pinchart]

Hi Steve,

On Wed, Mar 11, 2020 at 04:54:35PM -0700, Steve Longerbeam wrote:
> On 3/10/20 8:49 AM, Laurent Pinchart wrote:
> > Hi Steve and Rui,
> >
> > I've spent more time on the i.MX7 support in the i.MX media staging
> > driver today, and I've reached a point where I'm not comfortable moving
> > forward without your ack.
> >
> > I found format handling to be very broken, the driver enumerates formats
> > that are not supported by the device, and doesn't properly handle the
> > supported formats. While trying to fix that, I found out that the common
> > i.MX6 and i.MX7 helpers (imx-media-capture.c and imx-media-utils.c) get
> > in the way, as i.MX6 and i.MX7 format support are very entangled. I
> > would like to split the two in order to clean up the i.MX7 code, which
> > would also give an opportunity to later clean the i.MX6 code if desired.
> >
> > Before moving in that time-consuming direction, I want to make sure this
> > will be accepted, as I don't want to spend days of work for nothing. If
> > you want to discuss this in real time, I'm available in the #v4l channel
> > on Freenode (nickname pinchartl).
> 
> I'm on vacation returning March 16, maybe we can chat on irc when I 
> return. I'm in California (PDT).

Sure. Enjoy your vacation :-)

In the meantime, I've moved forward with development without duplicating
the whole imx-media-capture.c file, and I believe I'm close to reaching
a working result. Some parts were not pretty (it's a staging driver
after all), and if you agree with my approach, I think the end result
will be much cleaner. Let's talk about it when you come back.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/7] media: imx: Miscellaneous fixes for i.MX7

[Rui Miguel Silva]

Hi Laurent,
Many thanks for this patches.
On Thu 24 Oct 2019 at 01:41, Laurent Pinchart wrote:
> Hello,
>
> This patch series contains miscellaneous fixes I developed while trying
> to capture from a CSI-2 sensor on the i.MX7.
>
> Patches 1/7, 2/7 and 6/7 are small fixes or enhancements, please see
> individual commit messages. Patches 3/7, 4/7 and 5/7 add support for 10-
> and 12-bit greyscale formats (both on the i.MX7 CSI-2 receiver side and
> the CSI side). Patch 7/7 finally fixes a recent issue with video field
> handling.
>
> Laurent Pinchart (7):
>   media: imx: imx7_mipi_csis: Power off the source when stopping
>     streaming
>   media: imx: imx7_mipi_csis: Print the RESOL_CH0 register
>   media: imx: imx7_mipi_csis: Add greyscale formats support
>   media: imx: imx6-media-csi: Replace Y16 with Y10 and Y12
>   media: imx: imx7-media-csi: Add Y10 and Y12 formats support
>   media: imx: imx7-media-csi: Remove unneeded register read
>   media: imx: imx7-media-csi: Fix video field handling
>

For the all series, if the remarks from Steve and Fabio regarding
the fixes tags and changelog in 7/7 get fixed:

Reviewed-by: Rui Miguel Silva <rmfrfs@gmail.com>

---
Cheers,
	Rui


>
>  drivers/staging/media/imx/imx-media-csi.c   |  3 ++-
>  drivers/staging/media/imx/imx-media-utils.c | 13 ++++++++-----
>  drivers/staging/media/imx/imx7-media-csi.c  | 14 +++++++++++++-
>  drivers/staging/media/imx/imx7-mipi-csis.c  | 15 ++++++++++++++-
>  4 files changed, 37 insertions(+), 8 deletions(-)