On 2/17/25 11:31, Hans Verkuil wrote:
> On 15/02/2025 22:04, Dmitry Osipenko wrote:
>> From: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxx>
>> Signed-off-by: Dingxian Wen <shawn.wen@xxxxxxxxxxxxxx>
>> Signed-off-by: Shreeya Patel <shreeya.patel@xxxxxxxxxxxxx>
>> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@xxxxxxxxxxxxx>
>> ---
>> 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!
--
Best regards,
Dmitry
