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

[Daniel Vetter]

On Wed, Apr 05, 2023 at 10:07:54AM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.04.23 um 09:49 schrieb Thomas Zimmermann:
> > Hi
> > 
> > Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> > > This one nukes all framebuffers, which is a bit much. In reality
> > > gma500 is igpu and never shipped with anything discrete, so there should
> > > not be any difference.
> > > 
> > > v2: Unfortunately the framebuffer sits outside of the pci bars for
> > > gma500, and so only using the pci helpers won't be enough. Otoh if we
> > > only use non-pci helper, then we don't get the vga handling, and
> > > subsequent refactoring to untangle these special cases won't work.
> > > 
> > > It's not pretty, but the simplest fix (since gma500 really is the only
> > > quirky pci driver like this we have) is to just have both calls.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> > > Cc: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
> > > Cc: Thomas Zimmermann <tzimmermann at suse.de>
> > > Cc: Javier Martinez Canillas <javierm at redhat.com>
> > > ---
> > >   drivers/gpu/drm/gma500/psb_drv.c | 9 +++++++--
> > >   1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/gma500/psb_drv.c
> > > b/drivers/gpu/drm/gma500/psb_drv.c
> > > index 2ce96b1b9c74..f1e0eed8fea4 100644
> > > --- a/drivers/gpu/drm/gma500/psb_drv.c
> > > +++ b/drivers/gpu/drm/gma500/psb_drv.c
> > > @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev,
> > > const struct pci_device_id *ent)
> > >       /*
> > >        * We cannot yet easily find the framebuffer's location in
> > > memory. So
> > > -     * remove all framebuffers here.
> > > +     * remove all framebuffers here. Note that we still want the
> > > pci special
> > > +     * handling to kick out vgacon.
> > >        *
> > >        * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
> > >        *       might be able to read the framebuffer range from the
> > > device.
> > >        */
> > > -    ret = drm_aperture_remove_framebuffers(true, &driver);
> > > +    ret = drm_aperture_remove_framebuffers(false, &driver);
> > > +    if (ret)
> > > +        return ret;
> > > +
> > > +    ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
> > > &driver);
> > 
> > This simply isn't it. If you have to work around your own API, it's time
> > to rethink the API.
> 
> Here's a proposal:
> 
>  1) As you're changing aperture_remove_conflicting_devices() anyway, rename
> it to aperture_remove_conflicting_devices_at(), so it's clear that it takes
> a memory range.
> 
>  2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a
> PCI device and a memory range. It should do the is_primary and vgacon stuff,
> but kick out the range.
> 
>  3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with the
> full range. Even if we can restructure gma500 to detect the firmware
> framebuffer from its registers (there's this TODO item), we'd still need
> aperture_remove_conflicting_pci_devices_at() to do something useful with it.
> 
>  4) With that, aperture_remove_conflicting_devices_at() can drop the primary
> argument.
> 
> Of course, the DRM-related interface should be adapted as well. There's a
> bit of overlap in the implementation of both PCI aperture helpers, but the
> overall interface is clear.

This essentially just gives us a helper which does the above two
open-coded steps but all wrapped up. For gma500 only. Also I really don't
think I'm working around the api here, it's gma500 which is special:

- Normal pci devices all have their fw framebuffer within the memory bars,
  never outside. So for those the pci version is the right interface.

- If the framebuffer is outside of the pci bar then it's just not really a
  pci vga device anymore, but looks a lot more like a SoC design.

gma500 is somehow both at the same time, so it gets two calls. And having
both calls explicitly I think is better, because it highlights the dual
nature of gma500 of being both a pci vga devices and a SoC embedded
device. Trying to make a wrapper to hide this dual nature just so we have
a single call still seems worse to me. Aside from it's just boilerplate
for no gain.

Frankly at that point I think it would be clearer to call the gma500
function remove_conflicting_gma500 or something like that. Or at least
remove_conflicting_pci_and_separate_range_at.

This is imo similar to the hypothetical case of a SoC chip which also
happens to decode legacy vga, without being a pci device. We could add a
new interface function which just nukes the vga stuff (without the pci
device tie-in, maybe with some code sharing internally in aperture.c), and
then that driver does 2 calls: 1. nuke aperture range 2. nuke vga stuff.
And sure if you have a lot of those maybe you could make a helper to safe
a few lines of code, but semantically it's still two different things
your're removing.

Or another case: A pci device with 2 subfunctions, each a gpu device. This
happened with dual-head gpus 20 years ago because windows 2000 insisted
that each crtc needs its own pci function. You'd just call the pci removal
twice for that too (except not relevant because bios fw never figured out
how to enable both heads, so just nuking the first one is good enough).

iow, I don't understand why you think this is the wrong api. There's no
rule that a driver must be able remove all conflicting fw drivers in a
single call, if it's two things we need to nuke it can be two calls.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch