LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] i810fb: Fix console switch regression
@ 2008-03-16 18:42 Stefan Bauer
  2008-03-16 19:48 ` [Linux-fbdev-devel] " Geert Uytterhoeven
  2008-03-16 19:57 ` Henrique de Moraes Holschuh
  0 siblings, 2 replies; 8+ messages in thread
From: Stefan Bauer @ 2008-03-16 18:42 UTC (permalink / raw)
  To: linux-fbdev-devel, linux-kernel, Antonino Daplas

From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>

Commit eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! versus & precedence in 
various places") introduced a regression in console switching when using 
i810fb. Every 5th to 10th console switch causes 'pixel waste' - the same line 
of multi-colored pixels repeated over the whole screen.
This reverts eaa0ff1 for i810_main.c.

Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
Cc: Antonino Daplas <adaplas@pol.net>

---
As I'm not subscribed to the LKML, please CC me, thanks.

--- linux-2.6/drivers/video/i810/i810_main.c.orig
+++ linux-2.6/drivers/video/i810/i810_main.c
@@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
	struct i810fb_par *par = info->par;
	u8 __iomem *mmio = par->mmio_start_virtual;

-	if (!(par->dev_flags & LOCKUP))
+	if (!par->dev_flags & LOCKUP)
		return -ENXIO;

	if (cursor->image.width > 64 || cursor->image.height > 64)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [Linux-fbdev-devel] [PATCH] i810fb: Fix console switch regression
  2008-03-16 18:42 [PATCH] i810fb: Fix console switch regression Stefan Bauer
@ 2008-03-16 19:48 ` Geert Uytterhoeven
  2008-03-17  8:20   ` Stefan Bauer
  2008-03-16 19:57 ` Henrique de Moraes Holschuh
  1 sibling, 1 reply; 8+ messages in thread
From: Geert Uytterhoeven @ 2008-03-16 19:48 UTC (permalink / raw)
  To: Stefan Bauer
  Cc: Linux Frame Buffer Device Development, Linux Kernel Development,
	Antonino Daplas

On Sun, 16 Mar 2008, Stefan Bauer wrote:
> From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> 
> Commit eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! versus & precedence in 
> various places") introduced a regression in console switching when using 
> i810fb. Every 5th to 10th console switch causes 'pixel waste' - the same line 
> of multi-colored pixels repeated over the whole screen.
> This reverts eaa0ff1 for i810_main.c.
> 
> Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> Cc: Antonino Daplas <adaplas@pol.net>
> 
> ---
> As I'm not subscribed to the LKML, please CC me, thanks.
> 
> --- linux-2.6/drivers/video/i810/i810_main.c.orig
> +++ linux-2.6/drivers/video/i810/i810_main.c
> @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
> 	struct i810fb_par *par = info->par;
> 	u8 __iomem *mmio = par->mmio_start_virtual;
> 
> -	if (!(par->dev_flags & LOCKUP))
> +	if (!par->dev_flags & LOCKUP)
> 		return -ENXIO;

However, the original expression didn't make sense, as LOCKUP is 8 and
!par->dev_flags is either 0 or 1, so `!par->dev_flags & LOCKUP' is
always 0.

I took a quick look at the usage of the LOCKUP flag. Apparently when a
lock-up is detected, this flag is set, and the driver will fall back to
software operations instead of hardware accelerated operations.

Is it possible the intended code was

	if (par->dev_flags & LOCKUP)
		return -ENXIO;

?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i810fb: Fix console switch regression
  2008-03-16 18:42 [PATCH] i810fb: Fix console switch regression Stefan Bauer
  2008-03-16 19:48 ` [Linux-fbdev-devel] " Geert Uytterhoeven
@ 2008-03-16 19:57 ` Henrique de Moraes Holschuh
  2008-03-16 20:02   ` Henrique de Moraes Holschuh
  1 sibling, 1 reply; 8+ messages in thread
From: Henrique de Moraes Holschuh @ 2008-03-16 19:57 UTC (permalink / raw)
  To: Stefan Bauer; +Cc: linux-fbdev-devel, linux-kernel, Antonino Daplas

On Sun, 16 Mar 2008, Stefan Bauer wrote:
> From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> 
> Commit eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! versus & precedence in 
> various places") introduced a regression in console switching when using 
> i810fb. Every 5th to 10th console switch causes 'pixel waste' - the same line 
> of multi-colored pixels repeated over the whole screen.
> This reverts eaa0ff1 for i810_main.c.
> 
> Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> Cc: Antonino Daplas <adaplas@pol.net>
> 
> ---
> As I'm not subscribed to the LKML, please CC me, thanks.
> 
> --- linux-2.6/drivers/video/i810/i810_main.c.orig
> +++ linux-2.6/drivers/video/i810/i810_main.c
> @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
> 	struct i810fb_par *par = info->par;
> 	u8 __iomem *mmio = par->mmio_start_virtual;
> 
> -	if (!(par->dev_flags & LOCKUP))
> +	if (!par->dev_flags & LOCKUP)
> 		return -ENXIO;
> 
> 	if (cursor->image.width > 64 || cursor->image.height > 64)
> --

I think you need to add something to that line that avoids someone making
the same change as eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 some years down
the road.

Might I suggest using "((!par->dev_flags) & LOCKUP)"?

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i810fb: Fix console switch regression
  2008-03-16 19:57 ` Henrique de Moraes Holschuh
