Re: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem driver

[Hans Verkuil <hverkuil@xs4all.nl>]

On 15/09/2020 04:38, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thu, Sep 10, 2020 at 12:22:28PM +0200, Hans Verkuil wrote:
>> On 26/08/2020 16:10, Nicolas Dufresne wrote:
>>> Le mercredi 19 août 2020 à 19:56 +0300, Laurent Pinchart a écrit :
>>>> Hi Vishal,
>>>>
>>>> (Hans, there's a question for you below)
>>>>
>>>> On Wed, Aug 19, 2020 at 01:47:49PM +0000, Vishal Sagar wrote:
>>>>> On Thursday, July 16, 2020 3:03 AM Laurent Pinchart wrote:
>>>>>> On Thu, Jun 25, 2020 at 11:43:01AM +0200, Hans Verkuil wrote:
>>>>>>> On 18/06/2020 07:33, Vishal Sagar wrote:
>>>>>>>> The Xilinx UHD-SDI Rx subsystem soft IP is used to capture native SDI
>>>>>>>> streams from SDI sources like SDI broadcast equipment like cameras and
>>>>>>>> mixers. This block outputs either native SDI, native video or
>>>>>>>> AXI4-Stream compliant data stream for further processing. Please refer
>>>>>>>> to PG290 for details.
>>>>>>>>
>>>>>>>> The driver is used to configure the IP to add framer, search for
>>>>>>>> specific modes, get the detected mode, stream parameters, errors, etc.
>>>>>>>> It also generates events for video lock/unlock, bridge over/under flow.
>>>>>>>>
>>>>>>>> The driver supports 10/12 bpc YUV 422 media bus format currently. It
>>>>>>>> also decodes the stream parameters based on the ST352 packet embedded in the
>>>>>>>> stream. In case the ST352 packet isn't present in the stream, the core's
>>>>>>>> detected properties are used to set stream properties.
>>>>>>>>
>>>>>>>> The driver currently supports only the AXI4-Stream IP configuration.
>>>>>>>>
>>>>>>>> Signed-off-by: Vishal Sagar <vishal.sagar@xilinx.com>
>>>>>>>> ---
>>>>>>>> v3
>>>>>>>> - fixed KConfig with better description
>>>>>>>> - removed unnecessary header files
>>>>>>>> - converted uppercase to lowercase for all hex values
>>>>>>>> - merged core struct to state struct
>>>>>>>> - removed most one line functions and replaced with direct reg
>>>>>>>>   read/write or macros
>>>>>>>> - dt property bpp to bpc. default 10. not mandatory.
>>>>>>>> - fixed subscribe events, log_status, s_stream
>>>>>>>> - merged overflow/underflow to one event
>>>>>>>> - moved all controls to xilinx-sdirxss.h
>>>>>>>> - max events from 128 to 8
>>>>>>>> - used FIELD_GET() instead of custom macro
>>>>>>>> - updated the controls documentation
>>>>>>>> - added spinlock
>>>>>>>> - removed 3GB control and added mode to detect bitmask
>>>>>>>> - fixed format for (width, height, colorspace, xfer func, etc)
>>>>>>>> - added dv_timings_cap, s/g_dv_timings
>>>>>>>> - fixed set/get_format
>>>>>>>> - fix v4l control registrations
>>>>>>>> - fix order of registration / deregistration in probe() remove()
>>>>>>>> - fixed other comments from Hyun, Laurent and Hans
>>>>>>>> - things yet to close
>>>>>>>>   - adding source port for connector (Laurent's suggestion)
>>>>>>>>   - adding new FIELD type for Transport Stream V4L2_FIELD_ALTERNATE_PROG (Han's suggestion)
>>>>>>>>   - Update / remove EDH or CRC related controls
>>>>>>>>
>>>>>>>> v2
>>>>>>>> - Added DV timing support based on Hans Verkuilś feedback
>>>>>>>> - More documentation to custom v4l controls and events
>>>>>>>> - Fixed Hyunś comments
>>>>>>>> - Added macro for masking and shifting as per Joe Perches comments
>>>>>>>> - Updated to latest as per Xilinx github repo driver like
>>>>>>>>   adding new DV timings not in mainline yet uptill 03/21/20
>>>>>>>>
>>>>>>>>  drivers/media/platform/xilinx/Kconfig         |   11 +
>>>>>>>>  drivers/media/platform/xilinx/Makefile        |    1 +
>>>>>>>>  .../media/platform/xilinx/xilinx-sdirxss.c    | 2121 +++++++++++++++++
>>>>>>>>  include/uapi/linux/v4l2-controls.h            |    6 +
>>>>>>>>  include/uapi/linux/xilinx-sdirxss.h           |  283 +++
>>>>>>>>  5 files changed, 2422 insertions(+)
>>>>>>>>  create mode 100644 drivers/media/platform/xilinx/xilinx-sdirxss.c
>>>>>>>>  create mode 100644 include/uapi/linux/xilinx-sdirxss.h
>>>>
>>>> [snip]
>>>>
>>>>>>>> diff --git a/drivers/media/platform/xilinx/xilinx-sdirxss.c b/drivers/media/platform/xilinx/xilinx-sdirxss.c
>>>>>>>> new file mode 100644
>>>>>>>> index 000000000000..e39aab7c656a
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/media/platform/xilinx/xilinx-sdirxss.c
>>>>>>>> @@ -0,0 +1,2121 @@
>>>>
>>>> [snip]
>>>>
>>>>>>>> +	case V4L2_CID_XILINX_SDIRX_TS_IS_INTERLACED:
>>>>>>>> +		ctrl->val = xsdirxss->ts_is_interlaced;
>>>>>>>> +		break;
>>>>>>>
>>>>>>> I assume this control will disappear once you added support for
>>>>>>> FIELD_ALTERNATE_PROG?
>>>>>>
>>>>>> I'm not sure FIELD_ALTERNATE_PROG is a good idea. The v4l2_field
>>>>>> specifies today how frames are split into multiple buffers. There's an
>>>>>> implicit assumption that a frame split into two buffers is captured with
>>>>>> interlacing. In the SDI case, the two concepts get decoupled, a
>>>>>> progressive frame can be transmitted (and captured) in two separate
>>>>>> parts. If we add a *_PROG field, we'll need to duplicate most of the
>>>>>> v4l2_field values with a _PROG suffix, as the progressive frame can be
>>>>>> captured in alternate buffers on a video node, but also in separate odd
>>>>>> and even buffers on two video nodes. Tt the hardware level, data is
>>>>>> transmitted with odd lines on one link, and even lines on a second link.
>>>>>> There are then two instances of this IP core, one for each link. One
>>>>>> instance would receive and process the even lines, the other instance
>>>>>> the odd lines. The output of the two instances can then be connected to
>>>>>> two separate DMA engines, or combined in the FPGA fabric, depending on
>>>>>> how the user designs the system.
>>>>>
>>>>> My apologies to give incorrect info regarding this.
>>>>> In the progressive segmented frame, a progressive captured frame is sent
>>>>> across to receiver over an interlaced transport. The 2 fields received
>>>>> are similar to how V4L2_FIELD_ALTERNATE is except that the fields weren't
>>>>> captured at 2 different times.
>>>>
>>>> I've now read more about progressive segmented frames, and I was indeed
>>>> wrong about the fact that the two segments are transported over
>>>> different links.
>>>>
>>>> I still wonder, however, if a _PROG suffix is the best option. Wouldn't
>>>> we need to also add V4L2_FIELD_TOP_PROG, V4L2_FIELD_BOTTOM_PROG,
>>>> V4L2_FIELD_SEQ_TB_PROG and V4L2_FIELD_SEQ_BT_PROG, not necessarily for
>>>> this driver, but for other devices that would support capturing the
>>>> odd/even segments only, or support capturing both segments in a single
>>>> buffer, one after the other ?
>>>>
>>>> Maybe that's unavoidable, as enum v4l2_field combines both the buffer
>>>> layout and the fact that the frame is interlaced or progressive. If we
>>>> had to redesign it we could do better, but having to keep backward
>>>> compatibility, duplicating most values with a _PROG suffix may be the
>>>> best option.
>>>>
>>>> Hans, any opinion ?
>>
>> I don't believe there is any need to create those other V4L2_FIELD_ variants.
>> With V4L2_FIELD_ALTERNATE_PROG each buffer will be set to V4L2_FIELD_TOP (i.e.
>> odd lines) or V4L2_FIELD_BOTTOM (i.e. even lines).
> 
> What if an application wants to capture TOP or BOTTOM fields only though
> ? The DMA engine would need to be configured with either
> V4L2_FIELD_TOP_PROG or V4L2_FIELD_BOTTOM_PROG, wouldn't it ? Or should
> the _PROG information be reported by this subdev only, and not
> propagated through the pipeline ?

In the highly unlikely case that this is something you would want to support,
then there is no need for V4L2_FIELD_TOP/BOTTOM_PROG since it adds nothing
beyond what V4L2_FIELD_TOP/BOTTOM already do.

If you just capture the top field, then the knowledge that this is the top
field of a progressive vs interlaced frame is of no practical value.

The only reason to have V4L2_FIELD_ALTERNATE_PROG is to indicate that if
you capture both top and bottom fields, then you can just combine them into
a progressive frame and there is no need to do any deinterlacing. But otherwise
it acts exactly the same as V4L2_FIELD_ALTERNATE.

Regards,

	Hans

> 
>> There is nothing else you need here.
>>
>> A V4L2_FIELD_SEQ_TB_PROG might be needed if we get HW that does something
>> so strange.
>>
>>> Can't your receiver store these two fragment directly into a
>>> progressive buffer instead of leaking this HW specific thing into uAPI
>>> ? All you'd need is support for stride (bytesperline) at the HW
>>> writeback level, and then you can hide this complexicuty to userspace
>>> by filling the top/bottom line only. You simply multiply the stride by
>>> two in this context.
>>
>> Vishal, this is a good question from Nicolas.
>>
>> An alternative solution might be to DMA the odd and even lines to the
>> same buffer, but consecutive. I.e., instead of having to create a
>> V4L2_FIELD_ALTERNATE_PROG, you'd create a V4L2_FIELD_SEQ_TB_PROG, which
>> is identical to V4L2_FIELD_SEQ_TB, except that it is for a progressive
>> frame.
>>
>> If you can avoid V4L2_FIELD_ALTERNATE_PROG somehow and just return a
>> single buffer per frame, then that would be much better. One field per
>> buffer is a big pain for userspace.
> 
> That's out of control of this driver though, it depends on the rest of
> the pipeline. The SDI RX subdev produces alternate frames, it's up to
> the rest of the FPGA to decide how to store that in memory.
> 
>>>>> So I will add the V4L2_FIELD_ALTERNATE_PROG in next patch version.
>>>>
>>>> [snip]
>