Prev: [RELEASE] LTTng 0.217 for kernel 2.6.34
Next: hid Logitech G13 Driver 0.0.5
From: Martin Fuzzey on 19 Jun 2010 11:50
Hi Luotao,

Luotao Fu wrote:
> while the imxfb driver blanks the screen. The clock supply to the framebuffer
> controller is disabled. The same callback to disable the fb is, however, also
> used the remove and shutdown hooks of the imxfb platform driver. This means that
> the imxfb driver can run into a WARN in clock_disable while unloading or
> rebooting, if the screen is blanked previously, since the same clock will be
> than eventually disabled multiple times. This patch adds a flag to make sure
> that the clock will only be disabled, if it was not already disabled before.
>
>
Actually it's worse than that - you don't need to unload the
driver or reboot to run into this problem because imxfb_blank()
calls imxfb_disable_controller() for cases
case FB_BLANK_POWERDOWN:
case FB_BLANK_VSYNC_SUSPEND:
case FB_BLANK_HSYNC_SUSPEND:
case FB_BLANK_NORMAL:

Often an X server will call these after successive timeouts
(eg Normal, Suspend, Powerdown) so you can get into the
multiple suspend problem you describe just by waiting long
enough for at least two blank levels to be activated.

Furthermore once this happens, not only does a WARN occur
in clock_disable but the clock usage counter will actually become
negative so it will require multiple wake up requests to switch
the display back on.

I was just about to post a patch for this but I see you beat
me to it :).

Your patch fixes the clock_disable problem but fbi->backlight_power()
and fbi->lcd_power() can still be called multiple times.
Not sure if that is really a problem - they should be idempotent.

I did it by making all of imxfb_disable_controller() do nothing if the
disable was already done like this (just for illustration not submission,
lines may be mangled):


--- a/drivers/video/imxfb.c
+++ b/drivers/video/imxfb.c
@@ -175,6 +175,7 @@ struct imxfb_info {

struct imx_fb_videomode *mode;
int num_modes;
+ int enabled;

void (*lcd_power)(int);
void (*backlight_power)(int);
@@ -451,6 +452,9 @@ static int imxfb_set_par(struct fb_info *info)

static void imxfb_enable_controller(struct imxfb_info *fbi)
{
+ if (fbi->enabled)
+ return;
+
pr_debug("Enabling LCD controller\n");

writel(fbi->screen_dma, fbi->regs + LCDC_SSA);
@@ -470,10 +474,13 @@ static void imxfb_enable_controller(struct
imxfb_info *fbi)
fbi->backlight_power(1);
if (fbi->lcd_power)
fbi->lcd_power(1);
+ fbi->enabled = 1;
}

static void imxfb_disable_controller(struct imxfb_info *fbi)
{
+ if (!fbi->enabled)
+ return;
pr_debug("Disabling LCD controller\n");

if (fbi->backlight_power)
@@ -484,6 +491,7 @@ static void imxfb_disable_controller(struct
imxfb_info *fbi)
clk_disable(fbi->clk);

writel(0, fbi->regs + LCDC_RMCR);
+ fbi->enabled = 0;
}

static int imxfb_blank(int blank, struct fb_info *info)


Though this does miss the explicit clk_disable() in imxfb_remove()


Cheers,

Martin


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Luotao Fu on 21 Jun 2010 03:40
Hi Martin,

On Sat, Jun 19, 2010 at 05:48:08PM +0200, Martin Fuzzey wrote:
> Hi Luotao,
>
> Luotao Fu wrote:
> > while the imxfb driver blanks the screen. The clock supply to the framebuffer
> > controller is disabled. The same callback to disable the fb is, however, also
> > used the remove and shutdown hooks of the imxfb platform driver. This means that
> > the imxfb driver can run into a WARN in clock_disable while unloading or
> > rebooting, if the screen is blanked previously, since the same clock will be
> > than eventually disabled multiple times. This patch adds a flag to make sure
> > that the clock will only be disabled, if it was not already disabled before.
> >
> >
> Actually it's worse than that - you don't need to unload the
> driver or reboot to run into this problem because imxfb_blank()
> calls imxfb_disable_controller() for cases
> case FB_BLANK_POWERDOWN:
> case FB_BLANK_VSYNC_SUSPEND:
> case FB_BLANK_HSYNC_SUSPEND:
> case FB_BLANK_NORMAL:
>
> Often an X server will call these after successive timeouts
> (eg Normal, Suspend, Powerdown) so you can get into the
> multiple suspend problem you describe just by waiting long
> enough for at least two blank levels to be activated.
>
> Furthermore once this happens, not only does a WARN occur
> in clock_disable but the clock usage counter will actually become
> negative so it will require multiple wake up requests to switch
> the display back on.
>
> I was just about to post a patch for this but I see you beat
> me to it :).
>
> Your patch fixes the clock_disable problem but fbi->backlight_power()
> and fbi->lcd_power() can still be called multiple times.
> Not sure if that is really a problem - they should be idempotent.
>

Multiple calling of the backlight_power and lcd_power function hooks
should be no trouble on most platforms since they mostly control some
PWM or GPIO lines, which won't complain to be turned on or off again.
Never the less I agree with you that this is quite unneccessary.

> I did it by making all of imxfb_disable_controller() do nothing if the
> disable was already done like this (just for illustration not submission,
> lines may be mangled):
>

This solution looks better and more clear than mine, which only concentrate
on clock. Only nitpickings: I'd probably defined "enabled" as a bool. Further
it might be nice to change pr_debug so that one would see a failed
enable/disable call. some like
pr_debug("Disabling LCD controller ... ");

if (!fbi->enabled) {
pr_debug("already disabled\n");
return;
} else
pr_debug("\n");

Applied and tested on my board here. (consolefb only, no X running.)
so far:
Acked-by: Luotao Fu <l.fu(a)pengutronix.de>


thanks
and
cheers

Luotao Fu
--
Pengutronix e.K. | Dipl.-Ing. Luotao Fu |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
From: Luotao Fu on 1 Jul 2010 07:00
Hi Martin,

On Sat, Jun 19, 2010 at 05:48:08PM +0200, Martin Fuzzey wrote:
> Hi Luotao,
>
> Luotao Fu wrote:

....
> Your patch fixes the clock_disable problem but fbi->backlight_power()
> and fbi->lcd_power() can still be called multiple times.
> Not sure if that is really a problem - they should be idempotent.
>
> I did it by making all of imxfb_disable_controller() do nothing if the
> disable was already done like this (just for illustration not submission,
> lines may be mangled):
>

Anything new on this? 'd be nice if you could resend a clean patch based
on the described you sent in this mail. There're about 3 Patches about
this issue out there. More might come in, as long as the bug still
exists. ;-) So let's pack it up and push a proper fix to Sascha.

cheers
Luotao Fu

--
Pengutronix e.K. | Dipl.-Ing. Luotao Fu |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
 | 
Pages: 1
Prev: [RELEASE] LTTng 0.217 for kernel 2.6.34
Next: hid Logitech G13 Driver 0.0.5