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@xxxxxxxxxx> >>>>>> --- >>>>>> 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). 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. Regards, Hans > >> >>> So I will add the V4L2_FIELD_ALTERNATE_PROG in next patch version. >> >> [snip] >> >
