Re: [PATCH 13/13] drm/msm/dp: Add static HDR metadata support for DP and eDP

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dp: late HDR capability check in atomic_enable violates atomic contract
--

commit dd06afce910f68ea34833e6875a317b2680f0949
Author: Xilin Wu <sophon@radxa.com>

drm/msm/dp: Add static HDR metadata support for DP and eDP

This commit adds static HDR metadata processing for DP and eDP displays.
It introduces Generic2 packet formatting, revalidates sink capabilities,
and forces a modeset on metadata changes.

> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
[ ... ]
> @@ -708,7 +718,7 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
>  	const struct drm_display_info *info = &conn_state->connector->display_info;
>  	struct msm_dp_display_private *dp;
>  	unsigned int clock = mode->clock;
> -	int bpp;
> +	int bpp, ret;
>  
>  	dp = container_of(msm_dp_display, struct msm_dp_display_private, msm_dp_display);
>  
> @@ -717,17 +727,25 @@ int msm_dp_display_atomic_check(struct msm_dp *msm_dp_display,
>  
>  	guard(mutex)(&dp->plugged_lock);
>  
> +	ret = msm_dp_utils_pack_hdr_sdp(conn_state, &state->hdr_sdp);
> +	if (ret)
> +		return ret;
> +
> +	state->hdr_enabled = !!conn_state->hdr_output_metadata;

[Severity: High]
Should the hardware capability validation for HDR be moved here?

This is the correct function where the DP controller's hw_revision and all
static sink HDR capability checks must be performed. Validating here ensures
that userspace receives an error prior to the commit, rather than failing
late during the enable phase.

[ ... ]
> @@ -760,6 +778,18 @@ static int msm_dp_display_prepare_link(struct msm_dp_display_private *dp)
>  
>  	dp->stream_pm_active = true;
>  
> +	if (dp->panel->hdr_enabled) {
> +		const struct drm_display_info *info = &msm_dp_display->connector->display_info;
> +		const struct hdr_static_metadata *hdr = &info->hdr_sink_metadata.hdmi_type1;
> +		u8 eotf = dp->panel->hdr_sdp.db[2];
> +
> +		if (dp->panel->hw_revision < DP_HW_VERSION_1_0 ||
> +		    !(hdr->metadata_type & BIT(HDMI_STATIC_METADATA_TYPE1)) ||
> +		    !(hdr->eotf & BIT(eotf)) ||
> +		    (eotf != HDMI_EOTF_TRADITIONAL_GAMMA_SDR && info->bpc < 10))
> +			return -EINVAL;
> +	}
> +

[Severity: High]
Does this late hardware capability validation violate the atomic contract?

Returning an error during the atomic_enable phase (from 
msm_dp_display_prepare_link) causes a silent modesetting failure where the 
hardware state diverges from the software state, leading to a wedged display 
pipeline and DRM core warnings.

If a local user with DRM master privileges submits an atomic commit with HDR 
metadata on a DP controller with an older hw_revision, the commit passes 
validation but fails here. The atomic modesetting contract requires all such 
configuration constraints to be rejected during msm_dp_display_atomic_check.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260930-msm-dp-hdr10-v1-0-dbbf8b2b42bc@radxa.com?part=13