Xwayland 24.1 branching and schedule
Hi all In preparation for the forthcoming Xwayland 24.1.0 release, I've now created the xwayland-24.1 branch: https://gitlab.freedesktop.org/xorg/xserver/-/tree/xwayland-24.1 And there's also a milestone that can be used to tag issues and merge requests targeting Xwayland 24.1.0: https://gitlab.freedesktop.org/xorg/xserver/-/milestones/17 We are already quite late in the year so for schedule, I would like to start as soon as next week, and an RC every two weeks: * April 17th, 2024: Xwayland 24.1.0 RC1 * May 2nd, 2024: Xwayland 24.1.0 RC2 * May 15th, 2024: Xwayland 24.1.0 (if all goes well) Cheers Olivier
Re: issues and merge requests
Hi, On Wed, Feb 7, 2024 at 11:47 AM Enrico Weigelt, metux IT consult < i...@metux.net> wrote: > On 07.02.24 00:59, Peter Hutterer wrote: > >> Closes: xorg/xserver#1631 > > > > It supports that too, but afaik the use of Fixes is more common in the > > xorg repo. In (some?) gnome OTOH repos Closes refers to an issue anf > > Fixes usually to a commit. > > Okay, so settle to "Fixes:" ? > As Peter explained, "Fixes" refers to a commit in particular (like a regression was introduced, then fixed with a later commit, that later commit "fixes" the first commit) - That is useful for example when backporting between branches, because those can be squashed together, if caught on time before the first commit gets backported. e.g.: https://gitlab.freedesktop.org/xorg/xserver/-/commit/133e0d6 "Closes" refers to the issue in gitlab that that particular commit addresses: e.g.: https://gitlab.freedesktop.org/xorg/xserver/-/commit/e622466 Also, please note that both can be used in the same commit: e.g.: https://gitlab.freedesktop.org/xorg/xserver/-/commit/3ddb81b Perhaps it's time to write some little document about that. > > We already have some things in the Wiki, but seems to be a bit outdated. > I'd prefer having such docs within the source tree. > > >> By the way: how to do we handle fixes that might go to several branches > ? > > > > merge it into master, then `git cherry-pick -x` to the branch, file a > > merge request for that particular branch. The gitlab closed merge > > request page will have examples of those, usually prefixed with the > > branch they're supposed to be merged in to make them easier to identify. > > Yes, that's the technical side, but I've been wondering about a formal > process on how to decide which stuff should be backported, especially > for bugfixes. Some projects (e.g. Linux kernel) extract them (semi-) > automatically by git headers. > > Or maybe have some tags that one can set if one *thinks* something > might be worth backporting (or moving to another branch like Xwayland), > so the corresponding maintainer could be notified and decide on his > own ? > Just a note to clarify, xwayland is just like any other stable branch of the xserver, fixes are not /moved/ to xwayland, they get merged first into master and then get backported into the stable xwayland branch(es) as necessary. This is something I do on a regular basis as a maintainer of Xwayland, it's a manual process indeed. Cheers, Olivier
Re: Patches to libXau
On Thu, Feb 1, 2024 at 4:09 PM Enrico Weigelt, metux IT consult < i...@metux.net> wrote: > have you checked whether it's already on gitlab.freedesktop.org ? > In that case just make a PR there ? > Indeed, it's there: https://gitlab.freedesktop.org/xorg/lib/libxau Cheers Olivier
Re: Hitting X limit in launching maximum number of vkcube instances
Hi Anuj, On Mon, Aug 14, 2023 at 8:27 PM Anuj Phogat wrote: > - I'm still getting the "_XSERVTransSocketUNIXAccept: accept() failed" > error > after launching ~ 256 instances of vkcube. I was able to find the root > cause of error after patching libxtrans based on Adam's suggestion: > "_XSERVTransSocketUNIXAccept: accept() failed (Too many open files)" > - Based on the hint from the error string, I changed the system wide limit > to > increase the maximum number of open files allowed from default 1024 to > 'ulimit -n' confirmed the new limit. But, this change doesn't change > the error past launching ~ 256 instances of vkcube. > - I hit the same error but at ~330 instances when I try to run glxgears. > > Questions: > What am I missing when changing the open files limit ? > Make sure the X process has inherited that limit, depending on how/where you bumped the limit, the X server process may not have that applied. You can check using the /proc filesystem on Linux: $ cat /proc/$(pidof Xorg)/limits What else can I try to get past this error ? > If you can get 330 instances of glxgears, it means that you are already past the 256 limit, hence the maxclients worked. It is worth noting that the limit applies to all X11 clients, so if you run a window manager, an xterm, etc, everything counts. Also if a client opens more than one connection to the X server, that also counts. So you can expect that a limit of 512 might give you a lower actual limit. > Should I reopen xserver issue [1] or create a new issue to track it ? > I doubt that this is an X server issue. FWIW, I just tried here with Xwayland rootful and was able to run 474 instances of vkcube on a rootful stanlone Xwayland instance: $ Xwayland -maxclients 512 -decorate :12 7 $ for i in $(seq 1 512); do DISPLAY=:12 vkcube& done $ ps aux | grep vkcube | wc -l 474 I have seen some "Maximum number of clients reached" errors while the vkcube instances were spawning en masse, so I suspect maybe the vulkan implementation is opening a connection to the display for some reason temporarily, so having those start all at once may lead to a lower actual limit of clients.. To confirm that theory, I dedid the same test waiting a bit between each instance of vkcube. And nowI can reach the limit of 512: $ for i in $(seq 1 512); do DISPLAY=:12 vkcube; sleep .2 & done $ ps aux | grep vkcube | wc -l 512 So yeah, no bug in the Xserver AFAICS. This is with: $ cat /proc/$(pidof Xwayland)/limits Limit Soft Limit Hard Limit Units Max cpu time unlimitedunlimitedseconds Max file size unlimitedunlimitedbytes Max data size unlimitedunlimitedbytes Max stack size8388608 unlimitedbytes Max core file sizeunlimitedunlimitedbytes Max resident set unlimitedunlimitedbytes Max processes 6231962319 processes Max open files16777216 16777216 files Max locked memory 8388608 8388608 bytes Max address space unlimitedunlimitedbytes Max file locksunlimitedunlimitedlocks Max pending signals 6231962319signals Max msgqueue size 819200 819200 bytes Max nice priority 00 Max realtime priority 00 Max realtime timeout unlimitedunlimitedus [1] : https://gitlab.freedesktop.org/xorg/xserver/-/issues/1310 > Cheers Olivier
Re: Plan for an Xwayland 23.2 release
Hey Michel, On Fri, Jul 7, 2023 at 5:16 PM Michel Dänzer wrote: > > Sounds great to me. Hopefully we can merge > https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1131 before > cutting the branch, assuming no major issues with it come up in the meantime. Yes, I have that one in mind :) Cheers Olivier
Re: Plan for an Xwayland 23.2 release
Hi all, On Thu, Jun 15, 2023 at 11:01 AM Olivier Fourdan wrote: > > Hi all, > > There are changes in Xwayland that I would like to see landed as part > of another Xwayland 23.2 release this year, as those could not make it > on time for 23.1, namely libEI support (now that libEI 1.0 was > released), and possibly a few other changes: > > - Add EI support >https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/975 > - Implement wp_tearing_control_v1 >https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/665 > Both merge requests have now landed, so I'd like to start the process. For this purpose, I've created a new milestone "Xwayland-23.2.0": https://gitlab.freedesktop.org/xorg/xserver/-/milestones/16 And pushed the preparation work in a merge request: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1132 (** Not ** to be merged until the branch for xwayland 23.2 is created!) As for the schedule, I would like to propose the following: * July 19th, 2023: Xwayland 23.2.0 RC1 * August 2nd, 2023: Xwayland 23.2.0 RC2 * August 16th, 2023: Xwayland 23.2.0 Cheers Olivier
Plan for an Xwayland 23.2 release
Hi all, There are changes in Xwayland that I would like to see landed as part of another Xwayland 23.2 release this year, as those could not make it on time for 23.1, namely libEI support (now that libEI 1.0 was released), and possibly a few other changes: - Add EI support https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/975 - Implement wp_tearing_control_v1 https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/665 The latter unfortunately needs a new release of xorgproto because 2023.1 was missing a version bump for the Present proto, yikes! - https://gitlab.freedesktop.org/xorg/proto/xorgproto/-/merge_requests/75 I plan to make an xorgproto release with that fix today or tomorrow. It would be a good time to (re)consider the GLAMOR fixes that have been pending for some time: - https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests?label_name[]=glamor Thanks Olivier
Re: Hitting X limit in launching maximum number of vkcube instances
Hi On Thu, May 18, 2023 at 10:45 AM Anuj Phogat wrote: > > On Sat, May 6, 2023 at 9:04 AM Alan Coopersmith > wrote: > > > > On 5/5/23 16:07, Anuj Phogat wrote: > > > Thanks for the quick response Adam. I've seen this behavior across > > > multiple > > > graphics applications > > > and graphics drivers. This is an extreme use case I'm trying out. So, > > > there's a > > > possibility of this use > > > case not being handled correctly by any of the applications or graphics > > > drivers. > > > But I also see some basic X functionality, like closing the window using > > > the "x" > > > button, not working > > > properly with windows opened past the 256 limit. This happens with a > > > modified > > > limit of 512. I had to > > > use xkill to close those windows. Any thoughts about this? > > > > Closing the window with the "x" button is a function of the window manager. > > Does the window manager you are using assume the number of X clients is > > limited to 256? That xkill works suggests the X server is handling it > > correctly. > Alan, I took the window manager out of the equation by running multiple > instances of vkcube with Xorg without running any window manager. > Xorg.0.log file does show a log about set max clients through a .conf file: > (**) Option "MaxClients" "512" > > But, I start seeing below error messages at the end of the log file once > the number of vkcube instances go past 256: > "_XSERVTransSocketUNIXAccept: accept() failed" > > Seems like the modified max clients limit of 512 is not getting configured > correctly in X. > > What can I do to fix / debug this error ? I didn't find a mention of the actual version of the Xserver you're running (not knowing what Ubuntu ships), but this may be relevant for your issue: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1310 Fixed with: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/937 And backported to the 21.1 branch with: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/953 HTH Cheers Olivier
X.Org Security Advisory: CVE-2023-1393: X.Org Server Overlay Window Use-After-Free
X.Org Security Advisory: March 29, 2023 X.Org Server Overlay Window Use-After-Free == This issue can lead to local privileges elevation on systems where the X server is running privileged and remote code execution for ssh X forwarding sessions. ZDI-CAN-19866/CVE-2023-1393: X.Org Server Overlay Window Use-After-Free Local Privilege Escalation Vulnerability If a client explicitly destroys the compositor overlay window (aka COW), the Xserver would leave a dangling pointer to that window in the CompScreen structure, which will trigger a use-after-free later. Patches --- Patch for this issue have been committed to the xorg server git repository. xorg-server 21.1.8 will be released shortly and will include this patch. - commit 26ef545b3 - composite: Fix use-after-free of the COW (https://gitlab.freedesktop.org/xorg/xserver/-/commit/26ef545b3) ZDI-CAN-19866/CVE-2023-1393 If a client explicitly destroys the compositor overlay window (aka COW), we would leave a dangling pointer to that window in the CompScreen structure, which will trigger a use-after-free later. Make sure to clear the CompScreen pointer to the COW when the latter gets destroyed explicitly by the client. Thanks == The vulnerabilities have been discovered by Jan-Niklas Sohn working with Trend Micro Zero Day Initiative. OpenPGP_0x14706DBE1E4B4540.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: Xwayland 23.1 schedule
Hi all, On Tue, Feb 14, 2023 at 10:45 AM Olivier Fourdan wrote: > > I'm preparing the release of Xwayland 23.1 and for that purpose filed > an issue in gitlab: > > https://gitlab.freedesktop.org/xorg/xserver/-/issues/1426 > > I have also prepared the branch and posted a draft MR (not to be merged!) > here: > > https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1069 > > I am just duplicating here what I put in the xorg/xserver!1426 to > raise awareness for a wider audience, the plan is: > > * Branch out xwayland-23.1 by the end of this week Quick update, this very first step is now done: https://gitlab.freedesktop.org/xorg/xserver/-/tree/xwayland-23.1 > * February 22nd: rc1 > * March 8th: rc2 > * March 22nd: 23.1.0 release if all goes well > > Please let me know if that schedule works for you - Also, the > milestone "xwayland-23.1.0" in gitlab should be used to tag issues or > merge requests that need to be checked before Xwayland 23.1.0 is > released. Cheers Olivier
Xwayland 23.1 schedule
Hi all, I'm preparing the release of Xwayland 23.1 and for that purpose filed an issue in gitlab: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1426 I have also prepared the branch and posted a draft MR (not to be merged!) here: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1069 I am just duplicating here what I put in the xorg/xserver!1426 to raise awareness for a wider audience, the plan is: * Branch out xwayland-23.1 by the end of this week * February 22nd: rc1 * March 8th: rc2 * March 22nd: 23.1.0 release if all goes well Please let me know if that schedule works for you - Also, the milestone "xwayland-23.1.0" in gitlab should be used to tag issues or merge requests that need to be checked before Xwayland 23.1.0 is released. Cheers Olivier
Re: Enabling gitlab CI across all the X.Org repos
Hi Alan On Wed, 3 Aug 2022 at 00:19, Alan Coopersmith wrote: > lib/libxcvt- I'm not quite sure how to add it here given > the unusual CI template setup for this repo > […] > It is just using ci-fairy, it's not that much different, just probably overkill for that use though. Anyway, I posted https://gitlab.freedesktop.org/xorg/lib/libxcvt/-/merge_requests/12 to add the Security/SAST.gitlab-ci.yml template to enable static analysis. Cheers Olivier
Xwayland 22.1.1 tomorrow?
Hi all, It's been a month and half since Xwayland 22.1.0 was released. Since then, we landed a few but interesting fixes, so I plan to release Xwayland 22.1.1 tomorrow, unless someone disagrees. Cheers Olivier
Xwayland 22.1 schedule
Hi all, It's been a year since we released Xwayland standalone and the xwayland-21.1 branch. Some new (and nice!) features found their way in the master branch of the xserver since then and the time has come to consider a new xwayland-22.1 branch and release, similar to what Michel did a year or so ago for xwayland-21.1. For that purpose I prepared the branch and posted a draft MR (not to be merged) here: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/819 I see no reason to wait any longer so I'd propose the following schedule: * Create the branch xwayland-22.1 this week (week #2) * January 19th: 1st release candidate * February 2nd: 2nd release candidate * February 16th: 22.1.0 release if all goes well Please let me know if that schedule works for you - Also, the milestone xwayland-22.1.0 in gitlab should be used to tag issues or merge requests that need to be checked before Xwayland 22.1.0 is released. Cheers Olivier
Re: New libxcvt library
Hi Walter, On Fri, Apr 16, 2021 at 12:32 PM Walter Harms wrote: > i think it is a good idea. > Can you give a hint how big that lib is ? > Right now, it's extremely tiny, the lib is mostly one function taken from the xserver. https://gitlab.freedesktop.org/ofourdan/libxcvt Even when the idea is good it is not really helpful to have to many > different libs hanging around. Well, it's still better than duplicating code within the same project :) > I read Keith Packard was already considering putting that into xrandr, if > its not to big i would say it would > be a good idea to collect the monitor related things into one lib (EDID > ?). > I am completely open to adding more features and fixes to the lib, actually, it was one of the stated goals, the code is in need of TLC and having it in one place should help with that goal. Cheers Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
RFC: New libxcvt library
Hi all, Currently. the Xorg Xserver has its own implementation of the VESA CVT standard timing modelines generator in `hw/xfree86/modes/xf86cvt.c`. That code is placed in its own source file alone because it is also being used by the `cvt` utility. Because it's part of the Xorg DDX, Xwayland, which is another DDX also has a copy of the same code in `hw/xwayland/xwayland-cvt.c`. Now with Xwayland being a standalone package, mutter which uses the cvt utility at build time still needs to install `cvt` from Xorg to generate the modes used in Wayland. Some time ago, discussing with Jonas Adahl on irc, we thought it would be a good idea to have that code and utility part of its own standalone project, to avoid the code duplication and possibly ease contributions. That was https://gitlab.freedesktop.org/xorg/xserver/-/issues/1142 So I went ahead and did just that, called it “libxcvt” and placed in https://gitlab.freedesktop.org/ofourdan/libxcvt I also made a merge request to use that code in the Xserver: https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/637 Can we consider moving “libxcvt” to the xorg/lib namespace in gitlab? Cheers Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Why matchbox window manager calls XRenderComposite from top to bottom?
Hi, It actually renders the screen in two passes. First pass from top to bottom, rendering only the opaque regions, while adding up each opaque region to a global clipping region. Second pass, from bottom to top, rendering only the translucent areas, clipping out the opaque regions that it found in the first pass. That's more efficient than rendering in a single pass from bottom to top. FWIW, the logic is from xcompmgr, from Keith. Cheers Olivier On Fri, Jul 10, 2020 at 11:06 AM 徐星 wrote: > > Hi, > > Do you have any ideas why matchbox window manager render clients from > top(near) to bottom(far)? I understand that the painter algorithm > requires paints from far to near. > > > > > https://git.yoctoproject.org/cgit/cgit.cgi/matchbox-window-manager-2/tree/matchbox/comp-mgr/mb-wm-comp-mgr-xrender.c#n1619 > -- > NaCl! > ___ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel > ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: PATCH: add -defaultRepeatDelay and -defaultRepeatInterval flags
Hi Michael, On Thu, 2 Apr 2020 at 14:35, Michael Stapelberg < michael+freedesk...@stapelberg.ch> wrote: > Friendly ping? :) > Best is to submit your patches as a merge request in gitlab, see: https://gitlab.freedesktop.org/xorg/xserver HTH Cheers Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: MapNotify Event Is not received by the client after XMapWindow
Hi On Mon, Apr 29, 2019 at 9:41 AM Ikshwaku Chauhan wrote: > Hello All, > > > We are using Xorg version 1.14 and layermanger 1.1 with chromium browser > on NXP i.MX6 platform. > In one of our usecase chromium browser is crashing and we have found that, > this happens after XMapWindow api is called and browser is waiting for > MapNotify event with XWindowEvent call. Browser is waiting for MapNotify > event but XWindowEvent is not returning and which results browser crash > after sometime. > In our environment multiple X based apps are working > > Could anyone know about this issue? or having same insight about this > behavior? > Please provide you inputs. > Waiting for the first MapNotify event before drawing to the window is the correct way (e.g. [1]). You can easily check that the MapNotify event is emitted after the initial MapRequest using xev, e.g.: ``` $ xev Outer window is 0x101, inner window is 0x102 [...] CreateNotify event, serial 11, synthetic NO, window 0x101, parent 0x101, window 0x102, (10,10), width 50, height 50 border_width 4, override NO PropertyNotify event, serial 14, synthetic NO, window 0x101, atom 0xff (WM_PROTOCOLS), time 931817426, state PropertyNewValue MapNotify event, serial 15, synthetic NO, window 0x101, event 0x101, window 0x102, override NO ConfigureNotify event, serial 28, synthetic NO, window 0x101, [...] ``` If you get that MapNotify event with xev on first map request, then chances are the bug is not in the Xserver (I would be surprised if it were). HTH, Cheers, Olivier [1] https://tronche.com/gui/x/xlib-tutorial/ ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Xwayland on demand (Re: [PATCH xwayland 3/3] xwayland: Handle the case of windows being realized before redirection)
Hi,
On Thu, Jan 17, 2019 at 9:23 AM Pekka Paalanen wrote:
>
> On Wed, 16 Jan 2019 14:05:01 -0500
> Adam Jackson wrote:
>
> > On Wed, 2019-01-16 at 11:52 +0200, Pekka Paalanen wrote:
> >
> > > What I think we would need is a way to launch Xwayland, but ensure it
> > > does not process the real X11 apps until all the preparations (xrdb,
> > > XWM init, etc.) have finished. What the preparations are exactly, only
> > > the Wayland compositor (the desktop) will know.
> >
> > Who's launching this first X11 app, and why are they unable to defer
> > launching it until the Xwayland is prepared? There's quite a few ways
> > we could add this kind of interlock to setup, I just want to clarify
> > which problem we're solving first.
>
> Hi Adam,
>
> any user action could be starting the very first real X11 app:
> - user clicking a launcher icon on the desktop, e.g. for a game
> - user issuing a command in a shell in a terminal window
> - a Wayland app launching another app that just happens to be an X11 app
>
> In the current setup of on-demand Xwayland, the Wayland compositor will
> be listening on the X11 DISPLAY socket. When the Wayland compositor
> sees the first X11 client attempting to connect, it will launch
> Xwayland process and passes the listening socket to it.
>
> IOW, the real X11 apps exists and attempts to connect first, which then
> leads to spawning the Xwayland server. That can of course be changed if
> there are better ideas. The main point of on-demand launching is that
> Xwayland doesn't get started at the same time with the rest of the
> desktop environment, unless there really is a X11 app to be ran.
For xrdb specifically, actually, the X11 resources are stored in a
couple of properties on the root window ("RESOURCE_MANAGER" and
"SCREEN_RESOURCES") so Xwayland /could/ set those on init, it's more
the whole logic of merging resources from different files, optionally
invoking a C preprocessor, etc. that would be painful in Xwayland
IMHO.
So if there was a way to (re)use xrdb to send the resources string to
stdout instead of setting the property itself, Xwayland could possibly
get values to set the properties and do that on startup before any X11
client had a chance to connect.
Maybe that would be simplereasier than having side-channels or
interlocking in place at startup?
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Xwayland on demand (Re: [PATCH xwayland 3/3] xwayland: Handle the case of windows being realized before redirection)
On Wed, Jan 16, 2019 at 9:56 AM Olivier Fourdan wrote: > On Wed, Jan 16, 2019 at 9:55 AM Olivier Fourdan wrote: > > > > Hi > > > > On Wed, Jan 16, 2019 at 9:35 AM Pekka Paalanen wrote: > > > > > > On Tue, 15 Jan 2019 23:21:05 +0100 > > > Carlos Garnacho wrote: > > > > > > > If Xwayland gets to realize a window meant for composition before the > > > > compositor redirected windows (i.e. redirect mode is not > > > > RedirectDrawManual > > > > yet), the window would stay "invisible" as we wouldn't create a > > > > wl_surface/wl_shell_surface for it at any later point. > > > > > > > > This scenario may happen if the wayland compositor raises Xwayland on > > > > demand for a X11 client being started. In this case the first data on > > > > the socket is the client's, the compositor can hardly beat that in > > > > order to redirect subwindows before the client realizes a Window. > > > > > > > > In order to handle this case, allow the late creation of a matching > > > > (shell) surface for the WindowPtr on SetWindowPixmapProc, so it is > > > > ensured > > > > to be created after the compositor set up redirection. > > > > > > > > Signed-off-by: Carlos Garnacho > > > > --- > > > > hw/xwayland/xwayland.c | 37 + > > > > hw/xwayland/xwayland.h | 1 + > > > > 2 files changed, 38 insertions(+) > > > > > > Hi Carlos, > > > > > > this reminds me of other ordering issues with starting Xwayland on > > > demand: xrdb. I presume the X resources should be merged before any app > > > X11 client is processed, so that they see the user's resources at > > > start-up. AFAIK, that cannot happen if Xwayland is started on-demand. > > > Either it is too late if app start-up triggers it, or then you launch > > > Xwayland at desktop start-up, essentially not doing on-demand anymore. > > > Have you seen people complain about that? > > > > > > Could that be fixed somehow? > > > > > > Pass the X11 app socket to Xwayland later after the XWM init is > > > complete? Stall non-XWM clients inside Xwayland by default until XWM is > > > ready? > > > > I am not sure I understand what xwayland-on-demand changes there. > > > > Considering "xrdb" is a regular X11 client, it requires a display, > > hence it must have Xwayland running. > > > > If Xwayland is not running (i.e. xrdb is the first X11 client to be > > started), then Xwayland will be started (as there is a "demand" from a > > client, the client being xrdb) and other following X11 clients will > > connect to the same display and have the resources set in xrdb > > available. > > > > I remember Ray (halfline) advocating for Xwayland running xrdb itself > > at startup to avoid this issue. But that's a bit of a special case for > > specific X11 client (xrdb) which could be considered as legacy... > > (like, people may not want to slow down Xwayland startup waiting for > > xrdb to complete when they don't actually use any X11 application > > benefiting from the X11 resources database) > > There: https://bugzilla.gnome.org/show_bug.cgi?id=769644 > > > Maybe a more general design of a script being run automatically by > > Xwayland if present, so that users/distro-maintainers/sysadmins can > > tailor that startup script at will? Humm, thinking more about it, it would still be racy, because mutter for example (and I think weston as well) reckons Xwayland is started as soon as it writes to the displayfd, so it won't wait for a script to complete... So Xwayland would have to differ writing to displayfd until the "startup" script is complete (while still accepting connections so that the clients started from the script can run). Problem, `NotifyParentProcess()` (which writes to the displayfd) is invoked directly from `dix_main()` so Xwayland doesn't have much control over that. Cheers Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Xwayland on demand (Re: [PATCH xwayland 3/3] xwayland: Handle the case of windows being realized before redirection)
On Wed, Jan 16, 2019 at 9:55 AM Olivier Fourdan wrote: > > Hi > > On Wed, Jan 16, 2019 at 9:35 AM Pekka Paalanen wrote: > > > > On Tue, 15 Jan 2019 23:21:05 +0100 > > Carlos Garnacho wrote: > > > > > If Xwayland gets to realize a window meant for composition before the > > > compositor redirected windows (i.e. redirect mode is not > > > RedirectDrawManual > > > yet), the window would stay "invisible" as we wouldn't create a > > > wl_surface/wl_shell_surface for it at any later point. > > > > > > This scenario may happen if the wayland compositor raises Xwayland on > > > demand for a X11 client being started. In this case the first data on > > > the socket is the client's, the compositor can hardly beat that in > > > order to redirect subwindows before the client realizes a Window. > > > > > > In order to handle this case, allow the late creation of a matching > > > (shell) surface for the WindowPtr on SetWindowPixmapProc, so it is ensured > > > to be created after the compositor set up redirection. > > > > > > Signed-off-by: Carlos Garnacho > > > --- > > > hw/xwayland/xwayland.c | 37 + > > > hw/xwayland/xwayland.h | 1 + > > > 2 files changed, 38 insertions(+) > > > > Hi Carlos, > > > > this reminds me of other ordering issues with starting Xwayland on > > demand: xrdb. I presume the X resources should be merged before any app > > X11 client is processed, so that they see the user's resources at > > start-up. AFAIK, that cannot happen if Xwayland is started on-demand. > > Either it is too late if app start-up triggers it, or then you launch > > Xwayland at desktop start-up, essentially not doing on-demand anymore. > > Have you seen people complain about that? > > > > Could that be fixed somehow? > > > > Pass the X11 app socket to Xwayland later after the XWM init is > > complete? Stall non-XWM clients inside Xwayland by default until XWM is > > ready? > > I am not sure I understand what xwayland-on-demand changes there. > > Considering "xrdb" is a regular X11 client, it requires a display, > hence it must have Xwayland running. > > If Xwayland is not running (i.e. xrdb is the first X11 client to be > started), then Xwayland will be started (as there is a "demand" from a > client, the client being xrdb) and other following X11 clients will > connect to the same display and have the resources set in xrdb > available. > > I remember Ray (halfline) advocating for Xwayland running xrdb itself > at startup to avoid this issue. But that's a bit of a special case for > specific X11 client (xrdb) which could be considered as legacy... > (like, people may not want to slow down Xwayland startup waiting for > xrdb to complete when they don't actually use any X11 application > benefiting from the X11 resources database) There: https://bugzilla.gnome.org/show_bug.cgi?id=769644 > Maybe a more general design of a script being run automatically by > Xwayland if present, so that users/distro-maintainers/sysadmins can > tailor that startup script at will? Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Xwayland on demand (Re: [PATCH xwayland 3/3] xwayland: Handle the case of windows being realized before redirection)
Hi On Wed, Jan 16, 2019 at 9:35 AM Pekka Paalanen wrote: > > On Tue, 15 Jan 2019 23:21:05 +0100 > Carlos Garnacho wrote: > > > If Xwayland gets to realize a window meant for composition before the > > compositor redirected windows (i.e. redirect mode is not RedirectDrawManual > > yet), the window would stay "invisible" as we wouldn't create a > > wl_surface/wl_shell_surface for it at any later point. > > > > This scenario may happen if the wayland compositor raises Xwayland on > > demand for a X11 client being started. In this case the first data on > > the socket is the client's, the compositor can hardly beat that in > > order to redirect subwindows before the client realizes a Window. > > > > In order to handle this case, allow the late creation of a matching > > (shell) surface for the WindowPtr on SetWindowPixmapProc, so it is ensured > > to be created after the compositor set up redirection. > > > > Signed-off-by: Carlos Garnacho > > --- > > hw/xwayland/xwayland.c | 37 + > > hw/xwayland/xwayland.h | 1 + > > 2 files changed, 38 insertions(+) > > Hi Carlos, > > this reminds me of other ordering issues with starting Xwayland on > demand: xrdb. I presume the X resources should be merged before any app > X11 client is processed, so that they see the user's resources at > start-up. AFAIK, that cannot happen if Xwayland is started on-demand. > Either it is too late if app start-up triggers it, or then you launch > Xwayland at desktop start-up, essentially not doing on-demand anymore. > Have you seen people complain about that? > > Could that be fixed somehow? > > Pass the X11 app socket to Xwayland later after the XWM init is > complete? Stall non-XWM clients inside Xwayland by default until XWM is > ready? I am not sure I understand what xwayland-on-demand changes there. Considering "xrdb" is a regular X11 client, it requires a display, hence it must have Xwayland running. If Xwayland is not running (i.e. xrdb is the first X11 client to be started), then Xwayland will be started (as there is a "demand" from a client, the client being xrdb) and other following X11 clients will connect to the same display and have the resources set in xrdb available. I remember Ray (halfline) advocating for Xwayland running xrdb itself at startup to avoid this issue. But that's a bit of a special case for specific X11 client (xrdb) which could be considered as legacy... (like, people may not want to slow down Xwayland startup waiting for xrdb to complete when they don't actually use any X11 application benefiting from the X11 resources database) Maybe a more general design of a script being run automatically by Xwayland if present, so that users/distro-maintainers/sysadmins can tailor that startup script at will? Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xmodmap really slow
On Mon, 3 Dec 2018 at 11:13, Alan Hourihane wrote: > No, didn't help I'm afraid. I'm not surprised, the patch probably helps but its contribution is negligible in regard to the huge delays being reported... I'd say, considering the delay, strace as indicated in my previous email would probably be your best bet. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xmodmap really slow
On Fri, Nov 30, 2018 at 2:38 PM Alan Hourihane wrote: > [...] > Many thanks for following and providing a patch. > > Might take me till Monday, but running > > xmodmap -pke > /tmp/keydump > xmodmap /tmp/keydump > > Where the last command invokes multi-second delays should be easy to test. Unfortunately, I don't see any such slow down here, so I cannot reproduce nor test if the change makes any difference. Yet, several seconds is weird... I mean, even if optimizing that loop in ilog2() would be nice, I can't imagine this being the source of such long delays... Can you reproduce with, say, Xephyr? If so maybe try running is strace : $ strace -Trttto strace.log Xephyr :12 & $ xmodmap -d :12 -pke > /tmp/keydump $ xmodmap -d :12 /tmp/keydump That would tell if the Xserver (Xephyr in this case) is spending too much time in a syscall, like for example reading a file or something? Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xmodmap really slow
On Fri, Nov 30, 2018 at 1:06 PM Olivier Fourdan wrote: > > On Fri, Nov 30, 2018 at 12:27 PM Alan Hourihane wrote: > > Running perf top while it's happening shows this > > > > 26.77% Xorg [.] ResourceClientBits > > 15.09% Xorg [.] GrabMatchesSecond > > 12.62% Xorg [.] DetailSupersedesSecond.isra.1 > > 12.55% Xorg [.] DeletePassiveGrabFromList > > 8.83% Xorg [.] GrabSupersedesSecond > > 5.55% Xorg [.] AddPassiveGrabToList > > 3.09% Xorg [.] DeletePassiveGrab > > 2.00% Xorg [.] xi2mask_merge > > 1.16% libc-2.27.so [.] _int_malloc > > 0.92% libc-2.27.so [.] _int_free > > 0.59% Xorg [.] XkbSendMap > > > > Xorg is totally frozen while this is happening. No rendering occurs at all. > > ResourceClientBits() has a loop, I wonder how many times it gets > called for one xmodmap request, but we could probably cache the > resulting value since `LimitClients` is not supposed to change once > it's set and clients are started, that would save us the loop. Please let me know if that patch makes any difference: https://patchwork.freedesktop.org/series/53315/ If it helps, I'll prepare a MR. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] dix: cache ResourceClientBits() value
That saves running the same loop over and over.
Signed-off-by: Olivier Fourdan
---
dix/resource.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/dix/resource.c b/dix/resource.c
index b6ef99f10..e39df7773 100644
--- a/dix/resource.c
+++ b/dix/resource.c
@@ -620,7 +620,12 @@ ilog2(int val)
unsigned int
ResourceClientBits(void)
{
-return (ilog2(LimitClients));
+static int cached = 0;
+
+if (cached == 0)
+ cached = ilog2(LimitClients);
+
+return cached;
}
/*
--
2.19.2
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: xmodmap really slow
On Fri, Nov 30, 2018 at 12:27 PM Alan Hourihane wrote: > Running perf top while it's happening shows this > > 26.77% Xorg [.] ResourceClientBits > 15.09% Xorg [.] GrabMatchesSecond > 12.62% Xorg [.] DetailSupersedesSecond.isra.1 > 12.55% Xorg [.] DeletePassiveGrabFromList > 8.83% Xorg [.] GrabSupersedesSecond > 5.55% Xorg [.] AddPassiveGrabToList > 3.09% Xorg [.] DeletePassiveGrab > 2.00% Xorg [.] xi2mask_merge > 1.16% libc-2.27.so [.] _int_malloc > 0.92% libc-2.27.so [.] _int_free > 0.59% Xorg [.] XkbSendMap > > Xorg is totally frozen while this is happening. No rendering occurs at all. ResourceClientBits() has a loop, I wonder how many times it gets called for one xmodmap request, but we could probably cache the resulting value since `LimitClients` is not supposed to change once it's set and clients are started, that would save us the loop. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 0/2] RFC: implement and use `present_event_abandon()`
Hi,
On Mon, Oct 8, 2018 at 4:46 PM Olivier Fourdan wrote:
> I reckon https://bugs.freedesktop.org/show_bug.cgi?id=108249 ("[xwayland]
> Crash in Xpresent code on resume from suspend") is caused by the present
> code using a RRCrtcPtr previously freed by Xwayland.
>
> Reason for this is because Xwayland's `xwl_output_remove()` will destroy
> the RRCrtcPtr for the Wayland outputs when removed, but if there is a
> flip pending, the `xwl_present_sync_callback()` will trigger after the
> CRTC is destroyed and not much good will come out of this.
>
> So I was tempted to use `present_event_abandon(RRCrtcPtr crtc)` which looked
> like a good candidate, until I realized that there was no actual
> implementation
> for that function declaration in present.h.
>
> So, the two following patches are my attempt at implementing such a
> `present_event_abandon()` function and make use of it in Xwayland.
>
> There are marked as "RFC" because the Present code is still pretty much
> alien territory for me...
If that makes it easier for reviews/comments, I posted the same as a WIP/MR:
https://gitlab.freedesktop.org/xorg/xserver/merge_requests/45
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/2] present: implement `present_event_abandon()`
The function `present_event_abandon()` is marked as exported in the
present.h header but it's nowhere to be found.
Such a function might prove handy for the DDX such as Xwayland who
creates and destroys CRTCs as new outputs are added or removed.
`present_event_abandon()` checks all vblank for all windows for the
given CRTC and abort those who match, so that we don't leave events
behind referring to a `RRCtrcPtr` pointing to freed memory.
Signed-off-by: Olivier Fourdan
Bugzilla: https://bugs.freedesktop.org/108249
---
present/present_screen.c | 32
1 file changed, 32 insertions(+)
diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..35a106ada 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -167,6 +167,38 @@ present_destroy_window(WindowPtr window)
return ret;
}
+static int
+check_vblank_crtc(WindowPtr window, void *value)
+{
+ScreenPtr screen = window->drawable.pScreen;
+present_window_priv_ptr window_priv = present_window_priv(window);
+present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+present_vblank_ptr vblank, tmp;
+RRCrtcPtr target_crtc = value;
+
+if (!window_priv)
+return WT_WALKCHILDREN;
+
+xorg_list_for_each_entry_safe(vblank, tmp, _priv->vblank,
window_list) {
+if (target_crtc == vblank->crtc) {
+screen_priv->abort_vblank(window->drawable.pScreen,
+ window, vblank->crtc,
+ vblank->event_id, vblank->target_msc);
+present_vblank_destroy(vblank);
+}
+}
+
+return WT_WALKCHILDREN;
+}
+
+void
+present_event_abandon(RRCrtcPtr crtc)
+{
+ScreenPtr pScreen = crtc->pScreen;
+
+WalkTree(pScreen, check_vblank_crtc, (void *) crtc);
+}
+
/*
* Hook the config notify screen function to deliver present config notify
events
*/
--
2.19.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 0/2] RFC: implement and use `present_event_abandon()`
Hi,
I reckon https://bugs.freedesktop.org/show_bug.cgi?id=108249 ("[xwayland]
Crash in Xpresent code on resume from suspend") is caused by the present
code using a RRCrtcPtr previously freed by Xwayland.
Reason for this is because Xwayland's `xwl_output_remove()` will destroy
the RRCrtcPtr for the Wayland outputs when removed, but if there is a
flip pending, the `xwl_present_sync_callback()` will trigger after the
CRTC is destroyed and not much good will come out of this.
So I was tempted to use `present_event_abandon(RRCrtcPtr crtc)` which looked
like a good candidate, until I realized that there was no actual implementation
for that function declaration in present.h.
So, the two following patches are my attempt at implementing such a
`present_event_abandon()` function and make use of it in Xwayland.
There are marked as "RFC" because the Present code is still pretty much
alien territory for me...
Cheers,
Olivier
Olivier Fourdan (2):
present: implement `present_event_abandon()`
xwayland: abandon present events when removing CRTC
hw/xwayland/xwayland-output.c | 3 ++-
present/present_screen.c | 32
2 files changed, 34 insertions(+), 1 deletion(-)
--
2.19.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/2] xwayland: abandon present events when removing CRTC
Xwayland will add and remove CRTCs as Wayland outputs are added or removed. If there is a pending flip when this occurs, the `xwl_present_sync_callback()` will be triggered after the Xwayland output's RRCtrcPtr has been destroyed, hence causing a crash in Xwayland while trying to use freed memory: #1 abort () #2 OsAbort () at utils.c:1350 #3 AbortServer () at log.c:877 #4 FatalError () at log.c:1015 #5 OsSigHandler () at osinit.c:156 #6 #7 dixGetPrivate () at ../include/privates.h:122 #8 dixLookupPrivate () at ../include/privates.h:166 #9 present_screen_priv () at present_priv.h:198 #10 present_wnmd_flip () at present_wnmd.c:358 #11 present_wnmd_execute () at present_wnmd.c:466 #12 present_wnmd_re_execute () at present_wnmd.c:80 #13 xwl_present_sync_callback () at xwayland-present.c:287 #14 ffi_call_unix64 () from /lib64/libffi.so.6 #15 ffi_call () from /lib64/libffi.so.6 #16 wl_closure_invoke () at src/connection.c:1006 #17 dispatch_event () at src/wayland-client.c:1427 #18 dispatch_queue () at src/wayland-client.c:1573 #19 wl_display_dispatch_queue_pending () at src/wayland-client.c:1815 #20 wl_display_dispatch_pending () at src/wayland-client.c:1878 #21 xwl_read_events () at xwayland.c:814 #22 ospoll_wait () at ospoll.c:651 #23 WaitForSomething () at WaitFor.c:208 #24 Dispatch () at ../include/list.h:220 #25 dix_main () at main.c:276 To avoid the issue, make sure we call `present_event_abandon()` on the CRTC we're about to destroy in `xwl_output_remove()`. Signed-off-by: Olivier Fourdan Bugzilla: https://bugs.freedesktop.org/108249 --- hw/xwayland/xwayland-output.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c index cc68f0340..50f27c624 100644 --- a/hw/xwayland/xwayland-output.c +++ b/hw/xwayland/xwayland-output.c @@ -29,6 +29,7 @@ #include "xwayland.h" #include +#include #define DEFAULT_DPI 96 #define ALL_ROTATIONS (RR_Rotate_0 | \ @@ -409,7 +410,7 @@ xwl_output_remove(struct xwl_output *xwl_output) xorg_list_for_each_entry(it, _screen->output_list, link) output_get_new_size(it, need_rotate, , ); update_screen_size(xwl_output, width, height); - +present_event_abandon (xwl_output->randr_crtc); RRCrtcDestroy(xwl_output->randr_crtc); RROutputDestroy(xwl_output->randr_output); -- 2.19.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: keep `xwl_present_timer_callback()` private
`xwl_present_timer_callback()` is initially marked a private and later implemented as public. Let's keep that private, shall we. Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland-present.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 316e04443..98692b3ca 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -210,7 +210,7 @@ xwl_present_events_notify(struct xwl_present_window *xwl_present_window) } } -CARD32 +static CARD32 xwl_present_timer_callback(OsTimerPtr timer, CARD32 time, void *arg) -- 2.19.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 1/2] present: cancel flip on reparenting
Hi Roman,
On Wed, Oct 3, 2018 at 6:57 PM Roman Gilg wrote:
> I'm a bit unsure on that one. I thought there is no cleanup code
> necessary in Present on a reparent because in theory the current
> Present code alone allows clients to flip arbitrary many child windows
> to a certain parent window as long as they have the same dimension as
> the parent. Of course a client trying to do flips on multiple child
> windows at the same time all with the same dimension as the parent can
> be considered somewhat weird and should not exist in practice. So if
> there is a "flipping" window being reparented to one with a child
> window doing flips it could be done from Present's side without any
> cleanup.
>
> But in the Xwayland case the driver should disallow the reparented
> child window doing flips, which then does a cleanup on the Present
> side for this particular child window.
I'm a bit lost, those two patches were my attempt to translate into
code what you wrote im your reply previously:
https://lists.x.org/archives/xorg-devel/2018-September/057581.html
Quoted below:
> I believe now the root problem is that the window did in fact some flips in
> the past, but on the reparent operation a new parent
> window with a new xwl_window struct is set, which then has a different
> present_window value. That means when reparenting
> of a child window with flips the xwl_window->present_window values must be
> updated on the old parent (to NULL) and on the
> new parent (to the new child window). Or more generic this must be done on
> any unmap/map operation.
That would have been patch 2/2 "xwayland: update Xwayland present
window on reparent"
> One extra case must be considered: if there is already a different child
> window doing flips on the new parent window the
> reparented child window must be instructed to stop its flips.
Whereas this would have been this patch 1/2 "present: cancel flip on
reparenting"
So do you mean we don't need this part and should drop this patch instead?
> Some notes below:
>
> On Thu, Sep 27, 2018 at 5:31 PM Olivier Fourdan wrote:
> > diff --git a/present/present_screen.c b/present/present_screen.c
> > index c7e37c5fd..f3f2ddef9 100644
> > --- a/present/present_screen.c
> > +++ b/present/present_screen.c
> > @@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
> > wrap(screen_priv, screen, ClipNotify, present_clip_notify);
> > }
> >
> > +static void
> > +present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
> > +{
> > +ScreenPtr screen = pWin->drawable.pScreen;
> > +present_screen_priv_ptr screen_priv = present_screen_priv(screen);
> > +
> > +if (screen_priv->reparent_window)
> > +screen_priv->reparent_window(pWin);
> > +
> > +unwrap(screen_priv, screen, ReparentWindow)
> > +if (screen->ReparentWindow)
> > +screen->ReparentWindow(pWin, pPriorParent);
> > +wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
> > +}
> > +
> > static Bool
> > present_screen_register_priv_keys(void)
> > {
> > @@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
> > wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
> > wrap(screen_priv, screen, ConfigNotify, present_config_notify);
> > wrap(screen_priv, screen, ClipNotify, present_clip_notify);
> > +wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
> >
> > dixSetPrivate(>devPrivates, _screen_private_key,
> > screen_priv);
> >
> > diff --git a/present/present_wnmd.c b/present/present_wnmd.c
> > index 8f3836440..0c49a3507 100644
> > --- a/present/present_wnmd.c
> > +++ b/present/present_wnmd.c
> > @@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
> > (*screen_priv->wnmd_info->flush) (window);
> > }
> >
> > +static void
> > +present_wnmd_reparent_window(WindowPtr pWin)
> > +{
> > +present_window_priv_ptr parent_window_priv;
> > +
> > +parent_window_priv = present_window_priv(pWin->parent);
>
> The present priv struct of a parent does not carry the information of
> a potentially presenting child. These are saved to the child's priv
> struct directly. So currently one would need to iterate all children
> and check if one of them is presenting.
>
> > +if (!parent_window_priv)
> > +return;
> > +
> > +/* If the new parent window already has a child flipping, cancel the
> > + * flip on the reparented window
> > + */
> > +if (parent_window_priv->flip_p
[PATCH xserver 1/2] present: cancel flip on reparenting
If a window is reparented and the new parent already has a child
flipping, we need to cancel the flipping on the reparented window.
Install a new ReparentWindow handler in present screen with hooks in
wnmd implementation to check if the new parent has flip pending or
active in which case we cancel flip on the reparented window.
Signed-off-by: Olivier Fourdan
---
present/present_priv.h | 3 +++
present/present_scmd.c | 1 +
present/present_screen.c | 16
present/present_wnmd.c | 17 +
4 files changed, 37 insertions(+)
diff --git a/present/present_priv.h b/present/present_priv.h
index f62456755..668322416 100644
--- a/present/present_priv.h
+++ b/present/present_priv.h
@@ -106,6 +106,7 @@ typedef Bool (*present_priv_check_flip_ptr)(RRCrtcPtr crtc,
PresentFlipReason *reason);
typedef void (*present_priv_check_flip_window_ptr)(WindowPtr window);
typedef Bool (*present_priv_can_window_flip_ptr)(WindowPtr window);
+typedef void (*present_priv_reparent_window_ptr)(WindowPtr pWin);
typedef int (*present_priv_pixmap_ptr)(WindowPtr window,
PixmapPtr pixmap,
@@ -147,6 +148,7 @@ struct present_screen_priv {
ConfigNotifyProcPtr ConfigNotify;
DestroyWindowProcPtrDestroyWindow;
ClipNotifyProcPtr ClipNotify;
+ReparentWindowProcPtr ReparentWindow;
present_vblank_ptr flip_pending;
uint64_tunflip_event_id;
@@ -171,6 +173,7 @@ struct present_screen_priv {
present_priv_check_flip_ptr check_flip;
present_priv_check_flip_window_ptr check_flip_window;
present_priv_can_window_flip_ptrcan_window_flip;
+present_priv_reparent_window_ptrreparent_window;
present_priv_pixmap_ptr present_pixmap;
present_priv_create_event_id_ptrcreate_event_id;
diff --git a/present/present_scmd.c b/present/present_scmd.c
index 0803a0c0b..a3ca16333 100644
--- a/present/present_scmd.c
+++ b/present/present_scmd.c
@@ -828,6 +828,7 @@ present_scmd_init_mode_hooks(present_screen_priv_ptr
screen_priv)
screen_priv->abort_vblank = _scmd_abort_vblank;
screen_priv->flip_destroy = _scmd_flip_destroy;
+screen_priv->reparent_window= NULL;
}
Bool
diff --git a/present/present_screen.c b/present/present_screen.c
index c7e37c5fd..f3f2ddef9 100644
--- a/present/present_screen.c
+++ b/present/present_screen.c
@@ -207,6 +207,21 @@ present_clip_notify(WindowPtr window, int dx, int dy)
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
}
+static void
+present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ScreenPtr screen = pWin->drawable.pScreen;
+present_screen_priv_ptr screen_priv = present_screen_priv(screen);
+
+if (screen_priv->reparent_window)
+screen_priv->reparent_window(pWin);
+
+unwrap(screen_priv, screen, ReparentWindow)
+if (screen->ReparentWindow)
+screen->ReparentWindow(pWin, pPriorParent);
+wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
+}
+
static Bool
present_screen_register_priv_keys(void)
{
@@ -232,6 +247,7 @@ present_screen_priv_init(ScreenPtr screen)
wrap(screen_priv, screen, DestroyWindow, present_destroy_window);
wrap(screen_priv, screen, ConfigNotify, present_config_notify);
wrap(screen_priv, screen, ClipNotify, present_clip_notify);
+wrap(screen_priv, screen, ReparentWindow, present_reparent_window);
dixSetPrivate(>devPrivates, _screen_private_key,
screen_priv);
diff --git a/present/present_wnmd.c b/present/present_wnmd.c
index 8f3836440..0c49a3507 100644
--- a/present/present_wnmd.c
+++ b/present/present_wnmd.c
@@ -682,6 +682,22 @@ present_wnmd_flush(WindowPtr window)
(*screen_priv->wnmd_info->flush) (window);
}
+static void
+present_wnmd_reparent_window(WindowPtr pWin)
+{
+present_window_priv_ptr parent_window_priv;
+
+parent_window_priv = present_window_priv(pWin->parent);
+if (!parent_window_priv)
+return;
+
+/* If the new parent window already has a child flipping, cancel the
+ * flip on the reparented window
+ */
+if (parent_window_priv->flip_pending || parent_window_priv->flip_active)
+present_wnmd_cancel_flip(pWin);
+}
+
void
present_wnmd_init_mode_hooks(present_screen_priv_ptr screen_priv)
{
@@ -700,4 +716,5 @@ present_wnmd_init_mode_hooks(present_screen_priv_ptr
screen_priv)
screen_priv->abort_vblank = _wnmd_abort_vblank;
screen_priv->flip_destroy = _wnmd_flip_destroy;
+screen_priv->reparent_window= _wnmd_reparent_window;
}
--
2.19.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/2] xwayland: update Xwayland present window on reparent
When reparenting a window, the Xwayland present window either on the
prior parent or on the new parent may need to be updated:
1. If the window is flipping, clear the present window on its old
parent.
2. If the new parent has no flipping already, set its flipping window to
the reparented window.
Install new ReparentWindow hooks in Xwayland to handle those cases.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-present.c | 16
hw/xwayland/xwayland.c | 23 +++
hw/xwayland/xwayland.h | 2 ++
3 files changed, 41 insertions(+)
diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 316e04443..f54fe2ac4 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -558,3 +558,19 @@ xwl_present_init(ScreenPtr screen)
return present_wnmd_screen_init(screen, _present_info);
}
+
+void
+xwl_present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+struct xwl_window *xwl_window;
+
+/* If the window is flipping, clear the present window on its old parent */
+xwl_window = xwl_window_from_window(pPriorParent);
+if (xwl_present_is_flipping(pWin, xwl_window))
+xwl_window->present_window = NULL;
+
+/* If the new parent window has no child flipping, update its
present_window */
+xwl_window = xwl_window_from_window(pWin->parent);
+if (xwl_window && xwl_window->present_window == NULL)
+xwl_window->present_window = pWin;
+}
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 96b4db18c..345b62f59 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -660,6 +660,26 @@ xwl_destroy_window(WindowPtr window)
return ret;
}
+static void
+xwl_reparent_window(WindowPtr pWin, WindowPtr pPriorParent)
+{
+ScreenPtr screen = pWin->drawable.pScreen;
+struct xwl_screen *xwl_screen = xwl_screen_get(screen);
+
+#ifdef GLAMOR_HAS_GBM
+if (xwl_screen->present)
+xwl_present_reparent_window(pWin, pPriorParent);
+#endif
+
+/* Chain up with previous ReparentWindow, if any */
+if (xwl_screen->ReparentWindow) {
+screen->ReparentWindow = xwl_screen->ReparentWindow;
+(*screen->ReparentWindow) (pWin, pPriorParent);
+xwl_screen->ReparentWindow = screen->ReparentWindow;
+screen->ReparentWindow = xwl_reparent_window;
+}
+}
+
static void
xwl_window_post_damage(struct xwl_window *xwl_window)
{
@@ -1109,6 +1129,9 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
xwl_screen->CloseScreen = pScreen->CloseScreen;
pScreen->CloseScreen = xwl_close_screen;
+xwl_screen->ReparentWindow = pScreen->ReparentWindow;
+pScreen->ReparentWindow = xwl_reparent_window;
+
pScreen->CursorWarpedTo = xwl_cursor_warped_to;
pScreen->CursorConfinedTo = xwl_cursor_confined_to;
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 67819e178..a7c7f2aee 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -132,6 +132,7 @@ struct xwl_screen {
RealizeWindowProcPtr RealizeWindow;
UnrealizeWindowProcPtr UnrealizeWindow;
DestroyWindowProcPtr DestroyWindow;
+ReparentWindowProcPtr ReparentWindow;
XYToWindowProcPtr XYToWindow;
struct xorg_list output_list;
@@ -451,6 +452,7 @@ void xwl_glamor_egl_make_current(struct xwl_screen
*xwl_screen);
#ifdef GLAMOR_HAS_GBM
Bool xwl_present_init(ScreenPtr screen);
void xwl_present_cleanup(WindowPtr window);
+void xwl_present_reparent_window(WindowPtr pWin, WindowPtr pPriorParent);
#endif /* GLAMOR_HAS_GBM */
#ifdef XV
--
2.19.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 0/2] RE: xwayland: Avoid assert failure in flips_stop()
Hi Roman, On Fri, Sep 21, 2018 at 11:53 AM Roman Gilg wrote: > > Great detailed analysis in the backtrace! :) > > What confused me at first was that the present_wnmd_flips_stop function > is called at all in this state because it should only be called when at > least one flip has been done and in this case xwl_window->present_window > must have been set to the presenting (child) window. > > I believe now the root problem is that the window did in fact some flips > in the past, but on the reparent operation a new parent window with a > new xwl_window struct is set, which then has a different present_window > value. That means when reparenting of a child window with flips the > xwl_window->present_window values must be updated on the old parent > (to NULL) and on the new parent (to the new child window). Or more > generic this must be done on any unmap/map operation. > > One extra case must be considered: if there is already a different > child window doing flips on the new parent window the reparented child > window must be instructed to stop its flips. So the following two patches are my attempt at implementing your suggestion, but I am not familiar with Present so this is a bit of a shot in the dark for me, also because that issue is quite difficult to reproduce. Those patches have been barely tested, expect dragons! Anyway, please let me know if that's what you had in mind... Cheers, Olivier Olivier Fourdan (2): present: cancel flip on reparenting xwayland: update Xwayland present window on reparent hw/xwayland/xwayland-present.c | 16 hw/xwayland/xwayland.c | 23 +++ hw/xwayland/xwayland.h | 2 ++ present/present_priv.h | 3 +++ present/present_scmd.c | 1 + present/present_screen.c | 16 present/present_wnmd.c | 17 + 7 files changed, 78 insertions(+) -- 2.19.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Use `double` for `xwl_tablet_tool`
So we do not lose subpixel precision in Xwayland.
Suggested-by: Peter Hutterer
Signed-off-by: Olivier Fourdan
Tested-by: Jean-Loup Tastet
Closes: https://gitlab.freedesktop.org/libinput/libinput/issues/138
---
MR: https://gitlab.freedesktop.org/xorg/xserver/merge_requests/21
hw/xwayland/xwayland-input.c | 20 ++--
hw/xwayland/xwayland.h | 12 ++--
2 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 6fd3c416b..7f08b36e2 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -1604,8 +1604,8 @@ tablet_tool_motion(void *data, struct zwp_tablet_tool_v2
*tool,
struct xwl_tablet_tool *xwl_tablet_tool = data;
struct xwl_seat *xwl_seat = xwl_tablet_tool->seat;
int32_t dx, dy;
-int sx = wl_fixed_to_int(x);
-int sy = wl_fixed_to_int(y);
+double sx = wl_fixed_to_double(x);
+double sy = wl_fixed_to_double(y);
if (!xwl_seat->tablet_focus_window)
return;
@@ -1613,8 +1613,8 @@ tablet_tool_motion(void *data, struct zwp_tablet_tool_v2
*tool,
dx = xwl_seat->tablet_focus_window->window->drawable.x;
dy = xwl_seat->tablet_focus_window->window->drawable.y;
-xwl_tablet_tool->x = dx + sx;
-xwl_tablet_tool->y = dy + sy;
+xwl_tablet_tool->x = (double) dx + sx;
+xwl_tablet_tool->y = (double) dy + sy;
}
static void
@@ -1772,15 +1772,15 @@ tablet_tool_frame(void *data, struct zwp_tablet_tool_v2
*tool, uint32_t time)
int button;
valuator_mask_zero();
-valuator_mask_set(, 0, xwl_tablet_tool->x);
-valuator_mask_set(, 1, xwl_tablet_tool->y);
+valuator_mask_set_double(, 0, xwl_tablet_tool->x);
+valuator_mask_set_double(, 1, xwl_tablet_tool->y);
valuator_mask_set(, 2, xwl_tablet_tool->pressure);
-valuator_mask_set(, 3, xwl_tablet_tool->tilt_x);
-valuator_mask_set(, 4, xwl_tablet_tool->tilt_y);
-valuator_mask_set(, 5, xwl_tablet_tool->rotation +
xwl_tablet_tool->slider);
+valuator_mask_set_double(, 3, xwl_tablet_tool->tilt_x);
+valuator_mask_set_double(, 4, xwl_tablet_tool->tilt_y);
+valuator_mask_set_double(, 5, xwl_tablet_tool->rotation +
xwl_tablet_tool->slider);
QueuePointerEvents(xwl_tablet_tool->xdevice, MotionNotify, 0,
- POINTER_ABSOLUTE | POINTER_SCREEN, );
+ POINTER_ABSOLUTE | POINTER_DESKTOP, );
valuator_mask_zero();
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 1a6e2f380..67819e178 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -311,13 +311,13 @@ struct xwl_tablet_tool {
DeviceIntPtr xdevice;
uint32_t proximity_in_serial;
-uint32_t x;
-uint32_t y;
+double x;
+double y;
uint32_t pressure;
-float tilt_x;
-float tilt_y;
-float rotation;
-float slider;
+double tilt_x;
+double tilt_y;
+double rotation;
+double slider;
uint32_t buttons_now,
buttons_prev;
--
2.19.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[RFC PATCH xserver] xwayland: Avoid assert failure in flips_stop()
On `ClipNotify()`, `present_clip_notify()` will possibly end up issuing a `flips_stop()` if `check_flip()` returns `FALSE`. `present_wnmd_check_flip()` however can return `FALSE` in a variety of cases, before eventually checking with the driver's `check_flip2()` which in the case of `xwl_present_check_flip2()` makes sure that `xwl_window->present_window` is not `NULL`. Hence, if one of the preliminary conditions is not satisfied in `present_wnmd_check_flip()`, we may end up calling Xwayland's `xwl_present_flips_stop()` even though `xwl_window->present_window` is 'NULL', which will trigger an assertion failure and consequently a crash of Xwayland. A backtrace of such a case looks like: #0 __GI_raise (sig=sig@entry=6) #1 __GI_abort () at abort.c:79 #2 OsAbort () at utils.c:1350 #3 AbortServer () at log.c:877 #4 FatalError () at log.c:1015 #5 OsSigHandler () at osinit.c:156 #6 #7 __GI_raise (sig=sig@entry=6) #8 __GI_abort () at abort.c:79 #9 __assert_fail_base () at assert.c:92 #10 __GI___assert_fail () at assert.c:101 #11 xwl_present_flips_stop () at xwayland-present.c:521 #12 present_wnmd_flips_stop () at present_wnmd.c:159 #13 present_wnmd_check_flip_window () at present_wnmd.c:332 #14 present_clip_notify () at present_screen.c:203 #15 compClipNotify () at compwindow.c:317 #16 miComputeClips () at mivaltree.c:478 #17 miValidateTree () at mivaltree.c:681 #18 MapWindow () at window.c:2699 #19 ReparentWindow () at window.c:2600 #20 ProcReparentWindow () at dispatch.c:829 #21 Dispatch () at dispatch.c:478 #22 dix_main () at main.c:276 #23 __libc_start_main () at ../csu/libc-start.c:308 #24 _start () In this case, a forensic examination of the core file showed that `present_wnmd_check_flip()` returned `FALSE` because `window->redirectDraw` was `RedirectDrawManual` and not the expected `RedirectDrawNone`. Signed-off-by: Olivier Fourdan --- See: https://lists.x.org/archives/xorg-devel/2018-September/057566.html hw/xwayland/xwayland-present.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 316e04443..f77dc4d15 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -518,6 +518,9 @@ xwl_present_flips_stop(WindowPtr window) if (!xwl_window) return; +if (xwl_window->present_window == NULL) +return; + assert(xwl_window->present_window == window); xwl_window->present_window = NULL; -- 2.19.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2] present: fix freed pointer access
() retHi,
On Fri, Sep 7, 2018 at 1:16 PM Olivier Fourdan wrote:
>
> > > On 04/09/2018 16:27, Roman Gilg wrote:
> > >
> > > Ok, I just got a failing assert in xwl_present_flips_stop with the patch
> > > when opening a context menu in Steam.
> > > Seems the xwl_present_flips_stop call is coming in too late now after the
> > > presenting window has already been
> > > changed.
>
> I fail to understand how that can be related to Lionel's patch though.
>
> The patch simply moves `present_vblank_notify()` before
> `present_wnmd_flips_stop()` in `present_wnmd_flip_notify()` whereas
> the assertion failure comes from an eniterly different code path, down
> from `present_clip_notify()` which calls
> `present_wnmd_check_flip_window()` which does:
> [...]
I've hit that assertion in xwl_present_flips_stop() as well, but the
backtrace is slightly different, it happened during a
XReparentWindow().
In order to make that email short enough, I've attached how I ended up
to the conclusion that there is a perfectly valid code path that can
lead to xwl_present_flips_stop() with xwl_window->present_window being
NULL and that will cause a crash.
Cheers,
Olivier
(gdb) bt
#0 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1 0x7f2182f24895 in __GI_abort () at abort.c:79
#2 0x005943f0 in OsAbort () at utils.c:1350
#3 0x00599689 in AbortServer () at log.c:877
#4 0x0059a4fd in FatalError (
f=f@entry=0x5c0770 "Caught signal %d (%s). Server aborting\n")
at log.c:1015
#5 0x005916f5 in OsSigHandler (signo=6, sip=,
unused=) at osinit.c:156
#6
#7 __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#8 0x7f2182f24895 in __GI_abort () at abort.c:79
#9 0x7f2182f24769 in __assert_fail_base (
fmt=0x7f218308bea8 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=0x5a6c18 "xwl_window->present_window == window",
file=0x5a6c03 "xwayland-present.c", line=521, function=)
at assert.c:92
#10 0x7f2182f329f6 in __GI___assert_fail (
assertion=assertion@entry=0x5a6c18 "xwl_window->present_window == window",
file=file@entry=0x5a6c03 "xwayland-present.c", line=line@entry=521,
function=function@entry=0x5a6c40 <__PRETTY_FUNCTION__.25116>
"xwl_present_flips_stop") at assert.c:101
#11 0x004395d5 in xwl_present_flips_stop (window=0x21be120)
at xwayland-present.c:521
#12 0x004f8be8 in present_wnmd_flips_stop (window=)
at present_wnmd.c:159
#13 0x004f8e55 in present_wnmd_check_flip_window (window=0x21be120)
at present_wnmd.c:332
#14 0x004f73f3 in present_clip_notify (window=0x21be120, dx=0, dy=0)
at present_screen.c:203
#15 0x00547d4f in compClipNotify (pWin=0x21be120, dx=0, dy=0)
at compwindow.c:317
#16 0x00480b2b in miComputeClips (pParent=pParent@entry=0x21be120,
pScreen=pScreen@entry=0xbbbf00, universe=universe@entry=0x7ffd04c4d1d0,
kind=kind@entry=VTMap, exposed=exposed@entry=0x7ffd04c4d1f0)
at mivaltree.c:478
#17 0x00481057 in miValidateTree (pParent=0x12c5ed0, pChild=0x21be120,
kind=) at mivaltree.c:681
#18 0x0058778a in MapWindow (pWin=0x21be120, client=0x1629a30)
at window.c:2699
#19 0x0058ab2c in ReparentWindow (pWin=0x21be120, pParent=0x12c5ed0,
x=, y=, client=client@entry=0x1629a30)
at window.c:2600
#20 0x009c in ProcReparentWindow (client=0x1629a30)
at dispatch.c:829
#21 0x0055b78e in Dispatch () at dispatch.c:478
#22 0x0055f7d6 in dix_main (argc=12, argv=0x7ffd04c4d4f8,
envp=) at main.c:276
#23 0x7f2182f26413 in __libc_start_main (main=0x42e300 , argc=12,
argv=0x7ffd04c4d4f8, init=, fini=,
rtld_fini=, stack_end=0x7ffd04c4d4e8)
at ../csu/libc-start.c:308
#24 0x0042e33e in _start ()
Unfortunately, the `xwl_window` has been optimized out, so I can't get to the
value of `xwl_window->present_window` in `xwl_present_flips_stop()`:
(gdb) f 11
#11 0x004395d5 in xwl_present_flips_stop (window=0x21be120)
at xwayland-present.c:521
521 assert(xwl_window->present_window == window);
(gdb) p xwl_window->present_window
value has been optimized out
Fortunately, we can recompute its value:
512 static void
513 xwl_present_flips_stop(WindowPtr window)
514 {
515 struct xwl_window *xwl_window = xwl_window_from_window(window);
516 struct xwl_present_window *xwl_present_window =
xwl_present_window_priv(window);
With xwl_window_from_window() being:
259 xwl_window_from_window(WindowPtr window)
260 {
261 struct xwl_window *xwl_window;
262
263 while (window) {
264 xwl_window = xwl_window_get(window);
265 if (xwl_window)
266 return xwl_window;
267
268 window = window->parent;
Re: [PATCH xserver 0/3] Xv: add NV12 support in glamor
Hi Julien, On Fri, 7 Sep 2018 at 00:40, Julien Isorce wrote: > > Some video decoders can only output NV12 and currently glamor Xv only > supports I420 and YV12. > > Tested with xf86-video-ati, xf86-video-amdgpu and xf86-video-modesetting > on AMD graphics but should work on any setup that can use glamor. > > Test: gst-launch-1.0 videotestsrc ! video/x-raw, format=NV12 ! xvimagesink > > Julien Isorce (3): > xfree86: define FOURCC_NV12 and XVIMAGE_NV12 > glamor: add support for GL_RG > glamor: add support for NV12 in Xv > > glamor/glamor.c| 2 + > glamor/glamor.h| 1 + > glamor/glamor_priv.h | 4 +- > glamor/glamor_transfer.c | 10 ++- > glamor/glamor_utils.h | 4 + > glamor/glamor_xv.c | 180 > ++--- > hw/xfree86/common/fourcc.h | 20 + > 7 files changed, 193 insertions(+), 28 deletions(-) > > -- > 2.7.4 > Nice! Tested with Xwayland using gstreamer, so: Tested-by: Olivier Fourdan Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2] present: fix freed pointer access
Hi,
On Wed, Sep 5, 2018 at 1:13 PM Olivier Fourdan wrote:
> Hi,
> On Tue, Sep 4, 2018 at 6:28 PM Lionel Landwerlin
> wrote:
> >
> > Oh well...
> > I'm sure you'll be able to fix it faster than me :)
> >
> > -
> > Lionel
> >
> >
> [...]
>
> If I read bug 107314 correctly, the crash occurs after the window has
> been destroyed, so what about that other patch:
>
> https://patchwork.freedesktop.org/patch/247271/
>
> ** plus ** this patch below (just copied for testing purpose), does it
> fix your crash?
Sorry, that's unrelated, please ignore my last post...
So I agree with Lionel's v2 patch
(https://patchwork.freedesktop.org/patch/246007/) to fix the issue
with the use-after-free.
> > On 04/09/2018 16:27, Roman Gilg wrote:
> >
> > Ok, I just got a failing assert in xwl_present_flips_stop with the patch
> > when opening a context menu in Steam.
> > Seems the xwl_present_flips_stop call is coming in too late now after the
> > presenting window has already been
> > changed.
I fail to understand how that can be related to Lionel's patch though.
The patch simply moves `present_vblank_notify()` before
`present_wnmd_flips_stop()` in `present_wnmd_flip_notify()` whereas
the assertion failure comes from an eniterly different code path, down
from `present_clip_notify()` which calls
`present_wnmd_check_flip_window()` which does:
```
325 if (flip_pending) {
326 if (!present_wnmd_check_flip(flip_pending->crtc,
flip_pending->window, flip_pending->pixmap,
327 flip_pending->sync_flip, NULL, 0, 0, NULL))
328 present_wnmd_set_abort_flip(window);
329 } else if (flip_active) {
330 if (!present_wnmd_check_flip(flip_active->crtc,
flip_active->window, flip_active->pixmap,
331 flip_active->sync_flip, NULL,
0, 0, NULL))
332 present_wnmd_flips_stop(window);
333 }
```
`present_wnmd_check_flip()` calls `xwl_present_check_flip2()` which does:
```
424 * Do not flip if there is already another child window doing flips.
425 */
426 if (xwl_window->present_window &&
427 xwl_window->present_window != present_window)
428 return FALSE;
429
```
So here we end up with `assert(xwl_window->present_window == window)`
in `xwl_present_flips_stop()` when `present_wnmd_check_flip()` returns
FALSE, i.e. when `xwl_window->present_window != present_window`.
I'm confused :/
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] glx: check for indirect context in CreateContextAttribsARB()
Commit 99f0365b "Add a command line argument for disabling indirect GLX"
added a test to check if indirect context are enabled in
`DoCreateContext()` but `__glXDisp_CreateContextAttribsARB()` doesn't
use `DoCreateContext()` and doesn't check if indirect context is
enabled.
As a result, clients can still manage to create indirect contexts using
`glXCreateContextAttribsARB()` even if indirect contexts are disabled,
which can possibly crash Xservers such as Xwayland or Xephyr when the
context is destroyed.
To avoid the issue, check for `enableIndirectGLX` in
`__glXDisp_CreateContextAttribsARB()` as well.
Fixes: 99f0365b "Add a command line argument for disabling indirect GLX"
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107508
Signed-off-by: Olivier Fourdan
---
glx/createcontext.c | 12
1 file changed, 12 insertions(+)
diff --git a/glx/createcontext.c b/glx/createcontext.c
index 7d09c3a1c..24b02ddfb 100644
--- a/glx/createcontext.c
+++ b/glx/createcontext.c
@@ -28,6 +28,7 @@
#include "glxserver.h"
#include "glxext.h"
#include "indirect_dispatch.h"
+#include "opaque.h"
#define ALL_VALID_FLAGS \
(GLX_CONTEXT_DEBUG_BIT_ARB | GLX_CONTEXT_FORWARD_COMPATIBLE_BIT_ARB \
@@ -320,6 +321,17 @@ __glXDisp_CreateContextAttribsARB(__GLXclientState * cl,
GLbyte * pc)
err = BadAlloc;
}
else {
+/* Only allow creating indirect GLX contexts if allowed by
+ * server command line. Indirect GLX is of limited use (since
+ * it's only GL 1.4), it's slower than direct contexts, and
+ * it's a massive attack surface for buffer overflow type
+ * errors.
+ */
+if (!enableIndirectGLX) {
+client->errorValue = req->isDirect;
+return BadValue;
+}
+
ctx = glxScreen->createContext(glxScreen, config, shareCtx,
req->numAttribs, (uint32_t *) attribs,
);
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH v2] present: fix freed pointer access
Hi,
On Tue, Sep 4, 2018 at 6:28 PM Lionel Landwerlin
wrote:
>
> Oh well...
> I'm sure you'll be able to fix it faster than me :)
>
> -
> Lionel
>
> On 04/09/2018 16:27, Roman Gilg wrote:
>
> Ok, I just got a failing assert in xwl_present_flips_stop with the patch when
> opening a context menu in Steam. Seems the xwl_present_flips_stop call is
> coming in too late now after the presenting window has already been changed.
>
[...]
If I read bug 107314 correctly, the crash occurs after the window has
been destroyed, so what about that other patch:
https://patchwork.freedesktop.org/patch/247271/
** plus ** this patch below (just copied for testing purpose), does it
fix th your crash?
From 676597fcd6ee907f4d3f165dd0b5de746f7c8131 Mon Sep 17 00:00:00 2001
From: Olivier Fourdan
Date: Wed, 5 Sep 2018 13:08:03 +0200
Subject: [PATCH xserver] xwayland: ignore sync callback if window is destroyed
If the window is destroyed, there is no need to send the vblank notify
event.
This should avoid a crash in present_vblank_notify()
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107314
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-present.c | 4
1 file changed, 4 insertions(+)
diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 316e04443..b1751c846 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -276,6 +276,10 @@ xwl_present_sync_callback(void *data,
event->pending = FALSE;
+/* Is the window destroyed already ? */
+if (!xwl_present_window)
+return;
+
if (event->abort) {
/* Event might have been aborted */
if (event->buffer_released)
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v3] xwayland: Remove xwl_present_window from privates on cleanup
Hi, On Wed, Sep 5, 2018 at 12:05 PM Lionel Landwerlin wrote: > > On 05/09/2018 09:49, Olivier Fourdan wrote: > > [...] > > This is because `xwl_present_cleanup()` frees the memory but does not > > remove it from the window's privates, and `xwl_present_abort_vblank()` > > will still find it and hence try to access that freed memory... > > > > Remove `xwl_present_window` from window's privates on cleanup so that no > > other function can find and reuse that data once it's freed. > > > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269 > > > We also have this fdo bug : > https://bugs.freedesktop.org/show_bug.cgi?id=107314 Yeap, I know about this bug and the patch you sent, but the backtrace is different, it doesn't occur on DestroyWindow(), so I doubt this is the same (unless you tried my fix and it fixes your issue as well...) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v3] xwayland: Remove xwl_present_window from privates on cleanup
Xwayland's `xwl_destroy_window()` invokes `xwl_present_cleanup()` before the common `DestroyWindow()`. But then `DestroyWindow()` calls `present_destroy_window()` which will possibly end up in `xwl_present_abort_vblank()` which will try to access data that was previously freed by `xwl_present_cleanup()`: Invalid read of size 8 at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378) by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651) by 0x53695A: present_free_window_vblank (present_screen.c:87) by 0x53695A: present_destroy_window (present_screen.c:152) by 0x42A90D: xwl_destroy_window (xwayland.c:653) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) Address 0x182abde0 is 80 bytes inside a block of size 112 free'd at 0x4C2FDAC: free (vg_replace_malloc.c:530) by 0x42A937: xwl_destroy_window (xwayland.c:647) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) by 0x446DA5: ProcKillClient (dispatch.c:3279) by 0x4476AF: Dispatch (dispatch.c:479) Block was alloc'd at at 0x4C30B06: calloc (vg_replace_malloc.c:711) by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54) by 0x434228: xwl_present_get_crtc (xwayland-present.c:302) by 0x539728: proc_present_query_capabilities (present_request.c:227) by 0x4476AF: Dispatch (dispatch.c:479) by 0x44B5B5: dix_main (main.c:276) by 0x75F611A: (below main) (libc-start.c:308) This is because `xwl_present_cleanup()` frees the memory but does not remove it from the window's privates, and `xwl_present_abort_vblank()` will still find it and hence try to access that freed memory... Remove `xwl_present_window` from window's privates on cleanup so that no other function can find and reuse that data once it's freed. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269 Signed-off-by: Olivier Fourdan --- v2: Remove leftovers from broken initial patch... v3: Rephrase non-sensical explanation of the fix hw/xwayland/xwayland-present.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 81e0eb9ce..316e04443 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -147,6 +147,11 @@ xwl_present_cleanup(WindowPtr window) /* Clear timer */ xwl_present_free_timer(xwl_present_window); +/* Remove from privates so we don't try to access it later */ +dixSetPrivate(>devPrivates, + _present_window_private_key, + NULL); + free(xwl_present_window); } -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver v2] xwayland: Remove xwl_present_window from privates on cleanup
Xwayland's `xwl_destroy_window()` invokes `xwl_present_cleanup()` before the common `DestroyWindow()`. But then `DestroyWindow()` calls `present_destroy_window()` which will possibly end up in `xwl_present_abort_vblank()` which will try to access data that was previously freed by `xwl_present_cleanup()`: Invalid read of size 8 at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378) by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651) by 0x53695A: present_free_window_vblank (present_screen.c:87) by 0x53695A: present_destroy_window (present_screen.c:152) by 0x42A90D: xwl_destroy_window (xwayland.c:653) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) Address 0x182abde0 is 80 bytes inside a block of size 112 free'd at 0x4C2FDAC: free (vg_replace_malloc.c:530) by 0x42A937: xwl_destroy_window (xwayland.c:647) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) by 0x446DA5: ProcKillClient (dispatch.c:3279) by 0x4476AF: Dispatch (dispatch.c:479) Block was alloc'd at at 0x4C30B06: calloc (vg_replace_malloc.c:711) by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54) by 0x434228: xwl_present_get_crtc (xwayland-present.c:302) by 0x539728: proc_present_query_capabilities (present_request.c:227) by 0x4476AF: Dispatch (dispatch.c:479) by 0x44B5B5: dix_main (main.c:276) by 0x75F611A: (below main) (libc-start.c:308) This is because `xwl_present_cleanup()` frees the memory but does not remove it from the window's privates, and `xwl_present_abort_vblank()` will still find it and hence try to access that freed memory... Clear `xwl_present_cleanup()` after `DestroyWindow()` so that `xwl_present_abort_vblank()` can still access valid memory before it's freed. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269 Signed-off-by: Olivier Fourdan --- v2: Remove leftovers from broken initial patch... hw/xwayland/xwayland-present.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 81e0eb9ce..316e04443 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -147,6 +147,11 @@ xwl_present_cleanup(WindowPtr window) /* Clear timer */ xwl_present_free_timer(xwl_present_window); +/* Remove from privates so we don't try to access it later */ +dixSetPrivate(>devPrivates, + _present_window_private_key, + NULL); + free(xwl_present_window); } -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Remove xwl_present_window from privates on cleanup
Xwayland's `xwl_destroy_window()` invokes `xwl_present_cleanup()` before the common `DestroyWindow()`. But then `DestroyWindow()` calls `present_destroy_window()` which will possibly end up in `xwl_present_abort_vblank()` which will try to access data that was previously freed by `xwl_present_cleanup()`: Invalid read of size 8 at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378) by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651) by 0x53695A: present_free_window_vblank (present_screen.c:87) by 0x53695A: present_destroy_window (present_screen.c:152) by 0x42A90D: xwl_destroy_window (xwayland.c:653) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) Address 0x182abde0 is 80 bytes inside a block of size 112 free'd at 0x4C2FDAC: free (vg_replace_malloc.c:530) by 0x42A937: xwl_destroy_window (xwayland.c:647) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) by 0x446DA5: ProcKillClient (dispatch.c:3279) by 0x4476AF: Dispatch (dispatch.c:479) Block was alloc'd at at 0x4C30B06: calloc (vg_replace_malloc.c:711) by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54) by 0x434228: xwl_present_get_crtc (xwayland-present.c:302) by 0x539728: proc_present_query_capabilities (present_request.c:227) by 0x4476AF: Dispatch (dispatch.c:479) by 0x44B5B5: dix_main (main.c:276) by 0x75F611A: (below main) (libc-start.c:308) This is because `xwl_present_cleanup()` frees the memory but does not remove it from the window's privates, and `xwl_present_abort_vblank()` will still find it and hence try to access that freed memory... Clear `xwl_present_cleanup()` after `DestroyWindow()` so that `xwl_present_abort_vblank()` can still access valid memory before it's freed. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269 Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland-present.c | 5 + hw/xwayland/xwayland.c | 10 +- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 81e0eb9ce..316e04443 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -147,6 +147,11 @@ xwl_present_cleanup(WindowPtr window) /* Clear timer */ xwl_present_free_timer(xwl_present_window); +/* Remove from privates so we don't try to access it later */ +dixSetPrivate(>devPrivates, + _present_window_private_key, + NULL); + free(xwl_present_window); } diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c index 96b4db18c..63d69fb3a 100644 --- a/hw/xwayland/xwayland.c +++ b/hw/xwayland/xwayland.c @@ -642,11 +642,6 @@ xwl_destroy_window(WindowPtr window) struct xwl_screen *xwl_screen = xwl_screen_get(screen); Bool ret; -#ifdef GLAMOR_HAS_GBM -if (xwl_screen->present) -xwl_present_cleanup(window); -#endif - screen->DestroyWindow = xwl_screen->DestroyWindow; if (screen->DestroyWindow) @@ -657,6 +652,11 @@ xwl_destroy_window(WindowPtr window) xwl_screen->DestroyWindow = screen->DestroyWindow; screen->DestroyWindow = xwl_destroy_window; +#ifdef GLAMOR_HAS_GBM +if (xwl_screen->present) +xwl_present_cleanup(window); +#endif + return ret; } -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: Clean-up present after destroying common bits
On Wed, Sep 5, 2018 at 9:48 AM Olivier Fourdan wrote: > > Right now, `xwl_destroy_window()` invokes `xwl_present_cleanup()` before > the common `DestroyWindow()`. > > But `DestroyWindow()` calls in `present_destroy_window()` which will > then end up in `xwl_present_abort_vblank()` which will try to access > data that was previously freed by `xwl_present_cleanup()`: > > Invalid read of size 8 > at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378) > by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651) > by 0x53695A: present_free_window_vblank (present_screen.c:87) > by 0x53695A: present_destroy_window (present_screen.c:152) > by 0x42A90D: xwl_destroy_window (xwayland.c:653) > by 0x584298: compDestroyWindow (compwindow.c:613) > by 0x53CEE3: damageDestroyWindow (damage.c:1570) > by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) > by 0x46F7F6: FreeWindowResources (window.c:1031) > by 0x472847: DeleteWindow (window.c:1099) > by 0x46B54C: doFreeResource (resource.c:880) > by 0x46C706: FreeClientResources (resource.c:1146) > by 0x446ADE: CloseDownClient (dispatch.c:3473) >Address 0x182abde0 is 80 bytes inside a block of size 112 free'd > at 0x4C2FDAC: free (vg_replace_malloc.c:530) > by 0x42A937: xwl_destroy_window (xwayland.c:647) > by 0x584298: compDestroyWindow (compwindow.c:613) > by 0x53CEE3: damageDestroyWindow (damage.c:1570) > by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) > by 0x46F7F6: FreeWindowResources (window.c:1031) > by 0x472847: DeleteWindow (window.c:1099) > by 0x46B54C: doFreeResource (resource.c:880) > by 0x46C706: FreeClientResources (resource.c:1146) > by 0x446ADE: CloseDownClient (dispatch.c:3473) > by 0x446DA5: ProcKillClient (dispatch.c:3279) > by 0x4476AF: Dispatch (dispatch.c:479) >Block was alloc'd at > at 0x4C30B06: calloc (vg_replace_malloc.c:711) > by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54) > by 0x434228: xwl_present_get_crtc (xwayland-present.c:302) > by 0x539728: proc_present_query_capabilities (present_request.c:227) > by 0x4476AF: Dispatch (dispatch.c:479) > by 0x44B5B5: dix_main (main.c:276) > by 0x75F611A: (below main) (libc-start.c:308) > > Move `xwl_present_cleanup()` after `DestroyWindow()` so that > `xwl_present_abort_vblank()` can still access valid memory before it's > freed. > > Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269 > Signed-off-by: Olivier Fourdan > --- > hw/xwayland/xwayland.c | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c > index 96b4db18c..63d69fb3a 100644 > --- a/hw/xwayland/xwayland.c > +++ b/hw/xwayland/xwayland.c > @@ -642,11 +642,6 @@ xwl_destroy_window(WindowPtr window) > struct xwl_screen *xwl_screen = xwl_screen_get(screen); > Bool ret; > > -#ifdef GLAMOR_HAS_GBM > -if (xwl_screen->present) > -xwl_present_cleanup(window); > -#endif > - > screen->DestroyWindow = xwl_screen->DestroyWindow; > > if (screen->DestroyWindow) > @@ -657,6 +652,11 @@ xwl_destroy_window(WindowPtr window) > xwl_screen->DestroyWindow = screen->DestroyWindow; > screen->DestroyWindow = xwl_destroy_window; > > +#ifdef GLAMOR_HAS_GBM > +if (xwl_screen->present) > +xwl_present_cleanup(window); > +#endif > + > return ret; > } > > -- > 2.17.1 > Sorry, not enough coffee, I take that back, obviously the crash won't occur because `xwl_present_cleanup()` will fail to match the window once the resources have been freed so we return early in `xwl_present_cleanup()`... Will try again. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Clean-up present after destroying common bits
Right now, `xwl_destroy_window()` invokes `xwl_present_cleanup()` before the common `DestroyWindow()`. But `DestroyWindow()` calls in `present_destroy_window()` which will then end up in `xwl_present_abort_vblank()` which will try to access data that was previously freed by `xwl_present_cleanup()`: Invalid read of size 8 at 0x434184: xwl_present_abort_vblank (xwayland-present.c:378) by 0x53785B: present_wnmd_abort_vblank (present_wnmd.c:651) by 0x53695A: present_free_window_vblank (present_screen.c:87) by 0x53695A: present_destroy_window (present_screen.c:152) by 0x42A90D: xwl_destroy_window (xwayland.c:653) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) Address 0x182abde0 is 80 bytes inside a block of size 112 free'd at 0x4C2FDAC: free (vg_replace_malloc.c:530) by 0x42A937: xwl_destroy_window (xwayland.c:647) by 0x584298: compDestroyWindow (compwindow.c:613) by 0x53CEE3: damageDestroyWindow (damage.c:1570) by 0x4F1BB8: DbeDestroyWindow (dbe.c:1326) by 0x46F7F6: FreeWindowResources (window.c:1031) by 0x472847: DeleteWindow (window.c:1099) by 0x46B54C: doFreeResource (resource.c:880) by 0x46C706: FreeClientResources (resource.c:1146) by 0x446ADE: CloseDownClient (dispatch.c:3473) by 0x446DA5: ProcKillClient (dispatch.c:3279) by 0x4476AF: Dispatch (dispatch.c:479) Block was alloc'd at at 0x4C30B06: calloc (vg_replace_malloc.c:711) by 0x433F46: xwl_present_window_get_priv (xwayland-present.c:54) by 0x434228: xwl_present_get_crtc (xwayland-present.c:302) by 0x539728: proc_present_query_capabilities (present_request.c:227) by 0x4476AF: Dispatch (dispatch.c:479) by 0x44B5B5: dix_main (main.c:276) by 0x75F611A: (below main) (libc-start.c:308) Move `xwl_present_cleanup()` after `DestroyWindow()` so that `xwl_present_abort_vblank()` can still access valid memory before it's freed. Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1616269 Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c index 96b4db18c..63d69fb3a 100644 --- a/hw/xwayland/xwayland.c +++ b/hw/xwayland/xwayland.c @@ -642,11 +642,6 @@ xwl_destroy_window(WindowPtr window) struct xwl_screen *xwl_screen = xwl_screen_get(screen); Bool ret; -#ifdef GLAMOR_HAS_GBM -if (xwl_screen->present) -xwl_present_cleanup(window); -#endif - screen->DestroyWindow = xwl_screen->DestroyWindow; if (screen->DestroyWindow) @@ -657,6 +652,11 @@ xwl_destroy_window(WindowPtr window) xwl_screen->DestroyWindow = screen->DestroyWindow; screen->DestroyWindow = xwl_destroy_window; +#ifdef GLAMOR_HAS_GBM +if (xwl_screen->present) +xwl_present_cleanup(window); +#endif + return ret; } -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: Window scaling (aka owner sizes)
Hi Keith, On Mon, Aug 27, 2018 at 8:24 AM Keith Packard wrote: > > > I'm doing a bit of work to help support applications that want to run at > a small fixed size. The 'usual' way to make them visible on our modern > desktops is to flip video modes, but that means everything runs at low > resolution, plus you get the adventure of switching modes, which can > take a long time in complex monitor environments. > > https://keithp.com/blogs/window-scaling/ > > The basic plan is to make the size-as-seen-by-the-owner different from > the size seen by the rest of the system, and then to either have the > server scale the output or get the compositing manager to do it. > > This solves one of the main use cases for 'composite redirection' that I > talked about a long time ago and could never get to work. Restricting > the problem space to simple application scaling, instead of arbitrary > transformation, meant that I could place the input event scaling right > inside the X server, allowing it to be synchronous, instead of > attempting to have some external agent perform the transformation. > > When there's no compositing manager running, the window is placed into > automatic compositing mode and the X server performs the necessary > output scaling. > > When there is a compositing manager running, I've thought of three > possible ways to make it work: > > 1. The compositing manager can be extended to handle the scaling > operation. This seems like the best plan; just a single copy from > the native application size to the scanout buffer. > > 2. Alternatively, another window might be interposed between the scaled > client and the compositor to provide a place for the X server to do > the scaling; although that would naturally introduce a bunch more > expense and delay in the process. > > 3. It would also be possible to have the X server provide a scaled > version of the window pixmap to the compositing manager until it > advertised support for built-in scaling. Same rendering delay as > with an interposed window, but no changes required in the > environment, although a bit of work to be done in the X server. That > might be a good transition plan so that things didn't break when you > started a compositing manager without the required support. > > I'm wondering if there are others who would find this useful, and if > some of them might help get this into reasonable shape. Suggestions are > welcome! Much interesting indeed :) We've been considering something similar for Xwayland, scaling the window is relatively easy, but one possible issue with scaling the client window is the perception of the screen size from the client itself. Imagine the client is trying to size its window to match the screen or even output size, or tries to center its window on the current monitor. It does so based on the root window size or possibly the Xrandr information or even Xinerama, but since its window will be scaled afterwards, that computed size or position will end up being wrong. So that means that the screen size and xrandr info sent to the client depends on the client's window being scaled or not, eventually. Basically, the Xserver needs to lie to the client so that the client computes its window size/position right when mapped on screen. But things like DisplayWidth()/DisplayHeight() are defined as C macros in Xlib, to access the fields of the Screen sent when opening the connection with the Xserver, not something we can fix easily. Moreover, several properties from EWMH on the root window reflect the screen size (things like _NET_DESKTOP_GEOMETRY, _NET_WORKAREA) and are necessarily shared between all X11 clients (who share the same root window). How do you plan to deal with this in your approach? Cheers, Olivier PS: When considering this for Xwayland, I ended up thinking that it would be actually easier to have different X11 screens (i.e. :0.0, :0.1, etc.) of different size for different scales for the X11 clients ended up being scaled differently (which led me to https://gitlab.freedesktop.org/ofourdan/xserver/tree/xwayland-multi-screen), but that approach introduces a lot of complexity (the CM has to play the proxy between all those X11 screens for things like copy/paste, DnD, etc.) ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH v2 xserver] xwayland: Enable DRI3 for glamor
glamor_fds_from_pixmap() will bail out early if DRI3 is not enabled,
unfortunately Xwayland's glamor code would not set it as enabled which
would lead to blank pixmaps when using texture from pixmap.
Make sure to mark DRI3 as enabled from glamor_egl_screen_init() in
Xwayland.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107287
Fixes: c8c276c956 ("glamor: Implement PixmapFromBuffers and BuffersFromPixmap")
Signed-off-by: Olivier Fourdan
Reviewed-by: Michel Dänzer
---
v2: Fix "Fixes" vs. "Bugzilla"
Add Michel's r-b
hw/xwayland/xwayland-glamor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index f17914344..7ea6def61 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -57,6 +57,7 @@ glamor_egl_screen_init(ScreenPtr screen, struct
glamor_context *glamor_ctx)
{
struct xwl_screen *xwl_screen = xwl_screen_get(screen);
+glamor_enable_dri3(screen);
glamor_ctx->ctx = xwl_screen->egl_context;
glamor_ctx->display = xwl_screen->egl_display;
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: Enable DRI3 for glamor
glamor_fds_from_pixmap() will bail out early if DRI3 is not enabled,
unfortunately Xwayland's glamor code would not set it as enabled which
would lead to blank pixmaps when using texture from pixmap.
Make sure to mark DRI3 as enabled from glamor_egl_screen_init() in
Xwayland.
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=107287
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index f17914344..7ea6def61 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -57,6 +57,7 @@ glamor_egl_screen_init(ScreenPtr screen, struct
glamor_context *glamor_ctx)
{
struct xwl_screen *xwl_screen = xwl_screen_get(screen);
+glamor_enable_dri3(screen);
glamor_ctx->ctx = xwl_screen->egl_context;
glamor_ctx->display = xwl_screen->egl_display;
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v2] xwayland: rotate logical size for RRMode
Hi,
On Mon, Jul 16, 2018 at 10:47 AM Simon Ser wrote:
>
> On July 16, 2018 8:50 AM, Olivier Fourdan wrote:
> > Hi,
> >
> > Thanks for your follow up on that!
> >
> > On Fri, Jul 13, 2018 at 9:51 PM Simon Ser wrote:
> > >
> > > From: emersion
> > >
> > > The logical size is the size of the output in the global compositor
> > > space. The mode width/height should be scaled as in the logical
> > > size, but shouldn't be transformed. Thus we need to rotate back
> > > the logical size to be able to use it as the mode width/height.
> > >
> > > This fixes issues with pointer input on transformed outputs.
> > >
> > > Signed-Off-By: Simon Ser
> > > ---
> > >
> > > Changes from v1 to v2:
> > > - Fixed rotation when xdg-output isn't available
> > >
> > > I've made sure this works on both rootston (with xdg-output) and
> > > Weston (without xdg-output).
> > >
> > > hw/xwayland/xwayland-output.c | 12 +++-
> > > 1 file changed, 11 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> > > index 379062549..0d2ec7890 100644
> > > --- a/hw/xwayland/xwayland-output.c
> > > +++ b/hw/xwayland/xwayland-output.c
> > > @@ -213,6 +213,7 @@ apply_output_change(struct xwl_output *xwl_output)
> > > {
> > > struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
> > > struct xwl_output *it;
> > > +int mode_width, mode_height;
> > > int width = 0, height = 0, has_this_output = 0;
> > > RRModePtr randr_mode;
> > > Bool need_rotate;
> > > @@ -224,7 +225,16 @@ apply_output_change(struct xwl_output *xwl_output)
> > > /* xdg-output sends output size in compositor space. so already
> > > rotated */
> > > need_rotate = (xwl_output->xdg_output == NULL);
> > >
> > > -randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height,
> > > +/* We need to rotate back the logical size for the mode */
> > > +if (need_rotate || xwl_output->rotation & (RR_Rotate_0 |
> > > RR_Rotate_180)) {
> >
> > But is it `need_rotate` or `!need_rotate` here?
> >
> > `need_rotate` being `TRUE` means we don't have xdg-output available,
> > in which case we shouldn't have to do this. Basically, we need to
> > revert to the original width/height only if we have xdg-output (which
> > has already applied the rotation), so I reckon it should be
> > `!need_rotate` here.
>
> Yes, except this branch handles the "don't do anything" case: width = width,
> height = height. The other branch is when we actually need to rotate.
Yeap, agreed.
> This variable could probably be renamed to something more sensible (like
> `is_logical_size` and invert conditions?).
Agreed, the variable name could be better, I've never been very good
at picking good function and variable names :)
> > > +mode_width = xwl_output->width;
> > > +mode_height = xwl_output->height;
> > > +} else {
> > > +mode_width = xwl_output->height;
> > > +mode_height = xwl_output->width;
> > > +}
> >
> > So if we use `(!need_rotate)`, this addition becomes completely
> > similar to the code found in `output_get_new_size()` it might be
> > interesting to move that to a small helper function, e.g.
> > `output_get_transform_size()` that would return the swapped
> > width/height depending on the output rotation, so we don't duplicate
> > that small portion of code. Nit picking, I know :)
>
> The problem is, `output_get_new_size` needs the logical size and we need the
> mode size, so one is swapped and the other isn't. We could still factor those
> in
> a separate function, but I'm afraid this will make things more complicated
> than
> they already are.
Right
> > > +
> > > +randr_mode = xwayland_cvt(mode_width, mode_height,
> > >xwl_output->refresh / 1000.0, 0, 0);
> > > RROutputSetModes(xwl_output->randr_output, _mode, 1, 1);
> > > RRCrtcNotify(xwl_output->randr_crtc, randr_mode,
> > > --
> > > 2.18.0
> >
So this is
Reviewed-by: Olivier Fourdan
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver v2] xwayland: rotate logical size for RRMode
Hi,
Thanks for your follow up on that!
On Fri, Jul 13, 2018 at 9:51 PM Simon Ser wrote:
>
> From: emersion
>
> The logical size is the size of the output in the global compositor
> space. The mode width/height should be scaled as in the logical
> size, but shouldn't be transformed. Thus we need to rotate back
> the logical size to be able to use it as the mode width/height.
>
> This fixes issues with pointer input on transformed outputs.
>
> Signed-Off-By: Simon Ser
> ---
>
> Changes from v1 to v2:
> - Fixed rotation when xdg-output isn't available
>
> I've made sure this works on both rootston (with xdg-output) and
> Weston (without xdg-output).
>
> hw/xwayland/xwayland-output.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index 379062549..0d2ec7890 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -213,6 +213,7 @@ apply_output_change(struct xwl_output *xwl_output)
> {
> struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
> struct xwl_output *it;
> +int mode_width, mode_height;
> int width = 0, height = 0, has_this_output = 0;
> RRModePtr randr_mode;
> Bool need_rotate;
> @@ -224,7 +225,16 @@ apply_output_change(struct xwl_output *xwl_output)
> /* xdg-output sends output size in compositor space. so already rotated
> */
> need_rotate = (xwl_output->xdg_output == NULL);
>
> -randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height,
> +/* We need to rotate back the logical size for the mode */
> +if (need_rotate || xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180))
> {
But is it `need_rotate` or `!need_rotate` here?
`need_rotate` being `TRUE` means we don't have xdg-output available,
in which case we shouldn't have to do this. Basically, we need to
revert to the original width/height only if we have xdg-output (which
has already applied the rotation), so I reckon it should be
`!need_rotate` here.
> +mode_width = xwl_output->width;
> +mode_height = xwl_output->height;
> +} else {
> +mode_width = xwl_output->height;
> +mode_height = xwl_output->width;
> +}
So if we use `(!need_rotate)`, this addition becomes completely
similar to the code found in `output_get_new_size()` it might be
interesting to move that to a small helper function, e.g.
`output_get_transform_size()` that would return the swapped
width/height depending on the output rotation, so we don't duplicate
that small portion of code. Nit picking, I know :)
> +
> +randr_mode = xwayland_cvt(mode_width, mode_height,
>xwl_output->refresh / 1000.0, 0, 0);
> RROutputSetModes(xwl_output->randr_output, _mode, 1, 1);
> RRCrtcNotify(xwl_output->randr_crtc, randr_mode,
> --
> 2.18.0
Also, this is something I noticed the hard way some time ago
(completely unrelated to your patch, though), the width/height
parameters for `output_get_new_size()` are inverted (`height, width`
instead of `width, height` as usual) which is error prone (don't ask
how I noticed...).
I can't see a reason for this everywhere else in the code we use
`width, height`, I reckon it would be great if we could fix that (in a
separate patch) as well, while at it... Because things are already
complicated enough that we don't need to add more confusion by
changing the well established order of parameters just for the sake of
it :)
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: rotate logical size for RRMode
Hi,
On Wed, Jul 11, 2018 at 9:22 AM Simon Ser wrote:
>
> From: emersion
>
> The logical size is the size of the output in the global compositor
> space. The mode width/height should be scaled as in the logical
> size, but shouldn't be transformed. Thus we need to rotate back
> the logical size to be able to use it as the mode width/height.
>
> This fixes issues with pointer input on transformed outputs.
>
> Signed-Off-By: Simon Ser
> ---
> This supersedes my previous patch [1], and rotates the logical size
> instead of using the raw wl_output mode (so that scaling still
> works).
>
> [1]: https://lists.x.org/archives/xorg-devel/2018-July/057285.html
>
> hw/xwayland/xwayland-output.c | 12 +++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index 379062549..26e055867 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -213,6 +213,7 @@ apply_output_change(struct xwl_output *xwl_output)
> {
> struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
> struct xwl_output *it;
> +int mode_width, mode_height;
> int width = 0, height = 0, has_this_output = 0;
> RRModePtr randr_mode;
> Bool need_rotate;
> @@ -224,7 +225,16 @@ apply_output_change(struct xwl_output *xwl_output)
> /* xdg-output sends output size in compositor space. so already rotated
> */
> need_rotate = (xwl_output->xdg_output == NULL);
>
> -randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height,
> +/* We need to rotate back the logical size for the mode */
> +if (xwl_output->rotation & (RR_Rotate_0 | RR_Rotate_180)) {
> +mode_width = xwl_output->width;
> +mode_height = xwl_output->height;
> +} else {
> +mode_width = xwl_output->height;
> +mode_height = xwl_output->width;
> +}
But shouldn't this check `need_rotate` as well?
I mean, we should not do this in the case of `xdg-output` not being available.
> +
> +randr_mode = xwayland_cvt(mode_width, mode_height,
>xwl_output->refresh / 1000.0, 0, 0);
> RROutputSetModes(xwl_output->randr_output, _mode, 1, 1);
> RRCrtcNotify(xwl_output->randr_crtc, randr_mode,
> --
This is where I get surprised, because if I understand the problem,
you where stating that the pointer events stopped working below a
certain limit.
Those are limited by the actual “screen size” as set by
`update_desktop_dimensions()` at the end of `update_screen_size()`, I
don't see how changing the size passed to compute the randr mode will
help with the bug you're trying to address here...
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: don't use logical size for RRMode
Hi, On Tue, 10 Jul 2018 at 11:59, Simon Ser wrote: > Yes, that's intentional. When the physical size isn't available (e.g. when > using > projectors or virtual outputs) we just send zero. Do you think this is > harmful? According to the xrandr protocol definition: https://cgit.freedesktop.org/xorg/proto/randrproto/tree/randrproto.txt For the definition of RRGetOutputInfo: | 'widthInMillimeters' and 'heightInMillimeters' report the physical | size of the displayed area. If unknown, or not really fixed (e.g., | for a projector), these values are both zero. So I reckon using 0 for physical size is valid. > I like sending zero because it clearly says "this piece of information is not > relevant in the current situation" instead of faking data. Maybe Xwayland > should > fake it and use a default 96 DPI in this case? > I'm not aware of any X11 client that breaks because of this (yet). If zero for the physical size is a valid value (as stated by the protocol for RRGetOutputInfo), clients should be able to deal with that or else it's a client bug. Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: don't use logical size for RRMode
Hi,
On Mon, 9 Jul 2018 at 09:17, Simon Ser wrote:
>
> The logical size is the size of the output in the global compositor
> space. The mode width/height shouldn't be transformed and scaled.
>
> This fixes issues with pointer input on transformed outputs.
>
> Signed-Off-By: Simon Ser
> ---
> hw/xwayland/xwayland-output.c | 9 ++---
> hw/xwayland/xwayland.h| 3 ++-
> 2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
> index 379062549..44fd5c26a 100644
> --- a/hw/xwayland/xwayland-output.c
> +++ b/hw/xwayland/xwayland-output.c
> @@ -113,12 +113,15 @@ output_handle_mode(void *data, struct wl_output
> *wl_output, uint32_t flags,
> if (!(flags & WL_OUTPUT_MODE_CURRENT))
> return;
>
> +xwl_output->mode_width = width;
> +xwl_output->mode_height = height;
> +xwl_output->mode_refresh = refresh;
So, with that patch mode_width/height reflects the wl_output size.
> +
> /* Apply the change from wl_output only if xdg-output is not supported */
> if (!xwl_output->xdg_output) {
> xwl_output->width = width;
> xwl_output->height = height;
> }
> -xwl_output->refresh = refresh;
> }
>
> static inline void
> @@ -224,8 +227,8 @@ apply_output_change(struct xwl_output *xwl_output)
> /* xdg-output sends output size in compositor space. so already rotated
> */
> need_rotate = (xwl_output->xdg_output == NULL);
>
> -randr_mode = xwayland_cvt(xwl_output->width, xwl_output->height,
> - xwl_output->refresh / 1000.0, 0, 0);
> +randr_mode = xwayland_cvt(xwl_output->mode_width,
> xwl_output->mode_height,
> + xwl_output->mode_refresh / 1000.0, 0, 0);
And we'd advertise wl_ouput size as RR size? That's not how it's meant
to work, we want RR to reflect the xdg-output (logical) size.
> RROutputSetModes(xwl_output->randr_output, _mode, 1, 1);
> RRCrtcNotify(xwl_output->randr_crtc, randr_mode,
> xwl_output->x, xwl_output->y,
> diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
> index d70ad54bf..66daf58de 100644
> --- a/hw/xwayland/xwayland.h
> +++ b/hw/xwayland/xwayland.h
> @@ -368,7 +368,8 @@ struct xwl_output {
> struct xwl_screen *xwl_screen;
> RROutputPtr randr_output;
> RRCrtcPtr randr_crtc;
> -int32_t x, y, width, height, refresh;
> +int32_t x, y, width, height;
> +int32_t mode_width, mode_height, mode_refresh;
> Rotation rotation;
> Bool wl_output_done;
> Bool xdg_output_done;
> --
> 2.18.0
So, can you please elaborate exactly what this is supposed to fix and
how to reproduce the issue?
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] modesetting: use drmmode_bo_import() for rotate_fb
On Fri, Jun 15, 2018 at 8:57 AM, Olivier Fourdan wrote:
> drmmode_shadow_allocate() still uses drmModeAddFB() which may fail if
> the format is not as expected, preventing from using a rotated output.
>
> Change it to use the new function drmmode_bo_import() which takes care
> of calling the drmModeAddFB2() API.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106715
> Signed-off-by: Olivier Fourdan
> ---
> hw/xfree86/drivers/modesetting/drmmode_display.c | 7 ++-
> 1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
> b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 859a21a9d..ec11b3f56 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -1794,11 +1794,8 @@ drmmode_shadow_allocate(xf86CrtcPtr crtc, int width,
> int height)
> return NULL;
> }
>
> -ret = drmModeAddFB(drmmode->fd, width, height, crtc->scrn->depth,
> - drmmode->kbpp,
> - drmmode_bo_get_pitch(_crtc->rotate_bo),
> - drmmode_bo_get_handle(_crtc->rotate_bo),
> - _crtc->rotate_fb_id);
> +ret = drmmode_bo_import(drmmode, _crtc->rotate_bo,
> +_crtc->rotate_fb_id);
>
> if (ret) {
> ErrorF("failed to add rotate fb\n");
> --
> 2.17.1
>
Tested successfully downstream by Tom Pelka, so adding:
Tested-by: Tomas Pelka
Thanks Tom!
Olivier.
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 0/2] xwayland: couple of EGL backend API cleanup
Hi, On Fri, Jun 15, 2018 at 9:46 AM, Olivier Fourdan wrote: > Hi Emil, > > On Thu, Jun 14, 2018 at 7:38 PM, Emil Velikov > wrote: >> What's the blocker of pushing these - short on commit access, other? >> If the former, that should be addressed. You have done some serious >> work on Xwayland. > > I don't think I have commit access myself, never tried actually, never dare > to! Well, I could think of a reason why it hasn't been picked up yet... Those patch series are quite a lot of refactoring, and considering we haven't branched 1.20 yet, it might be unclear if this is material for master or xserver-1.20-branch (once it's created). FWIW, I reckon those series are a feature fix primarily, i.e. enable EGLStream support automatically when needed without requiring to change all compositors to add “-eglstream” explicitly, which would crash in different and creative ways with the current code (unknown option if Xwayland was built without EGLStream support, or crash if EGLStream support is there in Xwayland but the hardware isn't). So, IMHO, these patches should be part of 1.20 at some point, but I understand that it's quite a lot of change for a stable tree (even if released only recently) so I would leave that to whoever takes the role of release manager to decide :) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 0/2] xwayland: couple of EGL backend API cleanup
Hi Emil, On Thu, Jun 14, 2018 at 7:38 PM, Emil Velikov wrote: > What's the blocker of pushing these - short on commit access, other? > If the former, that should be addressed. You have done some serious > work on Xwayland. I don't think I have commit access myself, never tried actually, never dare to! > That said, the series is > Reviewed-by: Emil Velikov Thanks! Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] modesetting: use drmmode_bo_import() for rotate_fb
drmmode_shadow_allocate() still uses drmModeAddFB() which may fail if
the format is not as expected, preventing from using a rotated output.
Change it to use the new function drmmode_bo_import() which takes care
of calling the drmModeAddFB2() API.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106715
Signed-off-by: Olivier Fourdan
---
hw/xfree86/drivers/modesetting/drmmode_display.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
b/hw/xfree86/drivers/modesetting/drmmode_display.c
index 859a21a9d..ec11b3f56 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -1794,11 +1794,8 @@ drmmode_shadow_allocate(xf86CrtcPtr crtc, int width, int
height)
return NULL;
}
-ret = drmModeAddFB(drmmode->fd, width, height, crtc->scrn->depth,
- drmmode->kbpp,
- drmmode_bo_get_pitch(_crtc->rotate_bo),
- drmmode_bo_get_handle(_crtc->rotate_bo),
- _crtc->rotate_fb_id);
+ret = drmmode_bo_import(drmmode, _crtc->rotate_bo,
+_crtc->rotate_fb_id);
if (ret) {
ErrorF("failed to add rotate fb\n");
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: add "tablet" into the tablet device names
On Mon, 11 Jun 2018 at 11:40, Peter Hutterer wrote: > On Mon, Jun 11, 2018 at 11:21:25AM +0200, Olivier Fourdan wrote: [...] > > > -pad->xdevice = add_device(pad->seat, "xwayland-pad", > > > +pad->xdevice = add_device(pad->seat, "xwayland-tablet-pad", > > > > Previous ones used a space, here you use a dash between > > “xwayland-tablet” and the device type, I'd rather have that > > consistent. > > at the risk of disagreeing over the optical appearance of a velocipede > storage environment: the xwayland-tablet-pad refers to an interface in the > same way as xwayland-tablet does. The space-separated part is the tool which > we only have in the tablet interface, not the tablet-pad one. Oh, I see! There is a logic behind that apparent inconsistency! :) > Not that any of this really matters for the user who sees this :) Indeed, so either way, my R-b remains :) Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xwayland: add "tablet" into the tablet device names
Hi Peter,
On Mon, 11 Jun 2018 at 01:12, Peter Hutterer wrote:
>
> Changes the device name from "xwayland-stylus" to "xwayland-tablet stylus".
> This doesn't fully address #26 but it goes a little step into making it more
> human-readable.
>
> https://gitlab.freedesktop.org/wayland/wayland/issues/26
>
> Signed-off-by: Peter Hutterer
> ---
> hw/xwayland/xwayland-input.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 0a37f97bd..a602f0887 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -1389,19 +1389,19 @@ tablet_handle_done(void *data, struct zwp_tablet_v2
> *tablet)
> struct xwl_seat *xwl_seat = xwl_tablet->seat;
>
> if (xwl_seat->stylus == NULL) {
> -xwl_seat->stylus = add_device(xwl_seat, "xwayland-stylus",
> xwl_tablet_proc);
> +xwl_seat->stylus = add_device(xwl_seat, "xwayland-tablet stylus",
> xwl_tablet_proc);
> ActivateDevice(xwl_seat->stylus, TRUE);
> }
> EnableDevice(xwl_seat->stylus, TRUE);
>
> if (xwl_seat->eraser == NULL) {
> -xwl_seat->eraser = add_device(xwl_seat, "xwayland-eraser",
> xwl_tablet_proc);
> +xwl_seat->eraser = add_device(xwl_seat, "xwayland-tablet eraser",
> xwl_tablet_proc);
> ActivateDevice(xwl_seat->eraser, TRUE);
> }
> EnableDevice(xwl_seat->eraser, TRUE);
>
> if (xwl_seat->puck == NULL) {
> -xwl_seat->puck = add_device(xwl_seat, "xwayland-cursor",
> xwl_tablet_proc);
> +xwl_seat->puck = add_device(xwl_seat, "xwayland-tablet cursor",
> xwl_tablet_proc);
> ActivateDevice(xwl_seat->puck, TRUE);
> }
> EnableDevice(xwl_seat->puck, TRUE);
> @@ -2147,7 +2147,7 @@ tablet_pad_done(void *data,
> {
> struct xwl_tablet_pad *pad = data;
>
> -pad->xdevice = add_device(pad->seat, "xwayland-pad",
> +pad->xdevice = add_device(pad->seat, "xwayland-tablet-pad",
Previous ones used a space, here you use a dash between
“xwayland-tablet” and the device type, I'd rather have that
consistent.
>xwl_tablet_pad_proc);
> pad->xdevice->public.devicePrivate = pad;
> ActivateDevice(pad->xdevice, TRUE);
> --
> 2.14.4
>
> ___
> wayland-devel mailing list
> wayland-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Sounds like a reasonable thing to do... with a pretty low risk.
With the consistency nit picking addressed:
Reviewed-by: Olivier Fourdan
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 0/2] xwayland: couple of EGL backend API cleanup
Hi, The recent issue with Present due to the wrong pixmap size being given to xwl_glamor_pixmap_get_wl_buffer() got me thinking. Considering that the code always use the drawable size now, that passing a wrong size is error prone because once the buffer is created for the given pixmap, subsequent calls to xwl_glamor_pixmap_get_wl_buffer() will always return that same buffer with GBM backend and considering EGLStream has no use for the given size either, there is really no point in passing the width/height in the API... The second patch was suggested by Emil, both GBM and EGLStream backends implement init_wl_registry() and has_wl_interfaces() so there is no point in marking those as optional. Those patches go on top and complement the previous series, i.e.: https://patchwork.freedesktop.org/series/44302/ https://patchwork.freedesktop.org/series/44303/ https://patchwork.freedesktop.org/series/44423/ https://patchwork.freedesktop.org/series/44489/ I realize this is becoming a bit hard to follow, so I've put then entire series here: git://people.freedesktop.org/~ofourdan/xserver xwayland https://cgit.freedesktop.org/~ofourdan/xserver/log/?h=xwayland So it's easier to review/test. Thanks! Olivier Olivier Fourdan (2): xwayland: simplify xwl_glamor_pixmap_get_wl_buffer() xwayland: mandatory EGL backend API hw/xwayland/xwayland-glamor-eglstream.c | 2 -- hw/xwayland/xwayland-glamor-gbm.c | 4 ++-- hw/xwayland/xwayland-glamor.c | 14 ++ hw/xwayland/xwayland-present.c | 5 + hw/xwayland/xwayland.c | 2 -- hw/xwayland/xwayland.h | 10 ++ 6 files changed, 7 insertions(+), 30 deletions(-) -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/2] xwayland: mandatory EGL backend API
The API init_wl_registry() and has_wl_interfaces() are marked as being
optional, but both GBM And EGLStream backends implement them so there is
point in keeping those optional.
Suggested-by: Emil Velikov
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor.c | 8 +---
hw/xwayland/xwayland.h| 6 ++
2 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 61418e707..f17914344 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -72,14 +72,12 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
uint32_t version)
{
if (xwl_screen->gbm_backend.is_available &&
-xwl_screen->gbm_backend.init_wl_registry &&
xwl_screen->gbm_backend.init_wl_registry(xwl_screen,
registry,
id,
interface,
version)); /* no-op */
else if (xwl_screen->eglstream_backend.is_available &&
- xwl_screen->eglstream_backend.init_wl_registry &&
xwl_screen->eglstream_backend.init_wl_registry(xwl_screen,
registry,
id,
@@ -91,11 +89,7 @@ Bool
xwl_glamor_has_wl_interfaces(struct xwl_screen *xwl_screen,
struct xwl_egl_backend *xwl_egl_backend)
{
-if (xwl_egl_backend->has_wl_interfaces)
-return xwl_egl_backend->has_wl_interfaces(xwl_screen);
-
-/* If the backend has no requirement wrt WL interfaces, we're fine */
-return TRUE;
+return xwl_egl_backend->has_wl_interfaces(xwl_screen);
}
struct wl_buffer *
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index dc01c747c..d70ad54bf 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -64,16 +64,14 @@ struct xwl_egl_backend {
Bool is_available;
/* Called once for each interface in the global registry. Backends
- * should use this to bind to any wayland interfaces they need. This
- * callback is optional.
+ * should use this to bind to any wayland interfaces they need.
*/
Bool (*init_wl_registry)(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
uint32_t id, const char *name,
uint32_t version);
-/* Check that the required Wayland interfaces are available. This
- * callback is optional.
+/* Check that the required Wayland interfaces are available.
*/
Bool (*has_wl_interfaces)(struct xwl_screen *xwl_screen);
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/2] xwayland: simplify xwl_glamor_pixmap_get_wl_buffer()
When retrieving the Wayland buffer from a pixmap, if the buffer already
exists, the GBM backend will return that existing buffer.
However, as seen with the Present issues, if the call had previously
passed a wrong size, that buffer will remain at the wrong size for as
long as the buffer exists, which is error prone.
Considering that the width/height passed to get_wl_buffer() is always the
actual pixmap drawable size, and considering that the EGLStream backend
makes no use of the size either, there is really no point in passing the
width/height around.
Simplify the xwl_glamor_pixmap_get_wl_buffer() and EGL backends API by
removing the pixmap size, and use the drawable size instead.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 2 --
hw/xwayland/xwayland-glamor-gbm.c | 4 ++--
hw/xwayland/xwayland-glamor.c | 6 +-
hw/xwayland/xwayland-present.c | 5 +
hw/xwayland/xwayland.c | 2 --
hw/xwayland/xwayland.h | 4
6 files changed, 4 insertions(+), 19 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 43f34eed1..9950be94d 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -308,8 +308,6 @@ xwl_glamor_eglstream_destroy_pixmap(PixmapPtr pixmap)
static struct wl_buffer *
xwl_glamor_eglstream_get_wl_buffer_for_pixmap(PixmapPtr pixmap,
- unsigned short width,
- unsigned short height,
Bool *created)
{
/* XXX created? */
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index bb29cc28e..42d758e93 100644
--- a/hw/xwayland/xwayland-glamor-gbm.c
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -230,13 +230,13 @@ xwl_glamor_gbm_destroy_pixmap(PixmapPtr pixmap)
static struct wl_buffer *
xwl_glamor_gbm_get_wl_buffer_for_pixmap(PixmapPtr pixmap,
-unsigned short width,
-unsigned short height,
Bool *created)
{
struct xwl_screen *xwl_screen = xwl_screen_get(pixmap->drawable.pScreen);
struct xwl_pixmap *xwl_pixmap = xwl_pixmap_get(pixmap);
struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
+unsigned short width = pixmap->drawable.width;
+unsigned short height = pixmap->drawable.height;
int prime_fd;
int num_planes;
uint32_t strides[4];
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 2f64d0500..61418e707 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -100,17 +100,13 @@ xwl_glamor_has_wl_interfaces(struct xwl_screen
*xwl_screen,
struct wl_buffer *
xwl_glamor_pixmap_get_wl_buffer(PixmapPtr pixmap,
-unsigned short width,
-unsigned short height,
Bool *created)
{
struct xwl_screen *xwl_screen = xwl_screen_get(pixmap->drawable.pScreen);
if (xwl_screen->egl_backend->get_wl_buffer_for_pixmap)
return xwl_screen->egl_backend->get_wl_buffer_for_pixmap(pixmap,
-width,
-height,
-created);
+ created);
return NULL;
}
diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 29014a300..81e0eb9ce 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -456,10 +456,7 @@ xwl_present_flip(WindowPtr present_window,
xwl_window->present_window = present_window;
-buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap,
- pixmap->drawable.width,
- pixmap->drawable.height,
- _created);
+buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap, _created);
event->event_id = event_id;
event->xwl_present_window = xwl_present_window;
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 7ea01ab86..96b4db18c 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -678,8 +678,6 @@ xwl_window_post_damage(struct xwl_window *xwl_window)
#ifdef XWL_HAS_GLAMOR
if (xwl_screen->glamor)
buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap,
- pixmap->drawable.width,
- pixmap->drawable.height,
NULL);
else
#endif
diff --git a/hw/xwayland/xwa
[PATCH xserver v3] xwayland: use pixmap size on present flip
If the pixmap size does not match the present box size, flickering
occurs.
This can happen when the client changes its size (e.g. switching to
fullscreen), and since the buffer is kept as long as the pixmap is
valid, once the buffer is created, it remains at the wrong (old) size
and causes continuous flickering.
Use the actual pixmap's drawable size instead of the present box to
create the buffer so that it's sized appropriately.
Bugzilla: https://bugs.freedesktop.org/106841
Signed-off-by: Olivier Fourdan
Reviewed-by: Michel Dänzer
---
v2: Remove present_box, unused (thanks Michel)
hw/xwayland/xwayland-present.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c
index 4db0d1efc..29014a300 100644
--- a/hw/xwayland/xwayland-present.c
+++ b/hw/xwayland/xwayland-present.c
@@ -440,7 +440,7 @@ xwl_present_flip(WindowPtr present_window,
{
struct xwl_window *xwl_window =
xwl_window_from_window(present_window);
struct xwl_present_window *xwl_present_window =
xwl_present_window_priv(present_window);
-BoxPtr present_box, damage_box;
+BoxPtr damage_box;
Boolbuffer_created;
struct wl_buffer*buffer;
struct xwl_present_event*event;
@@ -448,7 +448,6 @@ xwl_present_flip(WindowPtr present_window,
if (!xwl_window)
return FALSE;
-present_box = RegionExtents(_window->winSize);
damage_box = RegionExtents(damage);
event = malloc(sizeof *event);
@@ -458,8 +457,8 @@ xwl_present_flip(WindowPtr present_window,
xwl_window->present_window = present_window;
buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap,
- present_box->x2 - present_box->x1,
- present_box->y2 - present_box->y1,
+ pixmap->drawable.width,
+ pixmap->drawable.height,
_created);
event->event_id = event_id;
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: use pixmap size on present flip
If the pixmap size does not match the present box size, flickering occurs. This can happen when the client changes its size (e.g. switching to fullscreen), and since the buffer is kept as long as the pixmap is valid, once the buffer is created, it remains at the wrong (old) size and causes continuous flickering. Use the actual pixmap's drawable size instead of the present box to create the buffer so that it's sized appropriately. Bugzilla: https://bugs.freedesktop.org/106841 Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland-present.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 4db0d1efc..32727310f 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -458,8 +458,8 @@ xwl_present_flip(WindowPtr present_window, xwl_window->present_window = present_window; buffer = xwl_glamor_pixmap_get_wl_buffer(pixmap, - present_box->x2 - present_box->x1, - present_box->y2 - present_box->y1, + pixmap->drawable.width, + pixmap->drawable.height, _created); event->event_id = event_id; -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver] xwayland: check pixmap size on present flip
If the pixmap size does not match the present box size, flickering occurs. This can happen when the client changes its size (e.g. switching to fullscreen), and since the buffer is kept as long as the pixmap is valid, once the buffer is created, it remains at the wrong (old) size and causes continuous flickering. Check that the pixmap size and present box size match in xwl_present_flip() and return FALSE otherwise, so that we skip the pixmap with the wrong size. Bugzilla: https://bugs.freedesktop.org/106841 Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland-present.c | 5 + 1 file changed, 5 insertions(+) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 4db0d1efc..4f1d1d1c5 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -451,6 +451,11 @@ xwl_present_flip(WindowPtr present_window, present_box = RegionExtents(_window->winSize); damage_box = RegionExtents(damage); +/* Size mismatch */ +if (pixmap->drawable.width != present_box->x2 - present_box->x1 || +pixmap->drawable.height != present_box->y2 - present_box->y1) +return FALSE; + event = malloc(sizeof *event); if (!event) return FALSE; -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] xawyland/glamor: Fix typo in xwl_glamor_pixmap_get_wl_buffer
Hi Vivek, On Fri, 8 Jun 2018 at 09:23, Vivek Das Mohapatra wrote: > > In the #else path of the #ifdef GBM_BO_WITH_MODIFIERS in > xwl_glamor_pixmap_get_wl_buffer there's a call to > gbm_go_get_stride: This should be gbm_bo_get_stride. > > Typo introduced in c8c276c9569b3ca1e695682a5443f1b615c606bd. > --- > hw/xwayland/xwayland-glamor-gbm.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/hw/xwayland/xwayland-glamor-gbm.c > b/hw/xwayland/xwayland-glamor-gbm.c > index 29325adac..68c2cc32e 100644 > --- a/hw/xwayland/xwayland-glamor-gbm.c > +++ b/hw/xwayland/xwayland-glamor-gbm.c > @@ -272,7 +272,7 @@ xwl_glamor_gbm_get_wl_buffer_for_pixmap(PixmapPtr pixmap, > #else > num_planes = 1; > modifier = DRM_FORMAT_MOD_INVALID; > -strides[0] = gbm_go_get_stride(xwl_pixmap->bo); > +strides[0] = gbm_bo_get_stride(xwl_pixmap->bo); > offsets[0] = 0; > #endif > > -- > 2.11.0 > > ___ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel Thanks for the patch, but this is a duplicate, a similar patch was already submitted and reviewed here: https://patchwork.freedesktop.org/series/43867/ Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver] present/wnmd: Preserve window pixmap's screen_x/y on flip
Hi Michel, On Thu, Jun 7, 2018 at 6:27 PM, Michel Dänzer wrote: > From: Michel Dänzer > > The incorrect values could result in the new pixmap's contents > getting corrupted down the line. > > Bugzilla: https://bugs.freedesktop.org/106841 > Fixes: 029608dd8020 "present: Add window flip mode" > Signed-off-by: Michel Dänzer > --- > present/present_wnmd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/present/present_wnmd.c b/present/present_wnmd.c > index 80ffb014e..1e3958bfc 100644 > --- a/present/present_wnmd.c > +++ b/present/present_wnmd.c > @@ -469,6 +469,8 @@ present_wnmd_execute(present_vblank_ptr vblank, uint64_t > ust, uint64_t crtc_msc) > PixmapPtr old_pixmap = screen->GetWindowPixmap(window); > > /* Replace window pixmap with flip pixmap */ > +vblank->pixmap->screen_x = old_pixmap->screen_x; > +vblank->pixmap->screen_y = old_pixmap->screen_y; > present_set_tree_pixmap(toplvl_window, old_pixmap, > vblank->pixmap); > vblank->pixmap->refcnt++; > dixDestroyPixmap(old_pixmap, old_pixmap->drawable.id); > -- > 2.17.1 > > ___ > xorg-devel@lists.x.org: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: https://lists.x.org/mailman/listinfo/xorg-devel Yeap, works for me! That one was starting to give me headaches... Tested-by: Olivier Fourdan Thanks Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 09/10] xwayland: check for EGLStream backend explicitly
Now that we have separate backends for EGLStream and GBM, we can explicitly check for the EGLStream backend to disable present support in that case. Signed-off-by: Olivier Fourdan --- hw/xwayland/xwayland-present.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/hw/xwayland/xwayland-present.c b/hw/xwayland/xwayland-present.c index 9e9e78917..4db0d1efc 100644 --- a/hw/xwayland/xwayland-present.c +++ b/hw/xwayland/xwayland-present.c @@ -547,10 +547,9 @@ xwl_present_init(ScreenPtr screen) struct xwl_screen *xwl_screen = xwl_screen_get(screen); /* - * doesn't work with the streams backend. we don't have an explicit - * boolean for that, but we do know gbm doesn't fill in this hook... + * doesn't work with the EGLStream backend. */ -if (xwl_screen->egl_backend->post_damage != NULL) +if (xwl_screen->egl_backend == _screen->eglstream_backend) return FALSE; if (!dixRegisterPrivateKey(_present_window_private_key, PRIVATE_WINDOW, 0)) -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 10/10] xwayland: EGL_IMG_context_priority required by EGLStream
xwl_glamor_eglstream_init_egl() uses "EGL_IMG_context_priority"
extension, make sure it's actually available before using it.
Suggested-by: Emil Velikov
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index c226c0089..43f34eed1 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -794,6 +794,12 @@ xwl_glamor_eglstream_init_egl(struct xwl_screen
*xwl_screen)
goto error;
}
+if (!epoxy_has_egl_extension(xwl_screen->egl_display,
+ "EGL_IMG_context_priority")) {
+ErrorF("EGL_IMG_context_priority not available\n");
+goto error;
+}
+
eglChooseConfig(xwl_screen->egl_display, config_attribs, , 1, );
if (!n) {
ErrorF("No acceptable EGL configs found\n");
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 06/10] xwayland: Add Wayland interfaces check
Introduces a new egl_backend function to let the EGL backend check for
the presence of the required Wayland interfaces.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 28 ++---
hw/xwayland/xwayland-glamor-gbm.c | 14 +
hw/xwayland/xwayland-glamor.c | 11 ++
hw/xwayland/xwayland.h | 7 +++
4 files changed, 52 insertions(+), 8 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index f3fd97e6e..bf74ae329 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -660,6 +660,25 @@ xwl_glamor_eglstream_init_wl_registry(struct xwl_screen
*xwl_screen,
}
}
+static Bool
+xwl_glamor_eglstream_has_wl_interfaces(struct xwl_screen *xwl_screen)
+{
+struct xwl_eglstream_private *xwl_eglstream =
+xwl_eglstream_get(xwl_screen);
+
+if (xwl_eglstream->display == NULL) {
+ErrorF("glamor: 'wl_eglstream_display' not supported\n");
+return FALSE;
+}
+
+if (xwl_eglstream->controller == NULL) {
+ErrorF("glamor: 'wl_eglstream_controller' not supported\n");
+return FALSE;
+}
+
+return TRUE;
+}
+
static inline void
xwl_eglstream_init_shaders(struct xwl_screen *xwl_screen)
{
@@ -819,14 +838,6 @@ xwl_glamor_eglstream_init_screen(struct xwl_screen
*xwl_screen)
xwl_eglstream_get(xwl_screen);
ScreenPtr screen = xwl_screen->screen;
-if (!xwl_eglstream->controller) {
-ErrorF("No eglstream controller was exposed in the wayland registry. "
- "This means your version of nvidia's EGL wayland libraries "
- "are too old, as we require support for this.\n");
-xwl_eglstream_cleanup(xwl_screen);
-return FALSE;
-}
-
/* We can just let glamor handle CreatePixmap */
screen->DestroyPixmap = xwl_glamor_eglstream_destroy_pixmap;
@@ -898,6 +909,7 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
xwl_screen->egl_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
+xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index b03e7ee17..33a576da2 100644
--- a/hw/xwayland/xwayland-glamor-gbm.c
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -746,6 +746,19 @@ xwl_glamor_gbm_init_wl_registry(struct xwl_screen
*xwl_screen,
xwl_screen_set_dmabuf_interface(xwl_screen, id, version);
}
+static Bool
+xwl_glamor_gbm_has_wl_interfaces(struct xwl_screen *xwl_screen)
+{
+struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
+
+if (xwl_gbm->drm == NULL) {
+ErrorF("glamor: 'wl_drm' not supported\n");
+return FALSE;
+}
+
+return TRUE;
+}
+
static Bool
xwl_glamor_gbm_init_egl(struct xwl_screen *xwl_screen)
{
@@ -887,6 +900,7 @@ xwl_glamor_init_gbm(struct xwl_screen *xwl_screen)
xwl_gbm);
xwl_screen->egl_backend.init_wl_registry = xwl_glamor_gbm_init_wl_registry;
+xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_gbm_has_wl_interfaces;
xwl_screen->egl_backend.init_egl = xwl_glamor_gbm_init_egl;
xwl_screen->egl_backend.init_screen = xwl_glamor_gbm_init_screen;
xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_gbm_get_wl_buffer_for_pixmap;
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 14706f6e8..72995de00 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -76,6 +76,17 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
id, interface, version);
}
+Bool
+xwl_glamor_has_wl_interfaces(struct xwl_screen *xwl_screen,
+struct xwl_egl_backend *xwl_egl_backend)
+{
+if (xwl_egl_backend->has_wl_interfaces)
+return xwl_egl_backend->has_wl_interfaces(xwl_screen);
+
+/* If the backend has no requirement wrt WL interfaces, we're fine */
+return TRUE;
+}
+
struct wl_buffer *
xwl_glamor_pixmap_get_wl_buffer(PixmapPtr pixmap,
unsigned short width,
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 6bbe72e46..191f1b297 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -69,6 +69,11 @@ struct xwl_egl_backend {
uint32_t id, const char *name,
[PATCH xserver 08/10] xwayland: refactor EGL backends for wayland registry
To be able to check for availability of the Wayland interfaces required
to run a given EGL backend (either GBM or EGLStream for now), we need
to have each backend structures and vfuncs in place before we enter the
Wayland registry dance.
That basically means that we should init all backends at first, connect
to the Wayland compositor and query the available interfaces and then
decide which backend is available and should be used (or none if either
the Wayland interfaces or the EGL extensions are not available).
For this purpose, hold an egl_backend struct for each backend we are to
consider prior to connect to the Wayland display so that, when we get to
query the Wayland interfaces, everything is in place for each backend to
handle the various Wayland interfaces.
Eventually, when we need to chose which EGL backend to use for glamor,
the available Wayland interfaces and EGL extensions available are all
known to Xwayland.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 37 +-
hw/xwayland/xwayland-glamor-gbm.c | 43 +++
hw/xwayland/xwayland-glamor.c | 94 -
hw/xwayland/xwayland-present.c | 2 +-
hw/xwayland/xwayland.c | 12 ++--
hw/xwayland/xwayland.h | 23 +++---
6 files changed, 148 insertions(+), 63 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index bf74ae329..c226c0089 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -638,7 +638,7 @@ const struct wl_eglstream_display_listener
eglstream_display_listener = {
.swapinterval_override =
xwl_eglstream_display_handle_swapinterval_override,
};
-static void
+static Bool
xwl_glamor_eglstream_init_wl_registry(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
uint32_t id, const char *name,
@@ -654,10 +654,15 @@ xwl_glamor_eglstream_init_wl_registry(struct xwl_screen
*xwl_screen,
wl_eglstream_display_add_listener(xwl_eglstream->display,
_display_listener,
xwl_screen);
+return TRUE;
} else if (strcmp(name, "wl_eglstream_controller") == 0) {
xwl_eglstream->controller = wl_registry_bind(
wl_registry, id, _eglstream_controller_interface, version);
+return TRUE;
}
+
+/* no match */
+return FALSE;
}
static Bool
@@ -882,23 +887,24 @@ out:
return device;
}
-Bool
+void
xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
{
struct xwl_eglstream_private *xwl_eglstream;
EGLDeviceEXT egl_device;
+xwl_screen->eglstream_backend.is_available = FALSE;
egl_device = xwl_eglstream_get_device(xwl_screen);
if (egl_device == EGL_NO_DEVICE_EXT)
-return FALSE;
+return;
if (!dixRegisterPrivateKey(_eglstream_private_key, PRIVATE_SCREEN, 0))
-return FALSE;
+return;
xwl_eglstream = calloc(sizeof(*xwl_eglstream), 1);
if (!xwl_eglstream) {
-ErrorF("Failed to allocate memory required to init eglstream
support\n");
-return FALSE;
+ErrorF("Failed to allocate memory required to init EGLStream
support\n");
+return;
}
dixSetPrivate(_screen->screen->devPrivates,
@@ -907,15 +913,12 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
xwl_eglstream->egl_device = egl_device;
xorg_list_init(_eglstream->pending_streams);
-xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
-xwl_screen->egl_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
-xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
-xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
-xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
-xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
-xwl_screen->egl_backend.allow_commits = xwl_glamor_eglstream_allow_commits;
-
-ErrorF("glamor: Using nvidia's eglstream interface, direct rendering
impossible.\n");
-ErrorF("glamor: Performance may be affected. Ask your vendor to support
GBM!\n");
-return TRUE;
+xwl_screen->eglstream_backend.init_egl = xwl_glamor_eglstream_init_egl;
+xwl_screen->eglstream_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
+xwl_screen->eglstream_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
+xwl_screen->eglstream_backend.init_screen =
xwl_glamor_eglstream_init_screen;
+xwl_screen->eglstream_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_
[PATCH xserver 03/10] xwayland: GBM should fail w/out "GL_OES_EGL_image"
Surely, we should fail to init GBM backend if "GL_OES_EGL_image" is
missing.
This seems to have been lost with commit 1545e2dba ("xwayland: Decouple
GBM from glamor").
Suggested-by: Emil Velikov
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-gbm.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index d6b979561..367e65a9a 100644
--- a/hw/xwayland/xwayland-glamor-gbm.c
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -807,8 +807,10 @@ xwl_glamor_gbm_init_egl(struct xwl_screen *xwl_screen)
goto error;
}
-if (!epoxy_has_gl_extension("GL_OES_EGL_image"))
+if (!epoxy_has_gl_extension("GL_OES_EGL_image")) {
ErrorF("GL_OES_EGL_image not available\n");
+goto error;
+}
if (epoxy_has_egl_extension(xwl_screen->egl_display,
"EXT_image_dma_buf_import") &&
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 07/10] xwayland: move EGL backend init to glamor
Move EGL backends initialization to its own function in
xwayland-glamor.c
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor.c | 17 +
hw/xwayland/xwayland.c| 16 ++--
hw/xwayland/xwayland.h| 2 ++
3 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 72995de00..3792dfa8c 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -162,6 +162,23 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
return 0;
}
+void
+xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstream)
+{
+#ifdef XWL_HAS_EGLSTREAM
+if (use_eglstream) {
+if (!xwl_glamor_init_eglstream(xwl_screen)) {
+ErrorF("xwayland glamor: failed to setup EGLStream backend\n");
+use_eglstream = FALSE;
+}
+}
+#endif
+if (!use_eglstream && !xwl_glamor_init_gbm(xwl_screen)) {
+ErrorF("xwayland glamor: failed to setup GBM backend, falling back to
sw accel\n");
+xwl_screen->glamor = 0;
+}
+}
+
Bool
xwl_glamor_init(struct xwl_screen *xwl_screen)
{
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 806d45675..8c02c02f8 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -992,20 +992,8 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
}
#ifdef XWL_HAS_GLAMOR
-if (xwl_screen->glamor) {
-#ifdef XWL_HAS_EGLSTREAM
-if (use_eglstreams) {
-if (!xwl_glamor_init_eglstream(xwl_screen)) {
-ErrorF("xwayland glamor: failed to setup EGLStream backend\n");
-use_eglstreams = FALSE;
-}
-}
-#endif
-if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) {
-ErrorF("xwayland glamor: failed to setup GBM backend, falling back
to sw accel\n");
-xwl_screen->glamor = 0;
-}
-}
+if (xwl_screen->glamor)
+xwl_glamor_init_backends(xwl_screen, use_eglstreams);
#endif
/* In rootless mode, we don't have any screen storage, and the only
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 191f1b297..304484ccc 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -423,6 +423,8 @@ Bool xwl_shm_destroy_pixmap(PixmapPtr pixmap);
struct wl_buffer *xwl_shm_pixmap_get_wl_buffer(PixmapPtr pixmap);
#ifdef XWL_HAS_GLAMOR
+void xwl_glamor_init_backends(struct xwl_screen *xwl_screen,
+ Bool use_eglstream);
Bool xwl_glamor_init(struct xwl_screen *xwl_screen);
Bool xwl_screen_set_drm_interface(struct xwl_screen *xwl_screen,
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 02/10] xwayland: swap "name" and "id" in init_wl_registry()
Both xwl_glamor_init_wl_registry() and the Wayland global registry
handler use the interface id/name in that order, using name/id in the
egl_backend vfunc makes things confusing and error prone.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 4 ++--
hw/xwayland/xwayland-glamor-gbm.c | 4 ++--
hw/xwayland/xwayland-glamor.c | 2 +-
hw/xwayland/xwayland.h | 2 +-
4 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 89c531f4a..f3fd97e6e 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -641,8 +641,8 @@ const struct wl_eglstream_display_listener
eglstream_display_listener = {
static void
xwl_glamor_eglstream_init_wl_registry(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
- const char *name,
- uint32_t id, uint32_t version)
+ uint32_t id, const char *name,
+ uint32_t version)
{
struct xwl_eglstream_private *xwl_eglstream =
xwl_eglstream_get(xwl_screen);
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index 29325adac..d6b979561 100644
--- a/hw/xwayland/xwayland-glamor-gbm.c
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -737,8 +737,8 @@ xwl_screen_set_dmabuf_interface(struct xwl_screen
*xwl_screen,
static void
xwl_glamor_gbm_init_wl_registry(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
-const char *name,
-uint32_t id, uint32_t version)
+uint32_t id, const char *name,
+uint32_t version)
{
if (strcmp(name, "wl_drm") == 0)
xwl_screen_set_drm_interface(xwl_screen, id, version);
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index c7ae51336..14706f6e8 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -73,7 +73,7 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
{
if (xwl_screen->egl_backend.init_wl_registry)
xwl_screen->egl_backend.init_wl_registry(xwl_screen, registry,
- interface, id, version);
+ id, interface, version);
}
struct wl_buffer *
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 0d4afdf8a..a32fcf6a5 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -117,7 +117,7 @@ struct xwl_screen {
*/
void (*init_wl_registry)(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
- const char *name, uint32_t id,
+ uint32_t id, const char *name,
uint32_t version);
/* Called before glamor has been initialized. Backends should setup a
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 05/10] xwayland: move egl_backend to its own struct
EGL backend availability requires both EGL extensions and Wayland
interfaces to be present, so we will need to consider multiple backends
during initialization.
As a preliminary work, move the egl_backend to its own struct so that we
can have more than one backend at any given time.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland.h | 99 ++
1 file changed, 51 insertions(+), 48 deletions(-)
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index a32fcf6a5..6bbe72e46 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -57,6 +57,56 @@ struct xwl_format {
struct xwl_pixmap;
struct xwl_window;
+struct xwl_screen;
+
+struct xwl_egl_backend {
+/* Called once for each interface in the global registry. Backends
+ * should use this to bind to any wayland interfaces they need. This
+ * callback is optional.
+ */
+void (*init_wl_registry)(struct xwl_screen *xwl_screen,
+ struct wl_registry *wl_registry,
+ uint32_t id, const char *name,
+ uint32_t version);
+
+/* Called before glamor has been initialized. Backends should setup a
+ * valid, glamor compatible EGL context in this hook.
+ */
+Bool (*init_egl)(struct xwl_screen *xwl_screen);
+
+/* Called after glamor has been initialized, and after all of the
+ * common Xwayland DDX hooks have been connected. Backends should use
+ * this to setup any required wraps around X server callbacks like
+ * CreatePixmap.
+ */
+ Bool (*init_screen)(struct xwl_screen *xwl_screen);
+
+ /* Called by Xwayland to retrieve a pointer to a valid wl_buffer for
+ * the given window/pixmap combo so that damage to the pixmap may be
+ * displayed on-screen. Backends should use this to create a new
+ * wl_buffer for a currently buffer-less pixmap, or simply return the
+ * pixmap they've prepared beforehand.
+ */
+ struct wl_buffer *(*get_wl_buffer_for_pixmap)(PixmapPtr pixmap,
+ unsigned short width,
+ unsigned short height,
+ Bool *created);
+
+ /* Called by Xwayland to perform any pre-wl_surface damage routines
+ * that are required by the backend. If your backend is poorly
+ * designed and lacks the ability to render directly to a surface,
+ * you should implement blitting from the glamor pixmap to the wayland
+ * pixmap here. Otherwise, this callback is optional.
+ */
+ void (*post_damage)(struct xwl_window *xwl_window,
+ PixmapPtr pixmap, RegionPtr region);
+
+ /* Called by Xwayland to confirm with the egl backend that the given
+ * pixmap is completely setup and ready for display on-screen. This
+ * callback is optional.
+ */
+ Bool (*allow_commits)(struct xwl_window *xwl_window);
+};
struct xwl_screen {
int width;
@@ -110,54 +160,7 @@ struct xwl_screen {
void *egl_display, *egl_context;
/* the current backend for creating pixmaps on wayland */
-struct {
-/* Called once for each interface in the global registry. Backends
- * should use this to bind to any wayland interfaces they need. This
- * callback is optional.
- */
-void (*init_wl_registry)(struct xwl_screen *xwl_screen,
- struct wl_registry *wl_registry,
- uint32_t id, const char *name,
- uint32_t version);
-
-/* Called before glamor has been initialized. Backends should setup a
- * valid, glamor compatible EGL context in this hook.
- */
-Bool (*init_egl)(struct xwl_screen *xwl_screen);
-
-/* Called after glamor has been initialized, and after all of the
- * common Xwayland DDX hooks have been connected. Backends should use
- * this to setup any required wraps around X server callbacks like
- * CreatePixmap.
- */
-Bool (*init_screen)(struct xwl_screen *xwl_screen);
-
-/* Called by Xwayland to retrieve a pointer to a valid wl_buffer for
- * the given window/pixmap combo so that damage to the pixmap may be
- * displayed on-screen. Backends should use this to create a new
- * wl_buffer for a currently buffer-less pixmap, or simply return the
- * pixmap they've prepared beforehand.
- */
-struct wl_buffer *(*get_wl_buffer_for_pixmap)(PixmapPtr pixmap,
- unsigned short width,
- unsigned short height,
- Bool *created);
-
-/* Called by Xwayland to perform any pre-wl_surface damage routines
[PATCH xserver 01/10] xwayland: move glamor specific routines
Functions such as:
xwl_glamor_egl_supports_device_probing()
xwl_glamor_egl_get_devices()
xwl_glamor_egl_device_has_egl_extensions()
Are of no use outside of EGLStream support, move them to the relevant
source file.
Similarly, the other glamor functions such as:
xwl_glamor_init()
xwl_screen_set_drm_interface()
xwl_screen_set_dmabuf_interface()
xwl_glamor_pixmap_get_wl_buffer()
xwl_glamor_init_wl_registry()
xwl_glamor_post_damage()
xwl_glamor_allow_commits()
xwl_glamor_egl_make_current()
Are useless without glamor support enabled, move those within a
a "#ifdef XWL_HAS_GLAMOR" in xwayland.h
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 79
hw/xwayland/xwayland-glamor.c | 80 -
hw/xwayland/xwayland.h | 24
3 files changed, 89 insertions(+), 94 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 8dd1cc304..89c531f4a 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -187,6 +187,85 @@ xwl_eglstream_cleanup(struct xwl_screen *xwl_screen)
free(xwl_eglstream);
}
+static Bool
+xwl_glamor_egl_supports_device_probing(void)
+{
+return epoxy_has_egl_extension(NULL, "EGL_EXT_device_base");
+}
+
+static void **
+xwl_glamor_egl_get_devices(int *num_devices)
+{
+EGLDeviceEXT *devices;
+Bool ret;
+int drm_dev_count = 0;
+int i;
+
+if (!xwl_glamor_egl_supports_device_probing())
+return NULL;
+
+/* Get the number of devices */
+ret = eglQueryDevicesEXT(0, NULL, num_devices);
+if (!ret || *num_devices < 1)
+return NULL;
+
+devices = calloc(*num_devices, sizeof(EGLDeviceEXT));
+if (!devices)
+return NULL;
+
+ret = eglQueryDevicesEXT(*num_devices, devices, num_devices);
+if (!ret)
+goto error;
+
+/* We're only ever going to care about devices that support
+ * EGL_EXT_device_drm, so filter out the ones that don't
+ */
+for (i = 0; i < *num_devices; i++) {
+const char *extension_str =
+eglQueryDeviceStringEXT(devices[i], EGL_EXTENSIONS);
+
+if (!epoxy_extension_in_string(extension_str, "EGL_EXT_device_drm"))
+continue;
+
+devices[drm_dev_count++] = devices[i];
+}
+if (!drm_dev_count)
+goto error;
+
+*num_devices = drm_dev_count;
+devices = realloc(devices, sizeof(EGLDeviceEXT) * drm_dev_count);
+
+return devices;
+
+error:
+free(devices);
+
+return NULL;
+}
+
+static Bool
+xwl_glamor_egl_device_has_egl_extensions(void *device,
+ const char **ext_list, size_t size)
+{
+EGLDisplay egl_display;
+int i;
+Bool has_exts = TRUE;
+
+egl_display = glamor_egl_get_display(EGL_PLATFORM_DEVICE_EXT, device);
+if (!egl_display || !eglInitialize(egl_display, NULL, NULL))
+return FALSE;
+
+for (i = 0; i < size; i++) {
+if (!epoxy_has_egl_extension(egl_display, ext_list[i])) {
+has_exts = FALSE;
+break;
+}
+}
+
+eglTerminate(egl_display);
+return has_exts;
+}
+
static void
xwl_eglstream_unref_pixmap_stream(struct xwl_pixmap *xwl_pixmap)
{
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index f543f321d..c7ae51336 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -52,86 +52,6 @@ xwl_glamor_egl_make_current(struct xwl_screen *xwl_screen)
xwl_screen->glamor_ctx->make_current(xwl_screen->glamor_ctx);
}
-Bool
-xwl_glamor_egl_supports_device_probing(void)
-{
-return epoxy_has_egl_extension(NULL, "EGL_EXT_device_base");
-}
-
-void **
-xwl_glamor_egl_get_devices(int *num_devices)
-{
-#ifdef XWL_HAS_EGLSTREAM
-EGLDeviceEXT *devices;
-Bool ret;
-int drm_dev_count = 0;
-int i;
-
-if (!xwl_glamor_egl_supports_device_probing())
-return NULL;
-
-/* Get the number of devices */
-ret = eglQueryDevicesEXT(0, NULL, num_devices);
-if (!ret || *num_devices < 1)
-return NULL;
-
-devices = calloc(*num_devices, sizeof(EGLDeviceEXT));
-if (!devices)
-return NULL;
-
-ret = eglQueryDevicesEXT(*num_devices, devices, num_devices);
-if (!ret)
-goto error;
-
-/* We're only ever going to care about devices that support
- * EGL_EXT_device_drm, so filter out the ones that don't
- */
-for (i = 0; i < *num_devices; i++) {
-const char *extension_str =
-eglQueryDeviceStringEXT(devices[i], EGL_EXTENSIONS);
-
-if (!epoxy_extension_in_string(extension_str, "EGL_EXT_device_drm"))
-continue;
-
-devices[drm_dev_count++] = devices[i];
-}
-if (!drm_dev_count)
-goto error;
-
-*num_devices = drm_dev_count;
-devic
[PATCH xserver 04/10] xwayland: skip drm authentication with render node
If using a render node, we can skip DRM authentication.
Suggested-by: Emil Velikov
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-gbm.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index 367e65a9a..b03e7ee17 100644
--- a/hw/xwayland/xwayland-glamor-gbm.c
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -837,11 +837,16 @@ error:
static Bool
xwl_glamor_gbm_init_screen(struct xwl_screen *xwl_screen)
{
+struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
+
if (!dri3_screen_init(xwl_screen->screen, _dri3_info)) {
ErrorF("Failed to initialize dri3\n");
goto error;
}
+if (xwl_gbm->fd_render_node)
+goto skip_drm_auth;
+
if (!dixRegisterPrivateKey(_auth_state_private_key, PRIVATE_CLIENT,
0)) {
ErrorF("Failed to register private key\n");
@@ -854,6 +859,7 @@ xwl_glamor_gbm_init_screen(struct xwl_screen *xwl_screen)
goto error;
}
+skip_drm_auth:
xwl_screen->screen->CreatePixmap = xwl_glamor_gbm_create_pixmap;
xwl_screen->screen->DestroyPixmap = xwl_glamor_gbm_destroy_pixmap;
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 5/5] xwayland: make xwl_output_get_xdg_output() static
Make xwl_output_get_xdg_output() private, it doesn't need to be
available elsewhere.
Signed-off-by: Olivier Fourdan
Reviewed-by: Lyude Paul
Reviewed-by: Emil Velikov
---
hw/xwayland/xwayland-output.c | 4 +++-
hw/xwayland/xwayland.h| 1 -
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
index 48faeb142..379062549 100644
--- a/hw/xwayland/xwayland-output.c
+++ b/hw/xwayland/xwayland-output.c
@@ -38,6 +38,8 @@
RR_Reflect_X | \
RR_Reflect_Y)
+static void xwl_output_get_xdg_output(struct xwl_output *xwl_output);
+
static Rotation
wl_transform_to_xrandr(enum wl_output_transform transform)
{
@@ -435,7 +437,7 @@ xwl_screen_init_output(struct xwl_screen *xwl_screen)
return TRUE;
}
-void
+static void
xwl_output_get_xdg_output(struct xwl_output *xwl_output)
{
struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 25112e2cb..39bc20a7e 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -440,7 +440,6 @@ void xwl_present_cleanup(WindowPtr window);
void xwl_screen_release_tablet_manager(struct xwl_screen *xwl_screen);
-void xwl_output_get_xdg_output(struct xwl_output *xwl_output);
void xwl_screen_init_xdg_output(struct xwl_screen *xwl_screen);
void xwl_glamor_egl_make_current(struct xwl_screen *xwl_screen);
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 4/5] xwayland: do not disable glamor if EGLStream failed
EGLStream requires glamor, but the opposite is not true. So if someone
passes "-eglstream" with a GPU which does not support EGLStream, we
could maybe still try GBM and be lucky.
That allows Wayland compositors to pass "-eglstream" regardless of the
actual hardware, if they want to enable EGLStream on GPU which support
it.
Signed-off-by: Olivier Fourdan
Reviewed-by: Lyude Paul
Reviewed-by: Emil Velikov
---
hw/xwayland/xwayland.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 9121ef666..806d45675 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -939,9 +939,7 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
struct xwl_screen *xwl_screen;
Pixel red_mask, blue_mask, green_mask;
int ret, bpc, green_bpc, i;
-#ifdef XWL_HAS_EGLSTREAM
Bool use_eglstreams = FALSE;
-#endif
xwl_screen = calloc(1, sizeof *xwl_screen);
if (xwl_screen == NULL)
@@ -998,12 +996,12 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
#ifdef XWL_HAS_EGLSTREAM
if (use_eglstreams) {
if (!xwl_glamor_init_eglstream(xwl_screen)) {
-ErrorF("xwayland glamor: failed to setup eglstream backend,
falling back to swaccel\n");
-xwl_screen->glamor = 0;
+ErrorF("xwayland glamor: failed to setup EGLStream backend\n");
+use_eglstreams = FALSE;
}
-} else
+}
#endif
-if (!xwl_glamor_init_gbm(xwl_screen)) {
+if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) {
ErrorF("xwayland glamor: failed to setup GBM backend, falling back
to sw accel\n");
xwl_screen->glamor = 0;
}
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 00/10] Re: Refactor egl_backends for wayland registry
Hi, This is a follow-up on https://patchwork.freedesktop.org/series/44095/ after Lyude and Emil reviews. Cheers, Olivier Olivier Fourdan (10): xwayland: move glamor specific routines xwayland: swap "name" and "id" in init_wl_registry() xwayland: GBM should fail w/out "GL_OES_EGL_image" xwayland: skip drm authentication with render node xwayland: move egl_backend to its own struct xwayland: Add Wayland interfaces check xwayland: move EGL backend init to glamor xwayland: refactor EGL backends for wayland registry xwayland: check for EGLStream backend explicitly xwayland: EGL_IMG_context_priority required by EGLStream hw/xwayland/xwayland-glamor-eglstream.c | 152 +++ hw/xwayland/xwayland-glamor-gbm.c | 69 +++-- hw/xwayland/xwayland-glamor.c | 186 hw/xwayland/xwayland-present.c | 5 +- hw/xwayland/xwayland.c | 28 ++-- hw/xwayland/xwayland.h | 151 ++- 6 files changed, 369 insertions(+), 222 deletions(-) -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 3/5] xwayland: process Wayland events after adding screen
When we're done adding a new screen, we need to process any pending Wayland events again. Hence we don't end up processing xdg_output events unexpectedly when glamor is disabled. Be that because "-shm" was passed or "-eglstream" has failed. Failing to do that could lead to a crash at startup: Xwayland: dixGetPrivateAddr: Assertion `key->initialized' failed. (EE) (EE) Backtrace: (EE) 0: Xwayland (OsSigHandler) (EE) 1: libpthread.so.0 (funlockfile) (EE) 2: libc.so.6 (gsignal) (EE) 3: libc.so.6 (abort) (EE) 4: libc.so.6 (?+0x0) (EE) 5: libc.so.6 (__assert_fail) (EE) 6: Xwayland (dixGetPrivateAddr) (EE) 7: Xwayland (_fbGetWindowPixmap) (EE) 8: Xwayland (getDrawableDamageRef) (EE) 9: Xwayland (damageRegionProcessPending) (EE) 10: Xwayland (damagePolyFillRect) (EE) 11: Xwayland (miPaintWindow) (EE) 12: Xwayland (miWindowExposures) (EE) 13: Xwayland (miHandleValidateExposures) (EE) 14: Xwayland (SetRootClip) (EE) 15: Xwayland (update_screen_size) (EE) 16: Xwayland (apply_output_change) (EE) 17: libffi.so.6 (ffi_call_unix64) (EE) 18: libffi.so.6 (ffi_call) (EE) 19: libwayland-client.so.0 (wl_log_set_handler_client) (EE) 20: libwayland-client.so.0 (_init) (EE) 21: libwayland-client.so.0 (wl_display_dispatch_queue_pending) (EE) 22: libwayland-client.so.0 (wl_display_roundtrip_queue) (EE) 23: Xwayland (InitInput) (EE) 24: Xwayland (dix_main) (EE) 25: libc.so.6 (__libc_start_main) (EE) 26: Xwayland (_start) (EE) (EE) Fatal server error: (EE) Caught signal 6 (Aborted). Server aborting (EE) Aborted (core dumped) Signed-off-by: Olivier Fourdan Reviewed-by: Lyude Paul Reviewed-by: Emil Velikov --- hw/xwayland/xwayland.c | 4 1 file changed, 4 insertions(+) diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c index d9548a874..9121ef666 100644 --- a/hw/xwayland/xwayland.c +++ b/hw/xwayland/xwayland.c @@ -1132,6 +1132,10 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv) AddCallback(, xwl_property_callback, pScreen); +wl_display_roundtrip(xwl_screen->display); +while (xwl_screen->expecting_event) +wl_display_roundtrip(xwl_screen->display); + return ret; } -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/5] xwayland: "EGL_EXT_device_base" required for EGLStream
eglQueryDevicesEXT() would abort if the required extensions are not available, meaning that enabling “-eglstream” on a non-EGLStream capable hardware would lead to an abort(). Check that "EGL_EXT_device_base" extension is available and bail out early if not, so we don't abort() later in eglQueryDevicesEXT(). Signed-off-by: Olivier Fourdan Reviewed-by: Lyude Paul Reviewed-by: Emil Velikov --- hw/xwayland/xwayland-glamor.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c index cdca072ed..f543f321d 100644 --- a/hw/xwayland/xwayland-glamor.c +++ b/hw/xwayland/xwayland-glamor.c @@ -67,6 +67,9 @@ xwl_glamor_egl_get_devices(int *num_devices) int drm_dev_count = 0; int i; +if (!xwl_glamor_egl_supports_device_probing()) +return NULL; + /* Get the number of devices */ ret = eglQueryDevicesEXT(0, NULL, num_devices); if (!ret || *num_devices < 1) -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/5] xwayland: allow "-eglstream" option
The command line option "-eglstream" used to enable EGLStream support
for NVidia GPU was made available only when Xwayland was built with
EGLStream support enabled.
Wayland compositors who spawn Xwayland have no easy way to tell whether
or not Xwayland was built with EGLStream support enabled, and adding
"-eglstream" command line option to Xwayland when it wasn't built with
EGLStream support would prevent Xwayland from starting (“Unrecognized
option” error).
Make sure we support the command line option "-eglstream" regardless of
EGLStream support in Xwayland. Obviously, if Xwayland was built without
EGLStream support, this has no effect.
Signed-off-by: Olivier Fourdan
Reviewed-by: Lyude Paul
Reviewed-by: Emil Velikov
---
hw/xwayland/xwayland.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index 1d6b49979..d9548a874 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -96,9 +96,7 @@ ddxUseMsg(void)
ErrorF("-rootless run rootless, requires wm support\n");
ErrorF("-wm fd create X client for wm on given fd\n");
ErrorF("-listen fd add give fd as a listen socket\n");
-#ifdef XWL_HAS_EGLSTREAM
ErrorF("-eglstream use eglstream backend for nvidia GPUs\n");
-#endif
}
int
@@ -117,11 +115,9 @@ ddxProcessArgument(int argc, char *argv[], int i)
else if (strcmp(argv[i], "-shm") == 0) {
return 1;
}
-#ifdef XWL_HAS_EGLSTREAM
else if (strcmp(argv[i], "-eglstream") == 0) {
return 1;
}
-#endif
return 0;
}
@@ -988,11 +984,13 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
else if (strcmp(argv[i], "-shm") == 0) {
xwl_screen->glamor = 0;
}
-#ifdef XWL_HAS_EGLSTREAM
else if (strcmp(argv[i], "-eglstream") == 0) {
+#ifdef XWL_HAS_EGLSTREAM
use_eglstreams = TRUE;
-}
+#else
+ErrorF("xwayland glamor: this build does not have EGLStream
support\n");
#endif
+}
}
#ifdef XWL_HAS_GLAMOR
--
2.17.1
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 0/5] Re: Xwayland fixes wrt eglstream support
Hi, This is a follow-up on https://patchwork.freedesktop.org/series/43704/ after Lyude and Emil reviews. Emil, for 1/5, I changed the error message if eglstream wasn't enabled at build time to: "xwayland glamor: this build does not have eglstream support" which sounds to me like the most accurate description :) I rephrased the commit messages in 1/3 and in 5/5 as per your wording. I also fixed a few typos in the commit messages and added the R-b from both Lyude and yourself. Cheers, Olivier Olivier Fourdan (5): xwayland: allow "-eglstream" option xwayland: "EGL_EXT_device_base" required for EGLStream xwayland: process Wayland events after adding screen xwayland: do not disable glamor if EGLStream failed xwayland: make xwl_output_get_xdg_output() static hw/xwayland/xwayland-glamor.c | 3 +++ hw/xwayland/xwayland-output.c | 4 +++- hw/xwayland/xwayland.c| 24 hw/xwayland/xwayland.h| 1 - 4 files changed, 18 insertions(+), 14 deletions(-) -- 2.17.1 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
Hi Emil,
Many thanks for your detailed review!
On Tue, Jun 5, 2018 at 12:37 PM, Emil Velikov wrote:
> Hi Olivier,
>
> There's a handful of mostly trivial suggestions below. The idea itself seems
> reasonable IMHO. One gripe is that we're 'leaking' twice as much as before.
>
> Namely: even if the current backend cleans-up after itself (it some cases it
> does not), the other backend 'leaks'. Not sure if/how much we should care in
> that case.
>
> Was skimming if we cannot move more init_egl/init_screen tripping points
> and I've noticed a few gotchas/bugs. None of which are requirement for the
> series, although great to have.
>
>
> xwl_glamor_gbm_init_egl
> - if !render_node error path is leaking
humm, not sure, leaking what? we haven't allocated anything yet, have we?
But anyhow, it's unralted to this refactoring I think.
> - surely we'd want to error out when GL_OES_EGL_image is missing
> xwl_glamor_gbm_init_screen
Sure, will add separate patch for that, seems it got lost with commit
1545e2dba ("xwayland: Decouple GBM from glamor").
> - no need to RegisterPrivateKey/AddCallback if a rendernode is opened
Yeap, will add a separate patch for this.
> xwl_glamor_eglstream_init_egl
> - using EGL_IMG_context_priority w/o checking for it
> xwl_glamor_eglstream_init_screen
Ditto.
> - the ->controller check is no longer needed
Ditto.
>> +static Bool
>> +xwl_glamor_gbm_has_egl_extension(void)
>> +{
>> +return epoxy_has_egl_extension(NULL, "EGL_MESA_platform_gbm");
> I'd also check for EGL_KHR_platform_gbm - it's trivial enough ;-)
Sure!
>> @@ -71,9 +71,24 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
>> uint32_t id, const char *interface,
>> uint32_t version)
>> {
>> -if (xwl_screen->egl_backend.init_wl_registry)
>> -xwl_screen->egl_backend.init_wl_registry(xwl_screen, registry,
>> - interface, id, version);
>> +#ifdef GLAMOR_HAS_GBM
>> +if (xwl_screen->gbm_backend.is_available &&
>> +xwl_screen->gbm_backend.init_wl_registry &&
>> +xwl_screen->gbm_backend.init_wl_registry(xwl_screen,
>> + registry,
>> + id,
>> + interface,
>> + version)); /* no-op */
>> +#endif
>> +#ifdef XWL_HAS_EGLSTREAM
>> +else if (xwl_screen->eglstreams_backend.is_available &&
>> + xwl_screen->eglstreams_backend.init_wl_registry &&
>> + xwl_screen->eglstreams_backend.init_wl_registry(xwl_screen,
>> + registry,
>> + id,
>> + interface,
>> + version)); /*
>> no-op */
>> +#endif
> Both ifdef guards can go.
Well, the idea was to save a few conditionals if the support was not
enabled at build time, but I can certainly remove those ifdefs.
>> @@ -119,8 +134,8 @@ xwl_glamor_allow_commits(struct xwl_window *xwl_window)
>> {
>> struct xwl_screen *xwl_screen = xwl_window->xwl_screen;
>>
>> -if (xwl_screen->egl_backend.allow_commits)
>> -return xwl_screen->egl_backend.allow_commits(xwl_window);
>> +if (xwl_screen->egl_backend && xwl_screen->egl_backend->allow_commits)
> Why the NULL check? Unless I'm missing something that will never happen.
We can have no “egl_backend” set at all if “-shm” was specified.
Either we keep that test here or we need to check for
"xwl_screen->glamor" in xwl_screen_post_damage(), I'll change for the
later for clarity.
>> @@ -165,17 +180,35 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
>> void
>> xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstreams)
>> {
>
> General nit:
> Drop the return type of xwl_glamor_init_$backend now that we check
> ::is_available.
Sure, it's unused anyway.
> Instead of relying on the coded probe order (alongside the
> is_available = false workaround),
> we could use -eglstream to control which backend is checked/probed first.
>
> If that fails we fall-back to the other.
>
> If that sounds reasonable, then the following can be reworked as follows:
Sounds reasonable.
>> +#ifdef GLAMOR_HAS_GBM
>> +/* Init GBM even if "-eglstream" is requested, as EGL streams may fail
>> */
>> +xwl_glamor_init_gbm(xwl_screen);
> Here we add:
>
> if (!xwl_screen->gbm_backend.is_available && !use_eglstreams)
> ErrorF(xwayland glamor: GBM (default) is not available);
>
>> +#endif
>> #ifdef XWL_HAS_EGLSTREAM
>> +xwl_glamor_init_eglstream(xwl_screen);
>
>> if (use_eglstreams) {
>> -if (!xwl_glamor_init_eglstream(xwl_screen)) {
>> -ErrorF("xwayland glamor: failed to setup eglstream backend\n");
Re: [PATCH xserver 1/5] xwayland: Allow "-eglstream" option
Hi Emil,
On 5 June 2018 at 12:41, Emil Velikov wrote:
> [...]
>
>> > +#else
> >> > +ErrorF("xwayland glamor: eglstream backend support not
> >> > enabled\n");
> >> Something is really weird here:
> >>
> >> On one hand '-eglstream' is recognised and used (by potential user) on
> >> the other "... support is not _enabled_" is printed.
> >> Surely you meant "not built", right? After all explicitly passing the
> >> enable (runtime) flag should be enough to enable it ;-)
> >
> >
> > Yes, I literally mean "enabled at build time".
> >
> This wording is even better.
>
Eventually, in the updated series I am about to post in a minute, I opted
for the following wording:
"xwayland glamor: this build does not have eglstream support"
which, I reckon, seems like the most accurate description :)
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH xserver 1/5] xwayland: Allow "-eglstream" option
Hi
On 4 June 2018 at 16:24, Emil Velikov wrote:
> On 24 May 2018 at 15:10, Olivier Fourdan wrote:
> > The command line option "-eglstream" used to enable EGLi stream support
> > for NVidia GPU was made available only when Xwayland was built with EGL
> > stream support enabled.
> >
> > Wayland compositors who spawn Xwayland have no easy way to tell whether
> > or not Xwayland was built with EGL stream support enabled, and adding
> > "-eglstream" command line option to Xwayland when it wasn't built with
> > EGL support would prevent Xwayland from starting (“Unrecognized option”
> > error).
> >
> > Make sure we support the command line option "-eglstream" regardless of
> > EGL stream support in Xwayland, obviously without EGL stream support
> > this has no effect.
> >
> > Signed-off-by: Olivier Fourdan
> > ---
> > hw/xwayland/xwayland.c | 10 --
> > 1 file changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
> > index 1d6b49979..b4049d2cc 100644
> > --- a/hw/xwayland/xwayland.c
> > +++ b/hw/xwayland/xwayland.c
> > @@ -96,9 +96,7 @@ ddxUseMsg(void)
> > ErrorF("-rootless run rootless, requires wm
> support\n");
> > ErrorF("-wm fd create X client for wm on given
> fd\n");
> > ErrorF("-listen fd add give fd as a listen socket\n");
> > -#ifdef XWL_HAS_EGLSTREAM
> > ErrorF("-eglstream use eglstream backend for nvidia
> GPUs\n");
> > -#endif
> > }
> >
> > int
> > @@ -117,11 +115,9 @@ ddxProcessArgument(int argc, char *argv[], int i)
> > else if (strcmp(argv[i], "-shm") == 0) {
> > return 1;
> > }
> > -#ifdef XWL_HAS_EGLSTREAM
> > else if (strcmp(argv[i], "-eglstream") == 0) {
> > return 1;
> > }
> > -#endif
> >
> > return 0;
> > }
> > @@ -988,11 +984,13 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char
> **argv)
> > else if (strcmp(argv[i], "-shm") == 0) {
> > xwl_screen->glamor = 0;
> > }
> > -#ifdef XWL_HAS_EGLSTREAM
> > else if (strcmp(argv[i], "-eglstream") == 0) {
> > +#ifdef XWL_HAS_EGLSTREAM
> > use_eglstreams = TRUE;
> > -}
> > +#else
> > +ErrorF("xwayland glamor: eglstream backend support not
> enabled\n");
> Something is really weird here:
>
> On one hand '-eglstream' is recognised and used (by potential user) on
> the other "... support is not _enabled_" is printed.
> Surely you meant "not built", right? After all explicitly passing the
> enable (runtime) flag should be enough to enable it ;-)
>
Yes, I literally mean "enabled at build time".
Cheers,
Olivier
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 2/5] xwayland: move egl_backend to its own struct
EGL backend availability requires both EGL extensions and Wayland
interfaces to be present, so we will need to consider multiple backends
during initialization.
As a preliminary work, move the egl_backend to its own struct so that we
can have more than one backend at any given time.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland.h | 99 ++
1 file changed, 51 insertions(+), 48 deletions(-)
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 0d4afdf8a..0d6a4c246 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -57,6 +57,56 @@ struct xwl_format {
struct xwl_pixmap;
struct xwl_window;
+struct xwl_screen;
+
+struct xwl_egl_backend {
+/* Called once for each interface in the global registry. Backends
+ * should use this to bind to any wayland interfaces they need. This
+ * callback is optional.
+ */
+void (*init_wl_registry)(struct xwl_screen *xwl_screen,
+ struct wl_registry *wl_registry,
+ const char *name, uint32_t id,
+ uint32_t version);
+
+/* Called before glamor has been initialized. Backends should setup a
+ * valid, glamor compatible EGL context in this hook.
+ */
+Bool (*init_egl)(struct xwl_screen *xwl_screen);
+
+/* Called after glamor has been initialized, and after all of the
+ * common Xwayland DDX hooks have been connected. Backends should use
+ * this to setup any required wraps around X server callbacks like
+ * CreatePixmap.
+ */
+ Bool (*init_screen)(struct xwl_screen *xwl_screen);
+
+ /* Called by Xwayland to retrieve a pointer to a valid wl_buffer for
+ * the given window/pixmap combo so that damage to the pixmap may be
+ * displayed on-screen. Backends should use this to create a new
+ * wl_buffer for a currently buffer-less pixmap, or simply return the
+ * pixmap they've prepared beforehand.
+ */
+ struct wl_buffer *(*get_wl_buffer_for_pixmap)(PixmapPtr pixmap,
+ unsigned short width,
+ unsigned short height,
+ Bool *created);
+
+ /* Called by Xwayland to perform any pre-wl_surface damage routines
+ * that are required by the backend. If your backend is poorly
+ * designed and lacks the ability to render directly to a surface,
+ * you should implement blitting from the glamor pixmap to the wayland
+ * pixmap here. Otherwise, this callback is optional.
+ */
+ void (*post_damage)(struct xwl_window *xwl_window,
+ PixmapPtr pixmap, RegionPtr region);
+
+ /* Called by Xwayland to confirm with the egl backend that the given
+ * pixmap is completely setup and ready for display on-screen. This
+ * callback is optional.
+ */
+ Bool (*allow_commits)(struct xwl_window *xwl_window);
+};
struct xwl_screen {
int width;
@@ -110,54 +160,7 @@ struct xwl_screen {
void *egl_display, *egl_context;
/* the current backend for creating pixmaps on wayland */
-struct {
-/* Called once for each interface in the global registry. Backends
- * should use this to bind to any wayland interfaces they need. This
- * callback is optional.
- */
-void (*init_wl_registry)(struct xwl_screen *xwl_screen,
- struct wl_registry *wl_registry,
- const char *name, uint32_t id,
- uint32_t version);
-
-/* Called before glamor has been initialized. Backends should setup a
- * valid, glamor compatible EGL context in this hook.
- */
-Bool (*init_egl)(struct xwl_screen *xwl_screen);
-
-/* Called after glamor has been initialized, and after all of the
- * common Xwayland DDX hooks have been connected. Backends should use
- * this to setup any required wraps around X server callbacks like
- * CreatePixmap.
- */
-Bool (*init_screen)(struct xwl_screen *xwl_screen);
-
-/* Called by Xwayland to retrieve a pointer to a valid wl_buffer for
- * the given window/pixmap combo so that damage to the pixmap may be
- * displayed on-screen. Backends should use this to create a new
- * wl_buffer for a currently buffer-less pixmap, or simply return the
- * pixmap they've prepared beforehand.
- */
-struct wl_buffer *(*get_wl_buffer_for_pixmap)(PixmapPtr pixmap,
- unsigned short width,
- unsigned short height,
- Bool *created);
-
-/* Called by Xwayland to perform any pre-wl_surface damage routines
[PATCH xserver 4/5] xwayland: Move egl_backends init to glamor
Move EGL backends initialization and choice to its own function in
xwayland-glamor.c
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor.c | 17 +
hw/xwayland/xwayland.c| 16 ++--
hw/xwayland/xwayland.h| 2 ++
3 files changed, 21 insertions(+), 14 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index 8e858394a..eb8ebf032 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -162,6 +162,23 @@ glamor_egl_fd_name_from_pixmap(ScreenPtr screen,
return 0;
}
+void
+xwl_glamor_init_backends(struct xwl_screen *xwl_screen, Bool use_eglstreams)
+{
+#ifdef XWL_HAS_EGLSTREAM
+if (use_eglstreams) {
+if (!xwl_glamor_init_eglstream(xwl_screen)) {
+ErrorF("xwayland glamor: failed to setup eglstream backend\n");
+use_eglstreams = FALSE;
+}
+}
+#endif
+if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) {
+ErrorF("xwayland glamor: failed to setup GBM backend, falling back to
sw accel\n");
+xwl_screen->glamor = 0;
+}
+}
+
Bool
xwl_glamor_init(struct xwl_screen *xwl_screen)
{
diff --git a/hw/xwayland/xwayland.c b/hw/xwayland/xwayland.c
index a08d58451..0584b53e6 100644
--- a/hw/xwayland/xwayland.c
+++ b/hw/xwayland/xwayland.c
@@ -992,20 +992,8 @@ xwl_screen_init(ScreenPtr pScreen, int argc, char **argv)
}
#ifdef XWL_HAS_GLAMOR
-if (xwl_screen->glamor) {
-#ifdef XWL_HAS_EGLSTREAM
-if (use_eglstreams) {
-if (!xwl_glamor_init_eglstream(xwl_screen)) {
-ErrorF("xwayland glamor: failed to setup eglstream backend\n");
-use_eglstreams = FALSE;
-}
-}
-#endif
-if (!use_eglstreams && !xwl_glamor_init_gbm(xwl_screen)) {
-ErrorF("xwayland glamor: failed to setup GBM backend, falling back
to sw accel\n");
-xwl_screen->glamor = 0;
-}
-}
+if (xwl_screen->glamor)
+xwl_glamor_init_backends(xwl_screen, use_eglstreams);
#endif
/* In rootless mode, we don't have any screen storage, and the only
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 674be3d49..40918e87b 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -423,6 +423,8 @@ Bool xwl_shm_destroy_pixmap(PixmapPtr pixmap);
struct wl_buffer *xwl_shm_pixmap_get_wl_buffer(PixmapPtr pixmap);
#ifdef XWL_HAS_GLAMOR
+void xwl_glamor_init_backends(struct xwl_screen *xwl_screen,
+ Bool use_eglstreams);
Bool xwl_glamor_init(struct xwl_screen *xwl_screen);
Bool xwl_screen_set_drm_interface(struct xwl_screen *xwl_screen,
--
2.17.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 0/5] Refactor egl_backends for wayland registry
Hi all, The idea is to be able to use EGL Streams if we can, without requiring the compositor to add "-eglstream" to the Xwayland command line, Xwayland would automatically try EGL streams backend if it can. To be able to use an EGL backend, we need: - The required EGL extension(s) to be supported - The required Wayland interface(s) to be available. The former is pretty easy, but the later requires to be connected to the Wayland display to query the Wayland registry. As the Wayland interfaces depend on the EGL backend being considered, we need to initialize all the backends, based on the EGL extensions available, and eventually choose which one to use based on the Wayland interfaces availability. The following patch series does that, by refactoring the existing code to have multiple EGL backends at first and be able to point to the one to use eventually. With this, if both GBM and EGL streams are available and usable, we would select GBM, but if GBM is not available or not usable, we would automatically try EGL streams, if available. And failing that, we would use SW. That makes the use of the command line "-eglstream" optional, yet if specified, it forces the EGL stream backend as before (and fallback to SW if EGL stream is not available, again, just as before). This new patch series applies on top of the previously submitted series: https://patchwork.freedesktop.org/series/43704/ And supersedes the previous patch series: https://patchwork.freedesktop.org/series/43783/ https://patchwork.freedesktop.org/series/43931/ Which will be marked as such in patchwork. Cheers, Olivier Olivier Fourdan (5): xwayland: Move glamor specific routines xwayland: move egl_backend to its own struct xwayland: Add Wayland interfaces check xwayland: Move egl_backends init to glamor xwayland: refactor egl_backends for wayland registry hw/xwayland/xwayland-glamor-eglstream.c | 124 -- hw/xwayland/xwayland-glamor-gbm.c | 50 ++-- hw/xwayland/xwayland-glamor.c | 163 +++- hw/xwayland/xwayland-present.c | 7 +- hw/xwayland/xwayland.c | 26 ++-- hw/xwayland/xwayland.h | 141 +++- 6 files changed, 319 insertions(+), 192 deletions(-) -- 2.17.0 ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 1/5] xwayland: Move glamor specific routines
Functions such as:
xwl_glamor_egl_supports_device_probing()
xwl_glamor_egl_get_devices()
xwl_glamor_egl_device_has_egl_extensions()
Are of no use outside of EGLstream support, move them to the relevant
source file.
Similarly, the other glamor function ssuch as:
xwl_glamor_init()
xwl_screen_set_drm_interface()
xwl_screen_set_dmabuf_interface()
xwl_glamor_pixmap_get_wl_buffer()
xwl_glamor_init_wl_registry()
xwl_glamor_post_damage()
xwl_glamor_allow_commits()
xwl_glamor_egl_make_current()
Are useless without glamor support enabled, move those withing a
a "#ifdef XWL_HAS_GLAMOR" in xwayland.h
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 79
hw/xwayland/xwayland-glamor.c | 80 -
hw/xwayland/xwayland.h | 24
3 files changed, 89 insertions(+), 94 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 8dd1cc304..89c531f4a 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -187,6 +187,85 @@ xwl_eglstream_cleanup(struct xwl_screen *xwl_screen)
free(xwl_eglstream);
}
+static Bool
+xwl_glamor_egl_supports_device_probing(void)
+{
+return epoxy_has_egl_extension(NULL, "EGL_EXT_device_base");
+}
+
+static void **
+xwl_glamor_egl_get_devices(int *num_devices)
+{
+EGLDeviceEXT *devices;
+Bool ret;
+int drm_dev_count = 0;
+int i;
+
+if (!xwl_glamor_egl_supports_device_probing())
+return NULL;
+
+/* Get the number of devices */
+ret = eglQueryDevicesEXT(0, NULL, num_devices);
+if (!ret || *num_devices < 1)
+return NULL;
+
+devices = calloc(*num_devices, sizeof(EGLDeviceEXT));
+if (!devices)
+return NULL;
+
+ret = eglQueryDevicesEXT(*num_devices, devices, num_devices);
+if (!ret)
+goto error;
+
+/* We're only ever going to care about devices that support
+ * EGL_EXT_device_drm, so filter out the ones that don't
+ */
+for (i = 0; i < *num_devices; i++) {
+const char *extension_str =
+eglQueryDeviceStringEXT(devices[i], EGL_EXTENSIONS);
+
+if (!epoxy_extension_in_string(extension_str, "EGL_EXT_device_drm"))
+continue;
+
+devices[drm_dev_count++] = devices[i];
+}
+if (!drm_dev_count)
+goto error;
+
+*num_devices = drm_dev_count;
+devices = realloc(devices, sizeof(EGLDeviceEXT) * drm_dev_count);
+
+return devices;
+
+error:
+free(devices);
+
+return NULL;
+}
+
+static Bool
+xwl_glamor_egl_device_has_egl_extensions(void *device,
+ const char **ext_list, size_t size)
+{
+EGLDisplay egl_display;
+int i;
+Bool has_exts = TRUE;
+
+egl_display = glamor_egl_get_display(EGL_PLATFORM_DEVICE_EXT, device);
+if (!egl_display || !eglInitialize(egl_display, NULL, NULL))
+return FALSE;
+
+for (i = 0; i < size; i++) {
+if (!epoxy_has_egl_extension(egl_display, ext_list[i])) {
+has_exts = FALSE;
+break;
+}
+}
+
+eglTerminate(egl_display);
+return has_exts;
+}
+
static void
xwl_eglstream_unref_pixmap_stream(struct xwl_pixmap *xwl_pixmap)
{
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index f543f321d..c7ae51336 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -52,86 +52,6 @@ xwl_glamor_egl_make_current(struct xwl_screen *xwl_screen)
xwl_screen->glamor_ctx->make_current(xwl_screen->glamor_ctx);
}
-Bool
-xwl_glamor_egl_supports_device_probing(void)
-{
-return epoxy_has_egl_extension(NULL, "EGL_EXT_device_base");
-}
-
-void **
-xwl_glamor_egl_get_devices(int *num_devices)
-{
-#ifdef XWL_HAS_EGLSTREAM
-EGLDeviceEXT *devices;
-Bool ret;
-int drm_dev_count = 0;
-int i;
-
-if (!xwl_glamor_egl_supports_device_probing())
-return NULL;
-
-/* Get the number of devices */
-ret = eglQueryDevicesEXT(0, NULL, num_devices);
-if (!ret || *num_devices < 1)
-return NULL;
-
-devices = calloc(*num_devices, sizeof(EGLDeviceEXT));
-if (!devices)
-return NULL;
-
-ret = eglQueryDevicesEXT(*num_devices, devices, num_devices);
-if (!ret)
-goto error;
-
-/* We're only ever going to care about devices that support
- * EGL_EXT_device_drm, so filter out the ones that don't
- */
-for (i = 0; i < *num_devices; i++) {
-const char *extension_str =
-eglQueryDeviceStringEXT(devices[i], EGL_EXTENSIONS);
-
-if (!epoxy_extension_in_string(extension_str, "EGL_EXT_device_drm"))
-continue;
-
-devices[drm_dev_count++] = devices[i];
-}
-if (!drm_dev_count)
-goto error;
-
-*num_devices = drm_dev_count;
-devic
[PATCH xserver 5/5] xwayland: refactor egl_backends for wayland registry
To be able to check for availability of the Wayland interfaces required
to run a given EGL backend (either GBM or EGL streams for now), we need
to have each backend structures and vfuncs in place before we enter the
Wayland registry dance.
That basically means that we should init all backends at first, connect
to the Wayland compositor and query the available interfaces and then
decide which backend is available and should be used (or none if either
the Wayland interfaces or the EGL extensions are not available).
For this purpose, hold an egl_backend struct for each backend we are to
consider prior to connect to the Wayland display so that, when we get to
query the Wayland interfaces, everything is in place for each backend to
handle the various Wayland interfaces.
Eventually, when we need to chose which EGL backend to use for glamor,
the available Wayland interfaces and EGL extensions available are all
known to Xwayland.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 27 ++
hw/xwayland/xwayland-glamor-gbm.c | 38 ++
hw/xwayland/xwayland-glamor.c | 69 ++---
hw/xwayland/xwayland-present.c | 7 +--
hw/xwayland/xwayland.c | 10 ++--
hw/xwayland/xwayland.h | 15 --
6 files changed, 118 insertions(+), 48 deletions(-)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 85b562c31..ee03c4a86 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -638,11 +638,11 @@ const struct wl_eglstream_display_listener
eglstream_display_listener = {
.swapinterval_override =
xwl_eglstream_display_handle_swapinterval_override,
};
-static void
+static Bool
xwl_glamor_eglstream_init_wl_registry(struct xwl_screen *xwl_screen,
struct wl_registry *wl_registry,
- const char *name,
- uint32_t id, uint32_t version)
+ uint32_t id, const char *name,
+ uint32_t version)
{
struct xwl_eglstream_private *xwl_eglstream =
xwl_eglstream_get(xwl_screen);
@@ -654,10 +654,15 @@ xwl_glamor_eglstream_init_wl_registry(struct xwl_screen
*xwl_screen,
wl_eglstream_display_add_listener(xwl_eglstream->display,
_display_listener,
xwl_screen);
+return TRUE;
} else if (strcmp(name, "wl_eglstream_controller") == 0) {
xwl_eglstream->controller = wl_registry_bind(
wl_registry, id, _eglstream_controller_interface, version);
+return TRUE;
}
+
+/* no match */
+return FALSE;
}
static Bool
@@ -896,6 +901,7 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
struct xwl_eglstream_private *xwl_eglstream;
EGLDeviceEXT egl_device;
+xwl_screen->eglstreams_backend.is_available = FALSE;
egl_device = xwl_eglstream_get_device(xwl_screen);
if (egl_device == EGL_NO_DEVICE_EXT)
return FALSE;
@@ -915,13 +921,14 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
xwl_eglstream->egl_device = egl_device;
xorg_list_init(_eglstream->pending_streams);
-xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
-xwl_screen->egl_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
-xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
-xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
-xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
-xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
-xwl_screen->egl_backend.allow_commits = xwl_glamor_eglstream_allow_commits;
+xwl_screen->eglstreams_backend.init_egl = xwl_glamor_eglstream_init_egl;
+xwl_screen->eglstreams_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
+xwl_screen->eglstreams_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
+xwl_screen->eglstreams_backend.init_screen =
xwl_glamor_eglstream_init_screen;
+xwl_screen->eglstreams_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
+xwl_screen->eglstreams_backend.post_damage =
xwl_glamor_eglstream_post_damage;
+xwl_screen->eglstreams_backend.allow_commits =
xwl_glamor_eglstream_allow_commits;
+xwl_screen->eglstreams_backend.is_available = TRUE;
ErrorF("glamor: Using nvidia's eglstream interface, direct rendering
impossible.\n");
ErrorF("glamor: Performance may be affected. Ask your vendor to support
GBM!\n");
diff --git a/hw/xwayland/xwayland-glamor-gbm.
[PATCH xserver 3/5] xwayland: Add Wayland interfaces check
Introduces a new egl_backend function to let the EGL backend check for
the presence of the required Wayland interfaces.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 20
hw/xwayland/xwayland-glamor-gbm.c | 14 ++
hw/xwayland/xwayland-glamor.c | 11 +++
hw/xwayland/xwayland.h | 7 +++
4 files changed, 52 insertions(+)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 89c531f4a..85b562c31 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -660,6 +660,25 @@ xwl_glamor_eglstream_init_wl_registry(struct xwl_screen
*xwl_screen,
}
}
+static Bool
+xwl_glamor_eglstream_has_wl_interfaces(struct xwl_screen *xwl_screen)
+{
+struct xwl_eglstream_private *xwl_eglstream =
+xwl_eglstream_get(xwl_screen);
+
+if (xwl_eglstream->display == NULL) {
+ErrorF("glamor: 'wl_eglstream_display' not supported\n");
+return FALSE;
+}
+
+if (xwl_eglstream->controller == NULL) {
+ErrorF("glamor: 'wl_eglstream_controller' not supported\n");
+return FALSE;
+}
+
+return TRUE;
+}
+
static inline void
xwl_eglstream_init_shaders(struct xwl_screen *xwl_screen)
{
@@ -898,6 +917,7 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
xwl_screen->egl_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
+xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_eglstream_has_wl_interfaces;
xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
diff --git a/hw/xwayland/xwayland-glamor-gbm.c
b/hw/xwayland/xwayland-glamor-gbm.c
index 29325adac..f34c45b9a 100644
--- a/hw/xwayland/xwayland-glamor-gbm.c
+++ b/hw/xwayland/xwayland-glamor-gbm.c
@@ -746,6 +746,19 @@ xwl_glamor_gbm_init_wl_registry(struct xwl_screen
*xwl_screen,
xwl_screen_set_dmabuf_interface(xwl_screen, id, version);
}
+static Bool
+xwl_glamor_gbm_has_wl_interfaces(struct xwl_screen *xwl_screen)
+{
+struct xwl_gbm_private *xwl_gbm = xwl_gbm_get(xwl_screen);
+
+if (xwl_gbm->drm == NULL) {
+ErrorF("glamor: 'wl_drm' not supported\n");
+return FALSE;
+}
+
+return TRUE;
+}
+
static Bool
xwl_glamor_gbm_init_egl(struct xwl_screen *xwl_screen)
{
@@ -879,6 +892,7 @@ xwl_glamor_init_gbm(struct xwl_screen *xwl_screen)
xwl_gbm);
xwl_screen->egl_backend.init_wl_registry = xwl_glamor_gbm_init_wl_registry;
+xwl_screen->egl_backend.has_wl_interfaces =
xwl_glamor_gbm_has_wl_interfaces;
xwl_screen->egl_backend.init_egl = xwl_glamor_gbm_init_egl;
xwl_screen->egl_backend.init_screen = xwl_glamor_gbm_init_screen;
xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_gbm_get_wl_buffer_for_pixmap;
diff --git a/hw/xwayland/xwayland-glamor.c b/hw/xwayland/xwayland-glamor.c
index c7ae51336..8e858394a 100644
--- a/hw/xwayland/xwayland-glamor.c
+++ b/hw/xwayland/xwayland-glamor.c
@@ -76,6 +76,17 @@ xwl_glamor_init_wl_registry(struct xwl_screen *xwl_screen,
interface, id, version);
}
+Bool
+xwl_glamor_has_wl_interfaces(struct xwl_screen *xwl_screen,
+struct xwl_egl_backend *xwl_egl_backend)
+{
+if (xwl_egl_backend->has_wl_interfaces)
+return xwl_egl_backend->has_wl_interfaces(xwl_screen);
+
+/* If the backend has no requirement wrt WL interfaces, we're fine */
+return TRUE;
+}
+
struct wl_buffer *
xwl_glamor_pixmap_get_wl_buffer(PixmapPtr pixmap,
unsigned short width,
diff --git a/hw/xwayland/xwayland.h b/hw/xwayland/xwayland.h
index 0d6a4c246..674be3d49 100644
--- a/hw/xwayland/xwayland.h
+++ b/hw/xwayland/xwayland.h
@@ -69,6 +69,11 @@ struct xwl_egl_backend {
const char *name, uint32_t id,
uint32_t version);
+/* Check that the required Wayland interfaces are available. This
+ * callback is optional.
+ */
+Bool (*has_wl_interfaces)(struct xwl_screen *xwl_screen);
+
/* Called before glamor has been initialized. Backends should setup a
* valid, glamor compatible EGL context in this hook.
*/
@@ -432,6 +437,8 @@ void xwl_glamor_init_wl_registry(struct xwl_screen
*xwl_screen,
struct wl_registry *registry,
uint32_t id, const char *interface,
uint32_t version);
+Bool xwl_glamor_has_wl_interfaces(struct xwl_screen *xwl_screen,
+
Re: [PATCH xserver 1/3] xwayland: Add hook to check wl interface for glamor
Hi Lyude, On Wed, May 30, 2018 at 9:31 PM, Lyude Paul wrote: > NAK. I'm starting to see the problems with this approach I originally hit now. > > So: there's some unexpected catches when it comes to EGL client extensions. On > a system using glvnd with the nvidia driver, the EGL client extension list > will have /both/ the client extensions for the nvidia EGL driver and the > client extensions for the Mesa driver. An example from my test machine: > > EGL client extensions string: > EGL_EXT_platform_base EGL_EXT_device_base # ← nvidia! > EGL_KHR_client_get_all_proc_addresses EGL_EXT_client_extensions > EGL_KHR_debug EGL_EXT_platform_x11 EGL_EXT_platform_device > EGL_EXT_platform_wayland EGL_MESA_platform_gbm # ← mesa! > EGL_MESA_platform_surfaceless > > This is fine, but what it means is that we're going to have to take the > presence of either of these extensions as a sign that their respective EGL > backends -might- be there. e.g. we can't try to make a choice on avoiding > trying EGLStreams simply because GBM might be there, which is currently what > this patch does: > > * During init, xwl_glamor_should_use_gbm() tells us we should prefer GBM over >EGLStreams > * We select the gbm egl backend's hook for checking for the wl interfaces > * In xwl_screen_init(), we check for wl_drm and don't see it > * xwl_glamor_has_wl_interface() returns FALSE, which makes us disable glamor >and fall back to software rendering I see... Problem is that we need to check for the interfaces and can do so only once we're connected to the Wayland display, after we called xwl_glamor_init_gbm() or xwl_glamor_init_eglstream() which setup the init_wl_registry() handler to read the wl_registry advertised by the compositor, and this is part of either xwl_gbm_private or xwl_eglstream_private structures which get allocated once we've selected the backend... So this is a chicken and egg situation here. So those interfaces for all backends, wl_drm, wl_eglstream_display, wl_eglstream_controller, and init_wl_registry() need to be moved out of the egl_backend, so that we can read *all* the available Wayland interfaces first and then decide which backend to use based on *both* EGL extensions available *and* Wayland interfaces as well. That's a bit of refactoring of the egl_backend code, working on it... [...] Cheers, Olivier ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xserver 3/3] xwayland: Check required interfaces for EGLstreams
Use the newly added “has_wl_interface” hook to check availability of
“wl_eglstream_display” and “wl_eglstream_controller” interfaces prior to
enable glamor with EGL Streams backend.
Signed-off-by: Olivier Fourdan
---
hw/xwayland/xwayland-glamor-eglstream.c | 21 +
1 file changed, 21 insertions(+)
diff --git a/hw/xwayland/xwayland-glamor-eglstream.c
b/hw/xwayland/xwayland-glamor-eglstream.c
index 8dd1cc304..fcf7a7149 100644
--- a/hw/xwayland/xwayland-glamor-eglstream.c
+++ b/hw/xwayland/xwayland-glamor-eglstream.c
@@ -581,6 +581,26 @@ xwl_glamor_eglstream_init_wl_registry(struct xwl_screen
*xwl_screen,
}
}
+static Bool
+xwl_glamor_eglstream_has_wl_interface(struct xwl_screen *xwl_screen)
+{
+struct xwl_eglstream_private *xwl_eglstream =
+xwl_eglstream_get(xwl_screen);
+
+if (xwl_eglstream->display == NULL) {
+ErrorF("glamor: 'wl_eglstream_display' not supported\n");
+return FALSE;
+}
+
+if (xwl_eglstream->controller == NULL) {
+ErrorF("glamor: 'wl_eglstream_controller' not supported\n");
+return FALSE;
+}
+
+return TRUE;
+}
+
+
static inline void
xwl_eglstream_init_shaders(struct xwl_screen *xwl_screen)
{
@@ -819,6 +839,7 @@ xwl_glamor_init_eglstream(struct xwl_screen *xwl_screen)
xwl_screen->egl_backend.init_egl = xwl_glamor_eglstream_init_egl;
xwl_screen->egl_backend.init_wl_registry =
xwl_glamor_eglstream_init_wl_registry;
+xwl_screen->egl_backend.has_wl_interface =
xwl_glamor_eglstream_has_wl_interface;
xwl_screen->egl_backend.init_screen = xwl_glamor_eglstream_init_screen;
xwl_screen->egl_backend.get_wl_buffer_for_pixmap =
xwl_glamor_eglstream_get_wl_buffer_for_pixmap;
xwl_screen->egl_backend.post_damage = xwl_glamor_eglstream_post_damage;
--
2.17.0
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
