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@xxxxxx>
>>> ---
>>>   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.