Re: [PATCH v8 2/2] Added Digiteq Automotive MGB4 driver documentation

[Hans Verkuil]

On 8/10/23 13:54, Martin Tůma wrote:
> On 26. 07. 23 12:40, Hans Verkuil wrote:
>> On 04/07/2023 15:13, tumic@xxxxxxxxxx wrote:
>>> From: Martin Tůma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
>>>
>>> The "admin-guide" documentation for the Digiteq Automotive MGB4 driver.
>>>
>>> Signed-off-by: Martin Tůma <martin.tuma@xxxxxxxxxxxxxxxxxxxxx>
>>> ---
>>>   Documentation/admin-guide/media/mgb4.rst      | 369 ++++++++++++++++++
>>>   .../admin-guide/media/pci-cardlist.rst        |   1 +
>>>   .../admin-guide/media/v4l-drivers.rst         |   1 +
>>>   3 files changed, 371 insertions(+)
>>>   create mode 100644 Documentation/admin-guide/media/mgb4.rst
>>>
>>> diff --git a/Documentation/admin-guide/media/mgb4.rst b/Documentation/admin-guide/media/mgb4.rst
>>> new file mode 100644
>>> index 000000000000..e1bb708a2265
>>> --- /dev/null
>>> +++ b/Documentation/admin-guide/media/mgb4.rst
>>> @@ -0,0 +1,369 @@
>>> +.. SPDX-License-Identifier: GPL-2.0
>>> +
>>> +====================
>>> +mgb4 sysfs interface
>>> +====================
>>> +
>>> +The mgb4 driver provides a sysfs interface, that is used to configure video
>>> +stream related parameters (some of them must be set properly before the v4l2
>>> +device can be opened) and obtain the video device/stream status.
>>> +
>>> +There are two types of parameters - global / PCI card related, found under
>>> +``/sys/class/video4linux/videoX/device`` and module specific found under
>>> +``/sys/class/video4linux/videoX``.
>>> +
>>> +
>>> +Global (PCI card) parameters
>>> +============================
>>> +
>>> +**module_type** (R):
>>> +    Module type.
>>> +
>>> +    | 0 - No module present
>>> +    | 1 - FPDL3
>>> +    | 2 - GMSL
>>> +
>>> +**module_version** (R):
>>> +    Module version number. Zero in case of a missing module.
>>> +
>>> +**fw_type** (R):
>>> +    Firmware type.
>>> +
>>> +    | 1 - FPDL3
>>> +    | 2 - GMSL
>>> +
>>> +**fw_version** (R):
>>> +    Firmware version number.
>>> +
>>> +**serial_number** (R):
>>> +    Card serial number. The format is::
>>> +
>>> +        PRODUCT-REVISION-SERIES-SERIAL
>>> +
>>> +    where each component is a 8b number.
>>> +
>>> +
>>> +Common FPDL3/GMSL input parameters
>>> +==================================
>>> +
>>> +**input_id** (R):
>>> +    Input number ID, zero based.
>>> +
>>> +**oldi_lane_width** (RW):
>>> +    Number of deserializer output lanes.
>>> +
>>> +    | 0 - single
>>> +    | 1 - dual
>>> +
>>> +**color_mapping** (RW):
>>> +    Mapping of the incoming bits in the signal to the colour bits of the pixels.
>>> +
>>> +    | 0 - OLDI/JEIDA
>>> +    | 1 - SPWG/VESA
>>> +
>>> +**link_status** (R):
>>> +    Video link status. If the link is locked, chips are properly connected and
>>> +    communicating at the same speed and protocol. The link can be locked without
>>> +    an active video stream.
>>> +
>>> +    A value of 0 is equivalent to the V4L2_IN_ST_NO_SYNC flag of the V4L2
>>> +    VIDIOC_ENUMINPUT status bits.
>>> +
>>> +    | 0 - unlocked
>>> +    | 1 - locked
>>> +
>>> +**stream_status** (R):
>>> +    Video stream status. A stream is detected if the link is locked, the input
>>> +    pixel clock is running and the DE signal is moving.
>>> +
>>> +    A value of 0 is equivalent to the V4L2_IN_ST_NO_SIGNAL flag of the V4L2
>>> +    VIDIOC_ENUMINPUT status bits.
>>> +
>>> +    | 0 - not detected
>>> +    | 1 - detected
>>> +
>>> +**video_width** (R):
>>> +    Video stream width. This is the actual width as detected by the HW.
>>> +
>>> +    The value is identical to what VIDIOC_QUERY_DV_TIMINGS returns in the width
>>> +    field of the v4l2_bt_timings struct.
>>> +
>>> +**video_height** (R):
>>> +    Video stream height. This is the actual height as detected by the HW.
>>> +
>>> +    The value is identical to what VIDIOC_QUERY_DV_TIMINGS returns in the height
>>> +    field of the v4l2_bt_timings struct.
>>> +
>>> +**vsync_status** (R):
>>> +    The type of VSYNC pulses as detected by the video format detector.
>>> +
>>> +    The value is equivalent to the flags returned by VIDIOC_QUERY_DV_TIMINGS in
>>> +    the polarities field of the v4l2_bt_timings struct.
>>> +
>>> +    | 0 - active low
>>> +    | 1 - active high
>>> +    | 2 - not available
>>> +
>>> +**hsync_status** (R):
>>> +    The type of HSYNC pulses as detected by the video format detector.
>>> +
>>> +    The value is equivalent to the flags returned by VIDIOC_QUERY_DV_TIMINGS in
>>> +    the polarities field of the v4l2_bt_timings struct.
>>> +
>>> +    | 0 - active low
>>> +    | 1 - active high
>>> +    | 2 - not available
>>> +
>>> +**vsync_gap_length** (RW):
>>> +    If the incoming video signal does not contain synchronization VSYNC and
>>> +    HSYNC pulses, these must be generated internally in the FPGA to achieve
>>> +    the correct frame ordering. This value indicates, how many "empty" pixels
>>
>> Pixels or lines? This is vsync, so lines would be more logical.
>>
>> Even if the hardware wants pixels, perhaps the driver should use lines and
>> translate it to pixels. It's much easier for userspace to work with lines.
>>
> 
> According to our HW engineers, this is properly documented. I do not 
> have the full insight to the "signal parameters" topic, so my answers 
> here will be just some kind of "free" translation of what I got. The 
> justification here was, that the vsync gap length (in our case/HW) 
> represents something slightly different than you may think.
> 
>>> +    (pixels with deasserted Data Enable signal) are necessary to generate the
>>> +    internal VSYNC pulse.
>>> +
>>> +**hsync_gap_length** (RW):
>>> +    If the incoming video signal does not contain synchronization VSYNC and
>>> +    HSYNC pulses, these must be generated internally in the FPGA to achieve
>>> +    the correct frame ordering. This value indicates, how many "empty" pixels
>>> +    (pixels with deasserted Data Enable signal) are necessary to generate the
>>> +    internal HSYNC pulse. The value must be greater than 1 and smaller than
>>> +    vsync_gap_length.
>>
>> Does this make sense? vsync_gap_length can be many video lines, which makes
>> not sense for hsync_gap_length.
>>
>> I wonder if it isn't easier to just change this to v/hsync_blanking_length
>> (lines for vsync, pixels for hsync) to indicate the length of the blanking
>> periods, and then let the driver pick a suitable hsync/vsync position.
>>
> 
> Dtto.

So the problem here is that if I don't understand what is meant here, how will
a user of this driver be able to understand it?

I think it would be better to give one or two examples of devices and their
configuration. Or refer to freely available documentation, if that is available.

Also note that v9 doesn't mentioned default values for these two properties.

Regards,

	Hans