Re: [PATCH v3 8/8] 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 1 potential issue(s) to consider:
- [High] Incorrect coordinates passed to xen_drm_front_mode_set: uses destination screen coordinates instead of source framebuffer offsets.
--

--- Patch [8]: [PATCH v3 8/8] drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers ---
commit c7c607527ef2df6f1f1c3266167b6754066820c2
Author: Ze Huang <ze.huang@oss.qualcomm.com>

drm/xen: replace struct drm_simple_display_pipe with regular atomic helpers

This commit replaces the Xen frontend simple display pipe with explicit
plane, CRTC, and encoder objects. It retains generic plane-state validation
and uses commit-local plane state in the CRTC enable path.

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

[ ... ]

> -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 *commit)
>  {
> -	struct xen_drm_front_drm_pipeline *pipeline =
> -			to_xen_drm_pipeline(pipe);
> -	struct drm_crtc *crtc = &pipe->crtc;
> -	struct drm_framebuffer *fb = plane_state->fb;
> +	struct xen_drm_front_drm_pipeline *pipeline = xen_drm_pipeline_from_crtc(crtc);
> +	struct drm_plane_state *pstate = drm_atomic_get_new_plane_state(commit, &pipeline->plane);
> +	struct drm_framebuffer *fb = pstate->fb;
>  	int ret, idx;
>  
> -	if (!drm_dev_enter(pipe->crtc.dev, &idx))
> +	if (!drm_dev_enter(crtc->dev, &idx))
>  		return;
>  
> -	ret = xen_drm_front_mode_set(pipeline, crtc->x, crtc->y,
> +	ret = xen_drm_front_mode_set(pipeline, pstate->crtc_x, pstate->crtc_y,
>  				     fb->width, fb->height,
>  				     fb->format->cpp[0] * 8,
>  				     xen_drm_front_fb_to_cookie(fb));

[Severity: High]
The commit message mentions passing the new plane source position, but does 
this pass the destination screen coordinates instead? 

It looks like pstate->crtc_x and pstate->crtc_y represent the destination
coordinates on the CRTC. Should this use the source coordinates instead 
(e.g., from pstate->src) to prevent breaking viewport offsets or causing 
out-of-bounds framebuffer reads by the backend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-drm-simple-kms-removal-v3-0-cd5dc89858c6@oss.qualcomm.com?part=8