Re: [PATCH 1/5] fbdev/au1100fb: Use FB blanking constants

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 18.10.26 um 15:52 schrieb Helge Deller:
> Hi Thomas,
>
> On 10/9/26 18:05, Thomas Zimmermann wrote:
>> Fbdev and its drivers use FB_BLANK constants for controlling the
>> display's blanking state. Semantics differ to similar VESA blanking
>> constants.
>>
>> Replace the VESA constants in au1100fb with FB_BLANK.
>
> I understand that this series drops the VESA constants.
> That seems logical and correct, but....
>
>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/video/fbdev/au1100fb.c | 18 +++++++++---------
>>   1 file changed, 9 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/video/fbdev/au1100fb.c 
>> b/drivers/video/fbdev/au1100fb.c
>> index 39f86ffb1ae3..6c375f389fbd 100644
>> --- a/drivers/video/fbdev/au1100fb.c
>> +++ b/drivers/video/fbdev/au1100fb.c
>> @@ -424,15 +424,15 @@ static int au1100fb_fb_blank(int blank_mode, 
>> struct fb_info *fbi)
>>         switch (blank_mode) {
>>   -    case VESA_NO_BLANKING:
>> +    case FB_BLANK_UNBLANK:
>>           /* Turn on panel */
>>           fbdev->regs->lcd_control |= LCD_CONTROL_GO;
>>           wmb(); /* drain writebuffer */
>>           break;
>> -
>> -    case VESA_VSYNC_SUSPEND:
>> -    case VESA_HSYNC_SUSPEND:
>> -    case VESA_POWERDOWN:
>> +    case FB_BLANK_NORMAL:
>> +    case FB_BLANK_VSYNC_SUSPEND:
>> +    case FB_BLANK_HSYNC_SUSPEND:
>> +    case FB_BLANK_POWERDOWN:
> I don't understand (this is not the fault of this/your patch),
> how this enum in fb.h works:
>
> enum {
>         /* screen: unblanked, hsync: on,  vsync: on */
>         FB_BLANK_UNBLANK       = VESA_NO_BLANKING,
>
>         /* screen: blanked,   hsync: on,  vsync: on */
>         FB_BLANK_NORMAL        = VESA_NO_BLANKING + 1,
>
>         /* screen: blanked,   hsync: on,  vsync: off */
>         FB_BLANK_VSYNC_SUSPEND = VESA_VSYNC_SUSPEND + 1,
>
>         /* screen: blanked,   hsync: off, vsync: on */
>         FB_BLANK_HSYNC_SUSPEND = VESA_HSYNC_SUSPEND + 1,
>
>         /* screen: blanked,   hsync: off, vsync: off */
>         FB_BLANK_POWERDOWN     = VESA_POWERDOWN + 1
>
>
> In vesa.h we have:
> /* VESA Blanking Levels */
> enum vesa_blank_mode {
>         VESA_NO_BLANKING        = 0,
> #define VESA_NO_BLANKING        VESA_NO_BLANKING
>         VESA_VSYNC_SUSPEND      = 1,
> #define VESA_VSYNC_SUSPEND      VESA_VSYNC_SUSPEND
>         VESA_HSYNC_SUSPEND      = 2,
> #define VESA_HSYNC_SUSPEND      VESA_HSYNC_SUSPEND
>         VESA_POWERDOWN          = VESA_VSYNC_SUSPEND | 
> VESA_HSYNC_SUSPEND,
> #define VESA_POWERDOWN          VESA_POWERDOWN
>         VESA_BLANK_MAX          = VESA_POWERDOWN,
>
>
> for example this one:
>         /* screen: blanked,   hsync: on,  vsync: off */
>         FB_BLANK_VSYNC_SUSPEND = VESA_VSYNC_SUSPEND + 1,
> which is effectively:
>         FB_BLANK_VSYNC_SUSPEND = VESA_HSYNC_SUSPEND,
> which seems wrong.
>
> Am I missing something?

No worries, it's super-confusing.

- FB_BLANK_ describes CRTC state plus monitor state. VESA_ only 
describes monitor state and assumes that the CRTC is off.

- FB_BLANK_UNBLANK means "monitor on and send pixels".  There is no VESA 
equivalent for this mode, because all VESA constants assume that the 
stream if pixel data has been disabled.

- FB_BLANK_NORMAL means "monitor on, but do not send pixels". This 
corresponds to VESA_BLANK_NO_BLANKING, but has a different integer value.

- The FB_BLANK_*_SUSPEND and _POWERDOWN constants correspond to the VESA 
constants of similar name. But they are all off by one.

Therefore some of the drivers (au1100fb) use VESA_NO_BLANKING when they 
mean FB_BLANK_UNBLANK. Others (xilinxfb) get it wrong and use 
VESA_POWERDOWN when they should use FB_BLANK_POWERDOWN.

Best regards
Thomas


>
> Helge
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Stefan Gaiser, Jochen Jaser, Abhinav Puri, (HRB 36809, AG Nürnberg)