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