@ 2008-03-16 20:02   ` Henrique de Moraes Holschuh
  0 siblings, 0 replies; 8+ messages in thread
From: Henrique de Moraes Holschuh @ 2008-03-16 20:02 UTC (permalink / raw)
  To: Stefan Bauer; +Cc: linux-fbdev-devel, linux-kernel, Antonino Daplas

On Sun, 16 Mar 2008, Henrique de Moraes Holschuh wrote:
> Might I suggest using "((!par->dev_flags) & LOCKUP)"?

Never mind, I read your other post about that line not even making any
sense the way it was written.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i810fb: Fix console switch regression
  2008-03-16 19:48 ` [Linux-fbdev-devel] " Geert Uytterhoeven
@ 2008-03-17  8:20   ` Stefan Bauer
  2008-03-17 15:41     ` Stefan Bauer
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Bauer @ 2008-03-17  8:20 UTC (permalink / raw)
  To: Linux Frame Buffer Device Development, Linux Kernel Development,
	Antonino Daplas

Am Sonntag, 16. März 2008 20:48 schrieb Geert Uytterhoeven:
> On Sun, 16 Mar 2008, Stefan Bauer wrote:
> > --- linux-2.6/drivers/video/i810/i810_main.c.orig
> > +++ linux-2.6/drivers/video/i810/i810_main.c
> > @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info
> > 	struct i810fb_par *par = info->par;
> > 	u8 __iomem *mmio = par->mmio_start_virtual;
> >
> > -	if (!(par->dev_flags & LOCKUP))
> > +	if (!par->dev_flags & LOCKUP)
> > 		return -ENXIO;
>
> However, the original expression didn't make sense, as LOCKUP is 8 and
> !par->dev_flags is either 0 or 1, so `!par->dev_flags & LOCKUP' is
> always 0.
>
> I took a quick look at the usage of the LOCKUP flag. Apparently when a
> lock-up is detected, this flag is set, and the driver will fall back to
> software operations instead of hardware accelerated operations.
>
> Is it possible the intended code was
>
> 	if (par->dev_flags & LOCKUP)
> 		return -ENXIO;
>
> ?

Yes, IMO you are right. 4c7ffe0 ("fbdev: prevent drivers that have hardware 
cursors from calling software cursor code") introduced the senseless code.

I'm going on testing today, but I think that's it.

Again, please CC me, thanks.

---
From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>

Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers that 
have hardware cursors from calling software cursor code") every call of 
i810fb_cursor fails with -ENXIO because of a incorrect "!".

This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! 
versus & precedence in various places") surrounded the expression with 
braces, so that the intended behavior was inverted. That caused 'pixel 
waste' - the same line of multi-colored pixels repeated over the whole 
screen - during console switch.

This switches back to the original pre-4c7ffe0 behavior.

Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Antonino Daplas <adaplas@pol.net>

---
--- linux-2.6/drivers/video/i810/i810_main.c.orig
+++ linux-2.6/drivers/video/i810/i810_main.c
@@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info
        struct i810fb_par *par = info->par;
        u8 __iomem *mmio = par->mmio_start_virtual;

-       if (!(par->dev_flags & LOCKUP))
+       if (par->dev_flags & LOCKUP)
                return -ENXIO;

        if (cursor->image.width > 64 || cursor->image.height > 64)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i810fb: Fix console switch regression
  2008-03-17  8:20   ` Stefan Bauer
@ 2008-03-17 15:41     ` Stefan Bauer
  2008-03-18  3:48       ` Andrew Morton
  0 siblings, 1 reply; 8+ messages in thread
From: Stefan Bauer @ 2008-03-17 15:41 UTC (permalink / raw)
  To: linux-fbdev-devel, Linux Kernel Development, Antonino Daplas

I'm sorry for the spaces in the second patch, here's a clean one with tabs.

---
From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>

Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers that 
have hardware cursors from calling software cursor code") every call of 
i810fb_cursor fails with -ENXIO because of a incorrect "!".

This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! 
versus & precedence in various places") surrounded the expression with 
braces, so that the intended behavior was inverted. That caused 'pixel 
waste' - the same line of multi-colored pixels repeated over the whole 
screen - during console switch.

This switches back to the original pre-4c7ffe0 behavior.

Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Antonino Daplas <adaplas@pol.net>

--- linux-2.6/drivers/video/i810/i810_main.c.orig
+++ linux-2.6/drivers/video/i810/i810_main.c
@@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
 	struct i810fb_par *par = info->par;
 	u8 __iomem *mmio = par->mmio_start_virtual;
 
