LinuxLists.cc - [PATCH] fbdev: fix fillrect for 24bpp modes

2009-04-13 17:11:13

Subject: [PATCH] fbdev: fix fillrect for 24bpp modes

2009-04-17 16:33:38

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

On Mon, 13 Apr 2009 19:09:54 +0200
Michal Januszewski <[email protected]> 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 <[email protected]>
> ---
> 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

2009-04-17 18:54:36

by Andrew Morton

[permalink] [raw]

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

On Fri, 17 Apr 2009 18:36:17 +0200
Krzysztof Helt <[email protected]> wrote:

> On Mon, 13 Apr 2009 19:09:54 +0200
> Michal Januszewski <[email protected]> wrote:
>
> ...
>
> 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).
>

OK, thanks for checking.

Michael, I'll await a version 2 on this patch. It looks like something
we want in 2.6.30.

2009-04-18 18:58:36

by Michal Januszewski

[permalink] [raw]

Subject: [PATCH v2] 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. 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.

Signed-off-by: Michał Januszewski <[email protected]>
---
This version of the patch should work correctly with modes where
line_length*8 % bpp != 0.

It also replaces the right shift by (ffs(bits) - 1) with a division
by bits, as both operations are optimized into a shift by a constant
and the latter one increases code readability.

In case this still breaks in some modes, I would appreciate a full
list of parameters describing the problem, i.e. bpp, line_length
and the top left coordinate of the rectangle that is painted
incorrectly.

diff --git a/drivers/video/cfbfillrect.c b/drivers/video/cfbfillrect.c
index 64b3576..faa1a39 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);
}
}

