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--- 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--- 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
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).
Hi, On 15-09-16 20:53, Michael Thayer wrote: Hello, On 15.09.2016 19:18, Hans de Goede wrote: Hi, * 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. /me goes and looks at the v3 patch again. Ah I see now, you do not just revert: https://cgit.freedesktop.org/xorg/xserver/commit/hw/xfree86/drivers/modesetting?id=af916477c65a083ec496ac3f088d766b410e8b6e You fix the issue it was fixing be replacing the: if (cursor_up) return drmmode_set_cursor() else return TRUE With an unconditional: return drmmode_set_cursor() Now the downside of that is that this will cause a cursor to show even if show_cursor was never called / the current cursor state is hidden. Reading between the lines you're claiming that this never happens. If that indeed never happens then your fix indeed is a good idea. Notice that I read over the replacing of "return TRUE" with return drmmode_set_cursor() the first time, this is my mistake, but this is also caused by you doing too much in a single commit. If you believe that the entire first_time dance is not necessary and that instead drmmode_load_cursor_argb_check() should always call drmmode_set_cursor(), not only loading the cursor but also showing it, then please submit a separate patch doing that, and only that. So assuming I've understood your intentions correctly, here is how I would like to see things move forward with this patch, turn it into a 3 patch patch-set: Patch 1: Re-introduce the -EINVAL check for trying the non "2" version of the drmcursor ioctl, with the reasoning you send me off-list in the commit msg Patch 2: Replace the first_time dance in drmmode_load_cursor_argb_check() with an unconditional call to drmmode_set_cursor(). Note I'm not yet convinced this is the right thing todo, I assume you've done some analysis of all the call paths to load_cursor_argb_check() which have made you come to the conclusion that this is the right thing todo, include that analysis in your commit msg please. Patch 3: Drop set_cursor2_failed caching and setting of cursor_info->MaxWidth = cursor_info->MaxHeight = 0; / drmmode_crtc->drmmode->sw_cursor = TRUE; from drmmode_set_cursor() so that a later drmmode_load_cursor_argb_check() call can upgrade the cursor from a sw cursor to a hw cursor. Thanks & Regards, Hans ___ 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 hardware
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).
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. * 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. 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 hardware which can display some cursors but not others and which would also need a change on these lines. So far we've not yet had any problems with such hardware. Either way with things being the way the currently are I believe that your reverting of the patch in question is wrong. It may be possible by also allowing show_cursor to return an error and deal with that correctly in the core. I believe we even had a patch for this a while back, but it was dropped because the current changes were deemed sufficient for all current real world scenarios. I guess no-one considered the virtual box corner case at that time.
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--- 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,
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors (v3).
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 ? * 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. 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... Regards, Hans Signed-off-by: Michael Thayer--- 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
[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--- 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--- 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 Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors.
Hi, On 14-09-16 15:53, Michael Thayer wrote: 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--- 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. I advise you to (directly) mail the author of that commit perhaps he can explain things. > 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. IIRC then during the review it was brought up that there might be other errors to, e.g. I've seen checks for -ENOSYS for other ioctls in some Xorg code. So I think it is probably safest to just always try the fallback. Regards, Hans 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]; ___ xorg-devel@lists.x.org: X.Org development Archives:
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--- 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, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
Re: [PATCH xserver] modesetting: allow switching from software to hardware cursors.
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--- 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. 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]; ___ 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.
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--- 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