[PATCH xserver 2/2] randr: rrCheckPixmapBounding: do not shrink the screen_pixmap

[Hans de Goede]

Hi,

On 22-11-16 15:28, Hans de Goede wrote:
> The purpose of rrCheckPixmapBounding is to make sure that the
> screen_pixmap is *large* enough for the slave-output which crtc is
> being configured.
>
> However until now rrCheckPixmapBounding would also shrink the
> screen_pixmap in certain scenarios leading to various problems.
>
> For example: Take a laptop with its internalscreen on a slave-output and
> currently disabled and an external monitor at 1920x1080+0+0.
> Now lets say that we want to drive the external monitor at its native
> resolution of 2560x1440 and have the internal screen mirror the top left
> part of the external monitor, so we run:
>
>   $ xrandr --output eDP --mode 1920x1080 --pos 0x0 --output HDMI \
>   --mode 2560x1440 --pos 0x0
>
> Here xrandr utility first calls RRSetScreenSize to 2560x1440, then it
> calls RRSetCrtc 1920x1080+0+0 on the eDP, since this is a slave output,
> rrCheckPixmapBounding gets called and resizes the screen_pixmap to
> 1920x1080, undoing the RRSetScreenSize. Then RRSetCrtc 2560x1440+0+0
> gets called on the HDMI, depending on crtc->transforms this will
> either result in a BadValue error from ProcRRSetCrtcConfig; or
> it will succeed, but the monitor ends up running at 2560x1440
> while showing a 1920x1080 screen_pixmap + black borders on the right
> and bottom. Neither of which is what we want.
>
> This commit removes the troublesome shrinking behavior, fixing this.
>
> Note:
>
> 1) One could argue that this will leave us with a too large screen_pixmap
> in some cases, but rrCheckPixmapBounding only gets called for slave
> outputs, so xrandr clients already must manually shrink the screen_pixmap
> after disabling crtcs in normal setups.
>
> 2) An alternative approach would be to also call rrCheckPixmapBounding
> on RRSetCrtc on normal (non-slave) outputs, but that would result in
> 2 unnecessary resizes of the screen_pixmap in the above example, which
> seems undesirable.
>
> Cc: Nikhil Mahale <nmahale at nvidia.com>
> Cc: Dave Airlie <airlied at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>

p.s.

Nikhil, this should actually fix the issue you were trying to fix with this
patch: https://patchwork.freedesktop.org/patch/88159/

Also I'm pretty sure your patch did not actually fix this, as it
disabled the BadValue check for slave-outputs (GPU-screens), but the
BadValue error will actually trigger when setting up the non-slave
output, after calling RRSetCrtc on the slave-output and
rrCheckPixmapBounding has shrunk the screen_pixmap.

Also just disabling the check is no good, the shrinking is the problem,
not the check.

Regards,

Hans



> ---
>  randr/rrcrtc.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
> index ac853ea..d1a51f0 100644
> --- a/randr/rrcrtc.c
> +++ b/randr/rrcrtc.c
> @@ -689,6 +689,12 @@ rrCheckPixmapBounding(ScreenPtr pScreen,
>      new_width = newsize->x2;
>      new_height = newsize->y2;
>
> +    if (new_width < screen_pixmap->drawable.width)
> +        new_width = screen_pixmap->drawable.width;
> +
> +    if (new_height < screen_pixmap->drawable.height)
> +        new_height = screen_pixmap->drawable.height;
> +
>      if (new_width == screen_pixmap->drawable.width &&
>          new_height == screen_pixmap->drawable.height) {
>      } else {
>