Does anyone else want to review this, or are Robert's (@nvidia.com) Reviewed-by and Cyril's Tested-by sufficient?
On Tue, Nov 16, 2010 at 12:17:33PM -0800, Aaron Plattner wrote: > When RandR 1.2's transformation code is enabled, it rotates the cursor > image so that it appears upright on a rotated screen. This code > completely mangles 2-color cursors on hardware where the the mask and > source images are not interleaved due to two problems: > > 1. stride is calculated as (width / 4) rather than (width / 8), so the > expression (y * stride) skips two lines instead of one for every > time y is incremented. > 2. cursor_bitpos ignores the 'mask' parameter if the hardware doesn't > specify any of the HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_* flags. > > To fix this, refactor the code to pass the whole xf86CursorInfoPtr > through to cursor_bitpos and compute the correct stride there based on > the flags. If none of the SOURCE_MASK_INTERLEAVE flags are set, use > the total cursor size to move the 'image' variable into the mask part > of the image before computing the desired byte pointer. > > Signed-off-by: Aaron Plattner <[email protected]> > Reviewed-by: Robert Morell <[email protected]> > --- > Could somebody with hardware that sets SOURCE_MASK_INTERLEAVE (e.g. > Intel) please give this patch a try and verify that it doesn't break > rotated cursors? > > hw/xfree86/modes/xf86Cursors.c | 62 +++++++++++++++++++++++++++------------ > 1 files changed, 43 insertions(+), 19 deletions(-) > > diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c > index ab07b60..0667447 100644 > --- a/hw/xfree86/modes/xf86Cursors.c > +++ b/hw/xfree86/modes/xf86Cursors.c > @@ -1,5 +1,6 @@ > /* > * Copyright © 2007 Keith Packard > + * Copyright © 2010 Aaron Plattner > * > * Permission to use, copy, modify, distribute, and sell this software and > its > * documentation for any purpose is hereby granted without fee, provided that > @@ -126,12 +127,33 @@ xf86_crtc_rotate_coord_back (Rotation rotation, > *y_src = y_dst; > } > > +struct cursor_bit { > + CARD8 *byte; > + char bitpos; > +}; > + > /* > * Convert an x coordinate to a position within the cursor bitmap > */ > -static int > -cursor_bitpos (int flags, int x, Bool mask) > +static struct cursor_bit > +cursor_bitpos (CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, > + Bool mask) > { > + const int flags = cursor_info->Flags; > + const Bool interleaved = > + !!(flags & (HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_1 | > + HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_8 | > + HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_16 | > + HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_32 | > + HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_64)); > + const int width = cursor_info->MaxWidth; > + const int height = cursor_info->MaxHeight; > + const int stride = interleaved ? width / 4 : width / 8; > + > + struct cursor_bit ret; > + > + image += y * stride; > + > if (flags & HARDWARE_CURSOR_SWAP_SOURCE_AND_MASK) > mask = !mask; > if (flags & HARDWARE_CURSOR_NIBBLE_SWAPPED) > @@ -149,29 +171,33 @@ cursor_bitpos (int flags, int x, Bool mask) > x = ((x & ~31) << 1) | (mask << 5) | (x & 31); > else if (flags & HARDWARE_CURSOR_SOURCE_MASK_INTERLEAVE_64) > x = ((x & ~63) << 1) | (mask << 6) | (x & 63); > - return x; > + else if (mask) > + image += stride * height; > + > + ret.byte = image + (x / 8); > + ret.bitpos = x & 7; > + > + return ret; > } > > /* > * Fetch one bit from a cursor bitmap > */ > static CARD8 > -get_bit (CARD8 *image, int stride, int flags, int x, int y, Bool mask) > +get_bit (CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool > mask) > { > - x = cursor_bitpos (flags, x, mask); > - image += y * stride; > - return (image[(x >> 3)] >> (x & 7)) & 1; > + struct cursor_bit bit = cursor_bitpos(image, cursor_info, x, y, mask); > + return (*bit.byte >> bit.bitpos) & 1; > } > > /* > * Set one bit in a cursor bitmap > */ > static void > -set_bit (CARD8 *image, int stride, int flags, int x, int y, Bool mask) > +set_bit (CARD8 *image, xf86CursorInfoPtr cursor_info, int x, int y, Bool > mask) > { > - x = cursor_bitpos (flags, x, mask); > - image += y * stride; > - image[(x >> 3)] |= 1 << (x & 7); > + struct cursor_bit bit = cursor_bitpos(image, cursor_info, x, y, mask); > + *bit.byte |= 1 << bit.bitpos; > } > > /* > @@ -186,7 +212,6 @@ xf86_crtc_convert_cursor_to_argb (xf86CrtcPtr crtc, > unsigned char *src) > CARD32 *cursor_image = (CARD32 *) xf86_config->cursor_image; > int x, y; > int xin, yin; > - int stride = cursor_info->MaxWidth >> 2; > int flags = cursor_info->Flags; > CARD32 bits; > > @@ -201,10 +226,10 @@ xf86_crtc_convert_cursor_to_argb (xf86CrtcPtr crtc, > unsigned char *src) > cursor_info->MaxWidth, > cursor_info->MaxHeight, > x, y, &xin, &yin); > - if (get_bit (src, stride, flags, xin, yin, TRUE) == > + if (get_bit (src, cursor_info, xin, yin, TRUE) == > ((flags & HARDWARE_CURSOR_INVERT_MASK) == 0)) > { > - if (get_bit (src, stride, flags, xin, yin, FALSE)) > + if (get_bit (src, cursor_info, xin, yin, FALSE)) > bits = xf86_config->cursor_fg; > else > bits = xf86_config->cursor_bg; > @@ -407,7 +432,6 @@ xf86_crtc_load_cursor_image (xf86CrtcPtr crtc, CARD8 *src) > int x, y; > int xin, yin; > int stride = cursor_info->MaxWidth >> 2; > - int flags = cursor_info->Flags; > > cursor_image = xf86_config->cursor_image; > memset(cursor_image, 0, cursor_info->MaxHeight * stride); > @@ -419,10 +443,10 @@ xf86_crtc_load_cursor_image (xf86CrtcPtr crtc, CARD8 > *src) > cursor_info->MaxWidth, > cursor_info->MaxHeight, > x, y, &xin, &yin); > - if (get_bit(src, stride, flags, xin, yin, FALSE)) > - set_bit(cursor_image, stride, flags, x, y, FALSE); > - if (get_bit(src, stride, flags, xin, yin, TRUE)) > - set_bit(cursor_image, stride, flags, x, y, TRUE); > + if (get_bit(src, cursor_info, xin, yin, FALSE)) > + set_bit(cursor_image, cursor_info, x, y, FALSE); > + if (get_bit(src, cursor_info, xin, yin, TRUE)) > + set_bit(cursor_image, cursor_info, x, y, TRUE); > } > } > crtc->funcs->load_cursor_image (crtc, cursor_image); > -- > 1.7.1 > > _______________________________________________ > [email protected]: X.Org development > Archives: http://lists.x.org/archives/xorg-devel > Info: http://lists.x.org/mailman/listinfo/xorg-devel > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
