System hangs when using VirtualBox with VMgfx emulation and 3D pass-through
Hello, Posting this here in the hope of getting the attention of someone who could help me debug it. When I try to start a VirtualBox virtual machine with VMgfx emulation and 3D pass-through enabled on my Ubuntu 18.10 system I quickly get a system hang with OOM killer messages in the system log. Killed processes have included the VM process and Xwayland, but in both cases the memory usage reported in the log did not look critical. Reproducible with a pure X session and GNOME Shell/Wayland (noting that VirtualBox is using the X protocol). The system is a Thinkpad T470 with HD Graphics 620. Some of the stack traces in the log have i915_gem functions in them. If anyone has time and knowledge to help me with debugging or to give me some pointers I would greatly appreciate it of course. On-list or off-list, though not sure if everyone reading the list is interested in following this. Regards Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher pEpkey.asc Description: application/pgp-keys ___ 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 fatal error when Wayland output disappears
Hello Pekka, On 27.10.2017 08:01, Pekka Paalanen wrote: On Thu, 26 Oct 2017 15:40:13 +0200 Michael Thayer <michael.tha...@oracle.com> wrote: [ Discussion of a global Wayland object disappearing triggering a fatal error in xwl_log_handler in Xwayland.] there is a known race around Wayland globals. If the Wayland server adds and removes a global in a very short time, it may succeed to remove the global (wl_output) before all clients have processed the add. If a client process an add after the server removed, you hit exactly this fatal error. It's a design flaw in Wayland, gone unnoticed for years until it was too late to fix properly. This issue is recorded: https://phabricator.freedesktop.org/T7722 There is a suggested mitigation, but I am not aware of anyone working on it. Trigger warning: a person who does not know the code base is about to make suggestions. I am wondering why Xwayland has to call FatalError in that log handler. From my naive point of view, the race you mentioned is not a design flaw in Wayland but part of the real world that Wayland and its clients ought to deal with. Other than that call in the log handler, I see no reason why Xwayland should not just notice that the object has gone away again and get on with life. As I said, I do not know the code base well. But for the sake of the argument, and since it is not likely to make things worse, I will try removing that locally and see what happens. Regards and thanks. Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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 fatal error when Wayland output disappears
Hello Oliver, I will do that later, but I should have provided more background. This actually occurs if I open and close my laptop a couple of times in a short period of time. I originally did that while experimenting to see what would make GNOME Shell recognise my external monitor, and then trying to reproduce the various bugs which I found along the way. Regards Michael On 26.10.2017 16:01, Olivier Fourdan wrote: Hi, Could you please post a backtrace, breaking on xwl_log_handler() ? Another interesting data to capture would be the WAYLAND_DEBUG logs (between Xwayland and gnome-shell) wl_registry_bind() occurs when actually binding to an interface, which in the case of wl_output occurs when adding a new output, not removing it. Cheers, Olivier On 26 October 2017 at 15:40, Michael Thayer <michael.tha...@oracle.com <mailto:michael.tha...@oracle.com>> wrote: Hello, Reporting this here in case it is of interest. I have been debugging regular desktop crashes on my new Ubuntu 17.10 systems. One of them seems to happen when Xwayland FatalError-s out because a Wayland output disappears. I have only investigated this so far, but hopefully this will be helpful to someone who knows it better: the error is "wl_registry@2: error 0: invalid global wl_output (28)", which seems to be posted by wl_registry_bind in wayland-server.c, and get picked up in xwl_log_handler in Xwayland. Xwayland exiting in turn causes GNOME Shell to panic. Regards Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ xorg-devel@lists.x.org <mailto:xorg-devel@lists.x.org>: X.Org development Archives: http://lists.x.org/archives/xorg-devel <http://lists.x.org/archives/xorg-devel> Info: https://lists.x.org/mailman/listinfo/xorg-devel <https://lists.x.org/mailman/listinfo/xorg-devel> -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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
Xwayland fatal error when Wayland output disappears
Hello, Reporting this here in case it is of interest. I have been debugging regular desktop crashes on my new Ubuntu 17.10 systems. One of them seems to happen when Xwayland FatalError-s out because a Wayland output disappears. I have only investigated this so far, but hopefully this will be helpful to someone who knows it better: the error is "wl_registry@2: error 0: invalid global wl_output (28)", which seems to be posted by wl_registry_bind in wayland-server.c, and get picked up in xwl_log_handler in Xwayland. Xwayland exiting in turn causes GNOME Shell to panic. Regards Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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 2/2 v2] Add keyboard shortcuts inhibitor
ocus, the + the compositor will restore its own keyboard shortcuts. + + When the compositor restores its own keyboard shortcuts, an + "inhibit_inactive" event is emitted to notify the client that the + keyboard shortcuts inhibitor is not effectively active for the + surface any more, and the client should not expect to receive all + keyboard events. + + When the keyboard shortcuts inhibitor is inactive, either because + the user has requested it using any mechanism the compositor may offer + or because the surface doesn't have keyboard focus, the client has + no way to forcibly reactivate the keyboard shortcuts inhibitor. + + When the surface regains keyboard focus, the inhibitor will take effect + again and an "inhibit_active" event emitted to notify the client. + + + + + Remove the keyboard shortcuts inhibitor from the associated wl_surface. + + + + + + This event indicates that the shortcut inhibitor is active. + + The compositor will send this event every time it deactivates its + shortcuts for the given surface, this occurs typically when the + surface which requested keyboard shortcuts inhibit regains focus + or when the initial request "inhibit_shortcuts" becomes active. + + + + + + This event indicates that the shortcut inhibitor is inactive, +regular shortcuts processing is restored by the compositor. + + The compositor will send this event when the surface loses keyboard + focus or when the user restores the keyboard shortcuts using any + mechanism offered by the compositor. + + + + + + + + + ___ 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 -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: [xserver PULL master] modesetting HW cursor patches
Thanks Adam! Regards Michael 08.02.2017 18:05, Adam Jackson wrote: On Thu, 2017-02-02 at 11:43 +0100, Michael Thayer wrote: Respectful but rather frustrated ping. Is it still worth my trying to get these changes in or should I just give up? Sorry for the delay. The cursor code is that perfect horrifying intersection of input and output that means I hate reviewing it. (To be fair, apparently so does everybody else...) Fixed up for ickle's input_lock change and merged: remote: E: failed to find patch for rev c02f6a687c3d6bd0727322b055ee788f8fefa005. remote: I: patch #113066 updated using rev ecd0a62323f26b333c49bddd7237dd5118482a35. remote: I: patch #113067 updated using rev eb04b20160d706d4e0e67122d0adb1e723c0da92. remote: I: 2 patch(es) updated to state Accepted. To ssh://git.freedesktop.org/git/xorg/xserver 3ef16df..eb04b20 master -> master - ajax -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: [xserver PULL master] modesetting HW cursor patches
Respectful but rather frustrated ping. Is it still worth my trying to get these changes in or should I just give up? Regards Michael 27.01.2017 17:51, Michael Thayer wrote: Hello Adam and Keith, These patches have been hanging around for a while (since October in fact), and never made it into the tree. They were reviewed by Hans, who suggested that I try preparing a rebased branch with the patches and send a pull request. So finally done that. Thanks Michael The following changes since commit 7617a0a180a2cd3427a8ffa9534152df6a8fecbf: dri2: refine dri2_probe_driver_name (v2) (2017-01-25 15:13:33 -0500) are available in the git repository at: https://github.com/michael-thayer/xserver.git for you to fetch changes up to 4163091129eed352166a59c2681edc2da313e436: modesetting: allow switching from software to hardware cursors (v5). (2017-01-27 16:09:08 +0100) Michael Thayer (3): xfree86: Immediately handle failure to set HW cursor, v5 modesetting: Immediately handle failure to set HW cursor, v5 modesetting: allow switching from software to hardware cursors (v5). hw/xfree86/drivers/modesetting/drmmode_display.c | 45 ++-- hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- hw/xfree86/modes/xf86Crtc.h | 4 ++- hw/xfree86/modes/xf86Cursors.c | 40 +++-- hw/xfree86/ramdac/xf86Cursor.h | 16 + hw/xfree86/ramdac/xf86HWCurs.c | 8 ++--- 6 files changed, 71 insertions(+), 44 deletions(-) -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: [xserver PULL master] modesetting HW cursor patches
Hello Adam and Keith, For some reason you got left off CC on that message. No idea why, you are still there in the copy in my "sent" folder... Regards Michael 27.01.2017 17:51, Michael Thayer wrote: Hello Adam and Keith, These patches have been hanging around for a while (since October in fact), and never made it into the tree. They were reviewed by Hans, who suggested that I try preparing a rebased branch with the patches and send a pull request. So finally done that. Thanks Michael The following changes since commit 7617a0a180a2cd3427a8ffa9534152df6a8fecbf: dri2: refine dri2_probe_driver_name (v2) (2017-01-25 15:13:33 -0500) are available in the git repository at: https://github.com/michael-thayer/xserver.git for you to fetch changes up to 4163091129eed352166a59c2681edc2da313e436: modesetting: allow switching from software to hardware cursors (v5). (2017-01-27 16:09:08 +0100) -------- Michael Thayer (3): xfree86: Immediately handle failure to set HW cursor, v5 modesetting: Immediately handle failure to set HW cursor, v5 modesetting: allow switching from software to hardware cursors (v5). hw/xfree86/drivers/modesetting/drmmode_display.c | 45 ++-- hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- hw/xfree86/modes/xf86Crtc.h | 4 ++- hw/xfree86/modes/xf86Cursors.c | 40 +++-- hw/xfree86/ramdac/xf86Cursor.h | 16 + hw/xfree86/ramdac/xf86HWCurs.c | 8 ++--- 6 files changed, 71 insertions(+), 44 deletions(-) -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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
[xserver PULL master] modesetting HW cursor patches
Hello Adam and Keith, These patches have been hanging around for a while (since October in fact), and never made it into the tree. They were reviewed by Hans, who suggested that I try preparing a rebased branch with the patches and send a pull request. So finally done that. Thanks Michael The following changes since commit 7617a0a180a2cd3427a8ffa9534152df6a8fecbf: dri2: refine dri2_probe_driver_name (v2) (2017-01-25 15:13:33 -0500) are available in the git repository at: https://github.com/michael-thayer/xserver.git for you to fetch changes up to 4163091129eed352166a59c2681edc2da313e436: modesetting: allow switching from software to hardware cursors (v5). (2017-01-27 16:09:08 +0100) Michael Thayer (3): xfree86: Immediately handle failure to set HW cursor, v5 modesetting: Immediately handle failure to set HW cursor, v5 modesetting: allow switching from software to hardware cursors (v5). hw/xfree86/drivers/modesetting/drmmode_display.c | 45 ++-- hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- hw/xfree86/modes/xf86Crtc.h | 4 ++- hw/xfree86/modes/xf86Cursors.c | 40 +++-- hw/xfree86/ramdac/xf86Cursor.h | 16 + hw/xfree86/ramdac/xf86HWCurs.c | 8 ++--- 6 files changed, 71 insertions(+), 44 deletions(-) -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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/3] xfree86: Immediately handle failure to set HW cursor, v5
Hello Hans, Polite ping on this one. Regards and thanks Michael 01.10.2016 12:01, Hans de Goede wrote: Hi, On 30-09-16 17:55, Michael Thayer wrote: Based on v4 by Alexandre Courbot <acour...@nvidia.com> There is currently no reliable way to report failure to set a HW cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM ioctl fails (which currently happens at least with modesetting on Tegra for format incompatibility reasons). As failures are currently handled by setting the HW cursor size to (0,0), the fallback to SW cursor will not happen until the next time the cursor changes and xf86CursorSetCursor() is called again. In the meantime, the cursor will be invisible to the user. This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans. This allows to propagate errors up to xf86CursorSetCursor(), which can then fall back to using the SW cursor immediately. v5: Updated the patch to apply to current git HEAD, split up into two patches (server and modesetting driver) and adjusted the code slightly to match surrounding code. I also removed the new exported function ShowCursorCheck(), as instead just changing ShowCursor() to return Bool should not affect its current callers. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> Series looks good to me and is: Reviewed-by: Hans de Goede <hdego...@redhat.com> As discussed this will need to wait till we start the 1.20 cycle before it can be merged (it contains a video driver ABI change) Regards, Hans --- hw/xfree86/modes/xf86Crtc.h| 4 +++- hw/xfree86/modes/xf86Cursors.c | 40 ++-- hw/xfree86/ramdac/xf86Cursor.h | 16 hw/xfree86/ramdac/xf86HWCurs.c | 8 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index 14ba9d7..215eb2a 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -195,6 +195,8 @@ typedef struct _xf86CrtcFuncs { */ void (*show_cursor) (xf86CrtcPtr crtc); +Bool + (*show_cursor_check) (xf86CrtcPtr crtc); /** * Hide cursor @@ -993,7 +995,7 @@ static _X_INLINE _X_DEPRECATED void xf86_reload_cursors(ScreenPtr screen) {} /** * Called from EnterVT to turn the cursors back on */ -extern _X_EXPORT void +extern _X_EXPORT Bool xf86_show_cursors(ScrnInfoPtr scrn); /** diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 1bc2b27..9543eed 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -210,9 +210,15 @@ set_bit(CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool mask) /* * Wrappers to deal with API compatibility with drivers that don't expose - * load_cursor_*_check + * *_cursor_*_check */ static inline Bool +xf86_driver_has_show_cursor(xf86CrtcPtr crtc) +{ +return crtc->funcs->show_cursor_check || crtc->funcs->show_cursor; +} + +static inline Bool xf86_driver_has_load_cursor_image(xf86CrtcPtr crtc) { return crtc->funcs->load_cursor_image_check || crtc->funcs->load_cursor_image; @@ -225,6 +231,15 @@ xf86_driver_has_load_cursor_argb(xf86CrtcPtr crtc) } static inline Bool +xf86_driver_show_cursor(xf86CrtcPtr crtc) +{ +if (crtc->funcs->show_cursor_check) +return crtc->funcs->show_cursor_check(crtc); +crtc->funcs->show_cursor(crtc); +return TRUE; +} + +static inline Bool xf86_driver_load_cursor_image(xf86CrtcPtr crtc, CARD8 *cursor_image) { if (crtc->funcs->load_cursor_image_check) @@ -333,16 +348,19 @@ xf86_hide_cursors(ScrnInfoPtr scrn) } } -static void +static Bool xf86_crtc_show_cursor(xf86CrtcPtr crtc) { -if (!crtc->cursor_shown && crtc->cursor_in_range) { -crtc->funcs->show_cursor(crtc); -crtc->cursor_shown = TRUE; -} +if (!crtc->cursor_in_range) +return TRUE; + +if (!crtc->cursor_shown) +crtc->cursor_shown = xf86_driver_show_cursor(crtc); + +return crtc->cursor_shown; } -void +Bool xf86_show_cursors(ScrnInfoPtr scrn) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); @@ -352,9 +370,11 @@ xf86_show_cursors(ScrnInfoPtr scrn) for (c = 0; c < xf86_config->num_crtc; c++) { xf86CrtcPtr crtc = xf86_config->crtc[c]; -if (crtc->enabled) -xf86_crtc_show_cursor(crtc); +if (crtc->enabled && !xf86_crtc_show_cursor(crtc)) +return FALSE; } + +return TRUE; } static void @@ -653,7 +673,7 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags) cursor_info->SetCursorPosition = xf86_set_cursor_position; cursor_info->LoadCursorImageCheck = xf86_load_cursor_image; cursor_info->HideCursor = xf86_hide_cursors; -
Re: [PATCH xserver 1/3] xfree86: Immediately handle failure to set HW cursor, v5
Hello Adam, On 05.10.2016 21:33, Adam Jackson wrote: On Fri, 2016-09-30 at 17:55 +0200, Michael Thayer wrote: v5: Updated the patch to apply to current git HEAD, split up into two patches (server and modesetting driver) and adjusted the code slightly to match surrounding code. I also removed the new exported function ShowCursorCheck(), as instead just changing ShowCursor() to return Bool should not affect its current callers. I really hate that I have to say this, because it's entirely our fault for not having merged this sooner in the cycle, but this is an ABI break we can't take after RC1. By changing the layout of xf86CrtcFuncsRec, drivers built against RC1 will fail when run against RC2. Even adding show_cursor_check to the end of the struct won't help, because the server then can't know if the final slot in the struct contains valid data. I have a workaround in mind that will preserve ABI, I'll send it along shortly. Thanks for looking at it. The question though, is whether it still makes sense to try to merge it for 1.19 or to wait for 1.20, which I think is Hans' preferred option. I must say that if it means it will get more testing before becoming part of a release then I tend to lean that way too. A few pairs of eyes checking the code is good, but not as good as real use. Regards, Michael - ajax -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] modesetting: allow switching from software to hardware cursors (v5).
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to only permanently switch to a software cursor if -ENXIO is returned (which means hardware cursors not supported), and to otherwise still try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in hardware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Changes since v1, v2 and v3: * take into account the switch to load_cursor_argb_check * keep the permanent software cursor fall-back if -ENXIO is returned * move parts of v3 into separate patches Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- hw/xfree86/drivers/modesetting/drmmode_display.c | 30 ++-- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 8e68c0e..bbe1a5f 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,37 +756,33 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); int ret = -EINVAL; -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -/* -EINVAL can mean that an old kernel supports drmModeSetCursor but - * not drmModeSetCursor2, though it can mean other things too. */ -if (ret == -EINVAL) -drmmode_crtc->set_cursor2_failed = TRUE; -} +ret = drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, +handle, ms->cursor_width, ms->cursor_height, +cursor->bits->xhot, cursor->bits->yhot); +/* -EINVAL can mean that an old kernel supports drmModeSetCursor but + * not drmModeSetCursor2, though it can mean other things too. */ if (ret == -EINVAL) ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, ms->cursor_width, ms->cursor_height); -if (ret) { +/* -ENXIO normally means that the current drm driver supports neither + * cursor_set nor cursor_set2. Disable hardware cursor support for + * the rest of the session in that case. */ +if (ret == -ENXIO) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; cursor_info->MaxWidth = cursor_info->MaxHeight = 0; drmmode_crtc->drmmode->sw_cursor = TRUE; +} + +if (ret) /* fallback to swcursor */ return FALSE; -} return TRUE; } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 3b0eb2b..f774250 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -92,7 +92,6 @@ typedef struct { int dpms_mode; struct dumb_bo *cursor_bo; Bool cursor_up; -Bool set_cursor2_failed; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- 2.9.3 ___ 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/3] modesetting: Immediately handle failure to set HW cursor, v5
Based on v4 by Alexandre Courbot <acour...@nvidia.com> There is currently no reliable way to report failure to set a HW cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM ioctl fails (which currently happens at least with modesetting on Tegra for format incompatibility reasons). As failures are currently handled by setting the HW cursor size to (0,0), the fallback to SW cursor will not happen until the next time the cursor changes and xf86CursorSetCursor() is called again. In the meantime, the cursor will be invisible to the user. This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans. This allows to propagate errors up to xf86CursorSetCursor(), which can then fall back to using the SW cursor immediately. v5: - Removed parts of patch already committed as part of 14c21ea1. - Adjusted code slightly to match surrounding code. - Effectively reverted af916477 which is made unnecessary by this patch. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- hw/xfree86/drivers/modesetting/drmmode_display.c | 15 +-- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 5 insertions(+), 11 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 61a0e27..8e68c0e 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -813,13 +813,8 @@ drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image) for (i = 0; i < ms->cursor_width * ms->cursor_height; i++) ptr[i] = image[i]; // cpu_to_le32(image[i]); -if (drmmode_crtc->cursor_up || !drmmode_crtc->first_cursor_load_done) { -Bool ret = drmmode_set_cursor(crtc); -if (!drmmode_crtc->cursor_up) -drmmode_hide_cursor(crtc); -drmmode_crtc->first_cursor_load_done = TRUE; -return ret; -} +if (drmmode_crtc->cursor_up) +return drmmode_set_cursor(crtc); return TRUE; } @@ -835,12 +830,12 @@ drmmode_hide_cursor(xf86CrtcPtr crtc) ms->cursor_width, ms->cursor_height); } -static void +static Bool drmmode_show_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_crtc->cursor_up = TRUE; -drmmode_set_cursor(crtc); +return drmmode_set_cursor(crtc); } static void @@ -1108,7 +1103,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = { .set_mode_major = drmmode_set_mode_major, .set_cursor_colors = drmmode_set_cursor_colors, .set_cursor_position = drmmode_set_cursor_position, -.show_cursor = drmmode_show_cursor, +.show_cursor_check = drmmode_show_cursor, .hide_cursor = drmmode_hide_cursor, .load_cursor_argb_check = drmmode_load_cursor_argb_check, diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..3b0eb2b 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -93,7 +93,6 @@ typedef struct { struct dumb_bo *cursor_bo; Bool cursor_up; Bool set_cursor2_failed; -Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- 2.9.3 ___ 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/3] xfree86: Immediately handle failure to set HW cursor, v5
Based on v4 by Alexandre Courbot <acour...@nvidia.com> There is currently no reliable way to report failure to set a HW cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM ioctl fails (which currently happens at least with modesetting on Tegra for format incompatibility reasons). As failures are currently handled by setting the HW cursor size to (0,0), the fallback to SW cursor will not happen until the next time the cursor changes and xf86CursorSetCursor() is called again. In the meantime, the cursor will be invisible to the user. This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans. This allows to propagate errors up to xf86CursorSetCursor(), which can then fall back to using the SW cursor immediately. v5: Updated the patch to apply to current git HEAD, split up into two patches (server and modesetting driver) and adjusted the code slightly to match surrounding code. I also removed the new exported function ShowCursorCheck(), as instead just changing ShowCursor() to return Bool should not affect its current callers. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- hw/xfree86/modes/xf86Crtc.h| 4 +++- hw/xfree86/modes/xf86Cursors.c | 40 ++-- hw/xfree86/ramdac/xf86Cursor.h | 16 hw/xfree86/ramdac/xf86HWCurs.c | 8 4 files changed, 53 insertions(+), 15 deletions(-) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index 14ba9d7..215eb2a 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -195,6 +195,8 @@ typedef struct _xf86CrtcFuncs { */ void (*show_cursor) (xf86CrtcPtr crtc); +Bool + (*show_cursor_check) (xf86CrtcPtr crtc); /** * Hide cursor @@ -993,7 +995,7 @@ static _X_INLINE _X_DEPRECATED void xf86_reload_cursors(ScreenPtr screen) {} /** * Called from EnterVT to turn the cursors back on */ -extern _X_EXPORT void +extern _X_EXPORT Bool xf86_show_cursors(ScrnInfoPtr scrn); /** diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 1bc2b27..9543eed 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -210,9 +210,15 @@ set_bit(CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool mask) /* * Wrappers to deal with API compatibility with drivers that don't expose - * load_cursor_*_check + * *_cursor_*_check */ static inline Bool +xf86_driver_has_show_cursor(xf86CrtcPtr crtc) +{ +return crtc->funcs->show_cursor_check || crtc->funcs->show_cursor; +} + +static inline Bool xf86_driver_has_load_cursor_image(xf86CrtcPtr crtc) { return crtc->funcs->load_cursor_image_check || crtc->funcs->load_cursor_image; @@ -225,6 +231,15 @@ xf86_driver_has_load_cursor_argb(xf86CrtcPtr crtc) } static inline Bool +xf86_driver_show_cursor(xf86CrtcPtr crtc) +{ +if (crtc->funcs->show_cursor_check) +return crtc->funcs->show_cursor_check(crtc); +crtc->funcs->show_cursor(crtc); +return TRUE; +} + +static inline Bool xf86_driver_load_cursor_image(xf86CrtcPtr crtc, CARD8 *cursor_image) { if (crtc->funcs->load_cursor_image_check) @@ -333,16 +348,19 @@ xf86_hide_cursors(ScrnInfoPtr scrn) } } -static void +static Bool xf86_crtc_show_cursor(xf86CrtcPtr crtc) { -if (!crtc->cursor_shown && crtc->cursor_in_range) { -crtc->funcs->show_cursor(crtc); -crtc->cursor_shown = TRUE; -} +if (!crtc->cursor_in_range) +return TRUE; + +if (!crtc->cursor_shown) +crtc->cursor_shown = xf86_driver_show_cursor(crtc); + +return crtc->cursor_shown; } -void +Bool xf86_show_cursors(ScrnInfoPtr scrn) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); @@ -352,9 +370,11 @@ xf86_show_cursors(ScrnInfoPtr scrn) for (c = 0; c < xf86_config->num_crtc; c++) { xf86CrtcPtr crtc = xf86_config->crtc[c]; -if (crtc->enabled) -xf86_crtc_show_cursor(crtc); +if (crtc->enabled && !xf86_crtc_show_cursor(crtc)) +return FALSE; } + +return TRUE; } static void @@ -653,7 +673,7 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags) cursor_info->SetCursorPosition = xf86_set_cursor_position; cursor_info->LoadCursorImageCheck = xf86_load_cursor_image; cursor_info->HideCursor = xf86_hide_cursors; -cursor_info->ShowCursor = xf86_show_cursors; +cursor_info->ShowCursorCheck = xf86_show_cursors; cursor_info->UseHWCursor = xf86_use_hw_cursor; if (flags & HARDWARE_CURSOR_ARGB) { cursor_info->UseHWCursorARGB = xf86_use_hw_cursor_argb; diff --git a/hw/xfree86/ramdac/xf86Cursor.h b/hw/xfree86/ramdac/xf86Cursor.h index 320ec0c..11a03b6 1
Re: [PATCH xserver] modesetting: always set a hardware cursor when requested to load one.
Hello Hans, On 29.09.2016 09:56, Hans de Goede wrote: Hi, On 28-09-16 16:54, Michael Thayer wrote: Hello Hans, On 28.09.2016 15:37, Hans de Goede wrote: Hi Michael, On 28-09-16 14:47, Michael Thayer wrote: [...] On 09/16/2016 06:52 PM, Michael Thayer wrote: When the X server asks us to load a hardware cursor, that request is always followed up by a request to show it if we report success, or to hide it if we report failure. Therefore it makes no sense to suppress the request if the cursor is not currently visible. [...] For the record, I looked at making show_cursor() return a boolean. It seemed feasible, and would allow for removing the first time hack in modesetting too Making show_cursor return an error (adding a show_cursor_check()) seems to be the best solution to me, we actually had a patch submitted for this, but it was deemed unnecessary. I guess it is necessary after all: https://patchwork.freedesktop.org/patch/94495/ but there was a catch in that the cursor could theoretically be visible but still hidden on all screens (with a strange screen layout), Yes that is possible. in which case show_cursor() would not be called until the cursor moved onto a screen. Ack. Making set_cursor_position() return a success value seemed a bit too invasive. But in the scenario you describe it is not the drivers set_cursor_position() which will get called (well not only) also the drivers' show_cursor will get called on the crtc where the cursor should now show, so just making show_cursor return an error should be enough. [...] Right, xf86_crtc_set_cursor_position() calls the driver's show_cursor(). What I meant was that that means that this function can now fail, and that failure needs to be propagated up and probably handled in xf86CursorMoveCursor() by reverting to a software cursor there. The alternative is another first time-style hack (if we are asked to show the cursor and it is not in range of any of the crtc-s then flicker it). Hmm, so 2 things: 1) We definitely do not want another first-time hack, so if we really need to lets do the propagate error thing 2) Thinking about holes in crtc layouts again, I think the xrandr cursor constraint code will warp the pointer to the closest crtc then (pretty sure actually) so I think this is a non issue. So just rebasing the patch I linked (and dropping the first time hack) should be enough. And since we're planning to do this for 1.20, I think we should just try doing things that way, we've an entire cycle to catch any issues then (which I do not believe there will be) I rebased Alexandre's patch (not sure if there is an official way to credit someone, I just did so in the commit message) as two new patches, one to xfree86 and one to modesetting (in which I also got rid of the first time hack), both slightly adjusted to be more like existing code. I rebased my software-back-to-hardware on top of that. I did not follow all code paths in detail, but it does look to me as if what you said about the constraint code is what is intended (can someone who knows RandR confirm?), so that if it does not work then it is the constraint code which should be fixed. I tested that switching to a software cursor happens as soon as drmmode_show_cursor() is called, that the test for hardware cursor support is repeated on every show or set call, and although I can't confirm visually (we only have a single hardware cursor, not one per crtc), I checked as best I could using break-points in gdb that showing and hiding cursors happened correctly on multiple screens. Feel free to suggest additional tests. Patches coming up. Regards, Michael Regards, Hans Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: always set a hardware cursor when requested to load one.
Hello Hans, On 28.09.2016 15:37, Hans de Goede wrote: Hi Michael, On 28-09-16 14:47, Michael Thayer wrote: [...] On 09/16/2016 06:52 PM, Michael Thayer wrote: When the X server asks us to load a hardware cursor, that request is always followed up by a request to show it if we report success, or to hide it if we report failure. Therefore it makes no sense to suppress the request if the cursor is not currently visible. [...] For the record, I looked at making show_cursor() return a boolean. It seemed feasible, and would allow for removing the first time hack in modesetting too Making show_cursor return an error (adding a show_cursor_check()) seems to be the best solution to me, we actually had a patch submitted for this, but it was deemed unnecessary. I guess it is necessary after all: https://patchwork.freedesktop.org/patch/94495/ but there was a catch in that the cursor could theoretically be visible but still hidden on all screens (with a strange screen layout), Yes that is possible. in which case show_cursor() would not be called until the cursor moved onto a screen. Ack. Making set_cursor_position() return a success value seemed a bit too invasive. But in the scenario you describe it is not the drivers set_cursor_position() which will get called (well not only) also the drivers' show_cursor will get called on the crtc where the cursor should now show, so just making show_cursor return an error should be enough. [...] Right, xf86_crtc_set_cursor_position() calls the driver's show_cursor(). What I meant was that that means that this function can now fail, and that failure needs to be propagated up and probably handled in xf86CursorMoveCursor() by reverting to a software cursor there. The alternative is another first time-style hack (if we are asked to show the cursor and it is not in range of any of the crtc-s then flicker it). Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: always set a hardware cursor when requested to load one.
Hello, On 28.09.2016 09:58, Hans de Goede wrote: Hi, On 28-09-16 05:05, Michel Dänzer wrote: [...] On 09/16/2016 06:52 PM, Michael Thayer wrote: When the X server asks us to load a hardware cursor, that request is always followed up by a request to show it if we report success, or to hide it if we report failure. Therefore it makes no sense to suppress the request if the cursor is not currently visible. [...] TBH, I feel like the issues you're having with VirtualBox would be better dealt with at the virtual GPU hardware level, by making the HW cursor work reliably for guests and making it the hypervisor's responsibility to draw the cursor on the host side when the HW cursor can't be used there. Yes that would be me suggestion too, I think that playing the whole now we support hw cursor now we don't game does not really work. Esp. since if the guest is just idle, it will not try any drmSetCursor calls, so it will not even know the support has changed underneath it. If that is the case then you should nack the third in the series too, as it does not make sense (or even apply) without the second. For the record, I looked at making show_cursor() return a boolean. It seemed feasible, and would allow for removing the first time hack in modesetting too, but there was a catch in that the cursor could theoretically be visible but still hidden on all screens (with a strange screen layout), in which case show_cursor() would not be called until the cursor moved onto a screen. Making set_cursor_position() return a success value seemed a bit too invasive. Replacing the first time hack with a "check if the cursor is up on at least one screen, else load it briefly" would work, but cause a cursor flicker each time the cursor went from hidden to shown. Thanks for the review time anyway. Regards, Michael Regards, Hans -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: always set a hardware cursor when requested to load one.
Hello, On 27.09.2016 12:06, Hans de Goede wrote: Hi, On 23-09-16 10:20, Hans de Goede wrote: Hi All, On 09/16/2016 06:52 PM, Michael Thayer wrote: When the X server asks us to load a hardware cursor, that request is always followed up by a request to show it if we report success, or to hide it if we report failure. Therefore it makes no sense to suppress the request if the cursor is not currently visible. Ok, I've just spend the last half hour tracing (using grep) all callers of load_cursor_argb_check and you're right, either the cursor is already visible (XRecolorCursor does this), or it will get shown directly after the drmmode_load_cursor_argb_check() call. And despite double checking I still missed a trouble-some scenario here. If we've 2 monitors active, then as the cursor moves from one to the other show()/hide() gets called on them. If the cursor then changes load_cursor_argb_check() gets called on all active crtc-s causing the cursor to now be shown on both monitors at the same time, not good. Indeed. Looking at other ways of handling this, but the whole thing is somewhat tricky due to the differences in how the X server and the kernel handle cursor setting. I think that the most correct way to fix this would be to allow show and hide cursor operations to return failure too, but that would be a very invasive change. A less invasive but also less clean change would be to extend the first time hack to also trigger if a load happens when the cursor is hidden globally (or perhaps better, if it is hidden on all crtc-s at the time of the load). For VirtualBox, if a cursor load succeeds on one crtc we know it will succeed on any, but I wonder whether that would apply to other users. I also wonder which other potential users there are. So far most other people use their own DDX instead of modesetting. Alexandre, did you say this was an issue for Tegra? And it is potentially interesting for Qemu for the same reasons that it is for us (relative input device support). Hans, any thoughts? Probably better not to try to force anything into 1.19. Regards and thanks, Michael So this patch is: Reviewed-by: Hans de Goede <hdego...@redhat.com> So I hereby withdraw my Reviewed-by, and have to NACK this in its current form as it is causing regressions in multi-monitor setups. I'll send a v2 my pull-req with fixes for 1.19, dropping this one and related follow-up patches. Regards, Hans Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Tested that both hardware and software cursors still work after applying the patch. hw/xfree86/drivers/modesetting/drmmode_display.c | 11 +-- hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 41810d9..7d171a3 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -813,14 +813,7 @@ drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image) for (i = 0; i < ms->cursor_width * ms->cursor_height; i++) ptr[i] = image[i]; // cpu_to_le32(image[i]); -if (drmmode_crtc->cursor_up || !drmmode_crtc->first_cursor_load_done) { -Bool ret = drmmode_set_cursor(crtc); -if (!drmmode_crtc->cursor_up) -drmmode_hide_cursor(crtc); -drmmode_crtc->first_cursor_load_done = TRUE; -return ret; -} -return TRUE; +return drmmode_set_cursor(crtc); } static void @@ -830,7 +823,6 @@ drmmode_hide_cursor(xf86CrtcPtr crtc) drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; -drmmode_crtc->cursor_up = FALSE; drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, 0, ms->cursor_width, ms->cursor_height); } @@ -839,7 +831,6 @@ static void drmmode_show_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; -drmmode_crtc->cursor_up = TRUE; drmmode_set_cursor(crtc); } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..bd82968 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -91,9 +91,7 @@ typedef struct { uint32_t vblank_pipe; int dpms_mode; struct dumb_bo *cursor_bo; -Bool cursor_up; Bool set_cursor2_failed; -Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgerich
Re: [PATCH xserver] modesetting: Do not call drmModeSetCursor2() twice on every cursor change
Each xf86ScreenSetCursor() call calls: xf86DriverLoadCursorARGB() infoPtr->ShowCursor() In succession, ending up in 2 drmModeSetCursor2() calls, with the second effectively being a no-op. Keep track of having set the cursor already in drmmode_load_cursor_argb_check() and unless hide() was called in the mean time make drmmode_show_cursor() a no-op. Signed-off-by: Hans de Goede --- Note this patch applies on top of Michael Thayer's "modesetting: allow switching from software to hardware cursors (v4)" series Sorry about this, for some reason this and a few other messages did not make it to my mailbox. Looks good to me, but if you like I can apply it when I have time and check in gdb that it does what it says on the box. Otherwise, Reviewed-by: Michael Thayer <michael.tha...@oracle.com> Regards, Michael --- hw/xfree86/drivers/modesetting/drmmode_display.c | 7 ++- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 + 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 46e981b..23c1db1 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -783,6 +783,8 @@ drmmode_set_cursor(xf86CrtcPtr crtc) if (ret) /* fallback to swcursor */ return FALSE; + +drmmode_crtc->cursor_up = TRUE; return TRUE; } @@ -817,6 +819,7 @@ drmmode_hide_cursor(xf86CrtcPtr crtc) drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; +drmmode_crtc->cursor_up = FALSE; drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, 0, ms->cursor_width, ms->cursor_height); } @@ -825,7 +828,9 @@ static void drmmode_show_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; -drmmode_set_cursor(crtc); + +if (!drmmode_crtc->cursor_up) +drmmode_set_cursor(crtc); } static void diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index f979b99..f774250 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -91,6 +91,7 @@ typedef struct { uint32_t vblank_pipe; int dpms_mode; struct dumb_bo *cursor_bo; +Bool cursor_up; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- 2.9.3 -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: modesetting: allow switching from software to hardware cursors (v4)
On 16.09.2016 17:49, Michael Thayer wrote: Split this up into the three following patches as suggested by Hans. Many thanks for your patience! Polite ping. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: allow switching from software to hardware cursors (v4).
On 16.09.2016 17:52, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to only permanently switch to a software cursor if -ENXIO is returned (which means hardware cursors not supported), and to otherwise still try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in hardware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Changes since v1, v2 and v3: * take into account the switch to load_cursor_argb_check * keep the permanent software cursor fall-back if -ENXIO is returned * move parts of v3 into separate patches Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Tested switching from a hardware to a software cursor and back. Tested the -ENXIO permanent fall-back to a software cursor in gdb. Michael hw/xfree86/drivers/modesetting/drmmode_display.c | 30 ++-- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 7d171a3..7b66795 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,37 +756,33 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); int ret; -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -/* -EINVAL can mean that an old kernel supports drmModeSetCursor but - * not drmModeSetCursor2, though it can mean other things too. */ -if (ret == -EINVAL) -drmmode_crtc->set_cursor2_failed = TRUE; -} +ret = drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, +handle, ms->cursor_width, ms->cursor_height, +cursor->bits->xhot, cursor->bits->yhot); +/* -EINVAL can mean that an old kernel supports drmModeSetCursor but + * not drmModeSetCursor2, though it can mean other things too. */ if (ret == -EINVAL) ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, ms->cursor_width, ms->cursor_height); -if (ret) { +/* -ENXIO normally means that the current drm driver supports neither + * cursor_set nor cursor_set2. Disable hardware cursor support for + * the rest of the session in that case. */ +if (ret == -ENXIO) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; cursor_info->MaxWidth = cursor_info->MaxHeight = 0; drmmode_crtc->drmmode->sw_cursor = TRUE; +} + +if (ret) /* fallback to swcursor */ return FALSE; -} return TRUE; } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index bd82968..f979b99 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -91,7 +91,6 @@ typedef struct { uint32_t vblank_pipe; int dpms_mode; struct dumb_bo *cursor_bo; -Bool set_cursor2_failed; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: allow switching from software to hardware cursors (v4).
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to only permanently switch to a software cursor if -ENXIO is returned (which means hardware cursors not supported), and to otherwise still try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in hardware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Changes since v1, v2 and v3: * take into account the switch to load_cursor_argb_check * keep the permanent software cursor fall-back if -ENXIO is returned * move parts of v3 into separate patches Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Tested switching from a hardware to a software cursor and back. hw/xfree86/drivers/modesetting/drmmode_display.c | 30 ++-- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 13 insertions(+), 18 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 7d171a3..7b66795 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,37 +756,33 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); int ret; -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -/* -EINVAL can mean that an old kernel supports drmModeSetCursor but - * not drmModeSetCursor2, though it can mean other things too. */ -if (ret == -EINVAL) -drmmode_crtc->set_cursor2_failed = TRUE; -} +ret = drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, +handle, ms->cursor_width, ms->cursor_height, +cursor->bits->xhot, cursor->bits->yhot); +/* -EINVAL can mean that an old kernel supports drmModeSetCursor but + * not drmModeSetCursor2, though it can mean other things too. */ if (ret == -EINVAL) ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, ms->cursor_width, ms->cursor_height); -if (ret) { +/* -ENXIO normally means that the current drm driver supports neither + * cursor_set nor cursor_set2. Disable hardware cursor support for + * the rest of the session in that case. */ +if (ret == -ENXIO) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; cursor_info->MaxWidth = cursor_info->MaxHeight = 0; drmmode_crtc->drmmode->sw_cursor = TRUE; +} + +if (ret) /* fallback to swcursor */ return FALSE; -} return TRUE; } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index bd82968..f979b99 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -91,7 +91,6 @@ typedef struct { uint32_t vblank_pipe; int dpms_mode; struct dumb_bo *cursor_bo; -Bool set_cursor2_failed; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- 2.9.3 ___ 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: always set a hardware cursor when requested to load one.
When the X server asks us to load a hardware cursor, that request is always followed up by a request to show it if we report success, or to hide it if we report failure. Therefore it makes no sense to suppress the request if the cursor is not currently visible. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Tested that both hardware and software cursors still work after applying the patch. hw/xfree86/drivers/modesetting/drmmode_display.c | 11 +-- hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- 2 files changed, 1 insertion(+), 12 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 41810d9..7d171a3 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -813,14 +813,7 @@ drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image) for (i = 0; i < ms->cursor_width * ms->cursor_height; i++) ptr[i] = image[i]; // cpu_to_le32(image[i]); -if (drmmode_crtc->cursor_up || !drmmode_crtc->first_cursor_load_done) { -Bool ret = drmmode_set_cursor(crtc); -if (!drmmode_crtc->cursor_up) -drmmode_hide_cursor(crtc); -drmmode_crtc->first_cursor_load_done = TRUE; -return ret; -} -return TRUE; +return drmmode_set_cursor(crtc); } static void @@ -830,7 +823,6 @@ drmmode_hide_cursor(xf86CrtcPtr crtc) drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; -drmmode_crtc->cursor_up = FALSE; drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, 0, ms->cursor_width, ms->cursor_height); } @@ -839,7 +831,6 @@ static void drmmode_show_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; -drmmode_crtc->cursor_up = TRUE; drmmode_set_cursor(crtc); } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..bd82968 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -91,9 +91,7 @@ typedef struct { uint32_t vblank_pipe; int dpms_mode; struct dumb_bo *cursor_bo; -Bool cursor_up; Bool set_cursor2_failed; -Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- 2.9.3 ___ 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: only fall back to drmModeSetCursor() on -EINVAL
This change effectively reverts commit 074cf58. We were falling back from drmModeSetCursor2() to drmModeSetCursor() whenever the first failed. This fall-back only makes sense on pre-mid-2013 kernels which implemented the cursor_set hook but not cursor_set2, and in this case the call to drmModeSetCursor2() will always return -EINVAL. Specifically, a return value of -ENXIO usually means that neither are supported. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- 1) Tested that hardware cursors still work with the patch applied. 2) Tested in gdb that the expected path is taken when drmModeSetCursor2() returns -EINVAL. hw/xfree86/drivers/modesetting/drmmode_display.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 6b933e4..41810d9 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -768,11 +768,15 @@ drmmode_set_cursor(xf86CrtcPtr crtc) if (!ret) return TRUE; -drmmode_crtc->set_cursor2_failed = TRUE; +/* -EINVAL can mean that an old kernel supports drmModeSetCursor but + * not drmModeSetCursor2, though it can mean other things too. */ +if (ret == -EINVAL) +drmmode_crtc->set_cursor2_failed = TRUE; } -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height); if (ret) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -- 2.9.3 ___ 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
modesetting: allow switching from software to hardware cursors (v4)
Split this up into the three following patches as suggested by Hans. Many thanks for your patience! Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: allow switching from software to hardware cursors (v3).
Hello, On 15.09.2016 19:18, Hans de Goede wrote: Hi, On 15-09-16 16:40, Michael Thayer wrote: Hello Hans, On 15.09.2016 16:12, Hans de Goede wrote: Hi, On 15-09-16 12:04, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Changes since v1 and v2: * take into account the switch to load_cursor_argb_check * altogether drop the fall-back path to SetCursor() if SetCursor2() fails: SetCursor2() has been supported in released kernels for three years now, and I think a software cursor fallback is acceptable if anyone has to use 1.19 with such an old kernel Nack for this change, the software cursor experience really is sub-optimal, I'm pretty sure people which are stuck on old kernels for some reason will greatly prefer needing 2 ioctls per set_cursor (it is not like that happens 1000-s times a second) vs software cursors. I mean isn't the whole purpose of this patch-set to avoid software cursors in virtualbox ? My assumption was that people stuck with old kernels would also stick to older X servers, but as I said, your call there. Ok, so lets agree to disagree and keep trying both ioctls please. Acknowledged. * drop the special case for loading a cursor sprite when the cursor is hidden: we set HARDWARE_CURSOR_UPDATE_UNHIDDEN, so all calls to load_cursor will be followed by calls to show_cursor unless the load fails (when a call to hide_cursor should follow) Nack again, have you read the actual commit msg of the commit which introduced the change ? : https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e If we revert this change (which really should be in a separate patch btw) then software cursor fallback will not work reliably because the first drmmode_load_cursor_argb_check() will succeed as it is a nop when the cursor is hidden, at which point the server will continue trying to use the hw-cursor until the cursor is changed to a different cursor. I did actually read the commit message, as well as the patch itself. My patch removes the problem which that patch was intended to fix, I do not think so, the problem as described there is that the first call to drmmode_load_cursor_argb_check() will succeed on systems without hw-cursor capability at all, causing the server to stay in hw-cursor mode. I honestly did understand what the problem is. But the reason that the call will succeed at all is because of a short section of code in modesetting/drmmode_display.c, which my patch removes. It has nothing to do with any code further up in the X server and is not generic X server behaviour. I justified in previous messages why it is safe to remove that code. In fact I just tested this by disabling hardware cursor support before X server start-up, and the first call to drmmode_load_cursor_argb_check() failed as I expected and intended. Here is how I understand what happens: 0) Driver inits, initial value of drmmode_crtc->cursor_up = FALSE; 1) server calls drmmode_load_cursor_argb_check() to set the initial cursor pixmap Without the patch you're in essence reverting this will always succeed since no attempt to actual set the cursor is done, so on non hw cursor capable hardware the server will still not enable sw-cursor support at this point in time 2) server calls drmmode_show_cursor() this calls drmmode_set_cursor() to actually upload and enable the cursor passed into drmmode_load_cursor_argb_check() earlier, this will fail, but drmmode_show_cursor() has a void return value, so the server still thinks it is happily using a hw-cursor 3) User is stuck with not having a cursor (until a future drmmode_load_cursor_argb_check() call) So the problem is that show_cursor cannot return errors, this also shows that things are broken wrt the now we support a hw-cursor now we don't dance virtual box does. AFAIK virtual box is unique in this, e.g. the qxl device always supports a hardware cursor from the guest pov independent of it is running in a mode where the guest and client cursor position map 1:1. as looking at the rest of the server and driver code it clearly makes no sense to skip the set_cursor call when the cursor is hidden. Except that showing it later may fail then, and at that point we cannot notify the core server code of this. This makes me wonder if your change is a good idea at all, I'm getting the feeling that the whole dynamic now we support hw cursors now we don't trick virtualbox is playing is really just a bad idea... Again, your call. As far as I know though, there is also physical hardw
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).
Hello Hans, On 15.09.2016 16:12, Hans de Goede wrote: Hi, On 15-09-16 12:04, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Changes since v1 and v2: * take into account the switch to load_cursor_argb_check * altogether drop the fall-back path to SetCursor() if SetCursor2() fails: SetCursor2() has been supported in released kernels for three years now, and I think a software cursor fallback is acceptable if anyone has to use 1.19 with such an old kernel Nack for this change, the software cursor experience really is sub-optimal, I'm pretty sure people which are stuck on old kernels for some reason will greatly prefer needing 2 ioctls per set_cursor (it is not like that happens 1000-s times a second) vs software cursors. I mean isn't the whole purpose of this patch-set to avoid software cursors in virtualbox ? My assumption was that people stuck with old kernels would also stick to older X servers, but as I said, your call there. * drop the special case for loading a cursor sprite when the cursor is hidden: we set HARDWARE_CURSOR_UPDATE_UNHIDDEN, so all calls to load_cursor will be followed by calls to show_cursor unless the load fails (when a call to hide_cursor should follow) Nack again, have you read the actual commit msg of the commit which introduced the change ? : https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e If we revert this change (which really should be in a separate patch btw) then software cursor fallback will not work reliably because the first drmmode_load_cursor_argb_check() will succeed as it is a nop when the cursor is hidden, at which point the server will continue trying to use the hw-cursor until the cursor is changed to a different cursor. I did actually read the commit message, as well as the patch itself. My patch removes the problem which that patch was intended to fix, as looking at the rest of the server and driver code it clearly makes no sense to skip the set_cursor call when the cursor is hidden. This makes me wonder if your change is a good idea at all, I'm getting the feeling that the whole dynamic now we support hw cursors now we don't trick virtualbox is playing is really just a bad idea... Again, your call. As far as I know though, there is also physical hardware which can display some cursors but not others and which would also need a change on these lines. If you can live with the second part of the change which you commented on I can change the first. Without the second change the patch does not work in its current form. Regards, Michael Regards, Hans Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- I hope that you are happy with the change I made to remove the set_cursor1 fall-back (it does rather simplify the code!); if not I will send v4. hw/xfree86/drivers/modesetting/drmmode_display.c | 40 hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- 2 files changed, 6 insertions(+), 36 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 6b933e4..bf5ca32 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,33 +756,12 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -int ret; - -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -drmmode_crtc->set_cursor2_failed = TRUE; -} - -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; - -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback t
[PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Changes since v1 and v2: * take into account the switch to load_cursor_argb_check * altogether drop the fall-back path to SetCursor() if SetCursor2() fails: SetCursor2() has been supported in released kernels for three years now, and I think a software cursor fallback is acceptable if anyone has to use 1.19 with such an old kernel * drop the special case for loading a cursor sprite when the cursor is hidden: we set HARDWARE_CURSOR_UPDATE_UNHIDDEN, so all calls to load_cursor will be followed by calls to show_cursor unless the load fails (when a call to hide_cursor should follow) Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- I hope that you are happy with the change I made to remove the set_cursor1 fall-back (it does rather simplify the code!); if not I will send v4. hw/xfree86/drivers/modesetting/drmmode_display.c | 40 hw/xfree86/drivers/modesetting/drmmode_display.h | 2 -- 2 files changed, 6 insertions(+), 36 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 6b933e4..bf5ca32 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,33 +756,12 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -int ret; - -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -drmmode_crtc->set_cursor2_failed = TRUE; -} - -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; - -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -return FALSE; -} +if (drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot)) +return FALSE; /* fallback to swcursor */ return TRUE; } @@ -809,14 +788,7 @@ drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image) for (i = 0; i < ms->cursor_width * ms->cursor_height; i++) ptr[i] = image[i]; // cpu_to_le32(image[i]); -if (drmmode_crtc->cursor_up || !drmmode_crtc->first_cursor_load_done) { -Bool ret = drmmode_set_cursor(crtc); -if (!drmmode_crtc->cursor_up) -drmmode_hide_cursor(crtc); -drmmode_crtc->first_cursor_load_done = TRUE; -return ret; -} -return TRUE; +return drmmode_set_cursor(crtc); } static void diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..f774250 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -92,8 +92,6 @@ typedef struct { int dpms_mode; struct dumb_bo *cursor_bo; Bool cursor_up; -Bool set_cursor2_failed; -Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; drmmode_bo rotate_bo; -- 2.9.3 ___ 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: allow switching from software to hardware cursors.
On 14.09.2016 12:07, Hans de Goede wrote: Hi, On 13-09-16 17:42, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Checked the current git source and this change is still needed. This is an updated patch which takes into account changes in the driver source since the original patch was created. Note that other than building I have not had a chance to test that the updated patch still works, but the difference against the original is pretty minimal. hw/xfree86/drivers/modesetting/drmmode_display.c | 36 +--- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 6b933e4..ad39f76 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,33 +756,23 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); int ret; -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -drmmode_crtc->set_cursor2_failed = TRUE; -} - -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) You're reintroducing the -EINVAL check which was deliberately dropped recently because sometimes another error is given while the non 2 version might still work, please drop this bit. Also please actually test the patch before posting a new version. I did some testing which did indeed show a problem with the patch (not sure if the original version had this problem, but I certainly did not hit it when I tested it): if the cursor is currently hidden then the cursor set is silently skipped. There is some logic added in af91647 to work around this once. This logic seems unnecessary to me, since all successful calls to set the cursor in the server code are immediately followed up by a call to show the cursor (see xf86HWCurs.c), and failed calls should result in the cursor being hidden anyway before the software cursor is enabled. Updated (tested) patch to follow. Thanks and regards, Michael Other then that this looks like an ok change to me. Regards, Hans +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, + ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -return FALSE; -} +if (ret) +return FALSE; /* fallback to swcursor */ return TRUE; } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..5170bf5 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -92,7 +92,6 @@ typedef struct { int dpms_mode; struct dumb_bo *cursor_bo; Bool cursor_up; -Bool set_cursor2_failed; Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registe
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors.
On 14.09.2016 12:07, Hans de Goede wrote: Hi, On 13-09-16 17:42, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Checked the current git source and this change is still needed. This is an updated patch which takes into account changes in the driver source since the original patch was created. Note that other than building I have not had a chance to test that the updated patch still works, but the difference against the original is pretty minimal. hw/xfree86/drivers/modesetting/drmmode_display.c | 36 +--- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 6b933e4..ad39f76 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,33 +756,23 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); int ret; -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -drmmode_crtc->set_cursor2_failed = TRUE; -} - -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) You're reintroducing the -EINVAL check which was deliberately dropped recently because sometimes another error is given while the non 2 version might still work, please drop this bit. I see that the commit message to 074cf587 states that sometimes ENXIO can be returned. Sorry if I am being blind here (it would not be the first time), but I cannot see which path that could happen in. It also seems a bit silly to unconditionally call drmModeSetCursor() every time drmModeSetCursor2() fails if it can be reasonably avoided. So if I am being blind, would it be alright if I made the test (ret == -EINVAL || ret == ENXIO)? Not that it would bring the world to an end, but still. Regards, Michael Also please actually test the patch before posting a new version. Other then that this looks like an ok change to me. Regards, Hans +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, + ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -return FALSE; -} +if (ret) +return FALSE; /* fallback to swcursor */ return TRUE; } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..5170bf5 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -92,7 +92,6 @@ typedef struct { int dpms_mode; struct dumb_bo *cursor_bo; Bool cursor_up; -Bool set_cursor2_failed; Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht,
[PATCH xserver] modesetting: allow switching from software to hardware cursors.
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Checked the current git source and this change is still needed. This is an updated patch which takes into account changes in the driver source since the original patch was created. Note that other than building I have not had a chance to test that the updated patch still works, but the difference against the original is pretty minimal. hw/xfree86/drivers/modesetting/drmmode_display.c | 36 +--- hw/xfree86/drivers/modesetting/drmmode_display.h | 1 - 2 files changed, 13 insertions(+), 24 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 6b933e4..ad39f76 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -756,33 +756,23 @@ drmmode_set_cursor(xf86CrtcPtr crtc) drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); +CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); int ret; -if (!drmmode_crtc->set_cursor2_failed) { -CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen); - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return TRUE; - -drmmode_crtc->set_cursor2_failed = TRUE; -} - -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, + ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -return FALSE; -} +if (ret) +return FALSE; /* fallback to swcursor */ return TRUE; } diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.h b/hw/xfree86/drivers/modesetting/drmmode_display.h index 50976b8..5170bf5 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.h +++ b/hw/xfree86/drivers/modesetting/drmmode_display.h @@ -92,7 +92,6 @@ typedef struct { int dpms_mode; struct dumb_bo *cursor_bo; Bool cursor_up; -Bool set_cursor2_failed; Bool first_cursor_load_done; uint16_t lut_r[256], lut_g[256], lut_b[256]; -- 2.9.3 ___ 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: resubmit dirty rects on EINVAL (v2)
On 18.07.2016 22:37, Adam Jackson wrote: On Mon, 2016-07-18 at 12:46 -0400, Adam Jackson wrote: This error code can mean we're submitting more rects at once than the driver can handle. If that happens, resubmit one at a time. v2: Make the rect submit loop more error-proof (Walter Harms) Signed-off-by: Adam Jackson <a...@redhat.com> Reviewed-by: Michael Thayer <michael.tha...@oracle.com> Went ahead and merged this: remote: I: patch #99393 updated using rev 4b311d23e84356bd0e9e736aeed7448dd6382118. remote: I: 1 patch(es) updated to state Accepted. To ssh://git.freedesktop.org/git/xorg/xserver 8d3a368..4b311d2 master -> master Thanks! What are the chances of this getting back-ported to 1.17 and 1.18? That rather increases the chances that distributions will adopt the fix and that I will not have to write a nasty work-around in my kernel driver. - ajax -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: resubmit dirty rects on EINVAL
On 16.07.2016 11:28, walter harms wrote: Am 15.07.2016 17:28, schrieb Adam Jackson: This error code can mean we're submitting more rects at once than the driver can handle. If that happens, resubmit one at a time. Signed-off-by: Adam Jackson <a...@redhat.com> --- hw/xfree86/drivers/modesetting/driver.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index 262a899..2de16f6 100644 --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn, /* TODO query connector property to see if this is needed */ ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects); + +/* if we're swamping it with work, try one at a time */ +if (ret == -EINVAL) { +for (i = 0; i < num_cliprects; i++) { +if (drmModeDirtyFB(ms->fd, fb_id, [i], 1) == -EINVAL) +break; +} +if (i == num_cliprects) +ret = 0; +} + maybe these more simple version works also ? if (ret == -EINVAL) { for (i = 0; i < num_cliprects; i++) { ret=drmModeDirtyFB(ms->fd, fb_id, [i], 1); if ( ret == -EINVAL) break; } } the whole thing works only if drmModeDirtyFB() never returns something else than -EINVAL. Perhaps it is more robust just to check for <0 ? (note: i am not an expert on this, just curious) That does make some sense, but since I have not yet reproduced the problem this patch fixes getting it tested is a bit of a long cycle. Would you consider it reasonably to review and commit it without that test? Or any thoughts about how one could easily trigger dirtying more than 256 rectangles at once? Regards, Michael re, wh free(clip); DamageEmpty(damage); } ___ 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 -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: resubmit dirty rects on EINVAL
Reviewed-by: Michael Thayer <michael.tha...@oracle.com> And successfully tested by one user who experienced buggy behaviour without the patch. On 15.07.2016 17:28, Adam Jackson wrote: This error code can mean we're submitting more rects at once than the driver can handle. If that happens, resubmit one at a time. Signed-off-by: Adam Jackson <a...@redhat.com> --- hw/xfree86/drivers/modesetting/driver.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index 262a899..2de16f6 100644 --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn, /* TODO query connector property to see if this is needed */ ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects); + +/* if we're swamping it with work, try one at a time */ +if (ret == -EINVAL) { +for (i = 0; i < num_cliprects; i++) { +if (drmModeDirtyFB(ms->fd, fb_id, [i], 1) == -EINVAL) +break; +} +if (i == num_cliprects) +ret = 0; +} + free(clip); DamageEmpty(damage); } -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: do not disable dirty rectangles on EINVAL.
On 15.07.2016 17:33, Adam Jackson wrote: On Tue, 2016-07-12 at 15:56 +0200, Michael Thayer wrote: I know it has just been two days, but still, a polite ping. I would also generally be interested in the question of what I can do to get this sort of patch moving. Would reviewing other people's patches be the right way to go, or creating a tree on fd.o? Or something else? I do find seeing my own patches reviewed to be a good reminder to review and merge others, yeah. Likewise pull requests are a slightly stronger statement that someone thinks the patches are ready to merge (from fdo or github or wherever, it really doesn't matter). Next question then: how thorough a review is required for a "reviewed by" tag? Instinctively I do not want to give that tag to patches that touch code I do not already know reasonably well, but I can understand that you might want to relax that a bit if there are simply not enough people reviewing. Regards, Michael - ajax -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: do not disable dirty rectangles on EINVAL.
On 10.07.2016 12:23, Michael Thayer wrote: Hello Adam, On 05.07.2016 20:40, Adam Jackson wrote: On Mon, 2016-07-04 at 21:43 +0200, Michael Thayer wrote: When submitting dirty rectangles to the kernel driver, modesetting checks the return value, and if it gets ENOSYS (driver does not support reporting) or EINVAL (invalid data submitted to the kernel driver) it disables reporting for the rest of the session. [...] The explanation makes sense, but I'm not sure the patch does. I'm not especially familiar with this code, but it seems like pretending that submitting dirty rects succeeded when it didn't is just hoping that some later update will compensate. What about something like: --- --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn, /* TODO query connector property to see if this is needed */ ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects); + +/* if we're swamping it with work, try one at a time */ +if (ret == -EINVAL) { +for (i = 0; i < num_cliprects; i++) { +if (drmModeDirtyFB(ms->fd, fb_id, [i], 1) == -EINVAL) +break; +} +if (i == num_cliprects) +ret = 0; +} + free(clip); DamageEmpty(damage); } I got one of the affected users to (successfully) test this patch. On X.Org Server 1.17.4, but I assume that is good enough. Would you be able to re-submit it with a "signed off by" so that I can review it? Or can I review it as it stands? I know it has just been two days, but still, a polite ping. I would also generally be interested in the question of what I can do to get this sort of patch moving. Would reviewing other people's patches be the right way to go, or creating a tree on fd.o? Or something else? Regards and thanks, Michael --- - ajax Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: do not disable dirty rectangles on EINVAL.
Hello Adam, On 05.07.2016 20:40, Adam Jackson wrote: On Mon, 2016-07-04 at 21:43 +0200, Michael Thayer wrote: When submitting dirty rectangles to the kernel driver, modesetting checks the return value, and if it gets ENOSYS (driver does not support reporting) or EINVAL (invalid data submitted to the kernel driver) it disables reporting for the rest of the session. The second is clearly wrong, and has been seen to trigger in practice when the X server submits more rectangles at once to the VirtualBox kernel driver than the kernel will accept. I would expect this too affect most or all other drivers for virtual graphics devices and some others. The explanation makes sense, but I'm not sure the patch does. I'm not especially familiar with this code, but it seems like pretending that submitting dirty rects succeeded when it didn't is just hoping that some later update will compensate. What about something like: --- --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn, /* TODO query connector property to see if this is needed */ ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects); + +/* if we're swamping it with work, try one at a time */ +if (ret == -EINVAL) { +for (i = 0; i < num_cliprects; i++) { +if (drmModeDirtyFB(ms->fd, fb_id, [i], 1) == -EINVAL) +break; +} +if (i == num_cliprects) +ret = 0; +} + free(clip); DamageEmpty(damage); } I got one of the affected users to (successfully) test this patch. On X.Org Server 1.17.4, but I assume that is good enough. Would you be able to re-submit it with a "signed off by" so that I can review it? Or can I review it as it stands? --- - ajax Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: do not disable dirty rectangles on EINVAL.
On 05.07.2016 22:15, Michael Thayer wrote: On 05.07.2016 20:40, Adam Jackson wrote: On Mon, 2016-07-04 at 21:43 +0200, Michael Thayer wrote: When submitting dirty rectangles to the kernel driver, modesetting checks the return value, and if it gets ENOSYS (driver does not support reporting) or EINVAL (invalid data submitted to the kernel driver) it disables reporting for the rest of the session. The second is clearly wrong, and has been seen to trigger in practice when the X server submits more rectangles at once to the VirtualBox kernel driver than the kernel will accept. I would expect this too affect most or all other drivers for virtual graphics devices and some others. The explanation makes sense, but I'm not sure the patch does. I'm not especially familiar with this code, but it seems like pretending that submitting dirty rects succeeded when it didn't is just hoping that some later update will compensate. What about something like: --- --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn, /* TODO query connector property to see if this is needed */ ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects); + +/* if we're swamping it with work, try one at a time */ +if (ret == -EINVAL) { +for (i = 0; i < num_cliprects; i++) { +if (drmModeDirtyFB(ms->fd, fb_id, [i], 1) == -EINVAL) +break; +} +if (i == num_cliprects) +ret = 0; +} + free(clip); DamageEmpty(damage); } That certainly looks sensible to me. I must admit that the main reason that I did not do something more elaborate was that I did not really have time to test more than the simple patch I sent (actually I got one of the users experiencing the problem to test it, which increased the loop length). So I decided that my patch was already an improvement - missing one dirty update versus missing all updates for the rest of the server life-time. If you are alright with your updated patch I would be happy to give it a reviewed-by. I asked the user to test your patch, so let's see. Regards and thanks, Michael --- - ajax -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: do not disable dirty rectangles on EINVAL.
On 05.07.2016 20:40, Adam Jackson wrote: On Mon, 2016-07-04 at 21:43 +0200, Michael Thayer wrote: When submitting dirty rectangles to the kernel driver, modesetting checks the return value, and if it gets ENOSYS (driver does not support reporting) or EINVAL (invalid data submitted to the kernel driver) it disables reporting for the rest of the session. The second is clearly wrong, and has been seen to trigger in practice when the X server submits more rectangles at once to the VirtualBox kernel driver than the kernel will accept. I would expect this too affect most or all other drivers for virtual graphics devices and some others. The explanation makes sense, but I'm not sure the patch does. I'm not especially familiar with this code, but it seems like pretending that submitting dirty rects succeeded when it didn't is just hoping that some later update will compensate. What about something like: --- --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -515,6 +515,17 @@ dispatch_dirty_region(ScrnInfoPtr scrn, /* TODO query connector property to see if this is needed */ ret = drmModeDirtyFB(ms->fd, fb_id, clip, num_cliprects); + +/* if we're swamping it with work, try one at a time */ +if (ret == -EINVAL) { +for (i = 0; i < num_cliprects; i++) { +if (drmModeDirtyFB(ms->fd, fb_id, [i], 1) == -EINVAL) +break; +} +if (i == num_cliprects) +ret = 0; +} + free(clip); DamageEmpty(damage); } That certainly looks sensible to me. I must admit that the main reason that I did not do something more elaborate was that I did not really have time to test more than the simple patch I sent (actually I got one of the users experiencing the problem to test it, which increased the loop length). So I decided that my patch was already an improvement - missing one dirty update versus missing all updates for the rest of the server life-time. If you are alright with your updated patch I would be happy to give it a reviewed-by. Regards and thanks, Michael --- - ajax -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: do not disable dirty rectangles on EINVAL.
When submitting dirty rectangles to the kernel driver, modesetting checks the return value, and if it gets ENOSYS (driver does not support reporting) or EINVAL (invalid data submitted to the kernel driver) it disables reporting for the rest of the session. The second is clearly wrong, and has been seen to trigger in practice when the X server submits more rectangles at once to the VirtualBox kernel driver than the kernel will accept. I would expect this too affect most or all other drivers for virtual graphics devices and some others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- hw/xfree86/drivers/modesetting/driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xfree86/drivers/modesetting/driver.c b/hw/xfree86/drivers/modesetting/driver.c index 256ca95..81c9b4c 100644 --- a/hw/xfree86/drivers/modesetting/driver.c +++ b/hw/xfree86/drivers/modesetting/driver.c @@ -517,7 +517,7 @@ dispatch_dirty(ScreenPtr pScreen) int ret; ret = dispatch_dirty_region(scrn, pixmap, ms->damage, fb_id); -if (ret == -EINVAL || ret == -ENOSYS) { +if (ret == -ENOSYS) { ms->dirty_enabled = FALSE; DamageUnregister(ms->damage); DamageDestroy(ms->damage); -- 2.7.4 ___ 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
Modesetting patch still awaiting review
Hello, I submitted this a month ago, but so far no reviews, so please consider this a polite ping. (I will be off-line for three weeks now, so will not respond immediately - thanks in advance to anyone who reviews it in that time.) modesetting: allow switching from software to hardware cursors. https://patchwork.freedesktop.org/patch/75985/ Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in hardware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Thanks and regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] xfree86: Immediately handle failure to set HW cursor
On 04.04.2016 22:09, Michael Thayer wrote: Hello, Please excuse the top posting. Tested this in a dual-head non-mirrored virtual machine and hit the following problem: xf86CursorSetCursor() in hw/xfree86/ramdac/xf86Cursor.c calls xf86SetCursor() which fails because the cursor is not visible on one of the two screens, and falls back to software cursor rendering as a result. Perhaps xf86_crtc_show_cursor() should return TRUE if crtc->cursor_shown is FALSE? I'm afraid I will be away from the computer again for a few weeks, so it will be a while before I can test again. I would suggest changing this function as follows: static Bool xf86_crtc_show_cursor(xf86CrtcPtr crtc) { if (!crtc->cursor_shown && crtc->cursor_in_range) { crtc->cursor_shown = TRUE; if (crtc->funcs->show_cursor_check) { return crtc->funcs->show_cursor_check(crtc); } else { crtc->funcs->show_cursor(crtc); } } return TRUE; } As mentioned previously, crtc->cursor_shown will be FALSE if the cursor is hidden on a particular crtc, and that is not an error. Also, xf86_crtc_show_cursor() is called from xf86_crtc_set_cursor_position() without checking the return value, sometimes with cursor_shown FALSE when the cursor moves from one screen to another, and if we keep cursor_shown FALSE we will get a call to the kernel driver every time the position changes. However, I also just realised that modesetting still does not implement load_cursor_argb_check() - I really don't know how I forgot that. Perhaps just implementing that would solve your problem? Regards, Michael On 30.03.2016 08:41, Alexandre Courbot wrote: There is currently no reliable way to report failure to set a HW cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM ioctl fails (which currently happens at least with modesetting on Tegra for format incompatibility reasons). As failures are currently handled by setting the HW cursor size to (0,0), the fallback to SW cursor will not happen until the next time the cursor changes and xf86CursorSetCursor() is called again. In the meantime, the cursor will be invisible to the user. This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans. This allows to propagate errors up to xf86CursorSetCursor(), which can then fall back to using the SW cursor immediately. Signed-off-by: Alexandre Courbot <acour...@nvidia.com> --- Changes since v1: - Keep the old hooks unchanged to preserve compatibility hw/xfree86/drivers/modesetting/drmmode_display.c | 15 +++- hw/xfree86/modes/xf86Crtc.h | 4 hw/xfree86/modes/xf86Cursors.c | 30 +--- hw/xfree86/ramdac/xf86Cursor.h | 1 + hw/xfree86/ramdac/xf86HWCurs.c | 18 ++ 5 files changed, 50 insertions(+), 18 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index f573a27f4985..28d663c3d22e 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -532,7 +532,7 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; @@ -551,7 +551,7 @@ drmmode_set_cursor(xf86CrtcPtr crtc) handle, ms->cursor_width, ms->cursor_height, cursor->bits->xhot, cursor->bits->yhot); if (!ret) -return; +return TRUE; if (ret == -EINVAL) use_set_cursor2 = FALSE; } @@ -566,7 +566,10 @@ drmmode_set_cursor(xf86CrtcPtr crtc) cursor_info->MaxWidth = cursor_info->MaxHeight = 0; drmmode_crtc->drmmode->sw_cursor = TRUE; /* fallback to swcursor */ +return FALSE; } + +return TRUE; } static void @@ -599,12 +602,12 @@ drmmode_hide_cursor(xf86CrtcPtr crtc) ms->cursor_width, ms->cursor_height); } -static void -drmmode_show_cursor(xf86CrtcPtr crtc) +static Bool +drmmode_show_cursor_check(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_crtc->cursor_up = TRUE; -drmmode_set_cursor(crtc); +return drmmode_set_cursor(crtc); } static void @@ -844,7 +847,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = { .set_mode_major = drmmode_set_mode_major, .set_cursor_colors = drmmode_set_cursor_colors, .set_cursor_position = drmmode_set_cursor_position, -.show_cursor = drmmode_show_cursor, +.show_cursor
Re: [PATCH xserver v2] xfree86: Immediately handle failure to set HW cursor
hown && crtc->cursor_in_range) { -crtc->funcs->show_cursor(crtc); -crtc->cursor_shown = TRUE; +if (crtc->funcs->show_cursor_check) { +crtc->cursor_shown = crtc->funcs->show_cursor_check(crtc); +} else { +crtc->funcs->show_cursor(crtc); +crtc->cursor_shown = TRUE; +} } + +return crtc->cursor_shown; } -void -xf86_show_cursors(ScrnInfoPtr scrn) +Bool +xf86_show_cursors_check(ScrnInfoPtr scrn) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); int c; @@ -352,9 +358,17 @@ xf86_show_cursors(ScrnInfoPtr scrn) for (c = 0; c < xf86_config->num_crtc; c++) { xf86CrtcPtr crtc = xf86_config->crtc[c]; -if (crtc->enabled) -xf86_crtc_show_cursor(crtc); +if (crtc->enabled && !xf86_crtc_show_cursor(crtc)) +return FALSE; } + +return TRUE; +} + +void +xf86_show_cursors(ScrnInfoPtr scrn) +{ +xf86_show_cursors_check(scrn); } void @@ -631,7 +645,7 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags) cursor_info->SetCursorPosition = xf86_set_cursor_position; cursor_info->LoadCursorImageCheck = xf86_load_cursor_image; cursor_info->HideCursor = xf86_hide_cursors; -cursor_info->ShowCursor = xf86_show_cursors; +cursor_info->ShowCursorCheck = xf86_show_cursors_check; cursor_info->UseHWCursor = xf86_use_hw_cursor; if (flags & HARDWARE_CURSOR_ARGB) { cursor_info->UseHWCursorARGB = xf86_use_hw_cursor_argb; diff --git a/hw/xfree86/ramdac/xf86Cursor.h b/hw/xfree86/ramdac/xf86Cursor.h index 6e88240d9ab7..a1afb1c86ed9 100644 --- a/hw/xfree86/ramdac/xf86Cursor.h +++ b/hw/xfree86/ramdac/xf86Cursor.h @@ -16,6 +16,7 @@ typedef struct _xf86CursorInfoRec { Bool (*LoadCursorImageCheck) (ScrnInfoPtr pScrn, unsigned char *bits); void (*HideCursor) (ScrnInfoPtr pScrn); void (*ShowCursor) (ScrnInfoPtr pScrn); +Bool (*ShowCursorCheck) (ScrnInfoPtr pScrn); unsigned char *(*RealizeCursor) (struct _xf86CursorInfoRec *, CursorPtr); Bool (*UseHWCursor) (ScreenPtr, CursorPtr); diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c index 84febe0df5cd..458781cae7e9 100644 --- a/hw/xfree86/ramdac/xf86HWCurs.c +++ b/hw/xfree86/ramdac/xf86HWCurs.c @@ -89,7 +89,8 @@ xf86InitHardwareCursor(ScreenPtr pScreen, xf86CursorInfoPtr infoPtr) if (!infoPtr->SetCursorPosition || !xf86DriverHasLoadCursorImage(infoPtr) || !infoPtr->HideCursor || -!infoPtr->ShowCursor || !infoPtr->SetCursorColors) +(!infoPtr->ShowCursor && !infoPtr->ShowCursorCheck) || +!infoPtr->SetCursorColors) return FALSE; if (infoPtr->RealizeCursor) { @@ -119,6 +120,15 @@ xf86InitHardwareCursor(ScreenPtr pScreen, xf86CursorInfoPtr infoPtr) return TRUE; } +static Bool +xf86ShowCursor(xf86CursorInfoPtr infoPtr) +{ +if (*infoPtr->ShowCursorCheck) +return (*infoPtr->ShowCursorCheck) (infoPtr->pScrn); +(*infoPtr->ShowCursor) (infoPtr->pScrn); +return TRUE; +} + Bool xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y) { @@ -161,8 +171,8 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y) (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y); -(*infoPtr->ShowCursor) (infoPtr->pScrn); -return TRUE; +return xf86ShowCursor(infoPtr); + } void @@ -184,7 +194,7 @@ xf86SetTransparentCursor(ScreenPtr pScreen) xf86DriverLoadCursorImage (infoPtr, ScreenPriv->transparentData); -(*infoPtr->ShowCursor) (infoPtr->pScrn); +xf86ShowCursor(infoPtr); } void -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Modesetting driver and cursor hot-spot
On 01.04.2016 22:22, Michael Thayer wrote: On 01.04.2016 22:12, Michael Thayer wrote: I have just been adjusting the cursor handling code in the VirtualBox kernel driver and realised that the modesetting user space driver is still adjusting the reported cursor position to take the hot-spot into account, even though it also reports the hot-spot using drmModeSetCursor2(). In theory this would be easy to fix, but that would create a new problem: kernel space would have no easy way to tell whether it was dealing with the old or with the new driver. Other drivers do not have this bug though, and I seem to be the first person to want to use both position and hot-spot information in modesetting. Does anyone have a good idea about how to deal with this? My best so far is for my driver to intercept DRM_IOCTL_MODE_CURSOR2 and just -EINVAL it to force fall-back. Ah, it gets more fun - the kernel code clearly also makes the same assumption: if a driver does not provide cursor_set2(), it falls back to calling cursor_set() with the same parameters, minus the hot-spot location. Reported cursor position does not take this into account. So this is clearly already a part of the kernel API, though probably as yet with no one actually depending on it. Excuse the noise. Looking at more code, this seems to be the intended use. I see other code looking at deltas in the hot-spot value rather than the absolute value of it, and simply adjusting the position inside cursor_set2() ahead of the upcoming call to cursor_move(). So all a false alarm on my part. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Modesetting driver and cursor hot-spot
On 01.04.2016 22:12, Michael Thayer wrote: I have just been adjusting the cursor handling code in the VirtualBox kernel driver and realised that the modesetting user space driver is still adjusting the reported cursor position to take the hot-spot into account, even though it also reports the hot-spot using drmModeSetCursor2(). In theory this would be easy to fix, but that would create a new problem: kernel space would have no easy way to tell whether it was dealing with the old or with the new driver. Other drivers do not have this bug though, and I seem to be the first person to want to use both position and hot-spot information in modesetting. Does anyone have a good idea about how to deal with this? My best so far is for my driver to intercept DRM_IOCTL_MODE_CURSOR2 and just -EINVAL it to force fall-back. Ah, it gets more fun - the kernel code clearly also makes the same assumption: if a driver does not provide cursor_set2(), it falls back to calling cursor_set() with the same parameters, minus the hot-spot location. Reported cursor position does not take this into account. So this is clearly already a part of the kernel API, though probably as yet with no one actually depending on it. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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
Modesetting driver and cursor hot-spot
Hello, I have just been adjusting the cursor handling code in the VirtualBox kernel driver and realised that the modesetting user space driver is still adjusting the reported cursor position to take the hot-spot into account, even though it also reports the hot-spot using drmModeSetCursor2(). In theory this would be easy to fix, but that would create a new problem: kernel space would have no easy way to tell whether it was dealing with the old or with the new driver. Other drivers do not have this bug though, and I seem to be the first person to want to use both position and hot-spot information in modesetting. Does anyone have a good idea about how to deal with this? My best so far is for my driver to intercept DRM_IOCTL_MODE_CURSOR2 and just -EINVAL it to force fall-back. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] xfree86/modes: Make sure the HW cursor is hidden when it should be
Looks good to me, but I don't feel I'm quite familiar enough with the code to give a full "reviewed-by". Regards, Michael On 24.03.2016 09:34, Michel Dänzer wrote: From: Michel Dänzer <michel.daen...@amd.com> When the HW cursor is hidden (e.g. because xf86CursorResetCursor triggers a switch from HW cursor to SW cursor), the driver isn't notified of this for disabled CRTCs. If the HW cursor was shown when the CRTC was disabled, it may still be displayed when the CRTC is enabled again. Prevent this by explicitly hiding the HW cursor again after setting a mode if it's currently supposed to be hidden. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=94560 Signed-off-by: Michel Dänzer <michel.daen...@amd.com> --- hw/xfree86/modes/xf86Crtc.c | 6 ++ 1 file changed, 6 insertions(+) diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c index 2639a30..6091b5e 100644 --- a/hw/xfree86/modes/xf86Crtc.c +++ b/hw/xfree86/modes/xf86Crtc.c @@ -368,6 +368,12 @@ xf86CrtcSetModeTransform(xf86CrtcPtr crtc, DisplayModePtr mode, xf86CrtcSetScreenSubpixelOrder(scrn->pScreen); if (scrn->ModeSet) scrn->ModeSet(scrn); + +/* Make sure the HW cursor is hidden if it's supposed to be, in case + * it was hidden while the CRTC was disabled + */ +if (!xf86_config->cursor_on) +xf86_hide_cursors(scrn); } else { crtc->x = saved_x; -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] xfree86: Immediately handle failure to set HW cursor
CrtcPtr crtc = xf86_config->crtc[c]; -if (crtc->enabled) -xf86_crtc_show_cursor(crtc); +if (crtc->enabled && !xf86_crtc_show_cursor(crtc)) +return FALSE; } + +return TRUE; } void diff --git a/hw/xfree86/ramdac/IBM.c b/hw/xfree86/ramdac/IBM.c index 6822be5f1dad..23fb55411b1e 100644 --- a/hw/xfree86/ramdac/IBM.c +++ b/hw/xfree86/ramdac/IBM.c @@ -468,16 +468,18 @@ IBMramdac640SetBpp(ScrnInfoPtr pScrn, RamDacRegRecPtr ramdacReg) } } -static void +static Bool IBMramdac526ShowCursor(ScrnInfoPtr pScrn) { RamDacRecPtr ramdacPtr = RAMDACSCRPTR(pScrn); /* Enable cursor - X11 mode */ (*ramdacPtr->WriteDAC) (pScrn, IBMRGB_curs, 0x00, 0x07); + +return TRUE; } -static void +static Bool IBMramdac640ShowCursor(ScrnInfoPtr pScrn) { RamDacRecPtr ramdacPtr = RAMDACSCRPTR(pScrn); @@ -485,6 +487,8 @@ IBMramdac640ShowCursor(ScrnInfoPtr pScrn) /* Enable cursor - mode2 (x11 mode) */ (*ramdacPtr->WriteDAC) (pScrn, RGB640_CURSOR_CONTROL, 0x00, 0x0B); (*ramdacPtr->WriteDAC) (pScrn, RGB640_CROSSHAIR_CONTROL, 0x00, 0x00); + +return TRUE; } static void diff --git a/hw/xfree86/ramdac/TI.c b/hw/xfree86/ramdac/TI.c index 0ae1db57ecc6..d1a617de206e 100644 --- a/hw/xfree86/ramdac/TI.c +++ b/hw/xfree86/ramdac/TI.c @@ -588,13 +588,15 @@ TIramdac3030SetBpp(ScrnInfoPtr pScrn, RamDacRegRecPtr ramdacReg) } } -static void +static Bool TIramdacShowCursor(ScrnInfoPtr pScrn) { RamDacRecPtr ramdacPtr = RAMDACSCRPTR(pScrn); /* Enable cursor - X11 mode */ (*ramdacPtr->WriteDAC) (pScrn, TIDAC_ind_curs_ctrl, 0, 0x03); + +return TRUE; } static void diff --git a/hw/xfree86/ramdac/xf86Cursor.h b/hw/xfree86/ramdac/xf86Cursor.h index 6e88240d9ab7..6da75e2f6731 100644 --- a/hw/xfree86/ramdac/xf86Cursor.h +++ b/hw/xfree86/ramdac/xf86Cursor.h @@ -15,7 +15,7 @@ typedef struct _xf86CursorInfoRec { void (*LoadCursorImage) (ScrnInfoPtr pScrn, unsigned char *bits); Bool (*LoadCursorImageCheck) (ScrnInfoPtr pScrn, unsigned char *bits); void (*HideCursor) (ScrnInfoPtr pScrn); -void (*ShowCursor) (ScrnInfoPtr pScrn); +Bool (*ShowCursor) (ScrnInfoPtr pScrn); unsigned char *(*RealizeCursor) (struct _xf86CursorInfoRec *, CursorPtr); Bool (*UseHWCursor) (ScreenPtr, CursorPtr); diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c index 84febe0df5cd..a6efaf5dae80 100644 --- a/hw/xfree86/ramdac/xf86HWCurs.c +++ b/hw/xfree86/ramdac/xf86HWCurs.c @@ -161,8 +161,7 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y) (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y); -(*infoPtr->ShowCursor) (infoPtr->pScrn); -return TRUE; +return (*infoPtr->ShowCursor) (infoPtr->pScrn); } void -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] Switch to SW cursor right after HW cursor failure
Hello Alex, On 18.03.2016 10:42, Alexandre Courbot wrote: Hello Michael, On 03/18/2016 06:09 PM, Michael Thayer wrote: In that context, see this patch to Modesetting which I send a couple of weeks ago, which should incidentally also fix your issue: https://patchwork.freedesktop.org/patch/75985/ I applied your patch, but sadly it didn't fix my issue. I agree that your approach (actually returning drmmode_set_cursor() errors instead of relying on a side-effect) is better though. But it appears that I would need drmmode_show_cursor()'s errors to be reported for the same approach to work in my case. I see that the load_cursor_argb_check() hook has been added apparently to palliate the fact that load_cursor_argb() does not return an error status, would it be acceptable to apply the same principle to show_cursor()? It certainly makes sense to me (noting that the *_check() hooks were Keith's idea). Not sure off-hand how much you would have to change in the main X server code to make that work. Regards, Michael Thanks, Alex. -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] Switch to SW cursor right after HW cursor failure
Hello Alexandre, In that context, see this patch to Modesetting which I send a couple of weeks ago, which should incidentally also fix your issue: https://patchwork.freedesktop.org/patch/75985/ Regards, Michael On 18.03.2016 09:19, Alexandre Courbot wrote: Modesetting currently signals a failure to display the HW cursor by setting its size to 0 in drmmode_set_cursor(). xf86CursorSetCursor() will then detect that condition and switch to the SW cursor upon the next invokation. The problem is that said invokation may not come before a while (i.e. before the cursor changes shape), and thus the user may be left with an invisible cursor for an undefined period of time. Therefore, check whether the HW cursor size has been set to 0 after calling xf86SetCursor(), and fall through the SW cursor fallback if this is the case in order to display the cursor immediately. Signed-off-by: Alexandre Courbot <acour...@nvidia.com> --- hw/xfree86/ramdac/xf86Cursor.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c index c061b8028ca8..08868ffb2459 100644 --- a/hw/xfree86/ramdac/xf86Cursor.c +++ b/hw/xfree86/ramdac/xf86Cursor.c @@ -357,7 +357,15 @@ xf86CursorSetCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs, ScreenPriv->isUp = TRUE; miPointerSetWaitForUpdate(pScreen, !infoPtr->pScrn->silkenMouse); -return; + + /* even if xf86SetCursor returns success, modesetting may +* have set the cursor size to 0 in order to switch to software +* cursor. If this happens, just fall through to switch to +* software cursor. If we don't do it here, the cursor will +* remain invisible until the next call to this function, which +* may not happen before a while */ +if (!(infoPtr->MaxWidth == 0 || infoPtr->MaxHeight == 0)) +return; } } -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] modesetting: allow switching from software to hardware cursors.
Hello Jasper, My case is virtual machine cursor pass-through. VirtualBox has a setting which lets the user enable or disable guest-host cursor integration. When it is enabled, the guest pointer position tracks the host one, and the guest cursor is rendered by the host. When it is disabled, the guest and host pointer positions are independent, and in this case the guest renders its cursor itself in software. The user can switch between the two at any time, though of course in practice switches do not occur in close succession. But if integration was previously enabled, then the user disables it and the guest re-loads the previous cursor, it will be told that hardware rendering is not possible, which it was previously (and vice-versa). Sorry that was so long - hope it was clear enough. Regards, Michael On 12.03.2016 20:19, Jasper St. Pierre wrote: Can you give an example of when drmSetCursor2 would fail sometimes but not always, enough to switch back and forth actively? I'm not a huge fan of the patch because now errors could cause really bizarre double-cursor, or no-cursor flickering, since there is no guarantee that the swapover happens in the same frame. We sort-of tried this in Weston for something else (we rendered certain windows in overlays) and had to turn it off and go to always SW because of the obnoxious flickering, at least until atomic landed. On Sat, Mar 12, 2016 at 3:54 AM, Michael Thayer <michael.tha...@oracle.com> wrote: Hello, I would just like to politely draw attention to this again. I would assume from the silence that the people who might review it don't think it is a completely wrong thing (that would not take long to say) but don't have the spare cycles to take a closer look. Any idea when someone might have twenty minutes to review it? Regards, Michael On 06.03.2016 12:56, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Made the commit message more verbose on Emil's request. hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++-- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..36c3093 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -static Bool use_set_cursor2 = TRUE; int ret; -if (use_set_cursor2) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -CursorPtr cursor = xf86_config->cursor; - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return; -if (ret == -EINVAL) -use_set_cursor2 = FALSE; -} +xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); +CursorPtr cursor = xf86_config->cursor; -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drm
Re: [PATCH] modesetting: allow switching from software to hardware cursors.
Hello, I would just like to politely draw attention to this again. I would assume from the silence that the people who might review it don't think it is a completely wrong thing (that would not take long to say) but don't have the spare cycles to take a closer look. Any idea when someone might have twenty minutes to review it? Regards, Michael On 06.03.2016 12:56, Michael Thayer wrote: Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Made the commit message more verbose on Emil's request. hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++-- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..36c3093 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -static Bool use_set_cursor2 = TRUE; int ret; -if (use_set_cursor2) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -CursorPtr cursor = xf86_config->cursor; - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return; -if (ret == -EINVAL) -use_set_cursor2 = FALSE; -} +xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); +CursorPtr cursor = xf86_config->cursor; -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -} +if (ret) +return FALSE; /* fallback to swcursor */ +return TRUE; } -static void +static Bool drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) { modesettingPtr ms = modesettingPTR(crtc->scrn); @@ -537,7 +529,8 @@ drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) ptr[i] = image[i]; // cpu_to_le32(image[i]); if (drmmode_crtc->cursor_up) -drmmode_set_cursor(crtc); +return drmmode_set_cursor(crtc); +return TRUE; } static void @@ -799,7 +792,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = { .set_cursor_position = drmmode_set_cursor_position, .show_cursor = drmmode_show_cursor, .hide_cursor = drmmode_hide_cursor, -.load_cursor_argb = drmmode_load_cursor_argb, +.load_cursor_argb_check = drmmode_load_cursor_argb, .gamma_set = drmmode_crtc_gamma_set, .destroy = NULL,/* XXX */ -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher __
Re: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors.
Hello Olivier, On 10.03.2016 09:11, Olivier Fourdan wrote: Hi Michael, [...] Re-sending as this did not seem to get noticed much the first time. I am not doing a formal review of your patch as it's outside of my area of competences, just a few comments, trying to help. Thank you, help welcome. I am not sure re-sending the patch as-is is the best course of action here, usually simply replying the original message with a gentle reminder suffices. Reason being that the original patch (sent only 4 days ago) is still accessible in patchwork and still marked as "New": https://patchwork.freedesktop.org/patch/75985/ And the new here is also available: https://patchwork.freedesktop.org/patch/76437/ So you should mark on of the two as superseded in patchwork. But, yes, patches do take some time for review. Right, I am still not too familiar with the patch process for X.Org. How do I mark the re-sent one, which as you said does not apply cleanly, as superseded? Another problem is this patch here won't apply here due to formatting issues apparently: $ pwc git-am 76437 Applying patch #76437 using 'git am' Description: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors. Applying: Re-send: [PATCH] modesetting: allow switching from software to hardware cursors. fatal: corrupt patch at line 8 Patch failed at 0001 Re-send: [PATCH] modesetting: allow switching from software to hardware cursors. See below. But the original patch (the first one) doesn't seem to suffer from the same formatting issues though. hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++-- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..36c3093 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -static Bool use_set_cursor2 = TRUE; int ret; -if (use_set_cursor2) { That won't apply. There are several occurrences of the same issue in the patch being resent, I think best would be to use git-send-email to send patches instead of Thunderbird -as seen in the message source- (or any other MUA for that matter) that can break the patch formatting and confuse git. Indeed, I sent the original with g-s-e and the re-send with Thunderbird. Thanks again. Regards, Michael HTH, Cheers, Olivier -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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-send: [PATCH] modesetting: allow switching from software to hardware cursors.
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Re-sending as this did not seem to get noticed much the first time. hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++-- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..36c3093 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -static Bool use_set_cursor2 = TRUE; int ret; -if (use_set_cursor2) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -CursorPtr cursor = xf86_config->cursor; - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return; -if (ret == -EINVAL) -use_set_cursor2 = FALSE; -} +xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); +CursorPtr cursor = xf86_config->cursor; -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -} +if (ret) +return FALSE; /* fallback to swcursor */ +return TRUE; } -static void +static Bool drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) { modesettingPtr ms = modesettingPTR(crtc->scrn); @@ -537,7 +529,8 @@ drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) ptr[i] = image[i]; // cpu_to_le32(image[i]); if (drmmode_crtc->cursor_up) -drmmode_set_cursor(crtc); +return drmmode_set_cursor(crtc); +return TRUE; } static void @@ -799,7 +792,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = { .set_cursor_position = drmmode_set_cursor_position, .show_cursor = drmmode_show_cursor, .hide_cursor = drmmode_hide_cursor, -.load_cursor_argb = drmmode_load_cursor_argb, +.load_cursor_argb_check = drmmode_load_cursor_argb, .gamma_set = drmmode_crtc_gamma_set, .destroy = NULL,/* XXX */ -- 2.5.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] modesetting: allow switching from software to hardware cursors.
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. This is needed because hardware may be able to handle some cursors in harware and others not, or virtual hardware may be able to handle hardware cursors at some times and not others. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- Made the commit message more verbose on Emil's request. hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++-- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..36c3093 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -static Bool use_set_cursor2 = TRUE; int ret; -if (use_set_cursor2) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -CursorPtr cursor = xf86_config->cursor; - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return; -if (ret == -EINVAL) -use_set_cursor2 = FALSE; -} +xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); +CursorPtr cursor = xf86_config->cursor; -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -} +if (ret) +return FALSE; /* fallback to swcursor */ +return TRUE; } -static void +static Bool drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) { modesettingPtr ms = modesettingPTR(crtc->scrn); @@ -537,7 +529,8 @@ drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) ptr[i] = image[i]; // cpu_to_le32(image[i]); if (drmmode_crtc->cursor_up) -drmmode_set_cursor(crtc); +return drmmode_set_cursor(crtc); +return TRUE; } static void @@ -799,7 +792,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = { .set_cursor_position = drmmode_set_cursor_position, .show_cursor = drmmode_show_cursor, .hide_cursor = drmmode_hide_cursor, -.load_cursor_argb = drmmode_load_cursor_argb, +.load_cursor_argb_check = drmmode_load_cursor_argb, .gamma_set = drmmode_crtc_gamma_set, .destroy = NULL,/* XXX */ -- 2.5.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: Modesetting can't switch back from a software to a hardware cursor (patch to follow)
On 05.03.2016 09:42, Michael Thayer wrote: [...] On 05.03.2016 00:19, Emil Velikov wrote: -EINVAL is interpreted by most/all of us as not supported (too old of a kernel). Although another thing was at the back of my mind - why are you removing the use_set_cursor2 static boolean. In all fairness, I doubt that we can get the actual integration happening between the two SetCursor* calls. So one might want to keep it... I dare say we could restore this if we fixed all kernel drivers (that care) never to return -EINVAL in their SetCursor2 call-back. Any thoughts what else they should return to say that a given cursor is (possibly temporarily) not supported by the hardware? On the other hand, currently only qxl and virtgpu should care. virtgpu returns -EINVAL if the cursor passed in by user space is invalid, and qxl could possibly pass up a -EINVAL from an API that it calls. Actually SetCursor2 is supported by all kernels since 3.11, released in September 2013. So if anything it would make sense these days not to try the fall-back at all. That said, I wouldn't mind if this patch could be applied to older server branches too, since we want to support older distributions (one of our important use cases), and that increases the chances. Regarding integration between the two APIs (I didn't answer that properly), on the whole we have the following cases: SetCursor2 is supported and the cursor is supported. In that case the first call to SetCursor2 will succeed and we will never fall back. SetCursor2 is supported and the cursor is not supported but the driver returns -EINVAL to say so. In that case we will try SetCursor which will fail too, no big issue it seems to me. SetCursor2 is supported and the driver returns something else. Then we will not try the fall-back. SetCursor2 is not supported. Then we will always try both APIs but only use SetCursor. Not optimal, but I think that keeping the code simple is more important. Generally, we will always use either SetCursor2 or SetCursor, in practice we will never mix and match, so I don't think the integration is a problems. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Modesetting can't switch back from a software to a hardware cursor (patch to follow)
Hello Emil, On 05.03.2016 00:19, Emil Velikov wrote: On 4 March 2016 at 19:02, Michael Thayer <michael.tha...@oracle.com> wrote: [...] On 04.03.2016 19:52, Emil Velikov wrote: [...] On 4 March 2016 at 14:26, Michael Thayer <michael.tha...@oracle.com> wrote: On 02.03.2016 18:43, Michael Thayer wrote: At present if modesetting ever fails to set a hardware cursor it switches back to a software one and stays that way until it is unloaded. [...] Note that I'm not the person who wrote any of that code, although I do wonder... Why do we have to attempt/probe for hardware cursor everything time ? If the first invocation has failed it is reasonable to expect that all/most sequential ones will also fail. Thanks for taking a look. In VirtualBox and possibly other virtualisation environments, the user may enable or disable mouse integration with the host system. Handling this involves enabling (for integration) and disabling (for no integration) hardware cursor support. So rendering a cursor in hardware can fail on one invocation and succeed for the same cursor on the next. Interesting. Have not seen such a option to toggle it as the VM was running, although it's been a couple of years since I used one. Do other drivers do the same thing to attach/detach the cursor ? I'd imagine so although it'll be worth a check. Actually I forgot the more important use case - many drivers for physical hardware can display some but not all cursors in hardware (Ajax told me once that the details of which can be and which not can be rather non-obvious). So forbidding all hardware cursor usage as soon as one fails is not optimal. Personally I would add that in the commit message, to make it abundantly obvious. Noted. Is the failed call to drmModeSetCursor/drmModeSetCursor2 an intentional behaviour, it suspiciously sound like a bug in the drm module (some along the line) to me. Which DRM module is that ? Do you mean the -EINVAL? That is used to mean that drmModeSetCursor2 is not supported, but can also mean that a particular cursor cannot be rendered in hardware. I suppose you could call that a bug if that is what you were referring to. -EINVAL is interpreted by most/all of us as not supported (too old of a kernel). Although another thing was at the back of my mind - why are you removing the use_set_cursor2 static boolean. In all fairness, I doubt that we can get the actual integration happening between the two SetCursor* calls. So one might want to keep it... I dare say we could restore this if we fixed all kernel drivers (that care) never to return -EINVAL in their SetCursor2 call-back. Any thoughts what else they should return to say that a given cursor is (possibly temporarily) not supported by the hardware? On the other hand, currently only qxl and virtgpu should care. virtgpu returns -EINVAL if the cursor passed in by user space is invalid, and qxl could possibly pass up a -EINVAL from an API that it calls. All in all please consider my questions/suggestions as general curiousness, as opposed to something being wrong with the patch. Thanks Emil Thanks, Michael P.S. Just realised we met at FOSDEM. Howdy Michael :-) Howdy! -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Modesetting can't switch back from a software to a hardware cursor (patch to follow)
Hello Emil, On 04.03.2016 19:52, Emil Velikov wrote: Hi Michael, On 4 March 2016 at 14:26, Michael Thayer <michael.tha...@oracle.com> wrote: On 02.03.2016 18:43, Michael Thayer wrote: At present if modesetting ever fails to set a hardware cursor it switches back to a software one and stays that way until it is unloaded. The following patch should fix that. I say "should" because I had difficulties testing it - the cursor simply disappeared when it should have been rendering in software, though the debugger showed that pixman_image_composite() was getting called whenever the cursor moved, and my kernel driver was getting dirty rectangle information. My feeling is that the patch is correct and something else is broken. I have not investigated in depth in case some one else immediately has an idea. My apologies for the noise. Without going into detail, the failure to show the software cursor was due to the unclean way in which we (VirtualBox) handle 3D acceleration, and nothing to do with the X server or modesetting. I tested my patch again, taking this into account, and it worked as expected. Note that I'm not the person who wrote any of that code, although I do wonder... Why do we have to attempt/probe for hardware cursor everything time ? If the first invocation has failed it is reasonable to expect that all/most sequential ones will also fail. Thanks for taking a look. In VirtualBox and possibly other virtualisation environments, the user may enable or disable mouse integration with the host system. Handling this involves enabling (for integration) and disabling (for no integration) hardware cursor support. So rendering a cursor in hardware can fail on one invocation and succeed for the same cursor on the next. Is the failed call to drmModeSetCursor/drmModeSetCursor2 an intentional behaviour, it suspiciously sound like a bug in the drm module (some along the line) to me. Which DRM module is that ? Do you mean the -EINVAL? That is used to mean that drmModeSetCursor2 is not supported, but can also mean that a particular cursor cannot be rendered in hardware. I suppose you could call that a bug if that is what you were referring to. Regards, Emil Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Modesetting can't switch back from a software to a hardware cursor (patch to follow)
On 02.03.2016 18:43, Michael Thayer wrote: At present if modesetting ever fails to set a hardware cursor it switches back to a software one and stays that way until it is unloaded. The following patch should fix that. I say "should" because I had difficulties testing it - the cursor simply disappeared when it should have been rendering in software, though the debugger showed that pixman_image_composite() was getting called whenever the cursor moved, and my kernel driver was getting dirty rectangle information. My feeling is that the patch is correct and something else is broken. I have not investigated in depth in case some one else immediately has an idea. My apologies for the noise. Without going into detail, the failure to show the software cursor was due to the unclean way in which we (VirtualBox) handle 3D acceleration, and nothing to do with the X server or modesetting. I tested my patch again, taking this into account, and it worked as expected. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] modesetting: allow switching from software to hardware cursors.
Currently if modesetting ever fails to set a hardware cursor it will switch to using a software cursor and never go back. Change this to try a hardware cursor first every time a new one is set. Signed-off-by: Michael Thayer <michael.tha...@oracle.com> --- hw/xfree86/drivers/modesetting/drmmode_display.c | 47 ++-- 1 file changed, 20 insertions(+), 27 deletions(-) diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..36c3093 100644 --- a/hw/xfree86/drivers/modesetting/drmmode_display.c +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c @@ -485,44 +485,36 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int y) drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); } -static void +static Bool drmmode_set_cursor(xf86CrtcPtr crtc) { drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; drmmode_ptr drmmode = drmmode_crtc->drmmode; uint32_t handle = drmmode_crtc->cursor_bo->handle; modesettingPtr ms = modesettingPTR(crtc->scrn); -static Bool use_set_cursor2 = TRUE; int ret; -if (use_set_cursor2) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -CursorPtr cursor = xf86_config->cursor; - -ret = -drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, - handle, ms->cursor_width, ms->cursor_height, - cursor->bits->xhot, cursor->bits->yhot); -if (!ret) -return; -if (ret == -EINVAL) -use_set_cursor2 = FALSE; -} +xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); +CursorPtr cursor = xf86_config->cursor; -ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, handle, - ms->cursor_width, ms->cursor_height); +ret = +drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height, + cursor->bits->xhot, cursor->bits->yhot); +if (!ret) +return TRUE; -if (ret) { -xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn); -xf86CursorInfoPtr cursor_info = xf86_config->cursor_info; +/* -EINVAL can mean bad cursor parameters or Cursor2 API not supported. */ +if (ret == -EINVAL) +ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, + handle, ms->cursor_width, ms->cursor_height); -cursor_info->MaxWidth = cursor_info->MaxHeight = 0; -drmmode_crtc->drmmode->sw_cursor = TRUE; -/* fallback to swcursor */ -} +if (ret) +return FALSE; /* fallback to swcursor */ +return TRUE; } -static void +static Bool drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) { modesettingPtr ms = modesettingPTR(crtc->scrn); @@ -537,7 +529,8 @@ drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) ptr[i] = image[i]; // cpu_to_le32(image[i]); if (drmmode_crtc->cursor_up) -drmmode_set_cursor(crtc); +return drmmode_set_cursor(crtc); +return TRUE; } static void @@ -799,7 +792,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = { .set_cursor_position = drmmode_set_cursor_position, .show_cursor = drmmode_show_cursor, .hide_cursor = drmmode_hide_cursor, -.load_cursor_argb = drmmode_load_cursor_argb, +.load_cursor_argb_check = drmmode_load_cursor_argb, .gamma_set = drmmode_crtc_gamma_set, .destroy = NULL,/* XXX */ -- 2.5.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
Modesetting can't switch back from a software to a hardware cursor (patch to follow)
At present if modesetting ever fails to set a hardware cursor it switches back to a software one and stays that way until it is unloaded. The following patch should fix that. I say "should" because I had difficulties testing it - the cursor simply disappeared when it should have been rendering in software, though the debugger showed that pixman_image_composite() was getting called whenever the cursor moved, and my kernel driver was getting dirty rectangle information. My feeling is that the patch is correct and something else is broken. I have not investigated in depth in case some one else immediately has an idea. Note that I did my testing against Ubuntu's 1.17.2 server. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Unplugging the main graphics device
On 22.02.2016 21:29, Dave Airlie wrote: On 22 February 2016 at 22:20, Michael Thayer <michael.tha...@oracle.com> wrote: On 19.02.2016 16:16, Michael Thayer wrote: I have been experimenting a bit with plugging and unplugging of graphics devices (using a dummy KMS driver which is udl stripped of the actual hardware poking) and how the X server copes with that. It seems to cope well with a secondary device being removed, but not with the only graphics device in the system disappearing (in that case the hot-pluggable device is not deemed to be a GPU device, and therefore not removable if I understood what is happening correctly). [...] Install driver, ask user to reboot. Trying to remove the first screen from X is a long and insanity inspiring process. I've spent months hacking up something that lets us migrate stuff from screen A to screen B, but it's really messy and the current X server code doesn't lend itself to it at all, so I pretty much gave up the last time I tried. Then perhaps having the place-holder first device in our kernel driver is a solution worth considering. As the main person in charge of the kernel DRM tree, is that something you could live with? I realise that making life easier for external drivers is not something which is normally done in the Linux kernel, but I think we have a valid reason for wanting to update the driver without updating the kernel. We will probably need to go that way with our out-of-tree drivers anyway to support older kernel-and-X.Org combinations. Or am I missing something here? In theory this could also be done directly in the X server, and since you spent months working on this you probably already thought about something on these lines. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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: Unplugging the main graphics device
On 19.02.2016 16:16, Michael Thayer wrote: I have been experimenting a bit with plugging and unplugging of graphics devices (using a dummy KMS driver which is udl stripped of the actual hardware poking) and how the X server copes with that. It seems to cope well with a secondary device being removed, but not with the only graphics device in the system disappearing (in that case the hot-pluggable device is not deemed to be a GPU device, and therefore not removable if I understood what is happening correctly). This is interesting for me because I am looking at putting a KMS driver for the VirtualBox video device into the upstream kernel, but would like to be able to update the driver at run-time, so that we are not stuck with whatever version some guest distribution which is no longer being updated happens to provide. My first idea for handling this was to simulate a device unplug so that the old driver could be removed and the new one added. (Not sure if I am handling the hotplug right in the vboxvideo driver yet of course, but I assume that my dummy driver, which just copies code from udl, does get it right.) I will take a look at this when I get a chance of course, but I thought I would write to the list before that in case anyone else has thoughts, ideas or fixes (potenially including how I could better handle the driver update). Re-reading the X server code a couple of days later makes it clear that this is intended. The question is how I should best deal with this. Would people be open to having the first graphics device hot-removable too? Obviously there are other use cases for this, like using DisplayLink with a headless embedded device. But I realise that the X server was designed with the assumption that this will not happen. Any other suggestions? I doubt having a dummy first device which can stay in place would go down well elsewhere. Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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
Unplugging the main graphics device
Hello, I have been experimenting a bit with plugging and unplugging of graphics devices (using a dummy KMS driver which is udl stripped of the actual hardware poking) and how the X server copes with that. It seems to cope well with a secondary device being removed, but not with the only graphics device in the system disappearing (in that case the hot-pluggable device is not deemed to be a GPU device, and therefore not removable if I understood what is happening correctly). This is interesting for me because I am looking at putting a KMS driver for the VirtualBox video device into the upstream kernel, but would like to be able to update the driver at run-time, so that we are not stuck with whatever version some guest distribution which is no longer being updated happens to provide. My first idea for handling this was to simulate a device unplug so that the old driver could be removed and the new one added. (Not sure if I am handling the hotplug right in the vboxvideo driver yet of course, but I assume that my dummy driver, which just copies code from udl, does get it right.) I will take a look at this when I get a chance of course, but I thought I would write to the list before that in case anyone else has thoughts, ideas or fixes (potenially including how I could better handle the driver update). Regards, Michael -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: Riesstraße 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher ___ 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] shadowfb: Fix initialization
On 21/05/14 15:23, Adam Jackson wrote: This has to run at initial CreateWindow time, at CreateScreenResources the root window doesn't actually exist yet. Tested-by: Michael Thayer michael.tha...@oracle.com Signed-off-by: Adam Jackson a...@redhat.com I'm not an expert in that area of the code, but I did also go over the actual content of the patch. So: Reviewed-by: Michael Thayer michael.tha...@oracle.com --- hw/xfree86/shadowfb/shadow.c | 26 +++--- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/hw/xfree86/shadowfb/shadow.c b/hw/xfree86/shadowfb/shadow.c index 10f72cc..d2481ed 100644 --- a/hw/xfree86/shadowfb/shadow.c +++ b/hw/xfree86/shadowfb/shadow.c @@ -29,14 +29,14 @@ #include picturestr.h static Bool ShadowCloseScreen(ScreenPtr pScreen); -static Bool ShadowCreateScreenResources(ScreenPtr pScreen); +static Bool ShadowCreateRootWindow(WindowPtr pWin); typedef struct { ScrnInfoPtr pScrn; RefreshAreaFuncPtr preRefresh; RefreshAreaFuncPtr postRefresh; CloseScreenProcPtr CloseScreen; -CreateScreenResourcesProcPtr CreateScreenResources; +CreateWindowProcPtr CreateWindow; } ShadowScreenRec, *ShadowScreenPtr; static DevPrivateKeyRec ShadowScreenKeyRec; @@ -71,10 +71,10 @@ ShadowFBInit2(ScreenPtr pScreen, pPriv-postRefresh = postRefreshArea; pPriv-CloseScreen = pScreen-CloseScreen; -pPriv-CreateScreenResources = pScreen-CreateScreenResources; +pPriv-CreateWindow = pScreen-CreateWindow; pScreen-CloseScreen = ShadowCloseScreen; -pScreen-CreateScreenResources = ShadowCreateScreenResources; +pScreen-CreateWindow = ShadowCreateRootWindow; return TRUE; } @@ -117,16 +117,21 @@ shadowfbReportPost(DamagePtr damage, RegionPtr reg, void *closure) } static Bool -ShadowCreateScreenResources(ScreenPtr pScreen) +ShadowCreateRootWindow(WindowPtr pWin) { Bool ret; -WindowPtr pWin = pScreen-root; +ScreenPtr pScreen = pWin-drawable.pScreen; ShadowScreenPtr pPriv = shadowfbGetScreenPrivate(pScreen); -pScreen-CreateScreenResources = pPriv-CreateScreenResources; -ret = pScreen-CreateScreenResources(pScreen); -pPriv-CreateScreenResources = pScreen-CreateScreenResources; -pScreen-CreateScreenResources = ShadowCreateScreenResources; +/* paranoia */ +if (pWin != pScreen-root) +ErrorF(ShadowCreateRootWindow called unexpectedly\n); + +/* call down, but don't hook ourselves back in; we know the first time + * we're called it's for the root window. + */ +pScreen-CreateWindow = pPriv-CreateWindow; +ret = pScreen-CreateWindow(pWin); /* this might look like it leaks, but the damage code reaps listeners * when their drawable disappears. @@ -159,7 +164,6 @@ ShadowCloseScreen(ScreenPtr pScreen) ShadowScreenPtr pPriv = shadowfbGetScreenPrivate(pScreen); pScreen-CloseScreen = pPriv-CloseScreen; -pScreen-CreateScreenResources = pPriv-CreateScreenResources; free(pPriv); -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] Add a return value to set_cursor_position() to allow it to report failure
On 30/04/14 19:23, Keith Packard wrote: Michael Thayer michael.tha...@oracle.com writes: On 22/04/14 07:00, Keith Packard wrote: Michael Thayer michael.tha...@oracle.com writes: set_cursor_position() may need to be able to fail and have the server fall back to a software cursor in at least the situation in which we are running on virtual hardware and using the host cursor as a hardware cursor for the guest but cannot change its position. Frankly, the usual solution for nested or virtual X servers is to just ignore the cursor position assignment. I would still very much like to have the change if it considered acceptable, and can submit an fixed patch like the one for set_cursor_argb() and friends to force a driver build break. Of course if you tell me it is something that you would rather not have in I can leave it at that. I don't see any value in allowing pointer warping to fail and setting a software cursor in response. If the next input event from the user isn't going to happen at the warped position, then you should be setting the cursor position before sending the next input event anyways. The alternative is for you to simply always use a software cursor, which would be easy to manage. The cases I had in mind were pointer confining and input events from a different input device, both of which cause the position of the cursor sprite to differ from that of that reported by the input device for more than a negligible time. No matter though, I can solve this as a fall-back with a client application which monitors the positions and tells the driver to disable the hardware cursor when needed. Thanks and regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] Bump ABI_VIDEODRV_VERSION for commit 4c39326
ABI version 18 introduces load_cursor_image_check, load_cursor_argb_check, LoadCursorImageCheck and LoadCursorARGBCheck. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- hw/xfree86/common/xf86Module.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h index 62ac95d..b848f53 100644 --- a/hw/xfree86/common/xf86Module.h +++ b/hw/xfree86/common/xf86Module.h @@ -80,7 +80,7 @@ typedef enum { * mask is 0x. */ #define ABI_ANSIC_VERSION SET_ABI_VERSION(0, 4) -#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(17, 0) +#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(18, 0) #define ABI_XINPUT_VERSION SET_ABI_VERSION(21, 0) #define ABI_EXTENSION_VERSION SET_ABI_VERSION(8, 0) #define ABI_FONT_VERSION SET_ABI_VERSION(0, 6) -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] hw/xfree86: Restore API compatibility for cursor loading functions
On 25/04/14 23:43, Keith Packard wrote: Keith Packard kei...@keithp.com writes: Create load_cursor_image_check, load_cursor_argb_check, LoadCursorImageCheck and LoadCursorARGBCheck that can return failure and use them in preference to the old unchecked variants. Signed-off-by: Keith Packard kei...@keithp.com Merged (with reviews): 99f0365..4c39326 master - master Of course, now that I think of it I should simply have added use_cursor_image() and use_cursor_argb() APIs to xf86CrtcFuncsRec and left xf86CursorInfoRec and the code around it as it was... Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] hw/xfree86: Restore API compatibility for cursor loading functions
On 25/04/14 18:54, Keith Packard wrote: Create load_cursor_image_check, load_cursor_argb_check, LoadCursorImageCheck and LoadCursorARGBCheck that can return failure and use them in preference to the old unchecked variants. Signed-off-by: Keith Packard kei...@keithp.com --- On IRC this morning, Hans and I thought this might provide the best compatibility story -- existing driver sources should work with only a recompile, which the existing ABI version checks will enforce. New drivers can provide the newer entry point where necessary and the server will choose the _check versions over the old ones when both are available. I've tested this with the upstream intel driver sources (without the return value change) and it works correctly Looks good to me too. Thanks a lot! I will do something similar for the set_cursor_position() patch... Regards, Michael Reviewed-by: Michael Thayer michael.tha...@oracle.com hw/xfree86/modes/xf86Crtc.h| 8 -- hw/xfree86/modes/xf86Cursors.c | 56 +- hw/xfree86/ramdac/IBM.c| 4 +-- hw/xfree86/ramdac/TI.c | 2 +- hw/xfree86/ramdac/xf86Cursor.h | 36 +-- hw/xfree86/ramdac/xf86HWCurs.c | 14 +-- 6 files changed, 95 insertions(+), 25 deletions(-) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index 5407deb..eebe6f4 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -186,14 +186,18 @@ typedef struct _xf86CrtcFuncs { /** * Load monochrome image */ -Bool +void (*load_cursor_image) (xf86CrtcPtr crtc, CARD8 *image); +Bool + (*load_cursor_image_check) (xf86CrtcPtr crtc, CARD8 *image); /** * Load ARGB image */ -Bool +void (*load_cursor_argb) (xf86CrtcPtr crtc, CARD32 *image); +Bool + (*load_cursor_argb_check) (xf86CrtcPtr crtc, CARD32 *image); /** * Clean up driver-specific bits of the crtc diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 10ef6f6..379a27a 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -209,6 +209,40 @@ set_bit(CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool mask) } /* + * Wrappers to deal with API compatibility with drivers that don't expose + * load_cursor_*_check + */ +static inline Bool +xf86_driver_has_load_cursor_image(xf86CrtcPtr crtc) +{ +return crtc-funcs-load_cursor_image_check || crtc-funcs-load_cursor_image; +} + +static inline Bool +xf86_driver_has_load_cursor_argb(xf86CrtcPtr crtc) +{ +return crtc-funcs-load_cursor_argb_check || crtc-funcs-load_cursor_argb; +} + +static inline Bool +xf86_driver_load_cursor_image(xf86CrtcPtr crtc, CARD8 *cursor_image) +{ +if (crtc-funcs-load_cursor_image_check) +return crtc-funcs-load_cursor_image_check(crtc, cursor_image); +crtc-funcs-load_cursor_image(crtc, cursor_image); +return TRUE; +} + +static inline Bool +xf86_driver_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *cursor_argb) +{ +if (crtc-funcs-load_cursor_argb_check) +return crtc-funcs-load_cursor_argb_check(crtc, cursor_argb); +crtc-funcs-load_cursor_argb(crtc, cursor_argb); +return TRUE; +} + +/* * Load a two color cursor into a driver that supports only ARGB cursors */ static Bool @@ -244,7 +278,7 @@ xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src) bits = 0; cursor_image[y * cursor_info-MaxWidth + x] = bits; } -return crtc-funcs-load_cursor_argb(crtc, cursor_image); +return xf86_driver_load_cursor_argb(crtc, cursor_image); } /* @@ -269,7 +303,7 @@ xf86_set_cursor_colors(ScrnInfoPtr scrn, int bg, int fg) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled !crtc-cursor_argb) { -if (crtc-funcs-load_cursor_image) +if (xf86_driver_has_load_cursor_image(crtc)) crtc-funcs-set_cursor_colors(crtc, bg, fg); else if (bits) xf86_crtc_convert_cursor_to_argb(crtc, bits); @@ -450,7 +484,7 @@ xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src) set_bit(cursor_image, cursor_info, x, y, TRUE); } } -return crtc-funcs-load_cursor_image(crtc, cursor_image); +return xf86_driver_load_cursor_image(crtc, cursor_image); } /* @@ -466,10 +500,10 @@ xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled) { -if (crtc-funcs-load_cursor_image) { +if (xf86_driver_has_load_cursor_image(crtc)) { if (!xf86_crtc_load_cursor_image(crtc, src)) return FALSE; -} else if (crtc-funcs-load_cursor_argb) { +} else if (xf86_driver_has_load_cursor_argb(crtc
Re: how to turn source code into debian compatible driver
Hello James, On 23/04/14 17:23, James Robb wrote: I have download this file: http://cgit.freedesktop.org/xorg/driver/xf86-input-synaptics/snapshot/xf86-input-synaptics-1.7.99.1.tar.gz under the impression it is the packaged code I need to compile the latest updates to the xserver-xorg-input-synaptics package for ubuntu 14.04. I have managed to run: ./configure ./make ./make install all successfully, but with the results from these commands, I don't see a compiled driver anywhere that I can inject into my own system. I have been a software developer for about 7 years now, but mostly in web application. I am starting to learn about linux development, but am having trouble finding the material I need to package this in a meaningful way for me. Is there something obvious I am missing or some resource I can be pointed towards? I really want to be able to wrap my head around this so that hopefully one day I can contribute more to the linux codebase. You might want to look at rebuilding the Ubuntu package using the updated source. There is a bit of a learning curve here, but see for starters: https://help.ubuntu.com/community/UpdatingADeb https://wiki.debian.org/BuildingAPackage You might also want to look at Ubuntu's semi-official X.Org update repositories, though I suspect that that will be at least a similar effort to rebuilding the packages for less educational gain: https://launchpad.net/~ubuntu-x-swat/+archive/x-updates https://launchpad.net/~xorg-edgers/+archive/ppa/+index?batch=75memo=75start=75 Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xf86-input-mouse] Make absolute input reporting in Solaris aware of resolution changes
Currently on Solaris absolute input reporting only takes resolution changes into account when the video driver is using the pre-RandR 1.2 APIs, and there it uses the physical resolution, not the virtual. This patch fixes those two things. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- This builds against master but actual testing was done against 1.7.2. I think that the two are close enough for this to be safe though. src/sun_mouse.c | 42 +- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/src/sun_mouse.c b/src/sun_mouse.c index 16434e6..90a0c23 100644 --- a/src/sun_mouse.c +++ b/src/sun_mouse.c @@ -57,6 +57,7 @@ #include mouse.h #include xisb.h #include mipointer.h +#include xf86Crtc.h #include sys/stropts.h #include sys/vuid_event.h #include sys/msio.h @@ -405,14 +406,11 @@ static void vuidMouseSendScreenSize(ScreenPtr pScreen, VuidMsePtr pVuidMse) ScrnInfoPtr pScr = XF86SCRNINFO(pScreen); int result; -if (!pScr-currentMode) -return; - -if ((pVuidMse-absres.width != pScr-currentMode-HDisplay) || -(pVuidMse-absres.height != pScr-currentMode-VDisplay)) +if ((pVuidMse-absres.width != pScr-virtualX) || +(pVuidMse-absres.height != pScr-virtualY)) { -pVuidMse-absres.width = pScr-currentMode-HDisplay; -pVuidMse-absres.height = pScr-currentMode-VDisplay; +pVuidMse-absres.width = pScr-virtualX; +pVuidMse-absres.height = pScr-virtualY; do { result = ioctl(pInfo-fd, MSIOSRESOLUTION, (pVuidMse-absres)); @@ -457,6 +455,24 @@ static void vuidMouseAdjustFrame(ADJUST_FRAME_ARGS_DECL) } } } + +static void vuidMouseCrtcNotify(ScreenPtr pScreen) +{ +xf86_crtc_notify_proc_ptr wrappedCrtcNotify += (xf86_crtc_notify_proc_ptr) vuidMouseGetScreenPrivate(pScreen); +VuidMsePtr m; +ScreenPtrptrCurScreen; + +if(wrappedCrtcNotify) +wrappedCrtcNotify(pScreen); + +for (m = vuidMouseList; m != NULL ; m = m-next) { +ptrCurScreen = miPointerGetScreen(m-pInfo-dev); +if (ptrCurScreen == pScreen) { +vuidMouseSendScreenSize(pScreen, m); +} +} +} #endif /* HAVE_ABSOLUTE_MOUSE_SCALING */ @@ -492,8 +508,16 @@ vuidMouseProc(DeviceIntPtr pPointer, int what) for (i = 0; i screenInfo.numScreens; i++) { ScreenPtr pScreen = screenInfo.screens[i]; ScrnInfoPtr pScrn = XF86SCRNINFO(pScreen); -vuidMouseSetScreenPrivate(pScreen, pScrn-AdjustFrame); -pScrn-AdjustFrame = vuidMouseAdjustFrame; +if (xf86CrtcConfigPrivateIndex != -1) { +xf86_crtc_notify_proc_ptr pCrtcNotify += xf86_wrap_crtc_notify(pScreen, +vuidMouseCrtcNotify); +vuidMouseSetScreenPrivate(pScreen, pCrtcNotify); +} else { +vuidMouseSetScreenPrivate(pScreen, + pScrn-AdjustFrame); +pScrn-AdjustFrame = vuidMouseAdjustFrame; +} } vuidMouseGeneration = serverGeneration; } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] Add a return value to set_cursor_position() to allow it to report failure
set_cursor_position() may need to be able to fail and have the server fall back to a software cursor in at least the situation in which we are running on virtual hardware and using the host cursor as a hardware cursor for the guest but cannot change its position. Rename relevant APIs to force driver build breaks as the return type change will only trigger warnings otherwise. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- hw/xfree86/common/xf86Module.h | 2 +- hw/xfree86/modes/xf86Crtc.h| 4 ++-- hw/xfree86/modes/xf86Cursors.c | 18 -- hw/xfree86/ramdac/IBM.c| 10 ++ hw/xfree86/ramdac/TI.c | 5 +++-- hw/xfree86/ramdac/xf86Cursor.c | 28 hw/xfree86/ramdac/xf86Cursor.h | 2 +- hw/xfree86/ramdac/xf86CursorPriv.h | 4 +++- hw/xfree86/ramdac/xf86HWCurs.c | 10 +- 9 files changed, 57 insertions(+), 26 deletions(-) diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h index 62ac95d..b848f53 100644 --- a/hw/xfree86/common/xf86Module.h +++ b/hw/xfree86/common/xf86Module.h @@ -80,7 +80,7 @@ typedef enum { * mask is 0x. */ #define ABI_ANSIC_VERSION SET_ABI_VERSION(0, 4) -#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(17, 0) +#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(18, 0) #define ABI_XINPUT_VERSION SET_ABI_VERSION(21, 0) #define ABI_EXTENSION_VERSION SET_ABI_VERSION(8, 0) #define ABI_FONT_VERSION SET_ABI_VERSION(0, 6) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index 5407deb..a5c05f6 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -168,8 +168,8 @@ typedef struct _xf86CrtcFuncs { /** * Set cursor position */ -void - (*set_cursor_position) (xf86CrtcPtr crtc, int x, int y); +Bool + (*set_cursor_position2) (xf86CrtcPtr crtc, int x, int y); /** * Show cursor diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 10ef6f6..828b3ba 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -355,7 +355,7 @@ xf86CrtcTransformCursorPos(xf86CrtcPtr crtc, int *x, int *y) *y -= dy; } -static void +static Bool xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y) { ScrnInfoPtr scrn = crtc-scrn; @@ -363,6 +363,7 @@ xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y) xf86CursorInfoPtr cursor_info = xf86_config-cursor_info; DisplayModePtr mode = crtc-mode; Bool in_range; +Bool ret = FALSE; /* * Transform position of cursor on screen @@ -388,18 +389,21 @@ xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y) crtc-cursor_in_range = in_range; if (in_range) { -crtc-funcs-set_cursor_position(crtc, x, y); +if (crtc-funcs-set_cursor_position2(crtc, x, y)) +ret = TRUE; xf86_crtc_show_cursor(crtc); } else xf86_crtc_hide_cursor(crtc); +return ret; } -static void +static Bool xf86_set_cursor_position(ScrnInfoPtr scrn, int x, int y) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); int c; +Bool ret = FALSE; /* undo what xf86HWCurs did to the coordinates */ x += scrn-frameX0; @@ -408,8 +412,10 @@ xf86_set_cursor_position(ScrnInfoPtr scrn, int x, int y) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled) -xf86_crtc_set_cursor_position(crtc, x, y); +if (xf86_crtc_set_cursor_position(crtc, x, y)) +ret = TRUE; } +return ret; } /* @@ -593,7 +599,7 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags) cursor_info-Flags = flags; cursor_info-SetCursorColors = xf86_set_cursor_colors; -cursor_info-SetCursorPosition = xf86_set_cursor_position; +cursor_info-SetCursorPosition2 = xf86_set_cursor_position; cursor_info-LoadCursorImage = xf86_load_cursor_image; cursor_info-HideCursor = xf86_hide_cursors; cursor_info-ShowCursor = xf86_show_cursors; @@ -666,7 +672,7 @@ xf86_reload_cursors(ScreenPtr screen) x += scrn-frameX0 + cursor_screen_priv-HotX; y += scrn-frameY0 + cursor_screen_priv-HotY; -(*cursor_info-SetCursorPosition) (scrn, x, y); +(*cursor_info-SetCursorPosition2) (scrn, x, y); } } diff --git a/hw/xfree86/ramdac/IBM.c b/hw/xfree86/ramdac/IBM.c index 872d3d4..21dc172 100644 --- a/hw/xfree86/ramdac/IBM.c +++ b/hw/xfree86/ramdac/IBM.c @@ -505,7 +505,7 @@ IBMramdac640HideCursor(ScrnInfoPtr pScrn) (*ramdacPtr-WriteDAC) (pScrn, RGB640_CURSOR_CONTROL, 0x00, 0x08); } -static void +static Bool IBMramdac526SetCursorPosition(ScrnInfoPtr pScrn, int x, int y) { RamDacRecPtr ramdacPtr = RAMDACSCRPTR(pScrn); @@ -519,9 +519,10 @@ IBMramdac526SetCursorPosition(ScrnInfoPtr pScrn, int x, int y) (*ramdacPtr-WriteDAC) (pScrn, IBMRGB_curs_xh
[PATCH] Fix commit 901fbfb by renaming load_cursor_argb() etc to force build breaks
Commit 901fbfb changed the return value of load_cursor_argb() and other APIs which caused silent breakage of drivers. Rename the changed APIs to make the breakage visible. The ABI version was already bumped with the original commit. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- Sorry for the unimaginitive new names... Regards, Michael hw/xfree86/modes/xf86Crtc.h| 4 ++-- hw/xfree86/modes/xf86Cursors.c | 22 +++--- hw/xfree86/ramdac/IBM.c| 4 ++-- hw/xfree86/ramdac/TI.c | 2 +- hw/xfree86/ramdac/xf86Cursor.c | 10 +- hw/xfree86/ramdac/xf86Cursor.h | 4 ++-- hw/xfree86/ramdac/xf86CursorPriv.h | 2 +- hw/xfree86/ramdac/xf86HWCurs.c | 16 8 files changed, 32 insertions(+), 32 deletions(-) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index 5407deb..286b785 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -187,13 +187,13 @@ typedef struct _xf86CrtcFuncs { * Load monochrome image */ Bool - (*load_cursor_image) (xf86CrtcPtr crtc, CARD8 *image); + (*load_cursor_image2) (xf86CrtcPtr crtc, CARD8 *image); /** * Load ARGB image */ Bool - (*load_cursor_argb) (xf86CrtcPtr crtc, CARD32 *image); + (*load_cursor_argb2) (xf86CrtcPtr crtc, CARD32 *image); /** * Clean up driver-specific bits of the crtc diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 10ef6f6..ea3cd64 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -244,7 +244,7 @@ xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src) bits = 0; cursor_image[y * cursor_info-MaxWidth + x] = bits; } -return crtc-funcs-load_cursor_argb(crtc, cursor_image); +return crtc-funcs-load_cursor_argb2(crtc, cursor_image); } /* @@ -269,7 +269,7 @@ xf86_set_cursor_colors(ScrnInfoPtr scrn, int bg, int fg) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled !crtc-cursor_argb) { -if (crtc-funcs-load_cursor_image) +if (crtc-funcs-load_cursor_image2) crtc-funcs-set_cursor_colors(crtc, bg, fg); else if (bits) xf86_crtc_convert_cursor_to_argb(crtc, bits); @@ -450,7 +450,7 @@ xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src) set_bit(cursor_image, cursor_info, x, y, TRUE); } } -return crtc-funcs-load_cursor_image(crtc, cursor_image); +return crtc-funcs-load_cursor_image2(crtc, cursor_image); } /* @@ -466,10 +466,10 @@ xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled) { -if (crtc-funcs-load_cursor_image) { +if (crtc-funcs-load_cursor_image2) { if (!xf86_crtc_load_cursor_image(crtc, src)) return FALSE; -} else if (crtc-funcs-load_cursor_argb) { +} else if (crtc-funcs-load_cursor_argb2) { if (!xf86_crtc_convert_cursor_to_argb(crtc, src)) return FALSE; } else @@ -549,7 +549,7 @@ xf86_crtc_load_cursor_argb(xf86CrtcPtr crtc, CursorPtr cursor) cursor_image[y * image_width + x] = bits; } -return crtc-funcs-load_cursor_argb(crtc, cursor_image); +return crtc-funcs-load_cursor_argb2(crtc, cursor_image); } static Bool @@ -594,14 +594,14 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int max_height, int flags) cursor_info-SetCursorColors = xf86_set_cursor_colors; cursor_info-SetCursorPosition = xf86_set_cursor_position; -cursor_info-LoadCursorImage = xf86_load_cursor_image; +cursor_info-LoadCursorImage2 = xf86_load_cursor_image; cursor_info-HideCursor = xf86_hide_cursors; cursor_info-ShowCursor = xf86_show_cursors; cursor_info-UseHWCursor = xf86_use_hw_cursor; #ifdef ARGB_CURSOR if (flags HARDWARE_CURSOR_ARGB) { cursor_info-UseHWCursorARGB = xf86_use_hw_cursor_argb; -cursor_info-LoadCursorARGB = xf86_load_cursor_argb; +cursor_info-LoadCursorARGB2 = xf86_load_cursor_argb; } #endif @@ -658,11 +658,11 @@ xf86_reload_cursors(ScreenPtr screen) dixLookupScreenPrivate(cursor-devPrivates, CursorScreenKey, screen); #ifdef ARGB_CURSOR -if (cursor-bits-argb cursor_info-LoadCursorARGB) -(*cursor_info-LoadCursorARGB) (scrn, cursor); +if (cursor-bits-argb cursor_info-LoadCursorARGB2) +(*cursor_info-LoadCursorARGB2) (scrn, cursor); else if (src) #endif -(*cursor_info-LoadCursorImage) (scrn, src); +(*cursor_info-LoadCursorImage2) (scrn, src); x += scrn-frameX0 + cursor_screen_priv-HotX; y += scrn-frameY0
Re: [PATCH xf86-input-mouse] Make absolute input reporting in Solaris aware of resolution changes
On 19/04/14 08:13, Alan Coopersmith wrote: This doesn't apply to current git at all. The git index of ad38ba4 suggests it's based on xf86-input-mouse 1.7.2, not the current 1.9.0 release or git master. Sorry about that - I was doing my testing using that version as I was having trouble getting the latest versions of everything building in my Solaris VM, though I thought I had submitted my forward-port to master. I will check that tomorrow and re-submit. Regards, Michael On 04/10/14 11:21 AM, Michael Thayer wrote: Currently on Solaris absolute input reporting only takes resolution changes into account when the video driver is using the pre-RandR 1.2 APIs, and there it uses the physical resolution, not the virtual. This patch fixes those two things. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- src/sun_mouse.c | 42 +- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/src/sun_mouse.c b/src/sun_mouse.c index ad38ba4..9ffd590 100644 --- a/src/sun_mouse.c +++ b/src/sun_mouse.c @@ -57,6 +57,7 @@ #include mouse.h #include xisb.h #include mipointer.h +#include xf86Crtc.h #include sys/stropts.h #include sys/vuid_event.h #include sys/msio.h @@ -401,14 +402,11 @@ static void vuidMouseSendScreenSize(ScreenPtr pScreen, VuidMsePtr pVuidMse) ScrnInfoPtr pScr = XF86SCRNINFO(pScreen); int result; -if (!pScr-currentMode) -return; - -if ((pVuidMse-absres.width != pScr-currentMode-HDisplay) || -(pVuidMse-absres.height != pScr-currentMode-VDisplay)) +if ((pVuidMse-absres.width != pScr-virtualX) || +(pVuidMse-absres.height != pScr-virtualY)) { -pVuidMse-absres.width = pScr-currentMode-HDisplay; -pVuidMse-absres.height = pScr-currentMode-VDisplay; +pVuidMse-absres.width = pScr-virtualX; +pVuidMse-absres.height = pScr-virtualY; do { result = ioctl(pInfo-fd, MSIOSRESOLUTION, (pVuidMse-absres)); @@ -452,6 +450,24 @@ static void vuidMouseAdjustFrame(int index, int x, int y, int flags) } } } + +static void vuidMouseCrtcNotify(ScreenPtr pScreen) +{ + xf86_crtc_notify_proc_ptr wrappedCrtcNotify + = (xf86_crtc_notify_proc_ptr) vuidMouseGetScreenPrivate(pScreen); + VuidMsePtrm; + ScreenPtr ptrCurScreen; + + if(wrappedCrtcNotify) + wrappedCrtcNotify(pScreen); + + for (m = vuidMouseList; m != NULL ; m = m-next) { + ptrCurScreen = miPointerGetScreen(m-pInfo-dev); + if (ptrCurScreen == pScreen) { + vuidMouseSendScreenSize(pScreen, m); + } + } +} #endif /* HAVE_ABSOLUTE_MOUSE_SCALING */ @@ -487,8 +503,16 @@ vuidMouseProc(DeviceIntPtr pPointer, int what) for (i = 0; i screenInfo.numScreens; i++) { ScreenPtr pScreen = screenInfo.screens[i]; ScrnInfoPtr pScrn = XF86SCRNINFO(pScreen); -vuidMouseSetScreenPrivate(pScreen, pScrn-AdjustFrame); -pScrn-AdjustFrame = vuidMouseAdjustFrame; +if (xf86CrtcConfigPrivateIndex != -1) { +xf86_crtc_notify_proc_ptr pCrtcNotify += xf86_wrap_crtc_notify(pScreen, +vuidMouseCrtcNotify); +vuidMouseSetScreenPrivate(pScreen, pCrtcNotify); +} else { +vuidMouseSetScreenPrivate(pScreen, + pScrn-AdjustFrame); +pScrn-AdjustFrame = vuidMouseAdjustFrame; +} } vuidMouseGeneration = serverGeneration; } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Adding a return value to set_cursor_position() to allow it to report failure
Since I did not get feedback about whether or not this patch would be of interest I just went ahead and wrote it. Sadly I have missed the merge window for 1.16 (so I had to bump the ABI version again in the patch). I am also thinking that my previous patch to add a return value to load_cursor_argb() makes the UseHWCursor*() call-backs irrelevant and that I should perhaps submit a patch to remove them. And on similar lines, is xf86ForceHWCursor() still useful? The only user seems to be the R128 DRI1 code. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] Add a return value to set_cursor_position() to allow it to report failure
set_cursor_position() may need to be able to fail and have the server fall back to a software cursor in at least the situation in which we are running on virtual hardware and using the host cursor as a hardware cursor for the guest but cannot change its position. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- hw/xfree86/common/xf86Module.h | 2 +- hw/xfree86/modes/xf86Crtc.h| 2 +- hw/xfree86/modes/xf86Cursors.c | 14 ++ hw/xfree86/ramdac/IBM.c| 6 -- hw/xfree86/ramdac/TI.c | 3 ++- hw/xfree86/ramdac/xf86Cursor.c | 28 hw/xfree86/ramdac/xf86Cursor.h | 2 +- hw/xfree86/ramdac/xf86CursorPriv.h | 4 +++- hw/xfree86/ramdac/xf86HWCurs.c | 4 ++-- 9 files changed, 48 insertions(+), 17 deletions(-) diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h index 62ac95d..b848f53 100644 --- a/hw/xfree86/common/xf86Module.h +++ b/hw/xfree86/common/xf86Module.h @@ -80,7 +80,7 @@ typedef enum { * mask is 0x. */ #define ABI_ANSIC_VERSION SET_ABI_VERSION(0, 4) -#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(17, 0) +#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(18, 0) #define ABI_XINPUT_VERSION SET_ABI_VERSION(21, 0) #define ABI_EXTENSION_VERSION SET_ABI_VERSION(8, 0) #define ABI_FONT_VERSION SET_ABI_VERSION(0, 6) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index 5407deb..6fe0b1b 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -168,7 +168,7 @@ typedef struct _xf86CrtcFuncs { /** * Set cursor position */ -void +Bool (*set_cursor_position) (xf86CrtcPtr crtc, int x, int y); /** diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 10ef6f6..f8fb941 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -355,7 +355,7 @@ xf86CrtcTransformCursorPos(xf86CrtcPtr crtc, int *x, int *y) *y -= dy; } -static void +static Bool xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y) { ScrnInfoPtr scrn = crtc-scrn; @@ -363,6 +363,7 @@ xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y) xf86CursorInfoPtr cursor_info = xf86_config-cursor_info; DisplayModePtr mode = crtc-mode; Bool in_range; +Bool ret = FALSE; /* * Transform position of cursor on screen @@ -388,18 +389,21 @@ xf86_crtc_set_cursor_position(xf86CrtcPtr crtc, int x, int y) crtc-cursor_in_range = in_range; if (in_range) { -crtc-funcs-set_cursor_position(crtc, x, y); +if (crtc-funcs-set_cursor_position(crtc, x, y)) +ret = TRUE; xf86_crtc_show_cursor(crtc); } else xf86_crtc_hide_cursor(crtc); +return ret; } -static void +static Bool xf86_set_cursor_position(ScrnInfoPtr scrn, int x, int y) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); int c; +Bool ret = FALSE; /* undo what xf86HWCurs did to the coordinates */ x += scrn-frameX0; @@ -408,8 +412,10 @@ xf86_set_cursor_position(ScrnInfoPtr scrn, int x, int y) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled) -xf86_crtc_set_cursor_position(crtc, x, y); +if (xf86_crtc_set_cursor_position(crtc, x, y)) +ret = TRUE; } +return ret; } /* diff --git a/hw/xfree86/ramdac/IBM.c b/hw/xfree86/ramdac/IBM.c index 872d3d4..73b3d20 100644 --- a/hw/xfree86/ramdac/IBM.c +++ b/hw/xfree86/ramdac/IBM.c @@ -505,7 +505,7 @@ IBMramdac640HideCursor(ScrnInfoPtr pScrn) (*ramdacPtr-WriteDAC) (pScrn, RGB640_CURSOR_CONTROL, 0x00, 0x08); } -static void +static Bool IBMramdac526SetCursorPosition(ScrnInfoPtr pScrn, int x, int y) { RamDacRecPtr ramdacPtr = RAMDACSCRPTR(pScrn); @@ -519,9 +519,10 @@ IBMramdac526SetCursorPosition(ScrnInfoPtr pScrn, int x, int y) (*ramdacPtr-WriteDAC) (pScrn, IBMRGB_curs_xh, 0x00, (x 8) 0xf); (*ramdacPtr-WriteDAC) (pScrn, IBMRGB_curs_yl, 0x00, y 0xff); (*ramdacPtr-WriteDAC) (pScrn, IBMRGB_curs_yh, 0x00, (y 8) 0xf); +return TRUE; } -static void +static Bool IBMramdac640SetCursorPosition(ScrnInfoPtr pScrn, int x, int y) { RamDacRecPtr ramdacPtr = RAMDACSCRPTR(pScrn); @@ -535,6 +536,7 @@ IBMramdac640SetCursorPosition(ScrnInfoPtr pScrn, int x, int y) (*ramdacPtr-WriteDAC) (pScrn, RGB640_CURS_X_HIGH, 0x00, (x 8) 0xf); (*ramdacPtr-WriteDAC) (pScrn, RGB640_CURS_Y_LOW, 0x00, y 0xff); (*ramdacPtr-WriteDAC) (pScrn, RGB640_CURS_Y_HIGH, 0x00, (y 8) 0xf); +return TRUE; } static void diff --git a/hw/xfree86/ramdac/TI.c b/hw/xfree86/ramdac/TI.c index 7d4e0d7..05914b7 100644 --- a/hw/xfree86/ramdac/TI.c +++ b/hw/xfree86/ramdac/TI.c @@ -606,7 +606,7 @@ TIramdacHideCursor(ScrnInfoPtr pScrn) (*ramdacPtr-WriteDAC) (pScrn, TIDAC_ind_curs_ctrl, 0, 0x00); } -static void +static Bool
Re: [PATCH xf86-input-mouse] Do not drop the result of protocol detection
No response on this yet. Who would be the right person for looking at patches to xf86-input-mouse? I do realise that it is no longer as widely used as it once was... Regards, Michael On 31/03/14 11:21, Michael Thayer wrote: In MousePickProtocol() with protocol PROT_AUTO we probe for the protocol to use but drop the result in most cases. This was causing DEVICE_INIT and DEVICE_ON to fail to be called with the VUID protocol. Git history suggests that this code was originally meant to cover both PS/2 auto-detection and OS- specific detection, but that only the first case was implemented at the time. Now that only the second is needed dropping the result to keep the protocol as PROT_AUTO is presumably no longer useful and seems to actively breaking things. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- src/mouse.c |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mouse.c b/src/mouse.c index 2da2b4d..ff8d799 100644 --- a/src/mouse.c +++ b/src/mouse.c @@ -843,11 +843,8 @@ MousePickProtocol(InputInfoPtr pInfo, const char* device, { const char *osProt; if (osInfo-SetupAuto (osProt = osInfo-SetupAuto(pInfo,NULL))) { -MouseProtocolID id = ProtocolNameToID(osProt); -if (id == PROT_UNKNOWN || id == PROT_UNSUP) { -protocolID = id; -protocol = osProt; -} +protocolID = ProtocolNameToID(osProt); +protocol = osProt; } } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xf86-input-mouse] Make absolute input reporting in Solaris aware of resolution changes
Currently on Solaris absolute input reporting only takes resolution changes into account when the video driver is using the pre-RandR 1.2 APIs, and there it uses the physical resolution, not the virtual. This patch fixes those two things. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- src/sun_mouse.c | 42 +- 1 file changed, 33 insertions(+), 9 deletions(-) diff --git a/src/sun_mouse.c b/src/sun_mouse.c index ad38ba4..9ffd590 100644 --- a/src/sun_mouse.c +++ b/src/sun_mouse.c @@ -57,6 +57,7 @@ #include mouse.h #include xisb.h #include mipointer.h +#include xf86Crtc.h #include sys/stropts.h #include sys/vuid_event.h #include sys/msio.h @@ -401,14 +402,11 @@ static void vuidMouseSendScreenSize(ScreenPtr pScreen, VuidMsePtr pVuidMse) ScrnInfoPtr pScr = XF86SCRNINFO(pScreen); int result; -if (!pScr-currentMode) - return; - -if ((pVuidMse-absres.width != pScr-currentMode-HDisplay) || - (pVuidMse-absres.height != pScr-currentMode-VDisplay)) +if ((pVuidMse-absres.width != pScr-virtualX) || + (pVuidMse-absres.height != pScr-virtualY)) { - pVuidMse-absres.width = pScr-currentMode-HDisplay; - pVuidMse-absres.height = pScr-currentMode-VDisplay; + pVuidMse-absres.width = pScr-virtualX; + pVuidMse-absres.height = pScr-virtualY; do { result = ioctl(pInfo-fd, MSIOSRESOLUTION, (pVuidMse-absres)); @@ -452,6 +450,24 @@ static void vuidMouseAdjustFrame(int index, int x, int y, int flags) } } } + +static void vuidMouseCrtcNotify(ScreenPtr pScreen) +{ + xf86_crtc_notify_proc_ptr wrappedCrtcNotify + = (xf86_crtc_notify_proc_ptr) vuidMouseGetScreenPrivate(pScreen); + VuidMsePtr m; + ScreenPtrptrCurScreen; + + if(wrappedCrtcNotify) +wrappedCrtcNotify(pScreen); + + for (m = vuidMouseList; m != NULL ; m = m-next) { + ptrCurScreen = miPointerGetScreen(m-pInfo-dev); + if (ptrCurScreen == pScreen) { + vuidMouseSendScreenSize(pScreen, m); + } + } +} #endif /* HAVE_ABSOLUTE_MOUSE_SCALING */ @@ -487,8 +503,16 @@ vuidMouseProc(DeviceIntPtr pPointer, int what) for (i = 0; i screenInfo.numScreens; i++) { ScreenPtr pScreen = screenInfo.screens[i]; ScrnInfoPtr pScrn = XF86SCRNINFO(pScreen); - vuidMouseSetScreenPrivate(pScreen, pScrn-AdjustFrame); - pScrn-AdjustFrame = vuidMouseAdjustFrame; + if (xf86CrtcConfigPrivateIndex != -1) { + xf86_crtc_notify_proc_ptr pCrtcNotify + = xf86_wrap_crtc_notify(pScreen, + vuidMouseCrtcNotify); + vuidMouseSetScreenPrivate(pScreen, pCrtcNotify); + } else { + vuidMouseSetScreenPrivate(pScreen, + pScrn-AdjustFrame); + pScrn-AdjustFrame = vuidMouseAdjustFrame; + } } vuidMouseGeneration = serverGeneration; } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] Add a return value to load_cursor_argb() to allow it to report failure
Hello Dave, On 04/04/14 01:21, Dave Airlie wrote: load_cursor_argb() may need to be able to fail and have the server fall back to a software cursor in at least the following circumstances. 1) The hardware can only support some ARGB cursors and this does not just depend on cursor size. 2) Virtual hardware may not wish to pass through a cursor to the host at a particular time but may wish to accept the same cursor at another time. This patch adds a return value to the API and makes the server do the software fall-back on failure. Signed-off-by: Michael Thayer michael.tha...@oracle.com I think this is a welcome ABI break, and I probably should have done something like it ages ago when doing -modesetting, Reviewed-by: Dave Airlie airl...@redhat.com Thanks for the review! Does X.Org custom say that I should sent patches (which I obviously can't test) for all or some of the public drivers, or that the driver maintainers should do that? And my question about whether a similar patch for set_cursor_position() would make sense is still open (I assume this is relevant for Qemu too): VirtualBox can use the host cursor as a hardware cursor for a guest system, but it can't change its position, so if the guest wants the cursor anywhere except where the host put it (e.g. another device controlling it, or the cursor confined to a screen region) it needs to draw it itself. Of course, set_cursor_position() should still be called even after it has failed once so that we could switch back if the positions matched again. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH] Add a return value to load_cursor_argb() to allow it to report failure
On 04/04/14 20:46, Jasper St. Pierre wrote: For qemu, this is handled at the kernel modesetting level with QXL. See drmModeSetCursor2. On Fri, Apr 4, 2014 at 3:30 AM, Michael Thayer michael.tha...@oracle.com mailto:michael.tha...@oracle.com wrote: And my question about whether a similar patch for set_cursor_position() would make sense is still open (I assume this is relevant for Qemu too): VirtualBox can use the host cursor as a hardware cursor for a guest system, but it can't change its position, so if the guest wants the cursor anywhere except where the host put it (e.g. another device controlling it, or the cursor confined to a screen region) it needs to draw it itself. Of course, set_cursor_position() should still be called even after it has failed once so that we could switch back if the positions matched again. Isn't drmModeSetCursor2() for passing through the hot-spot? I'm not sure this would help if the X server wanted to put the cursor in a different location to the host pointer, particularly if it didn't change the sprite at that time since then drmModeSetCursor2() would not get called. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Re-send of patches
Hello, On 31/03/14 11:16, Michael Thayer wrote: I currently have three patches in the air waiting for review and hopefully integration. It would be great if someone found time to go over them. Trying again here. I was hoping to get these into 1.16, but I am getting more and more doubtful that that will be an option, especially as the cursor patch will need driver changes. I fully understand when people feel reluctant to review patches which are not related to what they are currently working on. Nonetheless - is there anything I can do to get these moving? I will not re-send the patches a third time just now unless someone asks me to. Thanks and regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] Set a flag property on the root window to say if the X server VT is active
An X11 client may need to know whether the X server virtual terminal is currently the active one. This change adds a root window property which provides that information. Intended interface user: the VirtualBox Guest Additions. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- This is an updated version of a previous patch to address concerns expressed by Daniel Martin. Regards, Michael hw/xfree86/common/xf86Events.c | 28 hw/xfree86/common/xf86Init.c| 16 +++- hw/xfree86/common/xf86Privstr.h | 4 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c index 06af739..35a673d 100644 --- a/hw/xfree86/common/xf86Events.c +++ b/hw/xfree86/common/xf86Events.c @@ -56,6 +56,7 @@ #include X11/X.h #include X11/Xpoll.h #include X11/Xproto.h +#include X11/Xatom.h #include misc.h #include compiler.h #include xf86.h @@ -431,6 +432,29 @@ xf86EnableInputDeviceForVTSwitch(InputInfoPtr pInfo) pInfo-flags = ~XI86_DEVICE_DISABLED; } +/* + * xf86UpdateHasVTProperty -- + *Update a flag property on the root window to say whether the server VT + *is currently the active one as some clients need to know this. + */ +static void +xf86UpdateHasVTProperty(Bool hasVT) +{ +Atom property_name; +int32_t value = hasVT ? 1 : 0; +int i; + +property_name = MakeAtom(HAS_VT_ATOM_NAME, sizeof(HAS_VT_ATOM_NAME) - 1, + FALSE); +if (property_name == BAD_RESOURCE) +FatalError(Failed to retrieve \HAS_VT\ atom\n); +for (i = 0; i xf86NumScreens; i++) { +ChangeWindowProperty(xf86ScrnToScreen(xf86Screens[i])-root, + property_name, XA_INTEGER, 32, + PropModeReplace, 1, value, TRUE); +} +} + void xf86VTLeave(void) { @@ -490,6 +514,8 @@ xf86VTLeave(void) if (xorgHWAccess) xf86DisableIO(); +xf86UpdateHasVTProperty(FALSE); + return; switch_failed: @@ -574,6 +600,8 @@ xf86VTEnter(void) xf86platformVTProbe(); #endif +xf86UpdateHasVTProperty(TRUE); + OsReleaseSIGIO(); } diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c index 4579ff5..5a45004 100644 --- a/hw/xfree86/common/xf86Init.c +++ b/hw/xfree86/common/xf86Init.c @@ -387,6 +387,11 @@ InstallSignalHandlers(void) } } +/* The memory storing the initial value of the XFree86_has_VT root window + * property. This has to remain available until server start-up, so we just + * use a global. */ +static CARD32 HasVTValue = 1; + /* * InitOutput -- * Initialize screenInfo for all actually accessible framebuffers. @@ -731,7 +736,9 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv) if (xf86Info.vtno = 0) { #define VT_ATOM_NAME XFree86_VT Atom VTAtom = -1; +Atom HasVTAtom = -1; CARD32 *VT = NULL; +CARD32 *HasVT = HasVTValue; int ret; /* This memory needs to stay available until the screen has been @@ -744,6 +751,8 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv) *VT = xf86Info.vtno; VTAtom = MakeAtom(VT_ATOM_NAME, sizeof(VT_ATOM_NAME) - 1, TRUE); +HasVTAtom = MakeAtom(HAS_VT_ATOM_NAME, + sizeof(HAS_VT_ATOM_NAME) - 1, TRUE); for (i = 0, ret = Success; i xf86NumScreens ret == Success; i++) { @@ -751,9 +760,14 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv) xf86RegisterRootWindowProperty(xf86Screens[i]-scrnIndex, VTAtom, XA_INTEGER, 32, 1, VT); +if (ret == Success) +ret = xf86RegisterRootWindowProperty(xf86Screens[i] + -scrnIndex, + HasVTAtom, XA_INTEGER, + 32, 1, HasVT); if (ret != Success) xf86DrvMsg(xf86Screens[i]-scrnIndex, X_WARNING, - Failed to register VT property\n); + Failed to register VT properties\n); } } diff --git a/hw/xfree86/common/xf86Privstr.h b/hw/xfree86/common/xf86Privstr.h index f7a9c9f..410ef17 100644 --- a/hw/xfree86/common/xf86Privstr.h +++ b/hw/xfree86/common/xf86Privstr.h @@ -163,4 +163,8 @@ typedef struct _RootWinProp { #define WSCONS 32 #endif +/* Root window property to tell clients whether our VT is currently active. + * Name chosen to match the XFree86_VT property. */ +#define HAS_VT_ATOM_NAME XFree86_has_VT + #endif /* _XF86PRIVSTR_H */ -- ORACLE Deutschland B.V. Co. KG Michael Thayer
[PATCH xf86-input-mouse] Do not drop the result of protocol detection
In MousePickProtocol() with protocol PROT_AUTO we probe for the protocol to use but drop the result in most cases. This was causing DEVICE_INIT and DEVICE_ON to fail to be called with the VUID protocol. Git history suggests that this code was originally meant to cover both PS/2 auto-detection and OS- specific detection, but that only the first case was implemented at the time. Now that only the second is needed dropping the result to keep the protocol as PROT_AUTO is presumably no longer useful and seems to actively breaking things. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- src/mouse.c |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mouse.c b/src/mouse.c index 2da2b4d..ff8d799 100644 --- a/src/mouse.c +++ b/src/mouse.c @@ -843,11 +843,8 @@ MousePickProtocol(InputInfoPtr pInfo, const char* device, { const char *osProt; if (osInfo-SetupAuto (osProt = osInfo-SetupAuto(pInfo,NULL))) { -MouseProtocolID id = ProtocolNameToID(osProt); -if (id == PROT_UNKNOWN || id == PROT_UNSUP) { -protocolID = id; -protocol = osProt; -} +protocolID = ProtocolNameToID(osProt); +protocol = osProt; } } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] Add a return value to load_cursor_argb() to allow it to report failure
load_cursor_argb() may need to be able to fail and have the server fall back to a software cursor in at least the following circumstances. 1) The hardware can only support some ARGB cursors and this does not just depend on cursor size. 2) Virtual hardware may not wish to pass through a cursor to the host at a particular time but may wish to accept the same cursor at another time. This patch adds a return value to the API and makes the server do the software fall-back on failure. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- I would also be interested in having set_cursor_position() able to fail and trigger a software fall-back. VirtualBox can use the host cursor as a hardware cursor for a guest system, but it can't change its position, so if the guest wants the cursor anywhere except where the host put it (e.g. another device controlling it, or the cursor confined to a screen region) it needs to draw it itself. Of course, set_cursor_position() should still be called even after it has failed once so that we could switch back if the positions matched again. hw/xfree86/common/xf86Module.h | 2 +- hw/xfree86/modes/xf86Crtc.h| 4 ++-- hw/xfree86/modes/xf86Cursors.c | 35 ++- hw/xfree86/ramdac/IBM.c| 6 -- hw/xfree86/ramdac/TI.c | 3 ++- hw/xfree86/ramdac/xf86Cursor.c | 11 ++- hw/xfree86/ramdac/xf86Cursor.h | 4 ++-- hw/xfree86/ramdac/xf86CursorPriv.h | 2 +- hw/xfree86/ramdac/xf86HWCurs.c | 15 +-- 9 files changed, 49 insertions(+), 33 deletions(-) diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h index e8c24f2..62ac95d 100644 --- a/hw/xfree86/common/xf86Module.h +++ b/hw/xfree86/common/xf86Module.h @@ -80,7 +80,7 @@ typedef enum { * mask is 0x. */ #define ABI_ANSIC_VERSION SET_ABI_VERSION(0, 4) -#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(16, 0) +#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(17, 0) #define ABI_XINPUT_VERSION SET_ABI_VERSION(21, 0) #define ABI_EXTENSION_VERSION SET_ABI_VERSION(8, 0) #define ABI_FONT_VERSION SET_ABI_VERSION(0, 6) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index c127d78..5407deb 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -186,13 +186,13 @@ typedef struct _xf86CrtcFuncs { /** * Load monochrome image */ -void +Bool (*load_cursor_image) (xf86CrtcPtr crtc, CARD8 *image); /** * Load ARGB image */ -void +Bool (*load_cursor_argb) (xf86CrtcPtr crtc, CARD32 *image); /** diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 2b0db34..10ef6f6 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -211,7 +211,7 @@ set_bit(CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool mask) /* * Load a two color cursor into a driver that supports only ARGB cursors */ -static void +static Bool xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src) { ScrnInfoPtr scrn = crtc-scrn; @@ -244,7 +244,7 @@ xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src) bits = 0; cursor_image[y * cursor_info-MaxWidth + x] = bits; } -crtc-funcs-load_cursor_argb(crtc, cursor_image); +return crtc-funcs-load_cursor_argb(crtc, cursor_image); } /* @@ -415,7 +415,7 @@ xf86_set_cursor_position(ScrnInfoPtr scrn, int x, int y) /* * Load a two-color cursor into a crtc, performing rotation as needed */ -static void +static Bool xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src) { ScrnInfoPtr scrn = crtc-scrn; @@ -450,13 +450,13 @@ xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src) set_bit(cursor_image, cursor_info, x, y, TRUE); } } -crtc-funcs-load_cursor_image(crtc, cursor_image); +return crtc-funcs-load_cursor_image(crtc, cursor_image); } /* * Load a cursor image into all active CRTCs */ -static void +static Bool xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); @@ -466,12 +466,17 @@ xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled) { -if (crtc-funcs-load_cursor_image) -xf86_crtc_load_cursor_image(crtc, src); -else if (crtc-funcs-load_cursor_argb) -xf86_crtc_convert_cursor_to_argb(crtc, src); +if (crtc-funcs-load_cursor_image) { +if (!xf86_crtc_load_cursor_image(crtc, src)) +return FALSE; +} else if (crtc-funcs-load_cursor_argb) { +if (!xf86_crtc_convert_cursor_to_argb(crtc, src)) +return FALSE; +} else +return FALSE
[PATCH] Set a flag property on the root window to say if the X server VT is active
An X11 client may need to know whether the X server virtual terminal is currently the active one. This change adds a root window property which provides that information. Intended interface user: the VirtualBox Guest Additions. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- This is an updated version of my previous patch to address Daniel's concerns. Regards, Michael hw/xfree86/common/xf86Events.c | 28 hw/xfree86/common/xf86Init.c| 16 +++- hw/xfree86/common/xf86Privstr.h | 4 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c index 06af739..35a673d 100644 --- a/hw/xfree86/common/xf86Events.c +++ b/hw/xfree86/common/xf86Events.c @@ -56,6 +56,7 @@ #include X11/X.h #include X11/Xpoll.h #include X11/Xproto.h +#include X11/Xatom.h #include misc.h #include compiler.h #include xf86.h @@ -431,6 +432,29 @@ xf86EnableInputDeviceForVTSwitch(InputInfoPtr pInfo) pInfo-flags = ~XI86_DEVICE_DISABLED; } +/* + * xf86UpdateHasVTProperty -- + *Update a flag property on the root window to say whether the server VT + *is currently the active one as some clients need to know this. + */ +static void +xf86UpdateHasVTProperty(Bool hasVT) +{ +Atom property_name; +int32_t value = hasVT ? 1 : 0; +int i; + +property_name = MakeAtom(HAS_VT_ATOM_NAME, sizeof(HAS_VT_ATOM_NAME) - 1, + FALSE); +if (property_name == BAD_RESOURCE) +FatalError(Failed to retrieve \HAS_VT\ atom\n); +for (i = 0; i xf86NumScreens; i++) { +ChangeWindowProperty(xf86ScrnToScreen(xf86Screens[i])-root, + property_name, XA_INTEGER, 32, + PropModeReplace, 1, value, TRUE); +} +} + void xf86VTLeave(void) { @@ -490,6 +514,8 @@ xf86VTLeave(void) if (xorgHWAccess) xf86DisableIO(); +xf86UpdateHasVTProperty(FALSE); + return; switch_failed: @@ -574,6 +600,8 @@ xf86VTEnter(void) xf86platformVTProbe(); #endif +xf86UpdateHasVTProperty(TRUE); + OsReleaseSIGIO(); } diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c index 4579ff5..5a45004 100644 --- a/hw/xfree86/common/xf86Init.c +++ b/hw/xfree86/common/xf86Init.c @@ -387,6 +387,11 @@ InstallSignalHandlers(void) } } +/* The memory storing the initial value of the XFree86_has_VT root window + * property. This has to remain available until server start-up, so we just + * use a global. */ +static CARD32 HasVTValue = 1; + /* * InitOutput -- * Initialize screenInfo for all actually accessible framebuffers. @@ -731,7 +736,9 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv) if (xf86Info.vtno = 0) { #define VT_ATOM_NAME XFree86_VT Atom VTAtom = -1; +Atom HasVTAtom = -1; CARD32 *VT = NULL; +CARD32 *HasVT = HasVTValue; int ret; /* This memory needs to stay available until the screen has been @@ -744,6 +751,8 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv) *VT = xf86Info.vtno; VTAtom = MakeAtom(VT_ATOM_NAME, sizeof(VT_ATOM_NAME) - 1, TRUE); +HasVTAtom = MakeAtom(HAS_VT_ATOM_NAME, + sizeof(HAS_VT_ATOM_NAME) - 1, TRUE); for (i = 0, ret = Success; i xf86NumScreens ret == Success; i++) { @@ -751,9 +760,14 @@ InitOutput(ScreenInfo * pScreenInfo, int argc, char **argv) xf86RegisterRootWindowProperty(xf86Screens[i]-scrnIndex, VTAtom, XA_INTEGER, 32, 1, VT); +if (ret == Success) +ret = xf86RegisterRootWindowProperty(xf86Screens[i] + -scrnIndex, + HasVTAtom, XA_INTEGER, + 32, 1, HasVT); if (ret != Success) xf86DrvMsg(xf86Screens[i]-scrnIndex, X_WARNING, - Failed to register VT property\n); + Failed to register VT properties\n); } } diff --git a/hw/xfree86/common/xf86Privstr.h b/hw/xfree86/common/xf86Privstr.h index f7a9c9f..410ef17 100644 --- a/hw/xfree86/common/xf86Privstr.h +++ b/hw/xfree86/common/xf86Privstr.h @@ -163,4 +163,8 @@ typedef struct _RootWinProp { #define WSCONS 32 #endif +/* Root window property to tell clients whether our VT is currently active. + * Name chosen to match the XFree86_VT property. */ +#define HAS_VT_ATOM_NAME XFree86_has_VT + #endif /* _XF86PRIVSTR_H */ -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24
Re: Switching between a software and a hardware mouse cursor
On 18/03/14 11:17, Michael Thayer wrote: On 05/03/14 19:45, Adam Jackson wrote: On Thu, 2014-02-27 at 15:46 +0100, Michael Thayer wrote: Another would be to add a return value to the DDX CRTC functions load_cursor_argb, so that if the KMS driver failed to set the cursor, modesetting could pass this on to the X server. Actually we really should make load_cursor_argb return something other than void. Some particularly incapable server GPUs have format restrictions on their hardware cursors that mean _some_ ARGB cursors could be made to work but not all; and right now, the fallback code in -modesetting doesn't get this case right, and you end up with no cursor at all. I have done an initial patch to do this, see below. If I get positive feedback I will resend it formatted to be applied to the tree, but I expect a couple of iterations. I have been looking at what this would take, and what rather puts me off is that it would mean patching all supported video drivers without even the means to test them (the patches will be mostly be adding return TRUE in the right place of course, but you know how it is; how is this sort of thing normally handled?) This question is still relevant. [...] Even nicer would be a way in addition of telling the server that a particular hardware cursor is linked to a particular input device, and that it should use a software cursor if it wants the cursor to be somewhere other than the position reported by the device; I can't immediately think though where that would fit in cleanly, and neither can I immediately think of any other potential users of such an interface. This could be achieved by making SetCursorPosition() return a success value too. Would this be an acceptable idea? Regards, Michael --- diff --git a/hw/xfree86/common/xf86Module.h b/hw/xfree86/common/xf86Module.h index e8c24f2..62ac95d 100644 --- a/hw/xfree86/common/xf86Module.h +++ b/hw/xfree86/common/xf86Module.h @@ -80,7 +80,7 @@ typedef enum { * mask is 0x. */ #define ABI_ANSIC_VERSION SET_ABI_VERSION(0, 4) -#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(16, 0) +#define ABI_VIDEODRV_VERSION SET_ABI_VERSION(17, 0) #define ABI_XINPUT_VERSION SET_ABI_VERSION(21, 0) #define ABI_EXTENSION_VERSION SET_ABI_VERSION(8, 0) #define ABI_FONT_VERSION SET_ABI_VERSION(0, 6) diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h index c127d78..5407deb 100644 --- a/hw/xfree86/modes/xf86Crtc.h +++ b/hw/xfree86/modes/xf86Crtc.h @@ -186,13 +186,13 @@ typedef struct _xf86CrtcFuncs { /** * Load monochrome image */ -void +Bool (*load_cursor_image) (xf86CrtcPtr crtc, CARD8 *image); /** * Load ARGB image */ -void +Bool (*load_cursor_argb) (xf86CrtcPtr crtc, CARD32 *image); /** diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c index 2b0db34..10ef6f6 100644 --- a/hw/xfree86/modes/xf86Cursors.c +++ b/hw/xfree86/modes/xf86Cursors.c @@ -211,7 +211,7 @@ set_bit(CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool mask) /* * Load a two color cursor into a driver that supports only ARGB cursors */ -static void +static Bool xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src) { ScrnInfoPtr scrn = crtc-scrn; @@ -244,7 +244,7 @@ xf86_crtc_convert_cursor_to_argb(xf86CrtcPtr crtc, unsigned char *src) bits = 0; cursor_image[y * cursor_info-MaxWidth + x] = bits; } -crtc-funcs-load_cursor_argb(crtc, cursor_image); +return crtc-funcs-load_cursor_argb(crtc, cursor_image); } /* @@ -415,7 +415,7 @@ xf86_set_cursor_position(ScrnInfoPtr scrn, int x, int y) /* * Load a two-color cursor into a crtc, performing rotation as needed */ -static void +static Bool xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src) { ScrnInfoPtr scrn = crtc-scrn; @@ -450,13 +450,13 @@ xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src) set_bit(cursor_image, cursor_info, x, y, TRUE); } } -crtc-funcs-load_cursor_image(crtc, cursor_image); +return crtc-funcs-load_cursor_image(crtc, cursor_image); } /* * Load a cursor image into all active CRTCs */ -static void +static Bool xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src) { xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn); @@ -466,12 +466,17 @@ xf86_load_cursor_image(ScrnInfoPtr scrn, unsigned char *src) xf86CrtcPtr crtc = xf86_config-crtc[c]; if (crtc-enabled) { -if (crtc-funcs-load_cursor_image) -xf86_crtc_load_cursor_image(crtc, src); -else if (crtc-funcs-load_cursor_argb) -xf86_crtc_convert_cursor_to_argb(crtc, src); +if (crtc-funcs-load_cursor_image) { +if (!xf86_crtc_load_cursor_image(crtc, src)) +return FALSE; +} else if (crtc-funcs
[PATCH xf86-input-mouse] do not drop the result of protocol detection
In MousePickProtocol() with protocol PROT_AUTO we probe for the protocol to use but drop the result in most cases. This was causing DEVICE_INIT and DEVICE_ON to fail to be called with the VUID protocol. Git history suggests that this code was originally meant to cover both PS/2 auto-detection and OS- specific detection, but that only the first case was implemented at the time. Now that only the second is needed dropping the result to keep the protocol as PROT_AUTO is presumably no longer useful and seems to actively breaking things. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- src/mouse.c |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mouse.c b/src/mouse.c index 2da2b4d..ff8d799 100644 --- a/src/mouse.c +++ b/src/mouse.c @@ -843,11 +843,8 @@ MousePickProtocol(InputInfoPtr pInfo, const char* device, { const char *osProt; if (osInfo-SetupAuto (osProt = osInfo-SetupAuto(pInfo,NULL))) { -MouseProtocolID id = ProtocolNameToID(osProt); -if (id == PROT_UNKNOWN || id == PROT_UNSUP) { -protocolID = id; -protocol = osProt; -} +protocolID = ProtocolNameToID(osProt); +protocol = osProt; } } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Switching between a software and a hardware mouse cursor
On 05/03/14 19:45, Adam Jackson wrote: On Thu, 2014-02-27 at 15:46 +0100, Michael Thayer wrote: Another would be to add a return value to the DDX CRTC functions load_cursor_argb, so that if the KMS driver failed to set the cursor, modesetting could pass this on to the X server. Actually we really should make load_cursor_argb return something other than void. Some particularly incapable server GPUs have format restrictions on their hardware cursors that mean _some_ ARGB cursors could be made to work but not all; and right now, the fallback code in -modesetting doesn't get this case right, and you end up with no cursor at all. I have been looking at what this would take, and what rather puts me off is that it would mean patching all supported video drivers without even the means to test them (the patches will be mostly be adding return TRUE in the right place of course, but you know how it is; how is this sort of thing normally handled?) Thinking a bit more, what would be ideal for me would be a way of telling the X server to use or not to use the hardware cursor of a particular card. A RandR request might fit the picture here. Is this something which might be acceptable? Even nicer would be a way in addition of telling the server that a particular hardware cursor is linked to a particular input device, and that it should use a software cursor if it wants the cursor to be somewhere other than the position reported by the device; I can't immediately think though where that would fit in cleanly, and neither can I immediately think of any other potential users of such an interface. Of course, from my point of view, unlike using the load_cursor_argb API which just would just pass through to the KMS driver, these two would both be X.Org-only solutions to my problem which won't help whenever we want to start supporting Wayland. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Detecting whether the X server VT is the active one
On 05/03/14 19:50, Adam Jackson wrote: On Thu, 2014-02-27 at 16:01 +0100, Michael Thayer wrote: Another problem that I am running into is that the X11 client part of our Guest Additions needs to know whether or not the X server VT is currently the active one, for example so that it can disable our seamless windows functionality if the server is switched out. Currently we do this too with a hack in our DDX. This only needs to work with a standard X.Org server, so I thought about adding code to the server to add and update a root window property to pass on the information. Does that sound like an acceptable solution, and if not, does anyone have a better one? I think that sounds reasonable. There's already cases of people getting confused about things like XGetImage not working when VT switched away. If we're going to preserve that behaviour - which I do wish would eventually be optional - it's reasonable to give clients a way to know that it's happening in-band with the rest of X. (Consulting systemd or consolekit or whatever isn't generally useful, since if the client is on another machine there's no way it could talk to the appropriate daemon.) Sorry, it has taken me a while to get around to this (too many tasks competing for my attention!) A patch to do this is coming up. In order to reduce the area of code touched (and in particular so as not to have to register the property at initialisation time as per XFree86_VT) I did this as a property which is created when the VT is switched out and removed again when it is switched back in. Please tell me if this is acceptable or if it is too hack-ish. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH] Set a flag property on the root window when the server VT is not active as some clients need to know this.
Signed-off-by: Michael Thayer michael.tha...@oracle.com --- hw/xfree86/common/xf86Events.c | 36 1 file changed, 36 insertions(+) diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c index 06af739..a1b43bb 100644 --- a/hw/xfree86/common/xf86Events.c +++ b/hw/xfree86/common/xf86Events.c @@ -56,6 +56,7 @@ #include X11/X.h #include X11/Xpoll.h #include X11/Xproto.h +#include X11/Xatom.h #include misc.h #include compiler.h #include xf86.h @@ -103,6 +104,9 @@ Bool VTSwitchEnabled = TRUE;/* Allows run-time disabling for extern fd_set EnabledDevices; +/* Name chosen to match the XFree86_VT atom. */ +#define NO_VT_ATOM_NAME XFree86_NO_VT + #ifdef XF86PM extern void (*xf86OSPMClose) (void); #endif @@ -431,6 +435,34 @@ xf86EnableInputDeviceForVTSwitch(InputInfoPtr pInfo) pInfo-flags = ~XI86_DEVICE_DISABLED; } +/* + * xf86CreateRemoveNoVTProperty -- + *Set a flag property on the root window when the server VT is not active + *as some clients need to know this. + */ +static void +xf86CreateRemoveNoVTProperty(Bool create) +{ +Atom property_name; +int32_t value = 1; +int i; + +property_name = MakeAtom(NO_VT_ATOM_NAME, sizeof(NO_VT_ATOM_NAME) - 1, + TRUE); +if (property_name == BAD_RESOURCE) +FatalError(Failed to create or retrieve \NO_VT\ atom\n); +for (i = 0; i xf86NumScreens; i++) { +if (create) +ChangeWindowProperty(xf86ScrnToScreen(xf86Screens[i])-root, + property_name, XA_INTEGER, 32, + PropModeReplace, 1, value, TRUE); +else +DeleteProperty(serverClient, + xf86ScrnToScreen(xf86Screens[i])-root, + property_name); +} +} + void xf86VTLeave(void) { @@ -490,6 +522,8 @@ xf86VTLeave(void) if (xorgHWAccess) xf86DisableIO(); +xf86CreateRemoveNoVTProperty(TRUE); + return; switch_failed: @@ -574,6 +608,8 @@ xf86VTEnter(void) xf86platformVTProbe(); #endif +xf86CreateRemoveNoVTProperty(FALSE); + OsReleaseSIGIO(); } -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Switching between a software and a hardware mouse cursor
Hello, For background I am the member of the VirtualBox team responsible for (among other things) the guest graphics drivers for X.Org. On Linux we are in the process of moving from a DDX driver to a KMS driver which will work with the modesetting DDX, and one of the problems I have run into is how to support a feature that we offer, namely switching the guest to a software mouse cursor and back. The main usage case for this is switching to reporting relative events, e.g. for mouse controlled games, when using the host cursor - who's position the guest does not control - as a hardware cursor is not appropriate. In our DDX we do a check every time a new cursor is loaded as to whether we are currently in software or hardware cursor mode, and succeed or fail as appropriate. We have an X11 client tool which briefly grabs the pointer and flashes it to an hourglass when it detects that a switch has been requested. This works because we are still using the pre-RandR 1.2 DDX cursor APIs. I suspect though that a patch to make modesetting use those APIs would not be entirely welcome, so I was hoping for other suggestions about how I could handle this. One idea would be to add an output property to modesetting so that our client could tell it that we want to make the switch. Another would be to add a return value to the DDX CRTC functions load_cursor_argb, so that if the KMS driver failed to set the cursor, modesetting could pass this on to the X server. Neither of these feel quite satisfactory of course; any better suggestions are welcome. Thanks in advance. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Detecting whether the X server VT is the active one
Hello, Another problem that I am running into is that the X11 client part of our Guest Additions needs to know whether or not the X server VT is currently the active one, for example so that it can disable our seamless windows functionality if the server is switched out. Currently we do this too with a hack in our DDX. This only needs to work with a standard X.Org server, so I thought about adding code to the server to add and update a root window property to pass on the information. Does that sound like an acceptable solution, and if not, does anyone have a better one? Thanks. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Geschäftsführer: Jürgen Kunz Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Astrid Kepper, Val Maher ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/4] xserver: limit the kernel subsystems we look for devices in
On Wed, 2011-08-03 at 09:00 +1000, Peter Hutterer wrote: On Mon, Aug 01, 2011 at 11:18:48PM +0200, Michael Thayer wrote: On Thu, 2011-07-21 at 09:40 +1000, Peter Hutterer wrote: On Wed, Jul 20, 2011 at 05:52:16AM -0700, Dan Nicholson wrote: On Mon, Jul 18, 2011 at 12:17 PM, Lennart Poettering lenn...@poettering.net wrote: Don't enumerate/monitor all devices of the system (since that can be quite a few), but limit our search to devices from the input subsystem, as well as the tty subsystem (to cover Wacom tablets). This should make X start up a bit faster and reduce the number of unnecessary wake-ups of the X server. --- config/udev.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/config/udev.c b/config/udev.c index 5ac52a1..0763cc9 100644 --- a/config/udev.c +++ b/config/udev.c @@ -281,6 +281,9 @@ config_udev_init(void) if (!udev_monitor) return 0; +udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, input, NULL); +udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, tty, NULL); /* For Wacom serial devices */ + if (udev_monitor_enable_receiving(udev_monitor)) { ErrorF(config/udev: failed to bind the udev monitor\n); return 0; @@ -289,6 +292,10 @@ config_udev_init(void) enumerate = udev_enumerate_new(udev); if (!enumerate) return 0; + +udev_enumerate_add_match_subsystem(enumerate, input); +udev_enumerate_add_match_subsystem(enumerate, tty); + udev_enumerate_scan_devices(enumerate); devices = udev_enumerate_get_list_entry(enumerate); udev_list_entry_foreach(device, devices) { Last time this came up, we were a little uneasy about limiting the subsystems. I guess this should work for devices we care about, and any future input devices should fall under the input subsystem (I hope). One thing we could use help on in upstream udev is marking the appropriate serial devices with ID_INPUT* in 60-persistent-input.rules. I'm cc'ing Thomas since he was the one who originally requested that we not just filter to input. At this point, all input devices that I've seen* are either supported by the kernel or are legacy serial devices that don't have a kernel driver (yet). I think we'll be fine filtering for those two only. Sorry for the late response - I was rather behind on reading this list. I would greatly appreciate it if you could add the misc subsystem for the sake of the VirtualBox mouse integration. Not that I know anything about the details, but can we make the VirtualBox mouse integration appear in the input subsystem? that seems to be the better fix here. Not (as far as I can see) without writing a proper kernel input driver - which is something I do intend to do as soon as possible, but I hadn't actually planned to do it immediately. Of course if you say that enumerating misc is not acceptable to you then I will probably have to give it a priority boost. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 2/4] xserver: limit the kernel subsystems we look for devices in
On Thu, 2011-07-21 at 09:40 +1000, Peter Hutterer wrote: On Wed, Jul 20, 2011 at 05:52:16AM -0700, Dan Nicholson wrote: On Mon, Jul 18, 2011 at 12:17 PM, Lennart Poettering lenn...@poettering.net wrote: Don't enumerate/monitor all devices of the system (since that can be quite a few), but limit our search to devices from the input subsystem, as well as the tty subsystem (to cover Wacom tablets). This should make X start up a bit faster and reduce the number of unnecessary wake-ups of the X server. --- config/udev.c |7 +++ 1 files changed, 7 insertions(+), 0 deletions(-) diff --git a/config/udev.c b/config/udev.c index 5ac52a1..0763cc9 100644 --- a/config/udev.c +++ b/config/udev.c @@ -281,6 +281,9 @@ config_udev_init(void) if (!udev_monitor) return 0; +udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, input, NULL); +udev_monitor_filter_add_match_subsystem_devtype(udev_monitor, tty, NULL); /* For Wacom serial devices */ + if (udev_monitor_enable_receiving(udev_monitor)) { ErrorF(config/udev: failed to bind the udev monitor\n); return 0; @@ -289,6 +292,10 @@ config_udev_init(void) enumerate = udev_enumerate_new(udev); if (!enumerate) return 0; + +udev_enumerate_add_match_subsystem(enumerate, input); +udev_enumerate_add_match_subsystem(enumerate, tty); + udev_enumerate_scan_devices(enumerate); devices = udev_enumerate_get_list_entry(enumerate); udev_list_entry_foreach(device, devices) { Last time this came up, we were a little uneasy about limiting the subsystems. I guess this should work for devices we care about, and any future input devices should fall under the input subsystem (I hope). One thing we could use help on in upstream udev is marking the appropriate serial devices with ID_INPUT* in 60-persistent-input.rules. I'm cc'ing Thomas since he was the one who originally requested that we not just filter to input. At this point, all input devices that I've seen* are either supported by the kernel or are legacy serial devices that don't have a kernel driver (yet). I think we'll be fine filtering for those two only. Sorry for the late response - I was rather behind on reading this list. I would greatly appreciate it if you could add the misc subsystem for the sake of the VirtualBox mouse integration. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineering 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: Resend: [PATCH xev] Show RandR events.
On Thu, 2011-03-03 at 11:37 -0800, Aaron Plattner wrote: On Wed, Feb 16, 2011 at 04:00:25AM -0800, Michael Thayer wrote: +if (rr_major = 1 rr_minor = 2) This check seems wrong, since it will include these event masks for RandR version 2.2 but not 2.1 if we ever have a RandR 2. Fixed below. Signed-off-by: Michael Thayer michael.tha...@oracle.com --- I think the subject line says it all. I was wanting to see what RandR events clients were getting and saw that this was on the X.Org todo list. configure.ac |2 +- xev.c| 215 ++ 2 files changed, 216 insertions(+), 1 deletions(-) diff --git a/configure.ac b/configure.ac index bc31f6c..1945711 100644 --- a/configure.ac +++ b/configure.ac @@ -36,7 +36,7 @@ XORG_MACROS_VERSION(1.8) XORG_DEFAULT_OPTIONS # Checks for pkg-config packages -PKG_CHECK_MODULES(XEV, x11) +PKG_CHECK_MODULES(XEV, xrandr = 1.2 xrender x11) AC_CONFIG_FILES([ Makefile diff --git a/xev.c b/xev.c index c053171..23abe74 100644 --- a/xev.c +++ b/xev.c @@ -40,6 +40,7 @@ from the X Consortium. #include X11/Xlib.h #include X11/Xutil.h #include X11/Xproto.h +#include X11/extensions/Xrandr.h #define INNER_WINDOW_WIDTH 50 #define INNER_WINDOW_HEIGHT 50 @@ -71,6 +72,9 @@ XIC xic = NULL; Atom wm_delete_window; Atom wm_protocols; +Bool have_rr; +int rr_event_base, rr_error_base; + static void prologue (XEvent *eventp, char *event_name) { @@ -620,6 +624,191 @@ do_MappingNotify (XEvent *eventp) XRefreshKeyboardMapping(e); } +static void +print_SubPixelOrder (SubpixelOrder subpixel_order) +{ +switch (subpixel_order) { + case SubPixelUnknown:printf (SubPixelUnknown); return; + case SubPixelHorizontalRGB: printf (SubPixelHorizontalRGB); return; + case SubPixelHorizontalBGR: printf (SubPixelHorizontalBGR); return; + case SubPixelVerticalRGB:printf (SubPixelVerticalRGB); return; + case SubPixelVerticalBGR:printf (SubPixelVerticalBGR); return; + case SubPixelNone: printf (SubPixelNone); return; + default: printf (%d, subpixel_order); +} +} + +static void +print_Rotation (Rotation rotation) +{ +if (rotation RR_Rotate_0) +printf (RR_Rotate_0); +else if (rotation RR_Rotate_90) +printf (RR_Rotate_90); +else if (rotation RR_Rotate_180) +printf (RR_Rotate_180); +else if (rotation RR_Rotate_270) +printf (RR_Rotate_270); +else { +printf (%d, rotation); +return; +} +if (rotation RR_Reflect_X) +printf (, RR_Reflect_X); +if (rotation RR_Reflect_Y) +printf (, RR_Reflect_Y); +} + +static void +print_Connection (Connection connection) +{ +switch (connection) { + case RR_Connected: printf (RR_Connected); return; + case RR_Disconnected: printf (RR_Disconnected); return; + case RR_UnknownConnection: printf (RR_UnknownConnection); return; + default:printf (%d, connection); +} +} + +static void +do_RRScreenChangeNotify (XEvent *eventp) +{ +XRRScreenChangeNotifyEvent *e = (XRRScreenChangeNotifyEvent *) eventp; + +XRRUpdateConfiguration (eventp); +printf (root 0x%lx, timestamp %lu, config_timestamp %lu\n, +e-root, e-timestamp, e-config_timestamp); +printf (size_index %hu, e-size_index); +printf (, subpixel_order ); +print_SubPixelOrder (e-subpixel_order); +printf (\nrotation ); +print_Rotation (e-rotation); +printf(\nwidth %d, height %d, mwidth %d, mheight %d\n, + e-width, e-height, e-mwidth, e-mheight); +} + +static void +do_RRNotify_OutputChange (XEvent *eventp, XRRScreenResources *screen_resources) +{ +XRROutputChangeNotifyEvent *e = (XRROutputChangeNotifyEvent *) eventp; +XRROutputInfo *output_info = NULL; +XRRModeInfo *mode_info = NULL; + +if (screen_resources) { +int i; + +output_info = XRRGetOutputInfo (dpy, screen_resources, e-output); +for (i = 0; i screen_resources-nmode; i++) +if (screen_resources-modes[i].id == e-mode) { +mode_info = screen_resources-modes[i]; break; +} +} +printf (subtype XRROutputChangeNotifyEvent\n); +if (output_info) +printf (output %s, , output_info-name); +else +printf (output %lu, , e-output); +if (e-crtc) +printf(crtc %lu, , e-crtc); +else +printf(crtc None, ); +if (mode_info) +printf (mode %s (%dx%d)\n, mode_info-name, mode_info-width, +mode_info-height); +else if (e-mode) +printf (mode %lu\n, e-mode); +else +printf(mode None\n); +printf (rotation ); +print_Rotation (e-rotation); +printf (\nconnection ); +print_Connection (e-connection); +printf (, subpixel_order ); +print_SubPixelOrder (e-subpixel_order
Re: Resend: [PATCH xev] Show RandR events.
On Thu, 2011-03-03 at 13:54 -0800, Aaron Plattner wrote: The updated change looks good to me. Reviewed-by: Aaron Plattner aplatt...@nvidia.com Tested-by: Aaron Plattner aplatt...@nvidia.com Do you have a freedesktop.org account, or would you like me to submit this for you? Thanks for the review. I don't have an fd.o account, so it would be great if you could submit it. Regards, Michael -- ORACLE Deutschland B.V. Co. KG Michael Thayer Werkstrasse 24 VirtualBox engineer 71384 Weinstadt, Germany mailto:michael.tha...@oracle.com Hauptverwaltung: Riesstr. 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Rijnzathe 6, 3454PV De Meern, Niederlande Handelsregister der Handelskammer Midden-Niederlande, Nr. 30143697 Geschäftsführer: Jürgen Kunz, Marcel van de Molen, Alexander van der Ven ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
[PATCH xev] Show RandR events.
Signed-off-by: Michael Thayer michael.tha...@oracle.com --- I think the subject line says it all. I was wanting to see what RandR events clients were getting and saw that this was on the X.Org todo list. configure.ac |2 +- xev.c| 214 ++ 2 files changed, 215 insertions(+), 1 deletions(-) diff --git a/configure.ac b/configure.ac index bc31f6c..1945711 100644 --- a/configure.ac +++ b/configure.ac @@ -36,7 +36,7 @@ XORG_MACROS_VERSION(1.8) XORG_DEFAULT_OPTIONS # Checks for pkg-config packages -PKG_CHECK_MODULES(XEV, x11) +PKG_CHECK_MODULES(XEV, xrandr = 1.2 xrender x11) AC_CONFIG_FILES([ Makefile diff --git a/xev.c b/xev.c index c053171..667dc63 100644 --- a/xev.c +++ b/xev.c @@ -40,6 +40,7 @@ from the X Consortium. #include X11/Xlib.h #include X11/Xutil.h #include X11/Xproto.h +#include X11/extensions/Xrandr.h #define INNER_WINDOW_WIDTH 50 #define INNER_WINDOW_HEIGHT 50 @@ -71,6 +72,9 @@ XIC xic = NULL; Atom wm_delete_window; Atom wm_protocols; +Bool have_rr; +int rr_event_base, rr_error_base; + static void prologue (XEvent *eventp, char *event_name) { @@ -620,6 +624,191 @@ do_MappingNotify (XEvent *eventp) XRefreshKeyboardMapping(e); } +static void +print_SubPixelOrder (SubpixelOrder subpixel_order) +{ +switch (subpixel_order) { + case SubPixelUnknown:printf (SubPixelUnknown); return; + case SubPixelHorizontalRGB: printf (SubPixelHorizontalRGB); return; + case SubPixelHorizontalBGR: printf (SubPixelHorizontalBGR); return; + case SubPixelVerticalRGB:printf (SubPixelVerticalRGB); return; + case SubPixelVerticalBGR:printf (SubPixelVerticalBGR); return; + case SubPixelNone: printf (SubPixelNone); return; + default: printf (%d, subpixel_order); +} +} + +static void +print_Rotation (Rotation rotation) +{ +if (rotation RR_Rotate_0) +printf (RR_Rotate_0); +else if (rotation RR_Rotate_90) +printf (RR_Rotate_90); +else if (rotation RR_Rotate_180) +printf (RR_Rotate_180); +else if (rotation RR_Rotate_270) +printf (RR_Rotate_270); +else { +printf (%d, rotation); +return; +} +if (rotation RR_Reflect_X) +printf (, RR_Reflect_X); +if (rotation RR_Reflect_Y) +printf (, RR_Reflect_Y); +} + +static void +print_Connection (Connection connection) +{ +switch (connection) { + case RR_Connected: printf (RR_Connected); return; + case RR_Disconnected: printf (RR_Disconnected); return; + case RR_UnknownConnection: printf (RR_UnknownConnection); return; + default:printf (%d, connection); +} +} + +static void +do_RRScreenChangeNotify (XEvent *eventp) +{ +XRRScreenChangeNotifyEvent *e = (XRRScreenChangeNotifyEvent *) eventp; + +XRRUpdateConfiguration (eventp); +printf (root 0x%lx, timestamp %lu, config_timestamp %lu\n, +e-root, e-timestamp, e-config_timestamp); +printf (size_index %hu, e-size_index); +printf (, subpixel_order ); +print_SubPixelOrder (e-subpixel_order); +printf (\nrotation ); +print_Rotation (e-rotation); +printf(\nwidth %d, height %d, mwidth %d, mheight %d\n, + e-width, e-height, e-mwidth, e-mheight); +} + +static void +do_RRNotify_OutputChange (XEvent *eventp, XRRScreenResources *screen_resources) +{ +XRROutputChangeNotifyEvent *e = (XRROutputChangeNotifyEvent *) eventp; +XRROutputInfo *output_info = NULL; +XRRModeInfo *mode_info = NULL; + +if (screen_resources) { +int i; + +output_info = XRRGetOutputInfo (dpy, screen_resources, e-output); +for (i = 0; i screen_resources-nmode; i++) +if (screen_resources-modes[i].id == e-mode) { +mode_info = screen_resources-modes[i]; break; +} +} +printf (subtype XRROutputChangeNotifyEvent\n); +if (output_info) +printf (output %s, , output_info-name); +else +printf (output %lu, , e-output); +if (e-crtc) +printf(crtc %lu, , e-crtc); +else +printf(crtc None, ); +if (mode_info) +printf (mode %s (%dx%d)\n, mode_info-name, mode_info-width, +mode_info-height); +else if (e-mode) +printf (mode %lu\n, e-mode); +else +printf(mode None\n); +printf (rotation ); +print_Rotation (e-rotation); +printf (\nconnection ); +print_Connection (e-connection); +printf (, subpixel_order ); +print_SubPixelOrder (e-subpixel_order); +printf (\n); +XRRFreeOutputInfo (output_info); +} + +static void +do_RRNotify_CrtcChange (XEvent *eventp, XRRScreenResources *screen_resources) +{ +XRRCrtcChangeNotifyEvent *e = (XRRCrtcChangeNotifyEvent *) eventp; +XRRModeInfo *mode_info = NULL; + +if (screen_resources) { +int i
Re: Confusing or confused code
Le lundi 15 mars 2010 à 14:52 -0400, Adam Jackson a écrit : On Thu, 2010-03-11 at 12:59 +0100, Michael Thayer wrote: http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Mode.c?id=326429badfc76885e4652ddc72860810c0e8d102#n1300 until line 1313 is supposed to be freeing the old list at scrp-clockRanges, duplicating the user supplied list and storing it there. If that is right, then it looks to me though like it will not work unless scrp-clockRanges is initially NULL [snip] I think your analysis is right, that code's garbage. At least in the case of virtual drivers like vboxvideo, I think we should reasonably allow you to just pass in NULL for clockRanges. But it's trivial to set up just one range that's sufficient to cover everything. Gross, but sufficient. Certainly, and it's never caused me any pain - I just wanted to point it out in case it caught someone else at some point. And sorry for not trying a fix or anything, but I don't actually build the X server very often, just our own drivers. Regards, Michael -- Sun Microsystems GmbHMichael Thayer Werkstrasse 24 VirtualBox engineer 71384 Weinstadt, Germany mailto:michael.tha...@sun.com Sitz der Gesellschaft: Sun Microsystems GmbH, Sonnenallee 1, 85551 Kirchheim-Heimstetten Amtsgericht Muenchen: HRB 161028 Geschaeftsfuehrer: Thomas Schroeder ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Confusing or confused code
Hello, I suspect that this code: http://cgit.freedesktop.org/xorg/xserver/tree/hw/xfree86/common/xf86Mode.c?id=326429badfc76885e4652ddc72860810c0e8d102#n1300 until line 1313 is supposed to be freeing the old list at scrp-clockRanges, duplicating the user supplied list and storing it there. If that is right, then it looks to me though like it will not work unless scrp-clockRanges is initially NULL and the user only supplies a list with a single element. Did I miss something important there? Regards, Michael -- Sun Microsystems GmbHMichael Thayer Werkstrasse 24 VirtualBox engineer 71384 Weinstadt, Germany mailto:michael.tha...@sun.com Sitz der Gesellschaft: Sun Microsystems GmbH, Sonnenallee 1, 85551 Kirchheim-Heimstetten Amtsgericht Muenchen: HRB 161028 Geschaeftsfuehrer: Thomas Schroeder, Wolfgang Engels Vorsitzender des Aufsichtsrates: Martin Haering ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
Re: [ANNOUNCE] xorg-server 1.7.99.901
Le lundi 15 février 2010 à 10:27 +1000, Peter Hutterer a écrit : On Sun, Feb 14, 2010 at 10:34:52AM -0800, Dan Nicholson wrote: On Sun, Feb 14, 2010 at 10:30 AM, Julien Cristau jcris...@debian.org wrote: On Sun, Feb 14, 2010 at 13:05:07 -0500, Thomas Jaeger wrote: In order to be able to support hot-plugging for serial wacom tablet pcs, we'll need one of the attached patches -- I don't particularly care which one. See the discussion at http://lists.x.org/archives/xorg-devel/2010-January/004596.html I agree, I think ID_INPUT should be enough. The filter for subsystem was present in the first patches before the ID_INPUT check was added so I'd consider it a leftover deprecated now anyway. This would be great for the current VirtualBox mouse driver, as it gets its events from a device on the misc subsystem. I was already mentally preparing for some major surgery... Regards, Michael -- Sun Microsystems GmbHMichael Thayer Werkstrasse 24 VirtualBox engineer 71384 Weinstadt, Germany mailto:michael.tha...@sun.com Sitz der Gesellschaft: Sun Microsystems GmbH, Sonnenallee 1, 85551 Kirchheim-Heimstetten Amtsgericht Muenchen: HRB 161028 Geschaeftsfuehrer: Thomas Schroeder, Wolfgang Engels, Wolf Frenkel Vorsitzender des Aufsichtsrates: Martin Haering ___ xorg-devel mailing list xorg-devel@lists.x.org http://lists.x.org/mailman/listinfo/xorg-devel