Message-ID: <ae3814b877ea264b0231321d12d946761941e004.camel@ndufresne.ca>
Date:   Wed, 26 Aug 2020 10:10:17 -0400
From:   Nicolas Dufresne <nicolas@...fresne.ca>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Vishal Sagar <vsagar@...inx.com>,
        Hans Verkuil <hverkuil@...all.nl>
Cc:     Hyun Kwon <hyunk@...inx.com>,
        "mchehab@...nel.org" <mchehab@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        Michal Simek <michals@...inx.com>,
        "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "joe@...ches.com" <joe@...ches.com>,
        Sandip Kothari <sandipk@...inx.com>,
        Dinesh Kumar <dineshk@...inx.com>
Subject: Re: [PATCH v3 3/3] media: v4l: xilinx: Add Xilinx UHD-SDI Rx
 Subsystem driver

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@...inx.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 ?

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.

> 
> > So I will add the V4L2_FIELD_ALTERNATE_PROG in next patch version.
> 
> [snip]
> 

Powered by blists - more mailing lists