Description of problem: GDM in virtual machine (virt-manager) doesn't show mouse cursor. This is dependent on virtual machine configuration. If tablet hardware (EvTouch USB Graphics Tablet) is present cursor is not shown. If tablet hw is removed from virtual machine, GDM and mouse works as expected. Virt-manager adds tablet if Linux and Fedora is selected when creating new virtual machine. If generic is used, no tablet hw is added. Version-Release number of selected component (if applicable): gdm-3.15.91.2-1.fc22.x86_64 virt-manager-1.1.0-4.git310f6527.fc21.noarch libvirt-*-1.2.9.2-1.fc21.x86_64 Steps to Reproduce: 1. create virtual machine (explicitly select Linux and Fedora) with Fedora 2. boot/logout to see GDM 3. no mouse cursor is shown
Additional info: mouse cursor is not shown, but works and one can (blindly) click. This seems to be annoying bug for people using fedora in virtual machine (bug does not occur on bare metal) since default behavior of virt-manager (Fedora virtual machine) is that tablet hardware is added. This is probably freeze exception or even blocker bug. Closest criterion I found is beta blocker Guest on previous release: The release must install and boot successfully as a virtual guest in a situation where the virtual host is running the previous stable Fedora release. However, I'm not really sure, there should be discussion about this.
I propose this as final blocker as it violates the (beta) criterion: Guest on previous release The release must install and boot successfully as a virtual guest in a situation where the virtual host is running the previous stable Fedora release. The criterion itself is kind of vague and besides this the guest works as expected, but the problem happens on default installation in virt-manager. Blindly click in gdm is not really comfortable, tab itself just goes through user list and the only graphical way how to shutdown the machine is using (rather commonly unknown) ctrl-alt-tab. Although its beta criterion, I proposed this as final blocker because it affects gdm only in virtualized Fedora. If you set WaylandEnable=false in [daemon] section of /etc/gdm/custom.conf the problem goes away. See more in https://bugzilla.gnome.org/show_bug.cgi?id=746078#c2
Proposed as a Blocker for 22-final by Fedora user lbrabec using the blocker tracking app because: I propose this as final blocker as it violates the (beta) criterion: Guest on previous release The release must install and boot successfully as a virtual guest in a situation where the virtual host is running the previous stable Fedora release. The criterion itself is kind of vague and besides this the guest works as expected, but the problem happens on default installation in virt-manager. Blindly click in gdm is not really comfortable, tab itself just goes through user list and the only graphical way how to shutdown the machine is using (rather commonly unknown) ctrl-alt-tab.
Discussed at 2015-03-30 blocker review meeting: http://meetbot.fedoraproject.org/fedora-blocker-review/2015-03-30/f22-blocker-review.2015-03-30-16.04.log.txt . Rejected as a blocker: it's rather annoying, but not a showstopper, only affects VMs, and only really affects installed systems hence can be fixed with an update (you don't really encounter GDM on lives, usually).
It'd still be nice to fix this for release if we can, though. Ray, is there any hope? Should this be re-assigned to something X/kernel-ish?
I'll take a look today and see what I can figure out.
so i'm building mutter-3.16.1.1-4.fc22 today with a change that should workaround this, I think, but it seems the root problem is some sort of problem in the qxl kms driver. Basically, it looks like, as soon as drmModeSetCrtc is called the cursor disappears. The workaround is to repeat the drmModeSetCursor2 call after drmModeSetCrtc to make it come back.
As the freeze is about to go into effect, proposing this as a freeze exception issue. Seems good to fix this if we can.
Tested, and mutter-3.16.1.1-4.fc22 does seem to resolve this for me. Please submit an update for it when you can, thanks!
Thanks, Ray, for investigation. mutter-3.16.1.1-4.fc22 indeed works for me. Please submit an update, thanks.
(moving back to assigned since the built fix is just a workaround)
mutter-3.16.1.1-4.fc22 has been submitted as an update for Fedora 22. https://admin.fedoraproject.org/updates/mutter-3.16.1.1-4.fc22
Created attachment 1024708 [details] drm/qxl: only create primary on first SetCrtc call The qxl driver currently destroys and recreates the qxl "primary" any time the first crtc is set. A side-effect of destroying the primary is mouse state associated with the crtc is lost, which leads to disappearing mouse cursors on wayland sessions. This commit changes the driver to only create the primary the first time SetCrtc is called. It tracks this state by recycling a previously unused boolean called primary_created in the qxl_device struct. Signed-off-by: Ray Strode <rstrode>
Comment on attachment 1024708 [details] drm/qxl: only create primary on first SetCrtc call So the above patch attempts to fix the problem kernel side. It avoids destroying the primary on non-initial setcrtc calls. Needs review, since I don't know much about this code. Some things to double-check in review: 1) that destroying the primary is actually necessary at all (i assume so to make sure qemu is given fresh state on fresh boots, but maybe qemu handles that implicitly and it can be dropped wholesale?) 2) that skipping the destroy is okay for subsequent calls (no security implications?) 3) another alternative might be to save the cursor from the old primary and restore it in the new one . would that approach be better? 4) i'm storing state on the "primary_created" boolean in the qxl_device struct. it looks like that previously that variable was left over cruft that hitched its way into the initial import, but would be good to make sure i'm not missing anything. 5) Also, I don't think there's any locking concerns for the qxl_device struct, but would be good to make sure.
mutter-3.16.1.1-4.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.
The other option is we could fix this in QEMU. The cursor resetting was added here: http://git.qemu.org/?p=qemu.git;a=commit;h=30f6da6656c94964ba8677928588592d9667007e in order to fix a crasher. Instead of resetting the cursor we could save the cursor and restore it after the primary is recreated. I worry that would potentially affect other quest operating systems though, and it means anyone testing fedora 22 on a fedora 21 guest will end up with a blank cursor (unless we leave the mutter workaround in place, which we could do). Gerd, Christoph, Marc-Andre, what your guys' take?
(In reply to Ray Strode [halfline] from comment #14) > Comment on attachment 1024708 [details] > drm/qxl: only create primary on first SetCrtc call > > So the above patch attempts to fix the problem kernel side. It avoids > destroying the primary on non-initial setcrtc calls. Needs review, since I > don't know much about this code. Some things to double-check in review: > > 1) that destroying the primary is actually necessary at all (i assume so to > make sure qemu is given fresh state on fresh boots, but maybe qemu handles > that implicitly and it can be dropped wholesale?) It surely is. Try switching the video mode via xrandr with the patched kms driver, I suspect you'll get a dis-sorted display. The "create primary surface" also defines where the surface is located in video memory, so when skipping this page-flipping will not work correctly. With wayland allocating two buffers and flipping between them you'll probably see only every other frame. Probably not visually noticable. Reboot is no problem, machine reset will reset qxl too. > 2) that skipping the destroy is okay for subsequent calls (no security > implications?) Also not a problem. > 3) another alternative might be to save the cursor from the old primary and > restore it in the new one . would that approach be better? I'd say yes. The fundamental issue is that qxl hasn't support for pageflipping. IMO the whole concept of a "primary surface" is broken and should be dropped. We should be able to say "route this surface (=bo / fb in kms speak) to that crt" instead. It is much easier sayed that done though as the primary surface unfortunately is something very fundamental in the qxl device design and spice display protocol. With wayland we have two cases where setcrtc is called: (1) setting video mode, and (2) page flip. The destroy/create primary surface thing was designed for (1), not for (2), and it is very bad at handling (2). I guess the best bet is to detect page-flips (either by comparing old+new video mode, or by passing down a flag to qxl_crtc_mode_set), and restore the cursor in the page-flip case.
(In reply to Gerd Hoffmann from comment #17) > It surely is. Try switching the video mode via xrandr with the patched kms > driver, I suspect you'll get a dis-sorted display. yea, totally busted when using non-wayland. Makes sense. I wasn't thinking about the new frame case very carefully. > > 3) another alternative might be to save the cursor from the old primary and > > restore it in the new one . would that approach be better? > I'd say yes. Okay, airlied agreed with you too in my irc backlog. > With wayland we have two cases where setcrtc is called: (1) setting video > mode, and (2) page flip. The destroy/create primary surface thing was > designed for (1), not for (2), and it is very bad at handling (2). > > I guess the best bet is to detect page-flips (either by comparing old+new > video mode, or by passing down a flag to qxl_crtc_mode_set), and restore the > cursor in the page-flip case. I was with you until this point. Why wouldn't we want to restore the cursor on xrandr ? sometimes xrandr is run at login time, so in that situation we'd still lose the cursor right? (well, I guess not in practice, since in practice this problem only manifests on wayland, so I guess X must be resetting the cursor itself after mode switch)
Created attachment 1025178 [details] drm/qxl: reapply cursor after SetCrtc calls The qxl driver currently destroys and recreates the qxl "primary" any time the first crtc is set. A side-effect of destroying the primary is mouse state associated with the crtc is lost, which leads to disappearing mouse cursors on wayland sessions. This commit changes the driver to reapply the cursor any time SetCrtc is called. It achieves this by keeping a copy of the cursor data on the qxl_crtc struct. Signed-off-by: Ray Strode <rstrode>
So I took one more go at this. Again, I don't know the code well or even, really, general kernel programming practices so please review with scrutiny. Some potential things that might need work: Am I stashing the cursor away correctly? 1) I'm storing the cursor on the qxl_crtc as regular kmalloced memory. it could be that's unnecessary and it's somehow possible to keep the cursor buffer object alive across reset directly. Maybe by some QXL command, or by some ttm command ? 2) If keeping a copy around is right, is kmalloc the right function to use for keeping it? 3) qxl_crtc_stash_cursor currently checks if the stashed cursor object is the same size and avoids remallocing in that case. In practice, it will always be the same size, so maybe we should just curtail the check? 4) maybe, rather than stuffing a whole qxl_cursor object in the qxl_crtc struct, maybe we should just stuff the parts that aren't hardcoded (data, and hotspot coords) 5) I pass the drm_crtc struct to stash_cursor and apply_cursor, then extract qxl_crtc and other things I need from the struct. Would it be better from a coding style point of view to just pass those values in directly? 6) whitespace issues? other obvious style problems?
7) should the common bits between qxl_apply_cursor and qxl_crtc_cursor_set2 be factored out and moved to a common helper function
"and it means anyone testing fedora 22 on a fedora 21 guest will end up with a blank cursor" we definitely don't want that, if it can be avoided.
If possible, i'd like to get the kernel patch in and the mutter workaround dropped before release, since the mutter patch won't be going upstream. Josh, assuming attachment 1025178 [details] or some future incarnation passes review, does this seem like something that can land in time? or did we already miss the train?
We're past Final freeze now, so it's not a *great* time to be tinkering with kernel patches. Perhaps we could ship with the mutter workaround then switch in the kernel fix as a post-release update?
As Adam said, we're probably past the point of getting it into the GA kernel. It's mostly a timing thing. We can add the patch whenever airlied and Gerd feel comfortable with it and it will be in an update.
okay let's get this off the exception list then.
> > I guess the best bet is to detect page-flips (either by comparing old+new > > video mode, or by passing down a flag to qxl_crtc_mode_set), and restore the > > cursor in the page-flip case. > I was with you until this point. Why wouldn't we want to restore the cursor > on xrandr ? sometimes xrandr is run at login time, so in that situation > we'd still lose the cursor right? (well, I guess not in practice, since in > practice this problem only manifests on wayland, so I guess X must be > resetting the cursor itself after mode switch) Yes, when switching the mode it most likely isn't needed, this is what I had in mind. But on the other hand it should not hurt either, and making this conditional adds complexity for no good reason, so doing it unconditionally is fine.
(In reply to Ray Strode [halfline] from comment #20) > So I took one more go at this. Again, I don't know the code well or even, > really, general kernel programming practices so please review with scrutiny. > > Some potential things that might need work: > > Am I stashing the cursor away correctly? It's correct as far as I can see, and should be ok as quick stopgap. There is room per (performance) improvements though. > 1) I'm storing the cursor on the qxl_crtc as regular kmalloced memory. it > could be that's unnecessary and it's somehow possible to keep the cursor > buffer object alive across reset directly. Maybe by some QXL command, or by > some ttm command ? Th bo (== buffer object) allocated is qxl device memory. Cursor image is copyed over, so qxl can see & use it. So keeping a reference to that bo should be enough. You don't need to re-create a bo in each apply_cursor call then, and you also don't need to keep a copy of the cursor image itself. Re-sending the QXL_CURSOR_SET command is enough. Changing the cursor bo lifecycle is non-trivial enough that I wouldn't try it that close to GA though. Fencing logic for the cursor bo needs to change too to make sure the cursor bo isn't released before the qxl device has processed the command.
mutter-3.17.90-2.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-14964
*********** MASS BUG UPDATE ************** We apologize for the inconvenience. There is a large number of bugs to go through and several of them have gone stale. Due to this, we are doing a mass bug update across all of the Fedora 22 kernel bugs. Fedora 22 has now been rebased to 4.2.3-200.fc22. Please test this kernel update (or newer) and let us know if you issue has been resolved or if it is still present with the newer kernel. If you have moved on to Fedora 23, and are still experiencing this issue, please change the version to Fedora 23. If you experience different issues, please open a new bug report for those.
well, this was kinda in the developers' hands, per discussion above.
So we're still hitting this in F23; we've been tracking it at https://bugzilla.redhat.com/show_bug.cgi?id=1273247 but that was filed for SDDM and really we should track SDDM and GDM cases separately. So let's throw blocker and FE nominations at this. I'm probably -1 blocker - it's not really so bad, we could document it and fix with an update - but +1 FE, if we do a new RC we should pull this in. http://koji.fedoraproject.org/koji/buildinfo?buildID=693553 fixes this, an update is coming soon.
mutter-3.18.1-3.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-bebede9f5b
I'm +1 blocker on this one, as it breaks the login/out criteria.
Definitely +1 FE. I think it makes sense to try and roll a new RC with this, even when this isn't a clear blocker. I'd probably lean towards +1 blocker as well, but I'm happy to defer to adamw's gut feeling here :)
+1 FE for this.
I am +1 to FE not sure on blocker
-1 blocker +1 FE
Discussed at 2015-10-22 Go/No-Go meeting, acting as a blocker review meeting: https://meetbot-raw.fedoraproject.org/fedora-meeting-2/2015-10-22/f23-final-go_no_go-meeting.2015-10-22-16.00.log.txt . Rejected as a blocker, but accepted as a freeze exception: it's fairly easy to work around this (keyboard navigation in GDM is pretty simple) and it only affects KVMs, but fixing it would obviously be good.
mutter-3.18.1-4.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report. If you want to test the update, you can install it with $ su -c 'dnf --enablerepo=updates-testing update mutter' You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-bebede9f5b
mutter-3.18.1-4.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
reopening as per comment 11
Mutter patch is not included F25, the cursor is disappearing again.
oh yeah, think I saw this in a VM yesterday.
I don't mind putting the kernel patch into rawhide for a bit, provided something is happening upstream and we won't be carrying it forever.
Justin, that would be great. It seems to still mostly apply fine, though, now it looks like it needs to take into account hotspot position. If you want I can attached a rebased patch for you. I spent a little time yesterday trying to figure out the better strategy outlined in comment 28, and came up with a patch that just reuses the cursor bo instead of doing memcpy. It seems to work okay, but I don't fully grok the fencing complications Gerd mentioned. Granted, the qxl/ttm code is pretty unfamiliar to me. Here's my understanding: 1) The QXL driver has the concept of a "release" object. It's an object that is associated with a particular ring buffer used for command submission. The release object collects a list of buffer objects, and makes sure all those buffer objects get reserved until the command is processed. Once the command is processed the shared fence binding the objects together is completed and the objects are no longer reserved. An example of a command would be "set the cursor". 2) As long as a buffer object is reserved, it won't get moved around in memory. (For instance it won't get moved from GPU memory to swap, I guess). 3) The concern is that if we're keeping the cursor buffer object around long after it's been submitted and the initial reservation has been released, then next time we need to use it, it might not be accessible anymore? Looking at the code, all buffer objects added to a release object get ttm_bo_validate called on them when "reserve_list" is called on the release object. That should make sure the cursor buffer object is properly in GPU memory. There is the possibility, that the apply_cursor call is made while the initial cursor set operation is still pending, I guess. I don't think that should be a problem either, unless I'm missing something. We're not writing to the buffer object, just reading from it. Any time we set a new cursor we allocate a new buffer object, so there shouldn't ever be any contention. Gerd, can you go into more detail on the complications you alluded to in comment 28 ? I'll attach the patch I came up with so far, and we can iterate on it.
Created attachment 1191666 [details] drm/qxl: reapply cursor after SetCrtc calls The qxl driver currently destroys and recreates the qxl "primary" any time the first crtc is set. A side-effect of destroying the primary is mouse state associated with the crtc is lost, which leads to disappearing mouse cursors on wayland sessions. This commit changes the driver to reapply the cursor any time SetCrtc is called. It achieves this by keeping a reference to the cursor bo on the qxl_crtc struct.
*** Bug 1366061 has been marked as a duplicate of this bug. ***
I didn't mark it because I don't want things to autoclose at this point, but the patch has been included in 4.8-rc2.git3.1 and newer builds for rawhide and F25. I would love to see some testing on those.
to be clear, you added a forward ported version of attachment 1025178 [details] (thanks!) not attachment 1191666 [details] which is still waiting on feedback from Gerd
(In reply to Justin M. Forbes from comment #49) > I didn't mark it because I don't want things to autoclose at this point, but > the patch has been included in 4.8-rc2.git3.1 and newer builds for rawhide > and F25. I would love to see some testing on those. Alpha live media may be stuck since we're in freeze. But in the installed system kernel-4.8.0-0.rc2.git3.1.fc25.x86_64 does appear to fix the problem.
Yes, I added the old stash, not the new implementation. And it should make the live media for alpha since git 3 got a freeze exception as it fixed 2 boot bugs. It is on the way to stable now.
Discussed during the 2016-08-22 blocker review meeting: [1] The decision to classify this as an Alpha AcceptedFreezeException was made because this bug affects live images, so it cannot be fixed with an update. [1] https://meetbot.fedoraproject.org/fedora-blocker-review/2016-08-22/f25-blocker-review.2016-08-22-16.00.txt
great
Confirmed that 4.8.0-0.rc2.git3.1.fc25 does address this. I can reproduce the bug with an earlier live image (20160809.n.0) but not with 20160822.n.1 , which includes that kernel. Not closing per Justin's wishes, though.
*********** MASS BUG UPDATE ************** We apologize for the inconvenience. There is a large number of bugs to go through and several of them have gone stale. Due to this, we are doing a mass bug update across all of the Fedora 25 kernel bugs. Fedora 25 has now been rebased to 4.9.3-200.fc25. Please test this kernel update (or newer) and let us know if you issue has been resolved or if it is still present with the newer kernel. If you have moved on to Fedora 26, and are still experiencing this issue, please change the version to Fedora 26. If you experience different issues, please open a new bug report for those.
Justin, Ray, should this be closed now? Do we no longer need the mutter hack?
This message is a reminder that Fedora 25 is nearing its end of life. Approximately 4 (four) weeks from now Fedora will stop maintaining and issuing updates for Fedora 25. It is Fedora's policy to close all bug reports from releases that are no longer maintained. At that time this bug will be closed as EOL if it remains open with a Fedora 'version' of '25'. Package Maintainer: If you wish for this bug to remain open because you plan to fix it in a currently maintained version, simply change the 'version' to a later Fedora version. Thank you for reporting this issue and we are sorry that we were not able to fix it before Fedora 25 is end of life. If you would still like to see this bug fixed and are able to reproduce it against a later version of Fedora, you are encouraged change the 'version' to a later Fedora version prior this bug is closed as described in the policy above. Although we aim to fix as many bugs as possible during every release's lifetime, sometimes those efforts are overtaken by events. Often a more recent Fedora release includes newer upstream software that fixes bugs or makes them obsolete.
Fedora 25 changed to end-of-life (EOL) status on 2017-12-12. Fedora 25 is no longer maintained, which means that it will not receive any further security or bug fix updates. As a result we are closing this bug. If you can reproduce this bug against a currently maintained version of Fedora please feel free to reopen this bug against that version. If you are unable to reopen this bug, please file a new report against the current release. If you experience problems, please add a comment to this bug. Thank you for reporting this bug and we are sorry it could not be fixed.
The needinfo request[s] on this closed bug have been removed as they have been unresolved for 1000 days