On Mon, Nov 29, 2010 at 9:03 PM, Aaron Plattner <aplatt...@nvidia.com> wrote: > Does anyone else want to review this, or are Robert's (@nvidia.com) > Reviewed-by and Cyril's Tested-by sufficient?
Looks good to me. Reviewed-by: Alex Deucher <alexdeuc...@gmail.com> > > 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 <aplatt...@nvidia.com> >> Reviewed-by: Robert Morell <rmor...@nvidia.com> >> --- >> 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 >> >> _______________________________________________ >> 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 >> > _______________________________________________ > 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 > _______________________________________________ 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