Re: [PATCH] drm/edid: Add HDR Static Metadata Type 1 quirk for LG UltraGear

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The calculation for minimum luminance in `drm_calculate_luminance_range` always evaluates to 0 due to flawed integer division.
- [High] `drm_calculate_luminance_range` incorrectly calculates the display's maximum luminance using the Max Frame-average Luminance field instead of the Max Luminance field.
--

--- Patch [1]: [PATCH] drm/edid: Add HDR Static Metadata Type 1 quirk for LG UltraGear ---
commit f96110d8b7a4665ac963f7b030033a65f6235f0a
Author: Xela <[email protected]>

drm/edid: Add HDR Static Metadata Type 1 quirk for LG UltraGear

The LG UltraGear monitor (PNP ID GSM 0x7766) advertises HDR support in
its CTA-861 Extension block with SMPTE ST 2084 (PQ) EOTF support
(byte 0x9e = 0x05) and valid desired content luminance descriptors
(351 cd/m^2 max CLL/FALL at bytes 0xa0-0xa2).

However, due to a factory firmware bug, the HDR Static Metadata Data
Block descriptor byte (byte 0x9f) is reported as 0x00 instead of 0x01,
omitting the Static Metadata Type 1 capability bit (bit 0).

Because of this omitted bit, drm_parse_hdr_metadata_block() sets
metadata_type to 0. Consequently, drm_calculate_luminance_range()
rejects the block, and userspace attempts to perform atomic KMS commits
configuring HDR output metadata fail with -EINVAL (Error 22), preventing
compositors (Plasma, Gamescope, MPV) from engaging HDR on this display.

Introduce EDID_QUIRK_FORCE_HDR_STATIC_METADATA_TYPE1 to force the Static
Metadata Type 1 descriptor bit for displays with this defective EDID block,
and register the LG UltraGear (GSM 0x7766) in the EDID quirk table.

> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 07970e5b5f65e..dd279fad17a42 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c

[ ... ]

> @@ -5498,6 +5503,10 @@ drm_parse_hdr_metadata_block(struct drm_connector *connector, const u8 *db)
>  	hdr_metadata->eotf = eotf_supported(db);
>  	hdr_metadata->metadata_type = hdr_metadata_type(db);
>  
> +	if (drm_edid_has_internal_quirk(connector,
> +				       EDID_QUIRK_FORCE_HDR_STATIC_METADATA_TYPE1))
> +		hdr_metadata->metadata_type |= BIT(HDMI_STATIC_METADATA_TYPE1);
> +
>  	if (len >= 4)
>  		hdr_metadata->max_cll = db[4];
>  	if (len >= 5)

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be an issue
in how these values are used in drm_calculate_luminance_range() which is
called just after this block.

Does drm_calculate_luminance_range() incorrectly calculate max luminance
using the Max Frame-average Luminance field (max_fall) instead of the
Max Luminance field (max_cll)?

drivers/gpu/drm/drm_edid.c:drm_calculate_luminance_range() {
	...
	max_avg = hdr_metadata->max_fall;
	min_cll = hdr_metadata->min_cll;
	...
	q = max_avg >> 5;
	r = max_avg % 32;
	max = (1 << q) * pre_computed_values[r];
	...
}

It seems max_avg maps to max_fall, entirely ignoring hdr_metadata->max_cll.
Could this lead to underreporting the maximum peak luminance?

[Severity: High]
This is another pre-existing issue, but does the minimum luminance calculation
in drm_calculate_luminance_range() always evaluate to 0?

drivers/gpu/drm/drm_edid.c:drm_calculate_luminance_range() {
	...
	/* min luminance: maxLum * (CV/255)^2 / 100 */
	q = DIV_ROUND_CLOSEST(min_cll, 255);
	min = max * DIV_ROUND_CLOSEST((q * q), 100);
	...
}

Since min_cll is bounded to 255 (a 1-byte value), q will be at most 1, and
DIV_ROUND_CLOSEST(1, 100) truncates to 0. Would this force the computed
minimum luminance limit to always be exactly 0 for all panels? Can this
integer math sequence be adjusted to prevent the loss of precision?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1