Re: [PATCH v3 5/5] drm/qxl: pack dumb heads from their plane source rectangles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/qxl: hardcoded horizontal monitors_config breaks vertical multi-monitor layouts
--

commit 8c2ba11a8cac6cfc531b9cc6ef9a78703db0c342
Author: Dillon Amburgey <dillona@gmail.com>

drm/qxl: pack dumb heads from their plane source rectangles

This commit sizes packed heads from their plane source rectangles and
copies only those rectangles to their packed origins. It intercepts DirtyFB
clips with every matching plane source and applies horizontal and vertical
offsets to map modesets and dirty updates identically.

> diff --git a/drivers/gpu/drm/qxl/qxl_display.c b/drivers/gpu/drm/qxl/qxl_display.c
> --- a/drivers/gpu/drm/qxl/qxl_display.c
> +++ b/drivers/gpu/drm/qxl/qxl_display.c
> @@ -369,8 +369,10 @@ static void qxl_crtc_update_monitors_config(struct drm_crtc *crtc,
>  		head.y = crtc->y;
>  		if (qdev->monitors_config->count < i + 1)
>  			qdev->monitors_config->count = i + 1;
> -		if (to_qxl_plane_state(crtc->primary->state)->shadow)
> -			head.x += to_qxl_plane_state(crtc->primary->state)->x;
> +		if (to_qxl_plane_state(crtc->primary->state)->shadow) {
> +			head.x = to_qxl_plane_state(crtc->primary->state)->x;
> +			head.y = 0;
> +		}

[Severity: High]
Does overwriting head.x with the shadow buffer's horizontal offset and
explicitly zeroing head.y here discard the guest's actual vertical layout
information?

If a user configures a multi-monitor layout in the guest that is not purely
side-by-side (like a vertical or custom arrangement), it appears
qxl_crtc_update_monitors_config() will override the layout data sent to the
SPICE client. Could this break the SPICE client's window placement and
destroy absolute mouse input mapping on secondary monitors, since the
client's coordinate space would no longer match the guest's?

[ ... ]

> @@ -847,9 +877,8 @@ static int qxl_prepare_shadow(struct drm_plane *plane,
>  		other = to_qxl_plane_state(ps);
>  		if (ps == new_state)
>  			qps->x = surf.width;
> -		bo = gem_to_qxl_bo(ps->fb->obj[0]);
> -		surf.width += bo->surf.width;
> -		surf.height = max_t(u32, surf.height, bo->surf.height);
> +		surf.width += ps->src_w >> 16;
> +		surf.height = max_t(u32, surf.height, ps->src_h >> 16);

[Severity: High]
Does accumulating surf.width here in qxl_prepare_shadow() force all CRTCs
to be packed horizontally in the shadow buffer?

It seems this horizontal packing might be what necessitates overriding the
layout coordinates in qxl_crtc_update_monitors_config() above.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260926163123.39217-1-dillona@gmail.com?part=5