Message-ID: <BY5PR02MB68673B0477928FAC7FAF48B5A7540@BY5PR02MB6867.namprd02.prod.outlook.com>
Date:   Wed, 26 Aug 2020 14:50:44 +0000
From:   Vishal Sagar <vsagar@...inx.com>
To:     Nicolas Dufresne <nicolas@...fresne.ca>,
        Laurent Pinchart <laurent.pinchart@...asonboard.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


Hi Nicolas,

> -----Original Message-----
> From: Nicolas Dufresne <nicolas@...fresne.ca>
> Sent: Wednesday, August 26, 2020 7:40 PM
> 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;
> robh+dt@...nel.org; mark.rutland@....com; Michal Simek
> <michals@...inx.com>; linux-media@...r.kernel.org;
> devicetree@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; 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.
> 

The SDI Rx IP output is an AXI4 Streaming interface and can't store frames in memory without a dma engine.

Secondly the idea is to differentiate between 
1 content interlaced video frames
   whose 2 fields are captured at different times T0 and T1
2 transport interlaced progressive video frames
  whose 2 fields were captured at same time T0 but are being transported in interlaced manner.

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

Regards
Vishal Sagar

Powered by blists - more mailing lists