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 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;
--
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
