[PATCH] drm/amdgpu: only remove existing FBs for devices with displays

[Alex Deucher]

On Wed, Jan 17, 2024 at 2:42 AM Christian König
<ckoenig.leichtzumerken at gmail.com> wrote:
>
> Am 16.01.24 um 15:39 schrieb Alex Deucher:
> > Seems calling drm_aperture_remove_conflicting_pci_framebuffers()
> > will take away the apertures for unrelated devices on some kernel
> > versions.  E.g., calling this on a PCIe accelerator with no display
> > IP may take the apertures away from the actual PCIe display adapter
> > on the system, breaking efifb, depending on the kernel version.
> >
> > Just do this if there is display IP present.
>
> I would have checked the PCI device type instead, because a system BIOS
> most likely has no idea that a VGA device doesn't has a connector.

True, but we have GPUs of PCI class PCI_CLASS_DISPLAY_OTHER that can
have efifb bound to it.  Unfortunately, the combinations of PCI
classes and displays is complicated:

PCI_CLASS_DISPLAY_VGA - boards both with and without display hardware
PCI_CLASS_DISPLAY_OTHER - boards both with and without display hardware
PCI_CLASS_ACCELERATOR_PROCESSING - no display hardware

>
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++----
> >   1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 62772b58ef3d..353c38f008e8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4056,10 +4056,12 @@ int amdgpu_device_init(struct amdgpu_device *adev,
> >
> >       amdgpu_device_set_mcbp(adev);
> >
> > -     /* Get rid of things like offb */
> > -     r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, &amdgpu_kms_driver);
> > -     if (r)
> > -             return r;
> > +     if (amdgpu_device_ip_get_ip_block(adev, AMD_IP_BLOCK_TYPE_DCE)) {
>
> This certainly worth a comment why we do this.
>
> Apart from that I'm not sure we should upstream this, the customer
> kernel is most likely just missing this fix here:
>
> commit 5ae3716cfdcd286268133867f67d0803847acefc
> Author: Daniel Vetter <daniel.vetter at ffwll.ch>
> Date:   Thu Apr 6 15:21:07 2023 +0200
>
>      video/aperture: Only remove sysfb on the default vga pci device
>
>      Instead of calling aperture_remove_conflicting_devices() to remove the
>      conflicting devices, just call to aperture_detach_devices() to detach
>      the device that matches the same PCI BAR / aperture range. Since the
>      former is just a wrapper of the latter plus a sysfb_disable() call,
>      and now that's done in this function but only for the primary devices.
>
>      This fixes a regression introduced by commit ee7a69aa38d8 ("fbdev:
>      Disable sysfb device registration when removing conflicting FBs"),
>      where we remove the sysfb when loading a driver for an unrelated pci
>      device, resulting in the user losing their efifb console or similar.
>
>      Note that in practice this only is a problem with the nvidia blob,
>      because that's the only gpu driver people might install which does not
>      come with an fbdev driver of it's own. For everyone else the real gpu
>      driver will restore a working console.
>
>      Also note that in the referenced bug there's confusion that this same
>      bug also happens on amdgpu. But that was just another amdgpu specific
>      regression, which just happened to happen at roughly the same time and
>      with the same user-observable symptoms. That bug is fixed now, see
>      https://bugzilla.kernel.org/show_bug.cgi?id=216331#c15
>

yeah, that looks like the right fix.

Alex


> Regards,
> Christian.
>
> > +             /* Get rid of things like offb */
> > +             r = drm_aperture_remove_conflicting_pci_framebuffers(adev->pdev, &amdgpu_kms_driver);
> > +             if (r)
> > +                     return r;
> > +     }
> >
> >       /* Enable TMZ based on IP_VERSION */
> >       amdgpu_gmc_tmz_set(adev);
>