On 17/02/2025 19:26, Dmitry Osipenko wrote:

On 2/17/25 21:21, Dmitry Osipenko wrote:

quoted

On 2/17/25 18:44, Hans Verkuil wrote:

quoted

On 2/17/25 16:36, Dmitry Osipenko wrote:

quoted

On 2/17/25 11:31, Hans Verkuil wrote:

quoted

On 15/02/2025 22:04, Dmitry Osipenko wrote:

quoted

From: Shreeya Patel <redacted>

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 <redacted>
Signed-off-by: Dingxian Wen <redacted>
Signed-off-by: Shreeya Patel <redacted>
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>

quoted

+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.