On 15/09/2020 04:38, Laurent Pinchart wrote:

Hi Hans,

On Thu, Sep 10, 2020 at 12:22:28PM +0200, Hans Verkuil wrote:

quoted

On 26/08/2020 16:10, Nicolas Dufresne wrote:

quoted

Le mercredi 19 août 2020 à 19:56 +0300, Laurent Pinchart a écrit :

quoted

Hi Vishal,

(Hans, there's a question for you below)

On Wed, Aug 19, 2020 at 01:47:49PM +0000, Vishal Sagar wrote:

quoted

On Thursday, July 16, 2020 3:03 AM Laurent Pinchart wrote:

quoted

On Thu, Jun 25, 2020 at 11:43:01AM +0200, Hans Verkuil wrote:

quoted

On 18/06/2020 07:33, Vishal Sagar wrote:

quoted

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 <redacted>
---
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]

quoted

quoted

quoted

quoted

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]

quoted

quoted

quoted

quoted

+	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

quoted

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.

quoted

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.

quoted

quoted

quoted

quoted

So I will add the V4L2_FIELD_ALTERNATE_PROG in next patch version.

[snip]