[Intel-gfx] [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

[Patrik Jakobsson]

On Thu, Apr 6, 2023 at 9:32 AM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Wed, 5 Apr 2023 at 19:46, Patrik Jakobsson
> <patrik.r.jakobsson at gmail.com> wrote:
> >
> > On Wed, Apr 5, 2023 at 7:15 PM Daniel Vetter <daniel at ffwll.ch> wrote:
> > >
> > > On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
> > > <javierm at redhat.com> wrote:
> > > >
> > > > Daniel Vetter <daniel at ffwll.ch> writes:
> > > >
> > > > > On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
> > > >
> > > > [...]
> > > >
> > > > >> > > >        /*
> > > > >> > > >         * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > > > >> > > >         * otherwise the vga fbdev driver falls over.
> > > > >> > > >         */
> > > > >> > > >        ret = vga_remove_vgacon(pdev);
> > > > >> >
> > > > >> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
> > > >
> > > > Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.
> > > >
> > > > [...]
> > > >
> > > > >> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> > > > >> {
> > > > >>      aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> > > > >>
> > > > >>      return vga_remove_vgacon(pdev);
> > > > >> }
> > > > >>
> > > > >> And that can be called from gma500 and the pci aperture helper.
> > > > >
> > > > > But you still pass a pci_dev to that helper. Which just doesn't make any
> > > > > sense to me (assuming your entire point is that this isn't just a normal
> > > > > pci device but some special legacy vga thing), but if we go with (void)
> > > > > then there's more refactoring to do because the vga_remove_vgacon also
> > > > > wants a pdev.
> > > > >
> > > > > All so that we don't call aperture_detach_devices() on a bunch of pci
> > > > > bars, which apparently is not problem for any other driver, but absolutely
> > > > > is a huge problem for gma500 somehow.
> > > > >
> > > > > I don't understand why.
> > > > >
> > > >
> > > > Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
> > > > is needed then starts to get a little silly. Maybe one option is to add a
> > > > 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
> > > > to iterate over PCI bars and call aperture_remove_conflicting_devices() ?
> > >
> > > The thing I don't get: Why does this matter for gma500 and not any of
> > > the other pci devices? Look at your gpu, realize there's a lot more
> > > than the one pci bar for vram or stolen memory, realize that we're
> > > nuking bars that cannot possible contain the framebuffer for everyone
> > > else too. Like the entire "gpus have a lot of bars" thing is the
> > > reason why I pulled the sysfb_disable one level up, because we've been
> > > doing that quite a few times before this patch (yes it's not the main
> > > thing, but the side-effect cleanup is why I've gone down this rabbit
> > > hole and wrote the entire series here):
> > >
> > > https://lore.kernel.org/dri-devel/20230404201842.567344-7-daniel.vetter@ffwll.ch/
> > >
> > > But somehow for gma500 it's a problem, while for everyone else it's
> > > fine. That's the part I dont get, or Thomas&me have been talking past
> > > each another and there's another issue that I'm missing.
> > > -Daniel
> >
> > I'm also getting confused here.
> >
> > AFAIK the stolen memory works the same for gma500 hardware as other
> > Intel GPUs. Are you saying that there is a difference in how gma500
> > hardware works? I always assumed that i915 got away with not dealing
> > much with stolen memory because it simply doesn't use it for
> > allocations. In gma500 we use it for fbdev and cursors. The actual
> > pages reserved by the bios can be accessed through a pci bar if you
> > map it so (which IIRC we do) but I suppose that doesn't help
> > identifying it as a range reserved by other drivers.
>
> The other integrated gpu have their fw fb behind a pci bar, and stolen
> is often entirely hidden from the cpu for direct access. gma500 seems
> different with having stolen as just a specially marked up range of
> normal system memory. That's why the usual pci helper doesn't catch
> everything for gma500.

Right, now I get it. You don't have the GATT on some systems so the
range can never be inside the aperture on those systems anyway.

The GATT probably went away because there is no need for it since you
don't get coherency with the PowerVR parts anyway.

Thanks for explaining

>
> > The reason I've kept the stolen allocation logic is because some
> > gma500 systems don't have a lot of memory. But that is mostly the old
> > Pouslbo systems. Perhaps it is time to ditch the stolen allocation
> > code?
>
> Yeah that's all fine.
> -Daniel
>
> >
> > -Patrik
> >
> > >
> > > > > Consider this me throwing in the towel. If you&Javier are convinced this
> > > > > makes sense please type it up and merge it, but I'm not going to type
> > > > > something that just doesn't make sense to me.
> > > >
> > > > Honestly, I would just go with the double drm_aperture_remove_*() helper
> > > > calls (your original patch) unless that causes real issues. There is no
> > > > point on blocking all your series just for this IMO.
> > > >
> > > > Then latter if Thomas has strong opinions can send a follow-up patch for
> > > > the gma500 driver and the aperture helpers.
> > > >
> > > > > -Daniel
> > > > >
> > > >
> > > > --
> > > > Best regards,
> > > >
> > > > Javier Martinez Canillas
> > > > Core Platforms
> > > > Red Hat
> > > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch