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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Javier Martinez Canillas <javierm@redhat.com>
Subject: Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Date: Wed, 5 Apr 2023 10:07:54 +0200	[thread overview]
Message-ID: <3813a2f5-c74a-4760-34ce-1c88f187c91c@suse.de> (raw)
In-Reply-To: <5556a755-01a1-3620-8693-0fc69c6f627d@suse.de>

[-- Attachment #1.1: Type: text/plain, Size: 3566 bytes --]

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@intel.com>
>> Cc: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: Javier Martinez Canillas <javierm@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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]