@@ -281,7 +277,7 @@ bitfill_unaligned_rev(struct fb_info *p, unsigned long __iomem *dst,

void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
{
- unsigned long pat, fg;
+ unsigned long pat, pat2, fg;
unsigned long width = rect->width, height = rect->height;
int bits = BITS_PER_LONG, bytes = bits >> 3;
u32 bpp = p->var.bits_per_pixel;
@@ -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,16 @@ 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;
-
- /* rotate pattern to correct start position */
- pat = pat << rot | pat >> (bpp-rot);
-
- right = bpp-left;
+#ifdef __LITTLE_ENDIAN
+ right = left;
+ left = bpp - right;
+#else
+ right = bpp - left;
+#endif
switch (rect->rop) {
case ROP_XOR:
fill_op = bitfill_unaligned_rev;
@@ -357,12 +352,17 @@ void cfb_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
break;
}
while (height--) {
- dst += dst_idx >> (ffs(bits) - 1);
+ dst += dst_idx / bits;
dst_idx &= (bits - 1);
- fill_op(p, dst, dst_idx, pat, left, right,
+ r = dst_idx % bpp;
+ /* rotate pattern to the correct start position */
+#ifdef __LITTLE_ENDIAN
+ pat2 = pat << r | pat >> (bpp-r);
+#else
+ pat2 = pat << (bpp-r) | pat >> r;
+#endif
+ fill_op(p, dst, dst_idx, pat2, left, right,
width*bpp, bits);
- r = (p->fix.line_length*8) % bpp;
- pat = pat << (bpp-r) | pat >> r;
dst_idx += p->fix.line_length*8;
}
}
diff --git a/drivers/video/fb_draw.h b/drivers/video/fb_draw.h
index 1db6221..fa9626e 100644
--- a/drivers/video/fb_draw.h
+++ b/drivers/video/fb_draw.h
@@ -33,11 +33,11 @@ pixel_to_pat( u32 bpp, u32 pixel)
case 8:
return 0x0101010101010101ul*pixel;
case 12:
- return 0x0001001001001001ul*pixel;
+ return 0x1001001001001001ul*pixel;
case 16:
return 0x0001000100010001ul*pixel;
case 24:
- return 0x0000000001000001ul*pixel;
+ return 0x0001000001000001ul*pixel;
case 32:
return 0x0000000100000001ul*pixel;
default:
@@ -58,11 +58,11 @@ pixel_to_pat( u32 bpp, u32 pixel)
case 8:
return 0x01010101ul*pixel;
case 12:
- return 0x00001001ul*pixel;
+ return 0x01001001ul*pixel;
case 16:
return 0x00010001ul*pixel;
case 24:
- return 0x00000001ul*pixel;
+ return 0x01000001ul*pixel;
case 32:
return 0x00000001ul*pixel;
default:
diff --git a/drivers/video/sysfillrect.c b/drivers/video/sysfillrect.c
index f94d6b6..ff256a4 100644
--- a/drivers/video/sysfillrect.c
+++ b/drivers/video/sysfillrect.c
@@ -124,7 +124,7 @@ bitfill_unaligned(struct fb_info *p, unsigned long *dst, int dst_idx,

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

@@ -242,7 +242,7 @@ bitfill_unaligned_rev(struct fb_info *p, unsigned long *dst, int dst_idx,

void sys_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
{
- unsigned long pat, fg;
+ unsigned long pat, pat2, fg;
unsigned long width = rect->width, height = rect->height;
int bits = BITS_PER_LONG, bytes = bits >> 3;
u32 bpp = p->var.bits_per_pixel;
@@ -292,17 +292,16 @@ void sys_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 *dst,
int dst_idx, unsigned long pat, int left,
int right, unsigned n, int bits) = NULL;
-
- /* rotate pattern to correct start position */
- pat = pat << rot | pat >> (bpp-rot);
-
- right = bpp-left;
+#ifdef __LITTLE_ENDIAN
+ right = left;
+ left = bpp - right;
+#else
+ right = bpp - left;
+#endif
switch (rect->rop) {
case ROP_XOR:
fill_op = bitfill_unaligned_rev;
@@ -317,12 +316,17 @@ void sys_fillrect(struct fb_info *p, const struct fb_fillrect *rect)
break;
}
while (height--) {
- dst += dst_idx >> (ffs(bits) - 1);
+ dst += dst_idx / bits;
dst_idx &= (bits - 1);
- fill_op(p, dst, dst_idx, pat, left, right,
+ r = dst_idx % bpp;
+ /* rotate pattern to the correct start position */
+#ifdef __LITTLE_ENDIAN
+ pat2 = pat << r | pat >> (bpp-r);
+#else
+ pat2 = pat << (bpp-r) | pat >> r;
+#endif
+ fill_op(p, dst, dst_idx, pat2, left, right,
width*bpp, bits);
- r = (p->fix.line_length*8) % bpp;
- pat = pat << (bpp-r) | pat >> r;
dst_idx += p->fix.line_length*8;
}
}

2009-04-20 23:19:56

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH v2] fbdev: fix fillrect for 24bpp modes

On Sat, 18 Apr 2009 20:52:34 +0200
Michal Januszewski <[email protected]> wrote:

> while (height--) {
> - dst += dst_idx >> (ffs(bits) - 1);
> + dst += dst_idx / bits;
> dst_idx &= (bits - 1);
> - fill_op(p, dst, dst_idx, pat, left, right,
> + r = dst_idx % bpp;
> + /* rotate pattern to the correct start position */
> +#ifdef __LITTLE_ENDIAN
> + pat2 = pat << r | pat >> (bpp-r);
> +#else
> + pat2 = pat << (bpp-r) | pat >> r;
> +#endif
> + fill_op(p, dst, dst_idx, pat2, left, right,
> width*bpp, bits);
> - r = (p->fix.line_length*8) % bpp;
> - pat = pat << (bpp-r) | pat >> r;
> dst_idx += p->fix.line_length*8;

hm, that's fairly eye-popping.

Could we do something like

pat2 = le32_to_cpu(rol32(cpu_to_le32(pat), r));

(might be wrong, but you see what I mean...)

2009-04-21 05:07:20

by Krzysztof Helt

[permalink] [raw]

Subject: Re: [PATCH v2] fbdev: fix fillrect for 24bpp modes

On Sat, 18 Apr 2009 20:52:34 +0200
Michal Januszewski <[email protected]> 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.
>
> Signed-off-by: Micha? Januszewski <[email protected]>
> ---

I have tested the patch and it has worked with padded lines.

Acked-by: Krzysztof Helt <[email protected]>

However, there is a comment from Andrew Morton.

Regards,
Krzysztof

----------------------------------------------------------------------
Gotowka na koncie. Otworz konto direct i wez podwojny limit.
http://link.interia.pl/f2115