[PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

Wed Apr 5 09:26:51 UTC 2023

Hi

Am 05.04.23 um 10:59 schrieb 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

It's not "both at the same time." It like an SoC that can act as VGA. 
But it's not really a PCI graphics card on its own. Maybe that's just 
nitpicking, though.

> 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.

Yes. If you don't want a new _pci_devices_at() aperture helper, please 
duplicate the _pci_devices() helper within gma500 (with its sysfb and 
vgacon handling). Then let it take the gma500 memory range where the 
generic _pci() helper iterates over PCI BARs.

This would mark gma500 as special, while clearly communicating what's 
going on.

> 
> 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.

Your stated goal is to simplify the aperture interface and make it 
harder to misuse. But the reasoning behind the new code in gma500 is not 
understandable without following the discussion closely or without 
knowing the hardware. Remember that your first iteration of this patch 
actually got it wrong, because gma500 is different. So there should be 
one aperture call here that does the right thing for gma500.

Best regards
Thomas

> -Daniel

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230405/ab9510f0/attachment.sig>