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

[Dmitry Osipenko <dmitry.osipenko@collabora.com>]

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,
Will revisit it all again in a more details for v7.

-- 
Best regards,
Dmitry

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip