| Message ID | 20171129142658.3706-1-ofourdan@redhat.com |
|---|---|
| State | RFC |
| Headers | show |
| Series |
"xwayland: List all wl_output::mode(s) in xrandr"
( rev:
1
)
in
X.org (DEPRECATED - USE GITLAB) |
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c index 460caaf56..2a070014b 100644 --- a/hw/xwayland/xwayland-output.c +++ b/hw/xwayland/xwayland-output.c @@ -99,12 +99,71 @@ output_handle_geometry(void *data, struct wl_output *wl_output, int x, int y, xwl_output->rotation = wl_transform_to_xrandr(transform); } +static void +output_add_new_mode(struct xwl_output *xwl_output, uint32_t flags, + int width, int height, int refresh) +{ + struct xwl_mode *xwl_mode; + + xwl_mode = calloc(1, sizeof *xwl_mode); + if (!xwl_mode) + FatalError("Failed to create new mode"); + + xwl_mode->width = width; + xwl_mode->height = height; + xwl_mode->refresh = refresh; + xwl_mode->randr_mode = xwayland_cvt(width, height, refresh / 1000.0, 0, 0); + xorg_list_append(&xwl_mode->link, &xwl_output->modes_list); + + xwl_output->num_modes++; + if (flags & WL_OUTPUT_MODE_PREFERRED) + xwl_output->preferred_mode = xwl_output->num_modes; +} + +static RRModePtr +output_get_current_rr_mode(struct xwl_output *xwl_output) +{ + struct xwl_mode *xwl_mode; + + xorg_list_for_each_entry(xwl_mode, &xwl_output->modes_list, link) { + if (xwl_mode->width == xwl_output->width && + xwl_mode->height == xwl_output->height && + xwl_mode->refresh == xwl_output->refresh) + return xwl_mode->randr_mode; + } + + FatalError("Cannot find current mode [%ix%i]@%.2f", + xwl_output->width, xwl_output->height, xwl_output->refresh / 1000.0); +} + +static RRModePtr * +output_get_all_rr_modes(struct xwl_output *xwl_output) +{ + struct xwl_mode *xwl_mode; + RRModePtr *rr_modes; + int i = 0; + + rr_modes = xallocarray(xwl_output->num_modes, sizeof(RRModePtr)); + if (!rr_modes) + FatalError("Failed to create the list of RR modes"); + + xorg_list_for_each_entry(xwl_mode, &xwl_output->modes_list, link) { + rr_modes[i++] = xwl_mode->randr_mode; + } + + return rr_modes; +} + static void output_handle_mode(void *data, struct wl_output *wl_output, uint32_t flags, int width, int height, int refresh) { struct xwl_output *xwl_output = data; + /* Add available modes during the binding phase */ + if (xwl_output->binding) + output_add_new_mode (xwl_output, flags, width, height, refresh); + if (!(flags & WL_OUTPUT_MODE_CURRENT)) return; @@ -205,12 +264,21 @@ output_handle_done(void *data, struct wl_output *wl_output) struct xwl_output *it, *xwl_output = data; struct xwl_screen *xwl_screen = xwl_output->xwl_screen; int width = 0, height = 0, has_this_output = 0; - RRModePtr randr_mode; + RRModePtr randr_current_mode; - randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height, - xwl_output->refresh / 1000.0, 0, 0); - RROutputSetModes(xwl_output->randr_output, &randr_mode, 1, 1); - RRCrtcNotify(xwl_output->randr_crtc, randr_mode, + if (xwl_output->binding) { + RRModePtr *randr_modes; + + randr_modes = output_get_all_rr_modes(xwl_output); + RROutputSetModes(xwl_output->randr_output, randr_modes, + xwl_output->num_modes, xwl_output->preferred_mode); + free(randr_modes); + /* We're done with binding. do not expect further modes to list */ + xwl_output->binding = FALSE; + } + + randr_current_mode = output_get_current_rr_mode(xwl_output); + RRCrtcNotify(xwl_output->randr_crtc, randr_current_mode, xwl_output->x, xwl_output->y, xwl_output->rotation, NULL, 1, &xwl_output->randr_output); @@ -269,6 +337,7 @@ xwl_output_create(struct xwl_screen *xwl_screen, uint32_t id) goto err; } + xwl_output->binding = TRUE; xwl_output->server_output_id = id; wl_output_add_listener(xwl_output->output, &output_listener, xwl_output); @@ -288,7 +357,7 @@ xwl_output_create(struct xwl_screen *xwl_screen, uint32_t id) ErrorF("Failed creating RandR Output\n"); goto err; } - + xorg_list_init(&xwl_output->modes_list); RRCrtcGammaSetSize(xwl_output->randr_crtc, 256); RROutputSetCrtcs(xwl_output->randr_output, &xwl_output->randr_crtc, 1); RROutputSetConnection(xwl_output->randr_output, RR_Connected); @@ -316,8 +385,14 @@ xwl_output_remove(struct xwl_output *xwl_output) { struct xwl_output *it; struct xwl_screen *xwl_screen = xwl_output->xwl_screen; + struct xwl_mode *xwl_mode, *next_xwl_mode; int width = 0, height = 0; + xorg_list_for_each_entry_safe(xwl_mode, next_xwl_mode, &xwl_output->modes_list, link) { + RRModeDestroy(xwl_mode->randr_mode); + free(xwl_mode); + } + RRCrtcDestroy(xwl_output->randr_crtc); RROutputDestroy(xwl_output->randr_output); xorg_list_del(&xwl_output->link); diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h index 3adee82fa..0b6e212e5 100644 --- a/hw/xwayland/xwayland.h +++ b/hw/xwayland/xwayland.h @@ -255,11 +255,23 @@ struct xwl_tablet_pad { struct xorg_list pad_group_list; }; +struct xwl_mode { + struct xorg_list link; + int width; + int height; + int refresh; + RRModePtr randr_mode; +}; + struct xwl_output { struct xorg_list link; struct wl_output *output; uint32_t server_output_id; struct xwl_screen *xwl_screen; + struct xorg_list modes_list; + Bool binding; + int num_modes; + int preferred_mode; RROutputPtr randr_output; RRCrtcPtr randr_crtc; int32_t x, y, width, height, refresh;
On Wed, 29 Nov 2017 15:26:58 +0100 Olivier Fourdan <ofourdan@redhat.com> wrote: > Xwayland would only list the current wl_output mode in xrandr, even > though multiple modes might be advertised by the Wayland compositor. > > List all available modes listed by the Wayland compositor using > wl_output::mode in XrandR. > > Signed-off-by: Olivier Fourdan <ofourdan@redhat.com> > --- > Note: this works best with weston which lists multiple modes, unlike > gnome-shell/mutter which advertises only the current mode in > wl_output::mode. > > hw/xwayland/xwayland-output.c | 87 ++++++++++++++++++++++++++++++++++++++++--- > hw/xwayland/xwayland.h | 12 ++++++ > 2 files changed, 93 insertions(+), 6 deletions(-) Hi Olivier, do you also plan to let Xwayland change the mode? If not, what's the benefit of this? My naive guess is that if apps or users don't see multiple modes in the list, they are less likely to attempt to change it via RandR. Thanks, pq
Hi Pekka, > do you also plan to let Xwayland change the mode? If not, what's the > benefit of this? Yeah, sorry, I didn't give all the details on why this RFC patch, my bad. Basically, in downstream RH bug 1289714 [1], Robert Mader (cc'ed) is running some proof of concept to see how he can improve the support for older games in Xwayland. So this patch here is mostly a follow-up on my comment 15 in that bug [2], where I was arguing that Xwayland should not add fake modes by itself but use whatever the Wayland compositor advertises. > My naive guess is that if apps or users don't see multiple modes in the > list, they are less likely to attempt to change it via RandR. No, I don't plan to let users change the modes via xrandr, but from bug 1289714 it seems that listing available modes helps some games (not sure why, I don't play games), even though input transformation is broken. There is a lot more for this, this patch here is just a first (small) step (thus the RFC) to use the available modes listed by the Wayland compositor rather than faking arbitrary modes in Xwayland, and since I didn't reckon this patch would break anything, I posted that to the ML for more feedback. Cheers, Olivier [1] https://bugzilla.redhat.com/show_bug.cgi?id=1289714 [2] https://bugzilla.redhat.com/show_bug.cgi?id=1289714#c15
On Wed, 29 Nov 2017 10:18:47 -0500 (EST) Olivier Fourdan <ofourdan@redhat.com> wrote: > Hi Pekka, > > > do you also plan to let Xwayland change the mode? If not, what's the > > benefit of this? > > Yeah, sorry, I didn't give all the details on why this RFC patch, my bad. > > Basically, in downstream RH bug 1289714 [1], Robert Mader (cc'ed) is > running some proof of concept to see how he can improve the support > for older games in Xwayland. Ooh, exciting! I had a quick glimpse through the thread. > So this patch here is mostly a follow-up on my comment 15 in that bug > [2], where I was arguing that Xwayland should not add fake modes by > itself but use whatever the Wayland compositor advertises. Right, however, I would ask this: if Xwayland will not be able to actually set the modes, does it make a difference if the modes listed are real or artificial? I'm more curious than trying to argue against the patch, mind. > > My naive guess is that if apps or users don't see multiple modes in > > the list, they are less likely to attempt to change it via RandR. > > No, I don't plan to let users change the modes via xrandr, but from > bug 1289714 it seems that listing available modes helps some games > (not sure why, I don't play games), even though input transformation > is broken. Does it actually break the input transformation? Well, depends on how you handle the RandR mode change, I suppose. > There is a lot more for this, this patch here is just a first (small) > step (thus the RFC) to use the available modes listed by the Wayland > compositor rather than faking arbitrary modes in Xwayland, and since > I didn't reckon this patch would break anything, I posted that to the > ML for more feedback. Yes, letting Xwayland pretend successful mode switches is an interesting topic. I saw the input transformation fixup patch that seems special-case the fullscreen window. If there is a X11 window on top of the fullscreen window, that would still get wrong input coordinates, would it not? An idea comes to mind: what if you'd make RandR mode changes change the scaling of *all* Xwayland wl_surfaces? Say, xwl_screen size is 1600x1200, RandR "sets" 800x600, and suddenly all Xwayland wl_surfaces start using wp_viewport to set 2x up-scaling? I.e. X11 window size that is 800x600 will produce a 800x600 wl_buffer, but wp_viewport makes the wl_surface 1600x1200. I think visually that should match pretty close what an actual mode change would have done, for all X11 windows on the Wayland desktop. Obviously now X11 and Wayland windows are at different scale, but since for X11 the scaling by video mode is global (per Xwayland instance) anyway, it kind of... fits? To a Wayland compositor this would look like a small buffer upscaled to fill the screen, which, if implemented in a compositor, could even trigger a real video mode switch automatically based on which window is active and on top. Fixing up the input should be a little easier I imagine, because all X11 windows have the same scaling factor, which means the X11 coordinate space can stay consistent for all X11 objects... right? One thing I didn't think is how the XWM could cope with this scaling, does it mess up some window relative to window positioning. Crazy? I'll let you decide. :-) > [1] https://bugzilla.redhat.com/show_bug.cgi?id=1289714 > [2] https://bugzilla.redhat.com/show_bug.cgi?id=1289714#c15 Thanks, pq
Hi Pekka, > > [...] > > > > Basically, in downstream RH bug 1289714 [1], Robert Mader (cc'ed) is > > running some proof of concept to see how he can improve the support > > for older games in Xwayland. > > Ooh, exciting! I had a quick glimpse through the thread. > > > So this patch here is mostly a follow-up on my comment 15 in that bug > > [2], where I was arguing that Xwayland should not add fake modes by > > itself but use whatever the Wayland compositor advertises. > > Right, however, I would ask this: if Xwayland will not be able to > actually set the modes, does it make a difference if the modes listed > are real or artificial? No difference, I reckon, fake or real doesn't matter, but I'd rather let the compositor decide. My point being solely that Xwayland should not decide to come up with and force fake modes all by itself if the compositor doesn't know how to deal with those. > I'm more curious than trying to argue against the patch, mind. > > > > My naive guess is that if apps or users don't see multiple modes in > > > the list, they are less likely to attempt to change it via RandR. > > > > No, I don't plan to let users change the modes via xrandr, but from > > bug 1289714 it seems that listing available modes helps some games > > (not sure why, I don't play games), even though input transformation > > is broken. > > Does it actually break the input transformation? Well, depends on how > you handle the RandR mode change, I suppose. > > > There is a lot more for this, this patch here is just a first (small) > > step (thus the RFC) to use the available modes listed by the Wayland > > compositor rather than faking arbitrary modes in Xwayland, and since > > I didn't reckon this patch would break anything, I posted that to the > > ML for more feedback. > > Yes, letting Xwayland pretend successful mode switches is an > interesting topic. > > I saw the input transformation fixup patch that seems special-case the > fullscreen window. If there is a X11 window on top of the fullscreen > window, that would still get wrong input coordinates, would it not? Yeah that's one of the corner case, I'm sure there are a lot more. > An idea comes to mind: what if you'd make RandR mode changes change the > scaling of *all* Xwayland wl_surfaces? > > Say, xwl_screen size is 1600x1200, RandR "sets" 800x600, and suddenly > all Xwayland wl_surfaces start using wp_viewport to set 2x up-scaling? > I.e. X11 window size that is 800x600 will produce a 800x600 wl_buffer, > but wp_viewport makes the wl_surface 1600x1200. I think visually that > should match pretty close what an actual mode change would have done, > for all X11 windows on the Wayland desktop. > > Obviously now X11 and Wayland windows are at different scale, but since > for X11 the scaling by video mode is global (per Xwayland instance) > anyway, it kind of... fits? Now, I think this is a very interesting idea, also because randr isn't per client, even less per window (thus matching a surface to the X11 client issuing the randr request might not be trivial in xwl_randr_set_config()). So making the scaling global would solve that problem. > To a Wayland compositor this would look like a small buffer upscaled to > fill the screen, which, if implemented in a compositor, could even > trigger a real video mode switch automatically based on which window is > active and on top. > > Fixing up the input should be a little easier I imagine, because all > X11 windows have the same scaling factor, which means the X11 > coordinate space can stay consistent for all X11 objects... right? > > One thing I didn't think is how the XWM could cope with this scaling, > does it mess up some window relative to window positioning. > > Crazy? I'll let you decide. :-) If you ask me, probably one of the less crazy ideas I've heard around the topic to be honest, and definitely worth pursuing :) Copying Jonas and Carlos as they were looking into Robert's approach. Cheers, Olivier
Hey, > > Say, xwl_screen size is 1600x1200, RandR "sets" 800x600, and suddenly > > all Xwayland wl_surfaces start using wp_viewport to set 2x up-scaling? > > I.e. X11 window size that is 800x600 will produce a 800x600 wl_buffer, > > but wp_viewport makes the wl_surface 1600x1200. I think visually that > > should match pretty close what an actual mode change would have done, > > for all X11 windows on the Wayland desktop. Sorry, I forgot to add one last point. Right now, mutter doesn't list multiple modes and doesn't support wp_viewporter either, but weston does both so for now weston is probably a better candidate for experimenting with this (at least until mutter catches up on those). Cheers, Olivier
On Thu, 30 Nov 2017 04:14:08 -0500 (EST) Olivier Fourdan <ofourdan@redhat.com> wrote: > Hi Pekka, > > > > [...] > > > > > > Basically, in downstream RH bug 1289714 [1], Robert Mader (cc'ed) is > > > running some proof of concept to see how he can improve the support > > > for older games in Xwayland. > > > > Ooh, exciting! I had a quick glimpse through the thread. > > > > > So this patch here is mostly a follow-up on my comment 15 in that bug > > > [2], where I was arguing that Xwayland should not add fake modes by > > > itself but use whatever the Wayland compositor advertises. > > > > Right, however, I would ask this: if Xwayland will not be able to > > actually set the modes, does it make a difference if the modes > > listed are real or artificial? > > No difference, I reckon, fake or real doesn't matter, but I'd rather > let the compositor decide. That's a reasonable justification. > My point being solely that Xwayland should not decide to come up with > and force fake modes all by itself if the compositor doesn't know how > to deal with those. That somewhat depends on whether the compositor actually has to deal with anything. But, getting the mode list from the compositor is safer. If a game needs a specific mode, the Wayland compositor should arrange it. > > I'm more curious than trying to argue against the patch, mind. > > > > > > My naive guess is that if apps or users don't see multiple > > > > modes in the list, they are less likely to attempt to change it > > > > via RandR. > > > > > > No, I don't plan to let users change the modes via xrandr, but > > > from bug 1289714 it seems that listing available modes helps some > > > games (not sure why, I don't play games), even though input > > > transformation is broken. > > > > Does it actually break the input transformation? Well, depends on > > how you handle the RandR mode change, I suppose. > > > > > There is a lot more for this, this patch here is just a first > > > (small) step (thus the RFC) to use the available modes listed by > > > the Wayland compositor rather than faking arbitrary modes in > > > Xwayland, and since I didn't reckon this patch would break > > > anything, I posted that to the ML for more feedback. > > > > Yes, letting Xwayland pretend successful mode switches is an > > interesting topic. > > > > I saw the input transformation fixup patch that seems special-case > > the fullscreen window. If there is a X11 window on top of the > > fullscreen window, that would still get wrong input coordinates, > > would it not? > > Yeah that's one of the corner case, I'm sure there are a lot more. Just asked this in IRC as well: why is the input fixup patch to Xwayland necessary at all? If mutter decides to scale up something on its own, it should not change how the Wayland client sees input, because input is defined in surface-local coordinates. Is it a mutter bug? > > An idea comes to mind: what if you'd make RandR mode changes change > > the scaling of *all* Xwayland wl_surfaces? > > > > Say, xwl_screen size is 1600x1200, RandR "sets" 800x600, and > > suddenly all Xwayland wl_surfaces start using wp_viewport to set 2x > > up-scaling? I.e. X11 window size that is 800x600 will produce a > > 800x600 wl_buffer, but wp_viewport makes the wl_surface 1600x1200. > > I think visually that should match pretty close what an actual mode > > change would have done, for all X11 windows on the Wayland desktop. > > > > Obviously now X11 and Wayland windows are at different scale, but > > since for X11 the scaling by video mode is global (per Xwayland > > instance) anyway, it kind of... fits? > > Now, I think this is a very interesting idea, also because randr > isn't per client, even less per window (thus matching a surface to > the X11 client issuing the randr request might not be trivial in > xwl_randr_set_config()). > > So making the scaling global would solve that problem. While having lunch, I remembered something that makes it non-ideal: there can be multiple RandR outputs, but the mode switch is per RandR output, not global. Thanks, pq
Hej Pekka, On 30.11.2017 10:44, Pekka Paalanen wrote: >> My point being solely that Xwayland should not decide to come up with >> and force fake modes all by itself if the compositor doesn't know how >> to deal with those. I totally support this. Anyway we should make sure mutter lists the most important legacy modes (like 800x600), regardless whether supported by the display or not, as soon as it supports scaling. > > Just asked this in IRC as well: why is the input fixup patch to > Xwayland necessary at all? > > If mutter decides to scale up something on its own, it should not > change how the Wayland client sees input, because input is defined in > surface-local coordinates. Is it a mutter bug? > Yeah I think it is. Actually the fact that things get scaled up in fullscreen mode at all seems to be more of a side effect. I tried to move the coordination fix into mutter, but once scaled up, the surface didn't seem to have any reference to it's original size. So that xwayland patch was just for tinkering around. With this xwayland patch here, I'll look into implementing wp_viewport, which should handle input and output scaling. Thanks, Robert
Xwayland would only list the current wl_output mode in xrandr, even though multiple modes might be advertised by the Wayland compositor. List all available modes listed by the Wayland compositor using wl_output::mode in XrandR. Signed-off-by: Olivier Fourdan <ofourdan@redhat.com> --- Note: this works best with weston which lists multiple modes, unlike gnome-shell/mutter which advertises only the current mode in wl_output::mode. hw/xwayland/xwayland-output.c | 87 ++++++++++++++++++++++++++++++++++++++++--- hw/xwayland/xwayland.h | 12 ++++++ 2 files changed, 93 insertions(+), 6 deletions(-)