[PATCH] media: stm32-dcmi: add support of BT656 bus

[PATCH] media: stm32-dcmi: add support of BT656 bus

[Hugues Fruchet]

Add support of BT656 embedded synchronization bus.
This mode allows to save hardware synchro lines hsync & vsync
by replacing them with synchro codes embedded in data stream.
This bus type is only compatible with 8 bits width data bus.
Due to reserved values 0x00 & 0xff used for synchro codes,
valid data only vary from 0x1 to 0xfe, this is up to sensor
to clip accordingly pixel data. As a consequence of this
clipping, JPEG is not supported when using this bus type.
DCMI crop feature is also not available with this bus type.

Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
---
 drivers/media/platform/stm32/stm32-dcmi.c | 37 +++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index fd1c41c..d7d7cdb 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -157,6 +157,7 @@ struct stm32_dcmi {
 	struct vb2_queue		queue;
 
 	struct v4l2_fwnode_bus_parallel	bus;
+	enum v4l2_mbus_type		bus_type;
 	struct completion		complete;
 	struct clk			*mclk;
 	enum state			state;
@@ -777,6 +778,23 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
 		val |= CR_PCKPOL;
 
+	/*
+	 * BT656 embedded synchronisation bus mode.
+	 *
+	 * Default SAV/EAV mode is supported here with default codes
+	 * SAV=0xff000080 & EAV=0xff00009d.
+	 * With DCMI this means LSC=SAV=0x80 & LEC=EAV=0x9d.
+	 */
+	if (dcmi->bus_type == V4L2_MBUS_BT656) {
+		val |= CR_ESS;
+
+		/* Unmask all codes */
+		reg_write(dcmi->regs, DCMI_ESUR, 0xffffffff);/* FEC:LEC:LSC:FSC */
+
+		/* Trig on LSC=0x80 & LEC=0x9d codes, ignore FSC and FEC */
+		reg_write(dcmi->regs, DCMI_ESCR, 0xff9d80ff);/* FEC:LEC:LSC:FSC */
+	}
+
 	reg_write(dcmi->regs, DCMI_CR, val);
 
 	/* Set crop */
@@ -1067,8 +1085,9 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
 	if (ret)
 		return ret;
 
-	/* Disable crop if JPEG is requested */
-	if (pix->pixelformat == V4L2_PIX_FMT_JPEG)
+	/* Disable crop if JPEG is requested or BT656 bus is selected */
+	if (pix->pixelformat == V4L2_PIX_FMT_JPEG &&
+	    dcmi->bus_type != V4L2_MBUS_BT656)
 		dcmi->do_crop = false;
 
 	/* pix to mbus format */
@@ -1592,6 +1611,11 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
 			if (dcmi_formats[i].mbus_code != mbus_code.code)
 				continue;
 
+			/* Exclude JPEG if BT656 bus is selected */
+			if (dcmi_formats[i].fourcc == V4L2_PIX_FMT_JPEG &&
+			    dcmi->bus_type == V4L2_MBUS_BT656)
+				continue;
+
 			/* Code supported, have we got this fourcc yet? */
 			for (j = 0; j < num_fmts; j++)
 				if (sd_fmts[j]->fourcc ==
@@ -1873,9 +1897,18 @@ static int dcmi_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "CSI bus not supported\n");
 		return -ENODEV;
 	}
+
+	if (ep.bus_type == V4L2_MBUS_BT656 &&
+	    ep.bus.parallel.bus_width != 8) {
+		dev_err(&pdev->dev, "BT656 bus conflicts with %d bits bus width (8 bits required)\n",
+			ep.bus.parallel.bus_width);
+		return -ENODEV;
+	}
+
 	dcmi->bus.flags = ep.bus.parallel.flags;
 	dcmi->bus.bus_width = ep.bus.parallel.bus_width;
 	dcmi->bus.data_shift = ep.bus.parallel.data_shift;
+	dcmi->bus_type = ep.bus_type;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq <= 0)
-- 
2.7.4

Re: [PATCH] media: stm32-dcmi: add support of BT656 bus

[Sakari Ailus]

Hi Hugues,

