Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Javier Martinez Canillas <javierm@redhat.com>
To:     Pierre Asselin <pa@panix.com>
Cc:     Pierre Asselin <pa@panix.com>,
        Jocelyn Falempe <jfalempe@redhat.com>,
        Daniel Vetter <daniel.vetter@ffwll.ch>,
        linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        Hans de Goede <hdegoede@redhat.com>,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Ard Biesheuvel <ardb@kernel.org>
Subject: Re: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is
 calculated
In-Reply-To: <1afd3044c2aca9322ecf304941c7df66.squirrel@mail.panix.com>
References: <20230412150225.3757223-1-javierm@redhat.com>
 <2e07f818ccdff7023a060e732d7c4ef6.squirrel@mail.panix.com>
 <87jzyhror0.fsf@minerva.mail-host-address-is-not-set>
 <beeff0335ab4cc244d214a7baadba371.squirrel@mail.panix.com>
 <CAFOAJEdKBUg91pDmNYYw5xigUxjifBgOLz2YgD+xQ+WyEy=V2w@mail.gmail.com>
 <1afd3044c2aca9322ecf304941c7df66.squirrel@mail.panix.com>
Date:   Wed, 12 Apr 2023 23:04:15 +0200
Message-ID: <87fs94stgw.fsf@minerva.mail-host-address-is-not-set>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

"Pierre Asselin" <pa@panix.com> writes:

>> And can you share the "linelength=" print out from simplefb ?
>
> Okay.  Three cases, see below.
>
> Your patch tries to fix the stride, but what if it's the _depth_
> that's wrong ?  Grub sets the mode, the pre-regression kernel picks this:
>     format=x8r8g8b8, mode=1024x768x32, linelength=4096
>

Yes, it seems the VBE mode set by GRUB is x8r8g8b8. And the line length
calculation is also correct: 1024 * (32 / 8) = 4096.

> ========== Good ======================================================
> grub: gfxpayload=1024x768x24
> [    0.003333] Console: colour dummy device 128x48
> [    0.003333] printk: console [tty0] enabled
> [    0.417054] fbcon: Taking over console
> [    0.513399] pci 0000:01:05.0: vgaarb: setting as boot VGA device
> [    0.513431] pci 0000:01:05.0: vgaarb: bridge control possible
> [    0.513455] pci 0000:01:05.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [    0.513490] vgaarb: loaded
> [    3.337529] simple-framebuffer simple-framebuffer.0: framebuffer at
> 0xd8000000, 0x240000 bytes
> [    3.337567] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
> mode=1024x768x24, linelength=3072

This is also correct when GRUB sets it to r8g8b8, since the line length
is: 1024 * (24 / 8) = 3072.

> [    3.338000] Console: switching to colour frame buffer device 128x48
> [    3.566490] simple-framebuffer simple-framebuffer.0: fb0: simplefb
> registered!
>
> ========== Bad after patch, typing blind to log in !==================
> grub: gfxpayload=keep
> [    0.003333] Console: colour dummy device 128x48
> [    0.003333] printk: console [tty0] enabled
> [    0.423925] fbcon: Taking over console
> [    0.520030] pci 0000:01:05.0: vgaarb: setting as boot VGA device
> [    0.520061] pci 0000:01:05.0: vgaarb: bridge control possible
> [    0.520085] pci 0000:01:05.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [    0.520120] vgaarb: loaded
> [    3.290444] simple-framebuffer simple-framebuffer.0: framebuffer at
> 0xd8000000, 0x240000 bytes
> [    3.290483] simple-framebuffer simple-framebuffer.0: format=r8g8b8,
> mode=1024x768x24, linelength=3072

Now, this is the part where things start to break I believe. Because you
mentioned before that gfxpayload=keep used to set the format to xr8g8b8
but now after my patch (and also after the original commit f35cd3fa7729)
it is set to r8g8b8 instead.

That *shouldn't* be an issue because it only means that the alpha channel
is discarded but maybe it is an issue for your display controller?

By the way, in https://www.panix.com/~pa/linux-6.3-simplefb/selected-modes
that you shared before the gfxpayload=keep GRUB option used to also led to
the pixel format being set to r8g8b8 instead of xr8g8b8. The difference is
that in that output the line lenght didn't match the pixel format and size:

[    3.290596] simple-framebuffer simple-framebuffer.0: format=r8g8b8, mode=1024x768x24, linelength=4096

but after my patch you mentioned that is:

[    3.290483] simple-framebuffer simple-framebuffer.0: format=r8g8b8, mode=1024x768x24, linelength=3072

which at least matches, so in a way is an improvement (even when it still
doesn't work).

> [    3.290916] Console: switching to colour frame buffer device 128x48
> [    3.519523] simple-framebuffer simple-framebuffer.0: fb0: simplefb
> registered!
>
> ========== Good, earlier kernel before regression ====================
> grub: gfxpayload=keep
> [    0.226675] Console: colour dummy device 128x48
> [    0.228643] printk: console [tty0] enabled
> [    0.429214] fbcon: Taking over console
> [    0.524994] pci 0000:01:05.0: vgaarb: setting as boot VGA device
> [    0.525025] pci 0000:01:05.0: vgaarb: bridge control possible
> [    0.525049] pci 0000:01:05.0: vgaarb: VGA device added:
> decodes=io+mem,owns=io+mem,locks=none
> [    0.525082] vgaarb: loaded
> [    3.320474] simple-framebuffer simple-framebuffer.0: framebuffer at
> 0xd8000000, 0x300000 bytes
> [    3.320513] simple-framebuffer simple-framebuffer.0: format=x8r8g8b8,
> mode=1024x768x32, linelength=4096

Yes, and it works because is the correct mode it seems but for some reason
after commit f35cd3fa7729 the pixel format is calculated incorrectly. We
can see that the total framebuffer size is 0x300000 bytes, which matches a
1024x768x34 mode framebuffer: 1024 * 768 * (34 / 8) = 3342336 = 0x300000.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat