Re: [PATCH v6 4/6] media: platform: synopsys: Add support for HDMI input driver

[Hans Verkuil <hverkuil@xs4all.nl>]

On 17/02/2025 19:26, Dmitry Osipenko wrote:
> On 2/17/25 21:21, Dmitry Osipenko wrote:
>> On 2/17/25 18:44, Hans Verkuil wrote:
>>> On 2/17/25 16:36, Dmitry Osipenko wrote:
>>>> On 2/17/25 11:31, Hans Verkuil wrote:
>>>>> On 15/02/2025 22:04, Dmitry Osipenko wrote:
>>>>>> From: Shreeya Patel <shreeya.patel@collabora.com>
>>>>>>
>>>>>> Add initial support for the Synopsys DesignWare HDMI RX
>>>>>> Controller Driver used by Rockchip RK3588. The driver
>>>>>> supports:
>>>>>>  - HDMI 1.4b and 2.0 modes (HDMI 4k@60Hz)
>>>>>>  - RGB888, YUV422, YUV444 and YCC420 pixel formats
>>>>>>  - CEC
>>>>>>  - EDID configuration
>>>>>>
>>>>>> The hardware also has Audio and HDCP capabilities, but these are
>>>>>> not yet supported by the driver.
>>>>>>
>>>>>> Co-developed-by: Dingxian Wen <shawn.wen@rock-chips.com>
>>>>>> Signed-off-by: Dingxian Wen <shawn.wen@rock-chips.com>
>>>>>> Signed-off-by: Shreeya Patel <shreeya.patel@collabora.com>
>>>>>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
>>>>>> ---
>>>>>>  drivers/media/platform/Kconfig                |    1 +
>>>>>>  drivers/media/platform/Makefile               |    1 +
>>>>>>  drivers/media/platform/synopsys/Kconfig       |    3 +
>>>>>>  drivers/media/platform/synopsys/Makefile      |    2 +
>>>>>>  .../media/platform/synopsys/hdmirx/Kconfig    |   27 +
>>>>>>  .../media/platform/synopsys/hdmirx/Makefile   |    4 +
>>>>>>  .../platform/synopsys/hdmirx/snps_hdmirx.c    | 2715 +++++++++++++++++
>>>>>>  .../platform/synopsys/hdmirx/snps_hdmirx.h    |  394 +++
>>>>>>  .../synopsys/hdmirx/snps_hdmirx_cec.c         |  284 ++
>>>>>>  .../synopsys/hdmirx/snps_hdmirx_cec.h         |   44 +
>>>>>>  10 files changed, 3475 insertions(+)
>>>>>>  create mode 100644 drivers/media/platform/synopsys/Kconfig
>>>>>>  create mode 100644 drivers/media/platform/synopsys/Makefile
>>>>>>  create mode 100644 drivers/media/platform/synopsys/hdmirx/Kconfig
>>>>>>  create mode 100644 drivers/media/platform/synopsys/hdmirx/Makefile
>>>>>>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
>>>>>>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
>>>>>>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.c
>>>>>>  create mode 100644 drivers/media/platform/synopsys/hdmirx/snps_hdmirx_cec.h
>>>>>>
>>>>>
>>>>> <snip>
>>>>>
>>>>>> +static ssize_t
>>>>>> +hdmirx_debugfs_if_read(u32 type, void *priv, struct file *filp,
>>>>>> +		       char __user *ubuf, size_t count, loff_t *ppos)
>>>>>> +{
>>>>>> +	struct snps_hdmirx_dev *hdmirx_dev = priv;
>>>>>> +	u8 aviif[3 + 7 * 4];
>>>>>> +	int len;
>>>>>> +
>>>>>> +	if (type != V4L2_DEBUGFS_IF_AVI)
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	hdmirx_read_avi_infoframe(hdmirx_dev, aviif);
>>>>>> +
>>>>>> +	len = simple_read_from_buffer(ubuf, count, ppos,
>>>>>> +				      aviif, ARRAY_SIZE(aviif));
>>>>>> +
>>>>>> +	return len < 0 ? 0 : len;
>>>>>> +}
>>>>>
>>>>> Have you tested this with 'edid-decode -c -I /path/to/avi'? Also test that it is
>>>>> empty if there is no AVI InfoFrame (e.g. when there is no incoming video). I don't see
>>>>> a test for that in the code.
>>>>>
>>>>> I also see no sanity check regarding the length of the InfoFrame, it just outputs
>>>>> the full array, meaning you get padding as well since the AVI InfoFrame is smaller
>>>>> than ARRAY_SIZE(aviif). In fact, edid-decode will fail about that if the -c option
>>>>> is used.
>>>>>
>>>>> See tc358743_debugfs_if_read of how this is typically handled.
>>>>
>>>> I've tested with 'edid-decode -I /path/to/avi', including the empty AVI
>>>> InfoFrame. But without the '-c option'. I'd expect that debugfs should
>>>> provide a full-sized raw InfoFrame data, rather than a parsed version.
>>>> The parsed data isn't much useful for debugging purposes, IMO. I
>>>> intentionally removed the size check that tc358743_debugfs_if_read does
>>>> because it appeared wrong to me. Will re-check with '-c option', thanks!
>>>
>>> The HDMI header contains the actual length that was received. So debugfs should
>>> export the actual payload, not the maximum possible payload.
>>>
>>> It is common for hardware to reserve room in the register map for the maximum
>>> payload, but you only want to export what was actually received.
>>
>> If payload is corrupted, it should be handy to see a full payload.
>> Otherwise you won't be able to debug anything because driver returns
>> zero payload to userspace since it can't parse the header :)
> 
> Note those tc358743 and other drivers are parsing the IF header data.
> I'm pretty sure this is not what InfoFrame debugfs is intended to do,

It's exactly what it is intended to do (I know, I wrote it :-) ).

The HDMI interface transmits the InfoFrame data with that HDMI header, the
receiver captures that and stores the InfoFrame data. You have no control
over that, it is all implemented in hardware since it is transmitted over
the high-speed video lanes. If the header is corrupt, then the video is
almost certainly also corrupt.

The reason for storing the InfoFrame in debugfs is to see what the transmitter
is sending us in the InfoFrame: does the InfoFrame contents make sense? Is it
consistent with the EDID?

If for example the transmitter is sending us a video format that isn't supported
in the EDID, then it is helpful to see what the AVI InfoFrame says.

Regards,

	Hans

> Will revisit it all again in a more details for v7.
>