Re: [PATCH 9/9] drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardcoded `possible_crtcs` mask restricts all primary planes to CRTC 0, breaking multi-display support.
- [High] Direct access of global `plane->state` and `crtc->state` in atomic commit hooks leads to race conditions and stalled events.
--

commit 8a32b307abb3d8e006d032d01a1802aa1e4ae6f0
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers

Replace Xen frontend simple display pipe with explicit plane, CRTC and
encoder objects for each pipeline. Keep generic plane-state validation
before Xen-specific checks, and install GEM framebuffer prepare helper
explicitly.

> diff --git a/drivers/gpu/drm/xen/xen_drm_front_kms.c b/drivers/gpu/drm/xen/xen_drm_front_kms.c
> index 48772b5fe71cf..57e1bef452f83 100644
> --- a/drivers/gpu/drm/xen/xen_drm_front_kms.c
> +++ b/drivers/gpu/drm/xen/xen_drm_front_kms.c

[ ... ]

> @@ -105,17 +105,15 @@ static void send_pending_event(struct xen_drm_front_drm_pipeline *pipeline)
>  	spin_unlock_irqrestore(&dev->event_lock, flags);
>  }
>  
> -static void display_enable(struct drm_simple_display_pipe *pipe,
> -			   struct drm_crtc_state *crtc_state,
> -			   struct drm_plane_state *plane_state)
> +static void xen_drm_crtc_helper_atomic_enable(struct drm_crtc *crtc,
> +					      struct drm_atomic_commit *state)
>  {
> -	struct xen_drm_front_drm_pipeline *pipeline =
> -			to_xen_drm_pipeline(pipe);
> -	struct drm_crtc *crtc = &pipe->crtc;
> +	struct xen_drm_front_drm_pipeline *pipeline = to_xen_drm_pipeline(crtc);
> +	struct drm_plane_state *plane_state = pipeline->plane.state;

[Severity: High]
Is it safe to access the global pipeline->plane.state directly here? If
non-blocking commits overlap, this pointer might point to a newer,
uncommitted state. Should this code use drm_atomic_get_new_plane_state()
to extract the state from the drm_atomic_commit object instead?

>  	struct drm_framebuffer *fb = plane_state->fb;
>  	int ret, idx;

[ ... ]

> @@ -242,15 +259,19 @@ static int display_check(struct drm_simple_display_pipe *pipe,
>  	return 0;
>  }
>  
> -static void display_update(struct drm_simple_display_pipe *pipe,
> -			   struct drm_plane_state *old_plane_state)
> +static void xen_drm_plane_helper_atomic_update(struct drm_plane *plane,
> +					       struct drm_atomic_commit *state)
>  {
> -	struct xen_drm_front_drm_pipeline *pipeline =
> -			to_xen_drm_pipeline(pipe);
> -	struct drm_crtc *crtc = &pipe->crtc;
> +	struct drm_plane_state *old_plane_state = drm_atomic_get_old_plane_state(state, plane);
> +	struct drm_crtc *crtc = plane->state->crtc ?: old_plane_state->crtc;

[Severity: High]
Can accessing the global plane->state pointer here cause race conditions?
It might be safer to use drm_atomic_get_new_plane_state(state, plane)->crtc
to ensure the correct state is referenced during the atomic commit.

> +	struct xen_drm_front_drm_pipeline *pipeline;
>  	struct drm_pending_vblank_event *event;
>  	int idx;
>  
> +	if (!crtc)
> +		return;
> +
> +	pipeline = to_xen_drm_pipeline(crtc);
>  	event = crtc->state->event;

[Severity: High]
Similarly, does reading and modifying the global crtc->state directly risk
fetching state from a concurrent commit instead of the current one?

>  	if (event) {
>  		struct drm_device *dev = crtc->dev;

[ ... ]

> @@ -331,10 +395,37 @@ static int display_pipe_init(struct xen_drm_front_drm_info *drm_info,
>  
>  	formats = xen_drm_front_conn_get_formats(&format_count);
>  
> -	return drm_simple_display_pipe_init(dev, &pipeline->pipe,
> -					    &display_funcs, formats,
> -					    format_count, NULL,
> -					    &pipeline->conn);
> +	ret = drm_universal_plane_init(dev, &pipeline->plane, 1,
> +				       &display_plane_funcs,
> +				       formats, format_count,
> +				       NULL,
> +				       DRM_PLANE_TYPE_PRIMARY, NULL);

[Severity: High]
Does hardcoding the possible_crtcs mask to 1 restrict all primary planes to
CRTC 0? For pipelines with an index greater than 0, the CRTC might fail to
find a compatible primary plane during atomic validation. Would using
1 << pipeline->index allow multi-display configurations to work correctly?

> +	if (ret)
> +		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260705-drm-simple-kms-removal-v1-0-b4e1ca053623@oss.qualcomm.com?part=9