-	if (!(par->dev_flags & LOCKUP))
+	if (par->dev_flags & LOCKUP)
 		return -ENXIO;
 
 	if (cursor->image.width > 64 || cursor->image.height > 64)

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i810fb: Fix console switch regression
  2008-03-17 15:41     ` Stefan Bauer
@ 2008-03-18  3:48       ` Andrew Morton
  2008-03-18  7:09         ` Stefan Bauer
  0 siblings, 1 reply; 8+ messages in thread
From: Andrew Morton @ 2008-03-18  3:48 UTC (permalink / raw)
  To: Stefan Bauer; +Cc: linux-fbdev-devel, Linux Kernel Development, Antonino Daplas

On Mon, 17 Mar 2008 16:41:10 +0100 Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de> wrote:

> I'm sorry for the spaces in the second patch, here's a clean one with tabs.
> 
> ---
> From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> 
> Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers that 
> have hardware cursors from calling software cursor code") every call of 
> i810fb_cursor fails with -ENXIO because of a incorrect "!".
> 
> This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix ! 
> versus & precedence in various places") surrounded the expression with 
> braces, so that the intended behavior was inverted. That caused 'pixel 
> waste' - the same line of multi-colored pixels repeated over the whole 
> screen - during console switch.
> 
> This switches back to the original pre-4c7ffe0 behavior.
> 
> Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Cc: Antonino Daplas <adaplas@pol.net>
> 
> --- linux-2.6/drivers/video/i810/i810_main.c.orig
> +++ linux-2.6/drivers/video/i810/i810_main.c
> @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info 
>  	struct i810fb_par *par = info->par;
>  	u8 __iomem *mmio = par->mmio_start_virtual;
>  
> -	if (!(par->dev_flags & LOCKUP))
> +	if (par->dev_flags & LOCKUP)
>  		return -ENXIO;
>  
>  	if (cursor->image.width > 64 || cursor->image.height > 64)

Can you please confirm that this actually fixes the bug?  That wasn't clear.

Thanks.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH] i810fb: Fix console switch regression
  2008-03-18  3:48       ` Andrew Morton
@ 2008-03-18  7:09         ` Stefan Bauer
  0 siblings, 0 replies; 8+ messages in thread
From: Stefan Bauer @ 2008-03-18  7:09 UTC (permalink / raw)
  To: Andrew Morton
  Cc: linux-fbdev-devel, Linux Kernel Development, Antonino Daplas

Am Dienstag, 18. März 2008 04:48 schrieb Andrew Morton:
> On Mon, 17 Mar 2008 16:41:10 +0100 Stefan Bauer 
<stefan.bauer@cs.tu-chemnitz.de> wrote:
> > From: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> >
> > Since 4c7ffe0b9f7f40bd818fe3af51342f64c483908e ("fbdev: prevent drivers
> > that have hardware cursors from calling software cursor code") every call
> > of i810fb_cursor fails with -ENXIO because of a incorrect "!".
> >
> > This hasn't striked until eaa0ff15c30dc9799eb4d12660edb73aeb6d32c5 ("fix
> > ! versus & precedence in various places") surrounded the expression with
> > braces, so that the intended behavior was inverted. That caused 'pixel
> > waste' - the same line of multi-colored pixels repeated over the whole
> > screen - during console switch.
> >
> > This switches back to the original pre-4c7ffe0 behavior.
> >
> > Signed-off-by: Stefan Bauer <stefan.bauer@cs.tu-chemnitz.de>
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > Cc: Antonino Daplas <adaplas@pol.net>
> >
> > --- linux-2.6/drivers/video/i810/i810_main.c.orig
> > +++ linux-2.6/drivers/video/i810/i810_main.c
> > @@ -1476,7 +1476,7 @@ static int i810fb_cursor(struct fb_info
> >  	struct i810fb_par *par = info->par;
> >  	u8 __iomem *mmio = par->mmio_start_virtual;
> >
> > -	if (!(par->dev_flags & LOCKUP))
> > +	if (par->dev_flags & LOCKUP)
> >  		return -ENXIO;
> >
> >  	if (cursor->image.width > 64 || cursor->image.height > 64)
>
> Can you please confirm that this actually fixes the bug?  That wasn't
> clear.
>
> Thanks.

Yes, yesterday I worked the whole day with that machine running -rc6 + my 
patch. The regression definitely is fixed, fbi and fbset work w/o problems or 
new regressions.

	Regards
		Stefan

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2008-03-18  7:09 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-03-16 18:42 [PATCH] i810fb: Fix console switch regression Stefan Bauer
2008-03-16 19:48 ` [Linux-fbdev-devel] " Geert Uytterhoeven
2008-03-17  8:20   ` Stefan Bauer
2008-03-17 15:41     ` Stefan Bauer
2008-03-18  3:48       ` Andrew Morton
2008-03-18  7:09         ` Stefan Bauer
2008-03-16 19:57 ` Henrique de Moraes Holschuh
2008-03-16 20:02   ` Henrique de Moraes Holschuh

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).