On Wed, Oct 07, 2020 at 06:14:50PM +0200, Hugues Fruchet wrote:
> Add support of BT656 embedded synchronization bus.
> This mode allows to save hardware synchro lines hsync & vsync
> by replacing them with synchro codes embedded in data stream.
> This bus type is only compatible with 8 bits width data bus.
> Due to reserved values 0x00 & 0xff used for synchro codes,
> valid data only vary from 0x1 to 0xfe, this is up to sensor
> to clip accordingly pixel data. As a consequence of this
> clipping, JPEG is not supported when using this bus type.
> DCMI crop feature is also not available with this bus type.

You can have more than 62 characters per line. In fact, 75 is the
recommended maximum.

You should also amend the bindings to cover BT.656 mode. Also bus-type
should probably be made mandatory, too.

> 
> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
> ---
>  drivers/media/platform/stm32/stm32-dcmi.c | 37 +++++++++++++++++++++++++++++--
>  1 file changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index fd1c41c..d7d7cdb 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -157,6 +157,7 @@ struct stm32_dcmi {
>  	struct vb2_queue		queue;
>  
>  	struct v4l2_fwnode_bus_parallel	bus;
> +	enum v4l2_mbus_type		bus_type;
>  	struct completion		complete;
>  	struct clk			*mclk;
>  	enum state			state;
> @@ -777,6 +778,23 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>  		val |= CR_PCKPOL;
>  
> +	/*
> +	 * BT656 embedded synchronisation bus mode.
> +	 *
> +	 * Default SAV/EAV mode is supported here with default codes
> +	 * SAV=0xff000080 & EAV=0xff00009d.
> +	 * With DCMI this means LSC=SAV=0x80 & LEC=EAV=0x9d.
> +	 */
> +	if (dcmi->bus_type == V4L2_MBUS_BT656) {
> +		val |= CR_ESS;
> +
> +		/* Unmask all codes */
> +		reg_write(dcmi->regs, DCMI_ESUR, 0xffffffff);/* FEC:LEC:LSC:FSC */
> +
> +		/* Trig on LSC=0x80 & LEC=0x9d codes, ignore FSC and FEC */
> +		reg_write(dcmi->regs, DCMI_ESCR, 0xff9d80ff);/* FEC:LEC:LSC:FSC */
> +	}
> +
>  	reg_write(dcmi->regs, DCMI_CR, val);
>  
>  	/* Set crop */
> @@ -1067,8 +1085,9 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
>  	if (ret)
>  		return ret;
>  
> -	/* Disable crop if JPEG is requested */
> -	if (pix->pixelformat == V4L2_PIX_FMT_JPEG)
> +	/* Disable crop if JPEG is requested or BT656 bus is selected */
> +	if (pix->pixelformat == V4L2_PIX_FMT_JPEG &&
> +	    dcmi->bus_type != V4L2_MBUS_BT656)
>  		dcmi->do_crop = false;
>  
>  	/* pix to mbus format */
> @@ -1592,6 +1611,11 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
>  			if (dcmi_formats[i].mbus_code != mbus_code.code)
>  				continue;
>  
> +			/* Exclude JPEG if BT656 bus is selected */
> +			if (dcmi_formats[i].fourcc == V4L2_PIX_FMT_JPEG &&
> +			    dcmi->bus_type == V4L2_MBUS_BT656)
> +				continue;
> +
>  			/* Code supported, have we got this fourcc yet? */
>  			for (j = 0; j < num_fmts; j++)
>  				if (sd_fmts[j]->fourcc ==
> @@ -1873,9 +1897,18 @@ static int dcmi_probe(struct platform_device *pdev)
>  		dev_err(&pdev->dev, "CSI bus not supported\n");
>  		return -ENODEV;
>  	}
> +
> +	if (ep.bus_type == V4L2_MBUS_BT656 &&
> +	    ep.bus.parallel.bus_width != 8) {
> +		dev_err(&pdev->dev, "BT656 bus conflicts with %d bits bus width (8 bits required)\n",
> +			ep.bus.parallel.bus_width);

bus_width is unsigned here.

> +		return -ENODEV;
> +	}
> +
>  	dcmi->bus.flags = ep.bus.parallel.flags;
>  	dcmi->bus.bus_width = ep.bus.parallel.bus_width;
>  	dcmi->bus.data_shift = ep.bus.parallel.data_shift;
> +	dcmi->bus_type = ep.bus_type;
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq <= 0)

