Re: [PATCH RFC 04/25] video/hdmi: Add VTEM EMP packing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] video/hdmi: hdmi_infoframe_pack_only() missing EMP support
- [Medium] video/hdmi: missing bounds check for VTEM base_refresh_rate
- [Low] video/hdmi: VTEM pack returns -EINVAL on insufficient buffer
--

commit 453def06fd91297425481a25d9a912cdc54c08f8
Author: Derek Foreman <derek.foreman@collabora.com>

video/hdmi: Add VTEM EMP packing

VTEM EMPs are a type of infoframe that is required for Variable Refresh
Rate. Add code to pack them.

> diff --git a/drivers/video/hdmi.c b/drivers/video/hdmi.c
> index 45b42f14a7503..ed9abc7ad2285 100644
> --- a/drivers/video/hdmi.c
> +++ b/drivers/video/hdmi.c
> @@ -983,6 +983,10 @@ hdmi_infoframe_pack(union hdmi_infoframe *frame,
>  		length = hdmi_vendor_any_infoframe_pack(&frame->vendor,
>  							buffer, size);
>  		break;
> +	case HDMI_INFOFRAME_TYPE_EMP:
> +		length = hdmi_emp_infoframe_vtem_pack(&frame->vtem,
> +						      buffer, size);
> +		break;

[Severity: High]
This adds EMP support to hdmi_infoframe_pack(), but does
hdmi_infoframe_pack_only() also need an update?

Drivers that rely exclusively on hdmi_infoframe_pack_only() (such as i915 and
SDVO) will hit a WARN(1) backtrace and fail with -EINVAL if they attempt to
pack an EMP (VTEM) infoframe.

[ ... ]

> +ssize_t hdmi_emp_infoframe_vtem_pack(struct hdmi_emp_infoframe_vtem *vtem,
> +				     void *buffer, size_t size)

[Severity: High]
To be supported in hdmi_infoframe_pack_only(), wouldn't this function need
to accept a const pointer?

hdmi_infoframe_pack_only() guarantees a const union hdmi_infoframe *frame,
but the lack of a const qualifier on vtem prevents it from being safely
called without casting away constness.

> +{
> +	u8 *ptr = buffer;
> +
> +	if (size < HDMI_INFOFRAME_SIZE(VTEM))
> +		return -EINVAL;

[Severity: Low]
Should this return -ENOSPC instead of -EINVAL when the provided buffer is
smaller than the required frame size?

All other packing functions in drivers/video/hdmi.c (e.g.,
hdmi_avi_infoframe_pack_only(), hdmi_drm_infoframe_pack_only()) explicitly
return -ENOSPC when size is insufficient. Returning -EINVAL might break error
handling logic in callers that expect -ENOSPC.

> +
> +	/* 4 bits max */
> +	if (vtem->fva_factor_m1 > 0xF)
> +		return -EINVAL;
> +

[Severity: Medium]
Does base_refresh_rate also need a bounds check here?

The base_refresh_rate field is a u16 and can hold values up to 65535, but it
is restricted to 10 bits. If a caller supplies a value greater than 1023, it
will be silently truncated later in this function rather than gracefully
rejected. Could this result in the hardware transmitting an incorrect VRR
refresh rate to the display?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=4