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

[Thomas Zimmermann]

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.

Best regards
Thomas

> 
> Best regards
> Thomas
> 
>>       if (ret)
>>           return ret;
> 

-- 
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/ef5beaa5/attachment.sig>