Re: [Linux-fbdev-devel] [PATCH] fbdev: fix fillrect for 24bpp modes

On Mon, 13 Apr 2009 19:09:54 +0200
Michal Januszewski <spock@xxxxxxxxxx> wrote:

The software fillrect routines do not work properly when the number of
pixels per machine word is not an integer. To see that, run the following
command on a fbdev console with a 24bpp video mode, using a non-accelerated
driver such as (u)vesafb:

reset ; echo -e '\e[41mtest\e[K'

The expected result is 'test' displayed on a line with red background.
Instead of that, 'test' has a red background, but the rest of the line
(rendered using fillrect()) contains a distored colorful pattern.

This patch fixes the problem by correctly computing rotation shifts.
It has been tested in a 24bpp mode on 32- and 64-bit little-endian
machines.


I can confirm the patch fixes the issue it describes if the line is not padded
or padded in a special way. See my comments below.

Signed-off-by: Michal Januszewski <spock@xxxxxxxxxx>
---
diff --git a/drivers/video/cfbfillrect.c b/drivers/video/cfbfillrect.c
index 64b3576..396e676 100644
--- a/drivers/video/cfbfillrect.c
+++ b/drivers/video/cfbfillrect.c
@@ -9,10 +9,6 @@
*
* NOTES:
*
- * The code for depths like 24 that don't have integer number of pixels per
- * long is broken and needs to be fixed. For now I turned these types of
- * mode off.
- *
* Also need to add code to deal with cards endians that are different than
* the native cpu endians. I also need to deal with MSB position in the word.
*
@@ -139,7 +135,7 @@ bitfill_unaligned(struct fb_info *p, unsigned long __iomem *dst, int dst_idx,

// Trailing bits
if (last)
- FB_WRITEL(comp(pat, FB_READL(dst), first), dst);
+ FB_WRITEL(comp(pat, FB_READL(dst), last), dst);
}
}

@@ -297,7 +293,7 @@ void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
else
fg = rect->color;

- pat = pixel_to_pat( bpp, fg);
+ pat = pixel_to_pat(bpp, fg);

dst = (unsigned long __iomem *)((unsigned long)p->screen_base & ~(bytes-1));
dst_idx = ((unsigned long)p->screen_base & (bytes - 1))*8;
@@ -333,17 +329,20 @@ void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
dst_idx += p->fix.line_length*8;
}
} else {
- int right;
- int r;
- int rot = (left-dst_idx) % bpp;
+ int right, r;
void (*fill_op)(struct fb_info *p, unsigned long __iomem *dst,
int dst_idx, unsigned long pat, int left,
int right, unsigned n, int bits) = NULL;
-
+#ifdef __LITTLE_ENDIAN
+ right = left;
+ left = bpp - right;
+#else
+ right = bpp - left;
+#endif
/* rotate pattern to correct start position */
- pat = pat << rot | pat >> (bpp-rot);
+ r = dst_idx >> (ffs(bits) - 1);

The r is simply dst_idx / bits. Most compilers will optimize it away into
a simple shift because the bits has only one bit set (it is number of bits in a long variable, ie. 32 or 64).

+ pat = pat << ((left*r) % bpp) | pat >> ((right*r) % bpp);


If the r = (dst_idx / bits) it is number of long words. The shift by ((left*r) % bpp) does
not make much sense (try left = 3 and r = 24 words - it is always zero but should be 3).
It is even worse if a line is padded so line's length modulo bpp is not
zero it does not work. A colorful pattern is produced after the "mtest" text.

A dst_idx offset is not taken into account (it could be non-zero only for depths < 8 bits).

Kind regards,
Krzysztof


----------------------------------------------------------------------
Najlepsze dzwonki MP3 na telefon komorkowy!
Sprawdz >> http://link.interia.pl/f2128

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Relevant Pages

  • [PATCH v2] fbdev: fix fillrect for 24bpp modes
    ... pixels per machine word is not an integer. ... int dst_idx, unsigned long pat, int left, ...
    (Linux-Kernel)
  • [PATCH] fbdev: fix fillrect for 24bpp modes
    ... The software fillrect routines do not work properly when the number of ... pixels per machine word is not an integer. ... int dst_idx, unsigned long pat, int left, ...
    (Linux-Kernel)
  • [PATCH v3] fbdev: fix fillrect for 24bpp modes
    ... The software fillrect routines do not work properly when the number of ... pixels per machine word is not an integer. ... int dst_idx, unsigned long pat, int left, ...
    (Linux-Kernel)
  • Re: 4-level image compression ?
    ... I'm playing with a small system where the display is 320x240 pixels, ... at which point one could also encode the delta. ... the above would use 64 bytes for the encode table, and 256 for decode. ... int paeth ...
    (comp.compression)
  • Re: Transparent Blit with NT4
    ... >> I should copy pixels form src to dest skipping pixels with color ... > bool ChromaBlt(HDC outDC, int inX, int inY, int outWidth, int ... > If you can't use any kind of mask at all then if the image you're ... transparent colour set to &00FFFFFF. ...
    (microsoft.public.win32.programmer.gdi)