-- 
Regards,

Sakari Ailus

Re: [PATCH] media: stm32-dcmi: add support of BT656 bus

[Hugues FRUCHET]

Hi Sakari,

Thanks for reviewing,

On 10/13/20 11:07 AM, Sakari Ailus wrote:
> Hi Hugues,
> 
> On Wed, Oct 07, 2020 at 06:14:50PM +0200, Hugues Fruchet wrote:
>> Add support of BT656 embedded synchronization bus.
>> This mode allows to save hardware synchro lines hsync & vsync
>> by replacing them with synchro codes embedded in data stream.
>> This bus type is only compatible with 8 bits width data bus.
>> Due to reserved values 0x00 & 0xff used for synchro codes,
>> valid data only vary from 0x1 to 0xfe, this is up to sensor
>> to clip accordingly pixel data. As a consequence of this
>> clipping, JPEG is not supported when using this bus type.
>> DCMI crop feature is also not available with this bus type.
> 
> You can have more than 62 characters per line. In fact, 75 is the
> recommended maximum.
> 
> You should also amend the bindings to cover BT.656 mode. Also bus-type
> should probably be made mandatory, too.
Will do both.

> 
>>
>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>> ---
>>   drivers/media/platform/stm32/stm32-dcmi.c | 37 +++++++++++++++++++++++++++++--
>>   1 file changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
>> index fd1c41c..d7d7cdb 100644
>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>> @@ -157,6 +157,7 @@ struct stm32_dcmi {
>>   	struct vb2_queue		queue;
>>   
>>   	struct v4l2_fwnode_bus_parallel	bus;
>> +	enum v4l2_mbus_type		bus_type;
>>   	struct completion		complete;
>>   	struct clk			*mclk;
>>   	enum state			state;
>> @@ -777,6 +778,23 @@ static int dcmi_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>>   		val |= CR_PCKPOL;
>>   
>> +	/*
>> +	 * BT656 embedded synchronisation bus mode.
>> +	 *
>> +	 * Default SAV/EAV mode is supported here with default codes
>> +	 * SAV=0xff000080 & EAV=0xff00009d.
>> +	 * With DCMI this means LSC=SAV=0x80 & LEC=EAV=0x9d.
>> +	 */
>> +	if (dcmi->bus_type == V4L2_MBUS_BT656) {
>> +		val |= CR_ESS;
>> +
>> +		/* Unmask all codes */
>> +		reg_write(dcmi->regs, DCMI_ESUR, 0xffffffff);/* FEC:LEC:LSC:FSC */
>> +
>> +		/* Trig on LSC=0x80 & LEC=0x9d codes, ignore FSC and FEC */
>> +		reg_write(dcmi->regs, DCMI_ESCR, 0xff9d80ff);/* FEC:LEC:LSC:FSC */
>> +	}
>> +
>>   	reg_write(dcmi->regs, DCMI_CR, val);
>>   
>>   	/* Set crop */
>> @@ -1067,8 +1085,9 @@ static int dcmi_set_fmt(struct stm32_dcmi *dcmi, struct v4l2_format *f)
>>   	if (ret)
>>   		return ret;
>>   
>> -	/* Disable crop if JPEG is requested */
>> -	if (pix->pixelformat == V4L2_PIX_FMT_JPEG)
>> +	/* Disable crop if JPEG is requested or BT656 bus is selected */
>> +	if (pix->pixelformat == V4L2_PIX_FMT_JPEG &&
>> +	    dcmi->bus_type != V4L2_MBUS_BT656)
>>   		dcmi->do_crop = false;
>>   
>>   	/* pix to mbus format */
>> @@ -1592,6 +1611,11 @@ static int dcmi_formats_init(struct stm32_dcmi *dcmi)
>>   			if (dcmi_formats[i].mbus_code != mbus_code.code)
>>   				continue;
>>   
>> +			/* Exclude JPEG if BT656 bus is selected */
>> +			if (dcmi_formats[i].fourcc == V4L2_PIX_FMT_JPEG &&
>> +			    dcmi->bus_type == V4L2_MBUS_BT656)
>> +				continue;
>> +
>>   			/* Code supported, have we got this fourcc yet? */
>>   			for (j = 0; j < num_fmts; j++)
>>   				if (sd_fmts[j]->fourcc ==
>> @@ -1873,9 +1897,18 @@ static int dcmi_probe(struct platform_device *pdev)
>>   		dev_err(&pdev->dev, "CSI bus not supported\n");
>>   		return -ENODEV;
>>   	}
>> +
>> +	if (ep.bus_type == V4L2_MBUS_BT656 &&
>> +	    ep.bus.parallel.bus_width != 8) {
>> +		dev_err(&pdev->dev, "BT656 bus conflicts with %d bits bus width (8 bits required)\n",
>> +			ep.bus.parallel.bus_width);
> 
> bus_width is unsigned here.
I will fix it.

> 
>> +		return -ENODEV;
>> +	}
>> +
>>   	dcmi->bus.flags = ep.bus.parallel.flags;
>>   	dcmi->bus.bus_width = ep.bus.parallel.bus_width;
>>   	dcmi->bus.data_shift = ep.bus.parallel.data_shift;
>> +	dcmi->bus_type = ep.bus_type;
>>   
>>   	irq = platform_get_irq(pdev, 0);
>>   	if (irq <= 0)
> 

BR,
Hugues.

Re: [PATCH] media: stm32-dcmi: add support of BT656 bus

[Hugues FRUCHET]

Hi Sakari,

I have questions about "bus-type" handling below.

On 10/15/20 5:41 PM, Hugues FRUCHET wrote:
> Hi Sakari,
> 
> Thanks for reviewing,
> 
> On 10/13/20 11:07 AM, Sakari Ailus wrote:
>> Hi Hugues,
>>
>> On Wed, Oct 07, 2020 at 06:14:50PM +0200, Hugues Fruchet wrote:
>>> Add support of BT656 embedded synchronization bus.
>>> This mode allows to save hardware synchro lines hsync & vsync
>>> by replacing them with synchro codes embedded in data stream.
>>> This bus type is only compatible with 8 bits width data bus.
>>> Due to reserved values 0x00 & 0xff used for synchro codes,
>>> valid data only vary from 0x1 to 0xfe, this is up to sensor
>>> to clip accordingly pixel data. As a consequence of this
>>> clipping, JPEG is not supported when using this bus type.
>>> DCMI crop feature is also not available with this bus type.
>>
>> You can have more than 62 characters per line. In fact, 75 is the
>> recommended maximum.
>>
>> You should also amend the bindings to cover BT.656 mode. Also bus-type
>> should probably be made mandatory, too.
> Will do both.
> 

My understanding was that parallel BT656 bus is handled by the absence 
of hsync/vsync-active, as stated in 
Documentation/devicetree/bindings/media/video-interfaces.txt:
"  Note, that if HSYNC and VSYNC polarities are not specified, embedded
   synchronization may be required, where supported. "

Do you want to enforce usage of "bus-type" now in order to be more 
explicit on parallel or bt656 ?
If I change binding to make "bus-type" required, I have to change the 
current board devicetree files to add support of it, and I would prefer 
to not do that. Is there a way to put "bus-type" as not mandatory and 
rely on absence of hsync/vysnc to trig BT656 ? I will nevertheless amend 
bindings in order to document that.
What do you suggest ?

>>
>>>
>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com>
>>> ---
>>>   drivers/media/platform/stm32/stm32-dcmi.c | 37 
>>> +++++++++++++++++++++++++++++--
>>>   1 file changed, 35 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c 
>>> b/drivers/media/platform/stm32/stm32-dcmi.c
>>> index fd1c41c..d7d7cdb 100644
>>> --- a/drivers/media/platform/stm32/stm32-dcmi.c
>>> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
>>> @@ -157,6 +157,7 @@ struct stm32_dcmi {
>>>       struct vb2_queue        queue;
>>>       struct v4l2_fwnode_bus_parallel    bus;
>>> +    enum v4l2_mbus_type        bus_type;
>>>       struct completion        complete;
>>>       struct clk            *mclk;
>>>       enum state            state;
>>> @@ -777,6 +778,23 @@ static int dcmi_start_streaming(struct vb2_queue 
>>> *vq, unsigned int count)
>>>       if (dcmi->bus.flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
>>>           val |= CR_PCKPOL;
>>> +    /*
>>> +     * BT656 embedded synchronisation bus mode.
>>> +     *
>>> +     * Default SAV/EAV mode is supported here with default codes
>>> +     * SAV=0xff000080 & EAV=0xff00009d.
>>> +     * With DCMI this means LSC=SAV=0x80 & LEC=EAV=0x9d.
>>> +     */
>>> +    if (dcmi->bus_type == V4L2_MBUS_BT656) {
>>> +        val |= CR_ESS;
>>> +
>>> +        /* Unmask all codes */
>>> +        reg_write(dcmi->regs, DCMI_ESUR, 0xffffffff);/* 
>>> FEC:LEC:LSC:FSC */
>>> +
>>> +        /* Trig on LSC=0x80 & LEC=0x9d codes, ignore FSC and FEC */
>>> +        reg_write(dcmi->regs, DCMI_ESCR, 0xff9d80ff);/* 
>>> FEC:LEC:LSC:FSC */
>>> +    }
>>> +
>>>       reg_write(dcmi->regs, DCMI_CR, val);
>>>       /* Set crop */
>>> @@ -1067,8 +1085,9 @@ static int dcmi_set_fmt(struct stm32_dcmi 
>>> *dcmi, struct v4l2_format *f)
>>>       if (ret)
>>>           return ret;
>>> -    /* Disable crop if JPEG is requested */
>>> -    if (pix->pixelformat == V4L2_PIX_FMT_JPEG)
>>> +    /* Disable crop if JPEG is requested or BT656 bus is selected */
>>> +    if (pix->pixelformat == V4L2_PIX_FMT_JPEG &&
>>> +        dcmi->bus_type != V4L2_MBUS_BT656)
>>>           dcmi->do_crop = false;
>>>       /* pix to mbus format */
>>> @@ -1592,6 +1611,11 @@ static int dcmi_formats_init(struct stm32_dcmi 
>>> *dcmi)
>>>               if (dcmi_formats[i].mbus_code != mbus_code.code)
>>>                   continue;
>>> +            /* Exclude JPEG if BT656 bus is selected */
>>> +            if (dcmi_formats[i].fourcc == V4L2_PIX_FMT_JPEG &&
>>> +                dcmi->bus_type == V4L2_MBUS_BT656)
>>> +                continue;
>>> +
>>>               /* Code supported, have we got this fourcc yet? */
>>>               for (j = 0; j < num_fmts; j++)
>>>                   if (sd_fmts[j]->fourcc ==
>>> @@ -1873,9 +1897,18 @@ static int dcmi_probe(struct platform_device 
>>> *pdev)
>>>           dev_err(&pdev->dev, "CSI bus not supported\n");
>>>           return -ENODEV;
>>>       }
>>> +
>>> +    if (ep.bus_type == V4L2_MBUS_BT656 &&
>>> +        ep.bus.parallel.bus_width != 8) {
>>> +        dev_err(&pdev->dev, "BT656 bus conflicts with %d bits bus 
>>> width (8 bits required)\n",
>>> +            ep.bus.parallel.bus_width);
>>
>> bus_width is unsigned here.
> I will fix it.
> 
>>
>>> +        return -ENODEV;
>>> +    }
>>> +
>>>       dcmi->bus.flags = ep.bus.parallel.flags;
>>>       dcmi->bus.bus_width = ep.bus.parallel.bus_width;
>>>       dcmi->bus.data_shift = ep.bus.parallel.data_shift;
>>> +    dcmi->bus_type = ep.bus_type;
>>>       irq = platform_get_irq(pdev, 0);
>>>       if (irq <= 0)
>>
> 
> BR,
> Hugues.

BR,
Hugues.