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

On Mon Jul 27, 2026 at 3:56 AM CST, sashiko-bot wrote:
> 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 <[email protected]>
>
> 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?

I think using pstate->crtc_x/y is the intended replacement here.

The old code used crtc->x/y. The drm_crtc documentation [1] says these
legacy fields should not be used by atomic drivers, and atomic drivers
should use the primary plane's drm_plane_state.crtc_x/y instead.

Xen has only one primary plane, so taking crtc_x/y from that plane keeps
the same semantics while avoiding direct legacy CRTC state access.

[1] https://elixir.bootlin.com/linux/v7.1.3/source/include/drm/drm_crtc.h#L1067