Hi Hans,

-----Original Message-----
From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
Sent: Tuesday, June 18, 2019 5:38 PM
To: Vishal Sagar <redacted>
Cc: linux-kernel@vger.kernel.org; linux-media@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; devicetree@vger.kernel.org; Dinesh Kumar
[off-list ref]; Sandip Kothari [off-list ref]; Vishal Sagar
[off-list ref]; Hyun Kwon [off-list ref]; Laurent Pinchart
[off-list ref]; Mauro Carvalho Chehab
[off-list ref]; Michal Simek [off-list ref]; Rob Herring
[off-list ref]; Mark Rutland [off-list ref]; Sakari Ailus
[off-list ref]
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:

quoted

Hi Hans,

quoted

-----Original Message-----
From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
Sent: Saturday, June 15, 2019 1:25 PM
To: Vishal Sagar <redacted>
Cc: linux-kernel@vger.kernel.org; linux-media@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; devicetree@vger.kernel.org; Dinesh Kumar
[off-list ref]; Sandip Kothari [off-list ref]; Vishal Sagar
[off-list ref]; Hyun Kwon [off-list ref]; Laurent

Pinchart

quoted

quoted

[off-list ref]; Mauro Carvalho Chehab
[off-list ref]; Michal Simek [off-list ref]; Rob Herring
[off-list ref]; Mark Rutland [off-list ref]; Sakari

Ailus

quoted

quoted

[off-list ref]
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:

quoted

Hi Hans,

Thanks for reviewing this patch.

quoted

-----Original Message-----
From: Hans Verkuil [mailto:hverkuil@xs4all.nl]
Sent: Wednesday, June 05, 2019 6:28 PM
To: Vishal Sagar <redacted>; Hyun Kwon

[off-list ref];

quoted

quoted

Laurent Pinchart [off-list ref]; Mauro Carvalho
Chehab [off-list ref]; Michal Simek [off-list ref];

Rob

quoted

quoted

quoted

quoted

Herring [off-list ref]; Mark Rutland [off-list ref]
Cc: linux-kernel@vger.kernel.org; linux-media@vger.kernel.org; linux-arm-
kernel@lists.infradead.org; devicetree@vger.kernel.org; Dinesh Kumar
[off-list ref]; Sandip Kothari [off-list ref]
Subject: Re: [PATCH 2/2] media: v4l: xilinx: Add Xilinx UHD-SDI Rx

Subsystem

quoted

quoted

quoted

quoted

driver

EXTERNAL EMAIL

On 6/4/19 3:55 PM, Vishal Sagar wrote:

quoted

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

quoted

quoted

quoted

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 <redacted>
---
 drivers/media/platform/xilinx/Kconfig          |   11 +
 drivers/media/platform/xilinx/Makefile         |    1 +
 drivers/media/platform/xilinx/xilinx-sdirxss.c | 1846

++++++++++++++++++++++++

quoted

 include/uapi/linux/xilinx-sdirxss.h            |   63 +
 include/uapi/linux/xilinx-v4l2-controls.h      |   30 +
 include/uapi/linux/xilinx-v4l2-events.h        |    9 +

<snip>

quoted

quoted

I am concerned about this driver: I see that none of the *_dv_timings

callbacks

quoted

quoted

are implemented. I would expect to see that for a video receiver. There is

also

quoted

quoted

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

quoted

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

quoted

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,

quoted

progressive/interlaced  are determined from the ST352 packet payload or

from the

quoted

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

quoted

quoted

that userspace needs to be informed that it has changed at the source, it has

to

quoted

quoted

find and set the new timings, update the formats, possibly reallocate

memory

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

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

quoted

quoted

new
resolution.

Thanks for the explanation!

I will remove the extraneous video unlock event and stop the streaming when

video lock / unlock interrupt occurs.

quoted

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.

quoted

My assumption is that on SOURCE_CHANGE event, application can stop the

pipeline and then

quoted

call the G_FORMAT and G_FRAME_INTERVAL to get new frame size, type

(progressive / interlaced) and frame rate.

quoted

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.

quoted

Is it mandatory to implement QUERY_DV_TIMINGS with SOURCE_CHANGE

event?

Yes.

Thanks again for clarifying this. 

quoted

I also don't see any V4L2 framework supported events for overflow and

underflow.

quoted

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

quoted

Regards

Vishal Sagar

quoted

Note that G_DV_TIMINGS returns the last configured timings, not the

detected

quoted

quoted

timings: only QUERY_DV_TIMINGS does that.

In other words: userspace has to retain control of the full pipeline.

Regards,

	Hans

quoted

quoted

Some of the controls you add in this driver can likely be dropped.

Especially

quoted

quoted

quoted

quoted

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.

quoted

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.

quoted

Also, I'm no SDI expert, certainly not for the UHD-SDI.

Regards,

        Hans

Regards
Vishal Sagar