On Wed, 5 Apr 2023 at 19:46, Patrik Jakobsson
<patrik.r.jakobsson@xxxxxxxxx> wrote:
>
> On Wed, Apr 5, 2023 at 7:15 PM Daniel Vetter <daniel@xxxxxxxx> wrote:
> >
> > On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
> > <javierm@xxxxxxxxxx> wrote:
> > >
> > > Daniel Vetter <daniel@xxxxxxxx> 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@xxxxxxxx/
> >
> > 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.
> 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
