Hi Hans, > -----Original Message----- > From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] > Sent: Tuesday, June 18, 2019 5:38 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/18/19 1:51 PM, Vishal Sagar wrote: > > 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? > > No :-) > Good to have that cleared. :-D > After SOURCE_CHANGE is received an application calls QUERY_DV_TIMINGS. If > that > returns valid timings, then the application calls S_DV_TIMINGS with the > detected timings. The driver will now update the format, frame interval, etc. > according to the new timings. And the application can use that to reconfigure > the video pipeline. > > > > > Is it mandatory to implement QUERY_DV_TIMINGS with SOURCE_CHANGE > event? > > Yes. > Thanks again for clarifying this. > > > > 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? > > under/overflow of what? Internal fifos? You can keep the custom events for > that. > Yep these are custom events for internal fifos. I will keep them. Regards Vishal Sagar > Regards, > > Hans > > > > > 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 > >>> > >
