Re: [RESEND PATCH v6 1/1] Added Digiteq Automotive MGB4 driver

From: Martin Tůma
Date: Wed Jun 14 2023 - 09:43:52 EST

On 13. 06. 23 17:04, Hans Verkuil wrote:
Hi Martin,

On 13/06/2023 16:46, Martin Tůma wrote:
On 13. 06. 23 14:18, Hans Verkuil wrote:
Hi Martin,

On 12/06/2023 13:34, Martin Tůma wrote:
On 12. 06. 23 10:51, Hans Verkuil wrote:
On 08/06/2023 17:30, Martin Tůma wrote:
On 08. 06. 23 12:23, Hans Verkuil wrote:

Can you make a list of which sysfs properties correspond to existing V4L2
format or timing fields and which are 'new'?


On the left all the current mgb4 sysfs properties (see the admin-guide doc from the patch for description), on the right v4l2 structures where they could be mapped (may not be true for all of
them in
the patch, I will check it and update the code in v7)


--- PCIE CARD ---

module_type        -
module_version        -
fw_type            -
fw_version        -
serial_number        -
temperature        hwmon

--- INPUTS ---

input_id        -
oldi_lane_width        -
color_mapping        -
link_status        v4l2_input.status (V4L2_IN_ST_NO_SYNC)
stream_status        v4l2_input.status (V4L2_IN_ST_NO_SIGNAL)
video_width        v4l2_bt_timings.width
video_height        v4l2_bt_timings.height
vsync_status        v4l2_bt_timings.polarities
hsync_status        v4l2_bt_timings.polarities
vsync_gap_length    -
hsync_gap_length    -
pclk_frequency        v4l2_bt_timings.pixelclock
hsync_width        v4l2_bt_timings.hsync
vsync_width        v4l2_bt_timings.vsync
hback_porch        v4l2_bt_timings.hbackporch
hfront_porch        v4l2_bt_timings.hfrontporch
vback_porch        v4l2_bt_timings.vbackporch
vfront_porch        v4l2_bt_timings.vfrontporch
frequency_range        -
alignment        v4l2_pix_format.bytesperline
fpdl3_input_width    -
gmsl_mode        -
gmsl_stream_id        -
gmsl_fec        -

--- OUTPUTS ---

output_id        -
video_source        -
display_width        v4l2_bt_timings.width
display_height        v4l2_bt_timings.height
frame_rate        v4l2_frmivalenum

The frame rate is a property of the width/height+blanking and the
pixel clock frequency. IMHO it does not make sense to have this as
a writable property. Read-only is OK.

hsync_polarity        v4l2_bt_timings.polarities
vsync_polarity        v4l2_bt_timings.polarities
de_polarity        -
pclk_frequency        v4l2_bt_timings.pixelclock
hsync_width        v4l2_bt_timings.hsync
vsync_width        v4l2_bt_timings.vsync
vsync_width        v4l2_bt_timings.vsync
hback_porch        v4l2_bt_timings.hbackporch
hfront_porch        v4l2_bt_timings.hfrontporch
vback_porch        v4l2_bt_timings.vbackporch
vfront_porch        v4l2_bt_timings.vfrontporch
alignment        v4l2_pix_format.bytesperline
fpdl3_output_width    -


M.

The property I am most concerned with is alignment (both for input and output).
But it is not clear to me what the use-case is.


Hi,
The use-case is to provide the alignment required by some video processing chips. We have a product based on NVIDIA Jetson TX2 that uses the mgb4 cards and the HW video encoding needs a specific
alignment to work.

OK. I would suggest that for this property it has a default value of 0 (i.e. a 1 byte alignment),
and in that case VIDIOC_S_FMT allows userspace to set bytesperline to whatever they want. I.e.,
this is the normal behavior for DMA engines that can deal with custom padding at the end of each
line.

If it is > 0, then bytesperline is fixed, based on this value.

That way both methods are supported fairly cleanly.

BTW, what is missing in the property documentation for writable properties is what the default
value is. That must be documented as well.


The default value is 1 (no padding, 1 byte alignment), I will add it to the documentation.

I would really urge to stick with the "set all the properties at one place in sysfs, report them in v4l2" mechanism. Like with most of the properties, there are some special cases and HW related
dependencies across inputs/outputs (the output alignment has to comply with input alignment - see the documentation rst for details) and duplicating this logic together with some additional logic
handling changes from another source - VIDIOC_S_FMT - will make the driver much more complicated and "messy" for no benefit for the user (he will be even more confused).

When mainlining drivers it is important to support the standard APIs as much as possible,
otherwise it will become a big mess if every driver does something different. So as
maintainer it is my job to ensure that the standard APIs are used.

Looking at the properties that were introduced, most are related to timings, except
for alignment. That is really something for VIDIOC_S_FMT. And should be perfectly
fine to support as long as alignment is set to 1. If it is > 1, then bytesperline can
be set by the driver and userspace can't change it. It adds only very little complexity,
but it ensures that the default behavior is consistent with the V4L2 API.

I'm not very keen on all the properties at all, but given the specific nature of
this board I can understand it. They are to some extent similar to device tree
snippets to configure the device. But 'alignment' is not really part of that,
but I'm OK with it as long as the standard method is also supported. And in fact,
the property documentation should refer to the standard method as well.


Ok, I see your point. I will completely drop the alignment from sysfs and make it configurable only using VIDIOC_S_FMT. This "property" is/will be changed by special SW that uses the v4l2 API anyway. So it is not the case of all the required video stream properties where it is crucial for us that they can all be configured on a single place (using the same basic UDEV rules) and that generic SW like VLC or ffplay can be used to work with the video devices.

M.

Regards,

Hans