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

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

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 :)

-- 
Best regards,
Dmitry

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