On Monday, February 16, 2015 05:00:54 PM Takashi Iwai wrote: > The modesetting driver still has an everlasting bug of invisible > cursor on cirrus and other KMS drivers where no hardware cursor is > supported. This patch is a part of an attempt to address it. > > This patch particularly converts the current load_cursor_argb callback > of modesetting driver to load_cursor_argb_check so that it can return > whether the driver handles the hw cursor or falls back to the sw > cursor. > > Signed-off-by: Takashi Iwai <[email protected]> > --- > hw/xfree86/drivers/modesetting/drmmode_display.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c > b/hw/xfree86/drivers/modesetting/drmmode_display.c > index 25d16a233103..25e4d367de46 100644 > --- a/hw/xfree86/drivers/modesetting/drmmode_display.c > +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c > @@ -409,7 +409,7 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int x, int > y) > drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, x, y); > } >
Could we add a comment indicating what the boolean return value means? Something like: /** * The load_cursor_argb_check driver hook. * * Sets the hardware cursor by calling the drmModeSetCursor2 ioctl. * On failure, returns FALSE indicating that the X server should fall * back to software cursors. */ I had to chase through several layers of code to figure out whether this boolean value matched the expectations of the driver hook it was implementating. I found no comments at any point, but eventually found the answer via git blame and reading the commit message of 901fbfbbbd. And it does indeed match! Hopefully adding a comment will save people time in the future. With that added, patches 2-3 are: Reviewed-by: Kenneth Graunke <[email protected]> I don't feel qualified to review patch 4. > -static void > +static Bool > drmmode_set_cursor(xf86CrtcPtr crtc) > { > drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; > @@ -430,7 +430,7 @@ drmmode_set_cursor(xf86CrtcPtr crtc) > if (ret) > use_set_cursor2 = FALSE; > else > - return; > + return TRUE; > } > > ret = drmModeSetCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id, > handle, > @@ -443,11 +443,15 @@ 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 > -drmmode_load_cursor_argb(xf86CrtcPtr crtc, CARD32 *image) > +static void drmmode_hide_cursor(xf86CrtcPtr crtc); > + > +static Bool > +drmmode_load_cursor_argb_check(xf86CrtcPtr crtc, CARD32 *image) > { > modesettingPtr ms = modesettingPTR(crtc->scrn); > drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private; > @@ -461,7 +465,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 > @@ -665,7 +670,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_check, > > .gamma_set = drmmode_crtc_gamma_set, > .destroy = NULL, /* XXX */ >
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
