Re: [PATCH v2 2/3] drm/nouveau/kms/nv50-: Introduce nv50_wndw_default_state()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incorrect comparison between the enum value `pixel_blend_mode` and the bitmask `blend_modes` causes a valid default mode to be silently overwritten.
--

--- Patch [2]: [PATCH v2 2/3] drm/nouveau/kms/nv50-: Introduce nv50_wndw_default_state() ---
commit 43504d2c3aaff4f127ccc271e9439f3f380cd55b
Author: Lyude Paul <lyude@redhat.com>

drm/nouveau/kms/nv50-: Introduce nv50_wndw_default_state()

This commit introduces nv50_wndw_default_state() to enforce a valid
pixel_blend_mode for planes supporting alpha channels during driver startup.

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/wndw.c b/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> index 2635458d52acc..0f331f47bded8 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/wndw.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/wndw.c

[ ... ]

> @@ -848,6 +848,28 @@ static const u64 nv50_cursor_format_modifiers[] = {
>  	DRM_FORMAT_MOD_INVALID,
>  };
>  
> +/*
> + * Setup defaults for the atomic wndw state
> + */
> +void
> +nv50_wndw_default_state(struct nv50_wndw *wndw)
> +{
> +	struct nv50_wndw_atom *armw = nv50_wndw_atom(wndw->plane.state);
> +	const unsigned int blend_modes = wndw->func->blend_modes;
> +
> +	drm_modeset_lock_assert_held(&wndw->plane.mutex);
> +
> +	/* Ensure the plane's atomic state didn't default to a pixel_blend_mode we don't support */
> +	if (blend_modes && (!(armw->state.pixel_blend_mode & blend_modes))) {

[Severity: High]
Does this incorrectly perform a bitwise AND between an enum value and a bitmask?

Since DRM_MODE_BLEND_PREMULTI is defined as 0, when pixel_blend_mode defaults
to PREMULTI, armw->state.pixel_blend_mode & blend_modes will evaluate to 0.

This causes the negation !(0) to evaluate to true, unintentionally entering
this block and overwriting the valid default mode with DRM_MODE_BLEND_COVERAGE.

Should this comparison be using BIT(armw->state.pixel_blend_mode) & blend_modes
to properly check if the enum value is supported by the bitmask?

> +		if (blend_modes & BIT(DRM_MODE_BLEND_COVERAGE))
> +			armw->state.pixel_blend_mode = DRM_MODE_BLEND_COVERAGE;
> +		else if (blend_modes & BIT(DRM_MODE_BLEND_PREMULTI))
> +			armw->state.pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> +		else if (blend_modes & BIT(DRM_MODE_BLEND_PIXEL_NONE))
> +			armw->state.pixel_blend_mode = DRM_MODE_BLEND_PIXEL_NONE;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720205541.257781-1-lyude@redhat.com?part=2