Re: [PATCH] fixed sparkling issue on lcd panel when fb_blank mode is changed.

Re: [PATCH] fixed sparkling issue on lcd panel when fb_blank mode is changed.

[Richard Purdie]

Hi,

On Fri, 2010-03-19 at 13:36 +0900, InKi Dae wrote:
> This issue is a problem that lcd panel is spakled when fb_blank mode is changed
> From FB_BLANK_UNBLANK to FB_BLANK_POWER or FB_BLANK_POWER to FB_BLANK_UNBLANK.
> 
> In case of FB_BLANK_UNBLANK, screen = on, vsync = on, hsync = on and
> For FB_BLANK_POWERDOWN screen = blanked, vsync = off, hsync = off
> So like this when fb_blank mode becomes FB_BLANK_POWERDOWN,
> Display controller and lcd panel should become off to reduce power consumption.
> 
> Let's see fb_blank function of fbmem.c file.
> 
> fb_blank()
> {
>     if (info->fbops->fb_blank)
>             ret = info->fbops->fb_blank(blank, info(
>     if (!ret)
>             fb_notifier_call_chain(FB_EVENT_BLANK, &event);
> }
> 
> This problem is because this code calls fb_blank (fb_blank of device
> specific framebuffer driver)
> Earlier then fb_notifier_call_chain.
> 
> For example,
> Device specific fb_blank function is registered to generic framebuffer
> driver through register_framebuffer function
> And fb_notifier_call_chain calls any callback function registered to
> lcd class(lcd.c) through fb_notifier_callback function.
> So if fb_blank mode is changed to FB_BLANK_POWERDOWN then display
> controller would become off(clock disable)
> On the other hand, lcd panel would still be on. at this time, some
> situation on lcd panel occers like sparkling
> Because video clock to be delivered to ldi module of lcd panel is disabled also.
> 
> This patch get fb_notifier_call_chain to be called earlier then fb_blank.
> Please, review this patch.

So to summarise the problem more succinctly the LCD and the display
controllers power off in the wrong order causing visual defects
(sparkling) on the display.

I was a bit worried about event order and loops with this change so I
checked and there are three consumers of these events in the kernel:
consoles, LCD and backlight. None of them have a problem with this
change of order for power down so from that point of view I'm ok with
the change.

However this function also covers powerup from a screen blank to an
active screen. When powering up it makes sense to power up the video
device first, then the LCD, backlight and console as per the current
code, otherwise I can imagine "sparkling" upon powerup.

So it looks like we need to change order only on powerdown, not upon
resume, otherwise we'll get further regression reports on poweron?

Cheers,

Richard

Re: [PATCH] fixed sparkling issue on lcd panel when fb_blank mode is changed.

[InKi Dae]

yes, you are right.

when fb_blank mode is changed to FB_BLANK_NORMAL or
FB_BLANK_UNBLANK(power down -> power up)
it has a possibility that we would encounter same issue.

I'd try to modify my patch to be considered with your opinion and will
send it again.
Thank you for your answer.

Best Regards,
InKi Dae.

2010/3/19 Richard Purdie <rpurdie@rpsys.net>:
> Hi,
>
> On Fri, 2010-03-19 at 13:36 +0900, InKi Dae wrote:
>> This issue is a problem that lcd panel is spakled when fb_blank mode is changed
>> From FB_BLANK_UNBLANK to FB_BLANK_POWER or FB_BLANK_POWER to FB_BLANK_UNBLANK.
>>
>> In case of FB_BLANK_UNBLANK, screen = on, vsync = on, hsync = on and
>> For FB_BLANK_POWERDOWN screen = blanked, vsync = off, hsync = off
>> So like this when fb_blank mode becomes FB_BLANK_POWERDOWN,
>> Display controller and lcd panel should become off to reduce power consumption.
>>
>> Let's see fb_blank function of fbmem.c file.
>>
>> fb_blank()
>> {
>>     if (info->fbops->fb_blank)
>>             ret = info->fbops->fb_blank(blank, info(
>>     if (!ret)
>>             fb_notifier_call_chain(FB_EVENT_BLANK, &event);
>> }
>>
>> This problem is because this code calls fb_blank (fb_blank of device
>> specific framebuffer driver)
>> Earlier then fb_notifier_call_chain.
>>
>> For example,
>> Device specific fb_blank function is registered to generic framebuffer
>> driver through register_framebuffer function
>> And fb_notifier_call_chain calls any callback function registered to
>> lcd class(lcd.c) through fb_notifier_callback function.
>> So if fb_blank mode is changed to FB_BLANK_POWERDOWN then display
>> controller would become off(clock disable)
>> On the other hand, lcd panel would still be on. at this time, some
>> situation on lcd panel occers like sparkling
>> Because video clock to be delivered to ldi module of lcd panel is disabled also.
>>
>> This patch get fb_notifier_call_chain to be called earlier then fb_blank.
>> Please, review this patch.
>
> So to summarise the problem more succinctly the LCD and the display
> controllers power off in the wrong order causing visual defects
> (sparkling) on the display.
>
> I was a bit worried about event order and loops with this change so I
> checked and there are three consumers of these events in the kernel:
> consoles, LCD and backlight. None of them have a problem with this
> change of order for power down so from that point of view I'm ok with
> the change.
>
> However this function also covers powerup from a screen blank to an
> active screen. When powering up it makes sense to power up the video
> device first, then the LCD, backlight and console as per the current
> code, otherwise I can imagine "sparkling" upon powerup.
>
> So it looks like we need to change order only on powerdown, not upon
> resume, otherwise we'll get further regression reports on poweron?
>
> Cheers,
>
> Richard
>
>
>
>
>

Re: [PATCH] fixed sparkling issue on lcd panel when fb_blank mode is changed.

[InKi Dae]

[-- Attachment #1: Type: text/plain, Size: 3614 bytes --]

I has modified my patch.

this patch would change order only in case of FB_BLANK_POWERDOWN
with Richard's opinion.

if requested blank mode is less then FB_BLANK_POWERDOWN then
it would call device specific fb_blank and then fb_notifer_call_chan
otherwise fb_notifier_call_chain first, this case means power up
and display controller should become on earlier then other devices
like lcd panel.

Please review this path again.

signed-off-by : InKi Dae <inki.dae@samsung.com>

Best Regards,
InKi Dae.

2010/3/22 InKi Dae <daeinki@gmail.com>:
> yes, you are right.
>
> when fb_blank mode is changed to FB_BLANK_NORMAL or
> FB_BLANK_UNBLANK(power down -> power up)
> it has a possibility that we would encounter same issue.
>
> I'd try to modify my patch to be considered with your opinion and will
> send it again.
> Thank you for your answer.
>
> Best Regards,
> InKi Dae.
>
> 2010/3/19 Richard Purdie <rpurdie@rpsys.net>:
>> Hi,
>>
>> On Fri, 2010-03-19 at 13:36 +0900, InKi Dae wrote:
>>> This issue is a problem that lcd panel is spakled when fb_blank mode is changed
>>> From FB_BLANK_UNBLANK to FB_BLANK_POWER or FB_BLANK_POWER to FB_BLANK_UNBLANK.
>>>
>>> In case of FB_BLANK_UNBLANK, screen = on, vsync = on, hsync = on and
>>> For FB_BLANK_POWERDOWN screen = blanked, vsync = off, hsync = off
>>> So like this when fb_blank mode becomes FB_BLANK_POWERDOWN,
>>> Display controller and lcd panel should become off to reduce power consumption.
>>>
>>> Let's see fb_blank function of fbmem.c file.
>>>
>>> fb_blank()
>>> {
>>>     if (info->fbops->fb_blank)
>>>             ret = info->fbops->fb_blank(blank, info(
>>>     if (!ret)
>>>             fb_notifier_call_chain(FB_EVENT_BLANK, &event);
>>> }
>>>
>>> This problem is because this code calls fb_blank (fb_blank of device
>>> specific framebuffer driver)
>>> Earlier then fb_notifier_call_chain.
>>>
>>> For example,
>>> Device specific fb_blank function is registered to generic framebuffer
>>> driver through register_framebuffer function
>>> And fb_notifier_call_chain calls any callback function registered to
>>> lcd class(lcd.c) through fb_notifier_callback function.
>>> So if fb_blank mode is changed to FB_BLANK_POWERDOWN then display
>>> controller would become off(clock disable)
>>> On the other hand, lcd panel would still be on. at this time, some
>>> situation on lcd panel occers like sparkling
>>> Because video clock to be delivered to ldi module of lcd panel is disabled also.
>>>
>>> This patch get fb_notifier_call_chain to be called earlier then fb_blank.
>>> Please, review this patch.
>>
>> So to summarise the problem more succinctly the LCD and the display
>> controllers power off in the wrong order causing visual defects
>> (sparkling) on the display.
>>
>> I was a bit worried about event order and loops with this change so I
>> checked and there are three consumers of these events in the kernel:
>> consoles, LCD and backlight. None of them have a problem with this
>> change of order for power down so from that point of view I'm ok with
>> the change.
>>
>> However this function also covers powerup from a screen blank to an
>> active screen. When powering up it makes sense to power up the video
>> device first, then the LCD, backlight and console as per the current
>> code, otherwise I can imagine "sparkling" upon powerup.
>>
>> So it looks like we need to change order only on powerdown, not upon
>> resume, otherwise we'll get further regression reports on poweron?
>>
>> Cheers,
>>
>> Richard
>>
>>
>>
>>
>>
>

[-- Attachment #2: fbmem.patch --]
[-- Type: application/octet-stream, Size: 1378 bytes --]

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a15b44e..d80e097 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1007,24 +1007,33 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 
 int
 fb_blank(struct fb_info *info, int blank)
-{	
- 	int ret = -EINVAL;
+{
+	struct fb_event event;
+	int ret = -EINVAL;
 
- 	if (blank > FB_BLANK_POWERDOWN)
- 		blank = FB_BLANK_POWERDOWN;
+	if (blank > FB_BLANK_POWERDOWN)
+		blank = FB_BLANK_POWERDOWN;
 
-	if (info->fbops->fb_blank)
- 		ret = info->fbops->fb_blank(blank, info);
+	event.info = info;
+	event.data = &blank;
 
- 	if (!ret) {
-		struct fb_event event;
+	if (blank <  FB_BLANK_POWERDOWN) {
+		if (info->fbops->fb_blank)
+			ret = info->fbops->fb_blank(blank, info);
 
-		event.info = info;
-		event.data = &blank;
-		fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+		ret = fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+		if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
+			printk(KERN_ERR "notifier_call failed.\n");
+	} else {
+		ret = fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+		if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
+			printk(KERN_ERR "notifier_call failed.\n");
+
+		if (info->fbops->fb_blank)
+			ret = info->fbops->fb_blank(blank, info);
 	}
 
- 	return ret;
+	return ret;
 }
 
 static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,

Re: [PATCH] fixed sparkling issue on lcd panel when fb_blank mode is changed.

[InKi Dae]

[-- Attachment #1: Type: text/plain, Size: 3887 bytes --]

Sorry, I have modified typing error.

2010/3/22 InKi Dae <daeinki@gmail.com>:
> I have modified my patch.
>
> this patch would change order only in case of FB_BLANK_POWERDOWN
> with Richard's opinion.
>
> if requested blank mode is less then FB_BLANK_POWERDOWN then
> it would call device specific fb_blank and then fb_notifer_call_chan
> otherwise fb_notifier_call_chain first, this case means power up
----> otherwise fb_notifier_call_chain first, this case means power down

> and display controller should become on earlier then other devices
> like lcd panel.
>
> Please review this path again.
>
> signed-off-by : InKi Dae <inki.dae@samsung.com>
>
> Best Regards,
> InKi Dae.
>
> 2010/3/22 InKi Dae <daeinki@gmail.com>:
>> yes, you are right.
>>
>> when fb_blank mode is changed to FB_BLANK_NORMAL or
>> FB_BLANK_UNBLANK(power down -> power up)
>> it has a possibility that we would encounter same issue.
>>
>> I'd try to modify my patch to be considered with your opinion and will
>> send it again.
>> Thank you for your answer.
>>
>> Best Regards,
>> InKi Dae.
>>
>> 2010/3/19 Richard Purdie <rpurdie@rpsys.net>:
>>> Hi,
>>>
>>> On Fri, 2010-03-19 at 13:36 +0900, InKi Dae wrote:
>>>> This issue is a problem that lcd panel is spakled when fb_blank mode is changed
>>>> From FB_BLANK_UNBLANK to FB_BLANK_POWER or FB_BLANK_POWER to FB_BLANK_UNBLANK.
>>>>
>>>> In case of FB_BLANK_UNBLANK, screen = on, vsync = on, hsync = on and
>>>> For FB_BLANK_POWERDOWN screen = blanked, vsync = off, hsync = off
>>>> So like this when fb_blank mode becomes FB_BLANK_POWERDOWN,
>>>> Display controller and lcd panel should become off to reduce power consumption.
>>>>
>>>> Let's see fb_blank function of fbmem.c file.
>>>>
>>>> fb_blank()
>>>> {
>>>>     if (info->fbops->fb_blank)
>>>>             ret = info->fbops->fb_blank(blank, info(
>>>>     if (!ret)
>>>>             fb_notifier_call_chain(FB_EVENT_BLANK, &event);
>>>> }
>>>>
>>>> This problem is because this code calls fb_blank (fb_blank of device
>>>> specific framebuffer driver)
>>>> Earlier then fb_notifier_call_chain.
>>>>
>>>> For example,
>>>> Device specific fb_blank function is registered to generic framebuffer
>>>> driver through register_framebuffer function
>>>> And fb_notifier_call_chain calls any callback function registered to
>>>> lcd class(lcd.c) through fb_notifier_callback function.
>>>> So if fb_blank mode is changed to FB_BLANK_POWERDOWN then display
>>>> controller would become off(clock disable)
>>>> On the other hand, lcd panel would still be on. at this time, some
>>>> situation on lcd panel occers like sparkling
>>>> Because video clock to be delivered to ldi module of lcd panel is disabled also.
>>>>
>>>> This patch get fb_notifier_call_chain to be called earlier then fb_blank.
>>>> Please, review this patch.
>>>
>>> So to summarise the problem more succinctly the LCD and the display
>>> controllers power off in the wrong order causing visual defects
>>> (sparkling) on the display.
>>>
>>> I was a bit worried about event order and loops with this change so I
>>> checked and there are three consumers of these events in the kernel:
>>> consoles, LCD and backlight. None of them have a problem with this
>>> change of order for power down so from that point of view I'm ok with
>>> the change.
>>>
>>> However this function also covers powerup from a screen blank to an
>>> active screen. When powering up it makes sense to power up the video
>>> device first, then the LCD, backlight and console as per the current
>>> code, otherwise I can imagine "sparkling" upon powerup.
>>>
>>> So it looks like we need to change order only on powerdown, not upon
>>> resume, otherwise we'll get further regression reports on poweron?
>>>
>>> Cheers,
>>>
>>> Richard
>>>
>>>
>>>
>>>
>>>
>>
>

[-- Attachment #2: fbmem.patch --]
[-- Type: application/octet-stream, Size: 1378 bytes --]

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index a15b44e..d80e097 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -1007,24 +1007,33 @@ fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var)
 
 int
 fb_blank(struct fb_info *info, int blank)
-{	
- 	int ret = -EINVAL;
+{
+	struct fb_event event;
+	int ret = -EINVAL;
 
- 	if (blank > FB_BLANK_POWERDOWN)
- 		blank = FB_BLANK_POWERDOWN;
+	if (blank > FB_BLANK_POWERDOWN)
+		blank = FB_BLANK_POWERDOWN;
 
-	if (info->fbops->fb_blank)
- 		ret = info->fbops->fb_blank(blank, info);
+	event.info = info;
+	event.data = &blank;
 
- 	if (!ret) {
-		struct fb_event event;
+	if (blank <  FB_BLANK_POWERDOWN) {
+		if (info->fbops->fb_blank)
+			ret = info->fbops->fb_blank(blank, info);
 
-		event.info = info;
-		event.data = &blank;
-		fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+		ret = fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+		if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
+			printk(KERN_ERR "notifier_call failed.\n");
+	} else {
+		ret = fb_notifier_call_chain(FB_EVENT_BLANK, &event);
+		if ((ret & NOTIFY_STOP_MASK) == NOTIFY_STOP_MASK)
+			printk(KERN_ERR "notifier_call failed.\n");
+
+		if (info->fbops->fb_blank)
+			ret = info->fbops->fb_blank(blank, info);
 	}
 
- 	return ret;
+	return ret;
 }
 
 static long do_fb_ioctl(struct fb_info *info, unsigned int cmd,