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

From: Thomas Zimmermann <tzimmermann@suse.de>
To: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Javier Martinez Canillas <javierm@redhat.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Date: Wed, 5 Apr 2023 10:15:57 +0200	[thread overview]
Message-ID: <f00a1032-51bc-e556-6c0a-475e3ea89eb3@suse.de> (raw)
In-Reply-To: <CAMeQTsYH=gMv--qoOpQEc8-ozsW6ocN6zhw=Mjjat3L_xw=vwA@mail.gmail.com>

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

Hi Patrik

Am 05.04.23 um 10:05 schrieb Patrik Jakobsson:
> On Wed, Apr 5, 2023 at 9:49 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>
>> 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.
> 
> Would it help if we figure out the stolen range here? It can
> supposedly be found by reading pci config space, so no need to map vdc
> regs first.
> 
> GBSM is the stolen base and TOLUD - GBSM = stolen size. Or read the
> size out from GGC. Not sure which one is more reliable.

See my other email here. We'd still need a nice interface for the 
aperture helpers.

Best regards
Thomas

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

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