RE: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem driver

[Vishal Sagar]

Hi Hans,

> -----Original Message-----
> From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx]
> Sent: Saturday, June 15, 2019 1:25 PM
> To: Vishal Sagar <vsagar@xxxxxxxxxx>
> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Dinesh Kumar
> <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx>; Vishal Sagar
> <vishal.sagar@xxxxxxxxxx>; Hyun Kwon <hyunk@xxxxxxxxxx>; Laurent Pinchart
> <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho Chehab
> <mchehab@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Rob Herring
> <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>; Sakari Ailus
> <sakari.ailus@xxxxxxxxxxxxxxx>
> Subject: Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
> driver
> 
> On 6/14/19 1:44 PM, Vishal Sagar wrote:
> > Hi Hans,
> >
> > Thanks for reviewing this patch.
> >
> >> -----Original Message-----
> >> From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx]
> >> Sent: Wednesday, June 05, 2019 6:28 PM
> >> To: Vishal Sagar <vishal.sagar@xxxxxxxxxx>; Hyun Kwon
> <hyunk@xxxxxxxxxx>;
> >> Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>; Mauro Carvalho
> >> Chehab <mchehab@xxxxxxxxxx>; Michal Simek <michals@xxxxxxxxxx>; Rob
> >> Herring <robh+dt@xxxxxxxxxx>; Mark Rutland <mark.rutland@xxxxxxx>
> >> Cc: linux-kernel@xxxxxxxxxxxxxxx; linux-media@xxxxxxxxxxxxxxx; linux-arm-
> >> kernel@xxxxxxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Dinesh Kumar
> >> <dineshk@xxxxxxxxxx>; Sandip Kothari <sandipk@xxxxxxxxxx>
> >> Subject: Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx Subsystem
> >> driver
> >>
> >> EXTERNAL EMAIL
> >>
> >> On 6/4/19 3:55 PM, 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 only 10 bpc YUV 422 media bus format. 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 configuration.
> >>>
> >>> Signed-off-by: Vishal Sagar <vishal.sagar@xxxxxxxxxx>
> >>> ---
> >>>  drivers/media/platform/xilinx/Kconfig          |   11 +
> >>>  drivers/media/platform/xilinx/Makefile         |    1 +
> >>>  drivers/media/platform/xilinx/xilinx-sdirxss.c | 1846
> >> ++++++++++++++++++++++++
> >>>  include/uapi/linux/xilinx-sdirxss.h            |   63 +
> >>>  include/uapi/linux/xilinx-v4l2-controls.h      |   30 +
> >>>  include/uapi/linux/xilinx-v4l2-events.h        |    9 +
> 
> <snip>
> 
> >> I am concerned about this driver: I see that none of the *_dv_timings
> callbacks
> >> are implemented. I would expect to see that for a video receiver. There is
> also
> >> no g_input_status implemented.
> >>
> >> Take a look at another SDI driver: drivers/media/spi/gs1662.c
> >>
> >
> > I had a look at the gs1662 driver for the dv_timings callbacks. The gs1662
> driver
> > requires the timings because it is a SDI Transmitter.
> >
> > Here the timings are not required as the IP block generates a AXI4 Stream.
> > I think it may be required only in case of native / parallel video being
> outputted
> > as the output stream needs timing information to be decoded.
> >
> > Please feel free to correct my understanding if wrong.
> >
> > In the current driver, the input stream properties like width, height, frame
> rate,
> > progressive/interlaced  are determined from the ST352 packet payload or
> from the
> > properties detected by the core.
> >
> > See the xsdirx_get_stream_properties() for details.
> 
> You're wrong. In xsdirx_get_stream_properties() you set the format
> information.
> But you can't just change that: if the video resolution changes, then that means
> that userspace needs to be informed that it has changed at the source, it has to
> find and set the new timings, update the formats, possibly reallocate memory
> for
> the buffers, update other parts of the video pipeline with the new resolution
> etc.
> 
> The one thing you cannot do is just pass on the new resolution and hope that
> the
> video pipeline can handle it all.
> 
> The right sequence of events is:
> 
> 1) When a change is detected at the source the driver sends the
> SOURCE_CHANGE
> event and either stops transmitting to the video pipeline or keeps sending the
> old resolution (some devices have a freewheeling mode where they can do
> that).
> 
> 2) Userspace sees the event, calls QUERY_DV_TIMINGS to find a new timings (if
> any), usually stops streaming, and calls S_DV_TIMINGS to set the detected
> timings:
> at that point the driver can configure the output towards the video pipeline
> with
> the new timings. Userspace reallocates buffers and resumes streaming with the
> new
> resolution.
> 

Thanks for the explanation!

I will remove the extraneous video unlock event and stop the streaming when video lock / unlock interrupt occurs.
I will also implement the g_input_status() to return V4L2_IN_ST_NO_SYNC | V4L2_IN_ST_NO_SIGNAL in case video is unlocked.

My assumption is that on SOURCE_CHANGE event, application can stop the pipeline and then 
call the G_FORMAT and G_FRAME_INTERVAL to get new frame size, type (progressive / interlaced) and frame rate.
Is this assumption correct? 

Is it mandatory to implement QUERY_DV_TIMINGS with SOURCE_CHANGE event?

I also don't see any V4L2 framework supported events for overflow and underflow.
Is it ok to keep these or should they be removed too? 

Regards

Vishal Sagar

> Note that G_DV_TIMINGS returns the last configured timings, not the detected
> timings: only QUERY_DV_TIMINGS does that.
> 
> In other words: userspace has to retain control of the full pipeline.
> 
> Regards,
> 
> 	Hans
> 
> >
> >> Some of the controls you add in this driver can likely be dropped. Especially
> >> those controls that are not specific to the Xilinx implementation but are
> >> generic for any SDI receiver, should be looked at closely: those are
> >> candidates for becoming standard controls.
> >
> > I don't know how other SDI Receiver devices function.
> > So I am assuming all these controls are Xilinx specific implementations.
> >
> >>
> >> But the documentation above is simply insufficient for me to tell what is
> >> SDI specific and what is implementation specific.
> >>
> >
> > I will add more documentation for these controls.
> >
> >> Also, I'm no SDI expert, certainly not for the UHD-SDI.
> >>
> >> Regards,
> >>
> >>         Hans
> >
> > Regards
> > Vishal Sagar
> >