Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
MIME-Version: 1.0
References: <20231113085305.1823455-1-javierm@redhat.com> <CAL_JsqKHTN5hfd4qpg5RXbmQLKZNVywDkSj9mxvfGmjrcChQQg@mail.gmail.com>
 <87jzqi59bt.fsf@minerva.mail-host-address-is-not-set> <CAL_JsqJM9+cbNviwuKGB5+3YbyAP3UH+TxCxsU5nUtX-iRGP2w@mail.gmail.com>
In-Reply-To: <CAL_JsqJM9+cbNviwuKGB5+3YbyAP3UH+TxCxsU5nUtX-iRGP2w@mail.gmail.com>
From:   Ard Biesheuvel <ardb@kernel.org>
Date:   Fri, 17 Nov 2023 00:30:51 +1000
Message-ID: <CAMj1kXG7Xyk0ys9j-XRo7Rr8gYz1qJE8fFSixBOwVbm-pjeX+A@mail.gmail.com>
Subject: Re: [RFC PATCH] of/platform: Disable sysfb if a simple-framebuffer
 node is found
To:     Rob Herring <robh@kernel.org>
Cc:     Javier Martinez Canillas <javierm@redhat.com>,
        linux-kernel@vger.kernel.org,
        Thomas Zimmermann <tzimmermann@suse.de>,
        Sima Vetter <daniel.vetter@ffwll.ch>,
        dri-devel@lists.freedesktop.org,
        Andrew Worsley <amworsley@gmail.com>,
        Hector Martin <marcan@marcan.st>,
        Sergio Lopez <slp@redhat.com>,
        Frank Rowand <frowand.list@gmail.com>,
        devicetree@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Fri, 17 Nov 2023 at 00:09, Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Nov 16, 2023 at 3:36=E2=80=AFAM Javier Martinez Canillas
> <javierm@redhat.com> wrote:
> >
> > Rob Herring <robh@kernel.org> writes:
> >
> > Hello Rob,
> >
> > > On Mon, Nov 13, 2023 at 2:53=E2=80=AFAM Javier Martinez Canillas
> > > <javierm@redhat.com> wrote:
> > >>
> > >> Some DT platforms use EFI to boot and in this case the EFI Boot Serv=
ices
> > >> may register a EFI_GRAPHICS_OUTPUT_PROTOCOL handle, that will later =
be
> > >> queried by the Linux EFI stub to fill the global struct screen_info =
data.
> > >>
> > >> The data is used by the Generic System Framebuffers (sysfb) framewor=
k to
> > >> add a platform device with platform data about the system framebuffe=
r.
> > >>
> > >> But if there is a "simple-framebuffer" node in the DT, the OF core w=
ill
> > >> also do the same and add another device for the system framebuffer.
> > >>
> > >> This could lead for example, to two platform devices ("simple-frameb=
uffer"
> > >> and "efi-framebuffer") to be added and matched with their correspond=
ing
> > >> drivers. So both efifb and simpledrm will be probed, leading to foll=
owing:
> > >>
> > >> [    0.055752] efifb: framebuffer at 0xbd58dc000, using 16000k, tota=
l 16000k
> > >> [    0.055755] efifb: mode is 2560x1600x32, linelength=3D10240, page=
s=3D1
> > >> [    0.055758] efifb: scrolling: redraw
> > >> [    0.055759] efifb: Truecolor: size=3D2:10:10:10, shift=3D30:20:10=
:0
> > >> ...
> > >> [    3.295896] simple-framebuffer bd58dc000.framebuffer: [drm] *ERRO=
R*
> > >> could not acquire memory range [??? 0xffff79f30a29ee40-0x2a5000001a7
> > >> flags 0x0]: -16
> > >> [    3.298018] simple-framebuffer: probe of bd58dc000.framebuffer
> > >> failed with error -16
> > >>
> > >> To prevent the issue, make the OF core to disable sysfb if there is =
a node
> > >> with a "simple-framebuffer" compatible. That way only this device wi=
ll be
> > >> registered and sysfb would not attempt to register another one using=
 the
> > >> screen_info data even if this has been filled.
> > >>
> > >> This seems the correct thing to do in this case because:
> > >>
> > >> a) On a DT platform, the DTB is the single source of truth since is =
what
> > >>    describes the hardware topology. Even if EFI Boot Services are us=
ed to
> > >>    boot the machine.
> > >
> > > This is the opposite of what we do for memory and memory reservations=
.
> > > EFI is the source of truth for those.
> > >
> > > This could also lead to an interesting scenario. As simple-framebuffe=
r
> > > can define its memory in a /reserved-memory node, but that is ignored
> > > in EFI boot. Probably would work, but only because EFI probably
> > > generates its memory map table from the /reserved-memory nodes.
> > >
> >
> > I see. So what would be the solution then? Ignoring creating a platform
> > device for "simple-framebuffer" if booted using EFI and have an EFI-GOP=
?
>
> Shrug. I don't really know anything more about EFI FB, but I would
> guess it can't support handling resources like clocks, power domains,
> regulators, etc. that simple-fb can. So if a platform needs those, do
> we say they should not setup EFI-GOP? Or is there a use case for
> having both? Clients that don't muck with resources can use EFI-GOP
> and those that do use simple-fb. For example, does/can grub use
> EFI-GOP, but not simple-fb?
>

The EFI GOP is just a dumb framebuffer, and it is not even generally
possible to cross reference the GOP with a particular device in the
device hierarchy unless you e.g., compare the BARs of each device with
the region described by the GOP protocol.

GRUB for EFI will use the GOP and nothing else, but only at boot time
(the GOP protocol is more than a magic linear memory region, it also
implements a Blt() abstraction that permits the use of framebuffers
that are not mapped linearly into the address space at all, and GRUB
makes use of this)

The EFI stub will only expose GOPs to the kernel if they are in fact
linear framebuffers, but has zero insight into whether the hardware
needs clocks and regulators, and whether or not the framebuffer needs
IOMMU pass through (which might be the case if the scanout is using
DMA into system memory)

So calling EFI GOP 'source of truth' is rather generous, and I think
it makes sense to prioritize more accurate descriptions of the
underlying framebuffer over EFI GOP.

However, making 'simple-framebuffer' magic in this regard doesn't seem
like a great approach to me. Is there a better way we could get the
resource conflict to be decided in a way where the EFI GOP gets
superseded if its resources are claimed by another device?