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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3d4b1c45-cc00-4714-8582-0848e38c2ec4@collabora.com>
Date: Mon, 17 Feb 2025 18:36:56 +0300
From: Dmitry Osipenko <dmitry.osipenko@...labora.com>
To: Hans Verkuil <hverkuil@...all.nl>,
 Shreeya Patel <shreeya.patel@...labora.com>, Heiko Stuebner
 <heiko@...ech.de>, Mauro Carvalho Chehab <mchehab@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, jose.abreu@...opsys.com,
 nelson.costa@...opsys.com, shawn.wen@...k-chips.com,
 nicolas.dufresne@...labora.com,
 Sebastian Reichel <sebastian.reichel@...labora.com>
Cc: kernel@...labora.com, linux-media@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-rockchip@...ts.infradead.org, Tim Surber <me@...surber.de>
Subject: Re: [PATCH v6 4/6] media: platform: synopsys: Add support for HDMI
 input driver

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

-- 
Best regards,
Dmitry