[1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers

Commit Message

Daniel Vetter April 4, 2023, 8:18 p.m. UTC

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 <[e-mail]>
Cc: Patrik Jakobsson <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: Javier Martinez Canillas <[e-mail]>
---
 drivers/gpu/drm/gma500/psb_drv.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Thomas Zimmermann April 5, 2023, 7:49 a.m. UTC | #1

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 <[e-mail]>
> Cc: Patrik Jakobsson <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Javier Martinez Canillas <[e-mail]>
> ---
>   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.

Best regards
Thomas

>   	if (ret)
>   		return ret;
>

Patrik Jakobsson April 5, 2023, 8:05 a.m. UTC | #2

On Wed, Apr 5, 2023 at 9:49 AM Thomas Zimmermann <[e-mail]> 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 <[e-mail]>
> > Cc: Patrik Jakobsson <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: Javier Martinez Canillas <[e-mail]>
> > ---
> >   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.

-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 April 5, 2023, 8:07 a.m. UTC | #3

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 <[e-mail]>
>> Cc: Patrik Jakobsson <[e-mail]>
>> Cc: Thomas Zimmermann <[e-mail]>
>> Cc: Javier Martinez Canillas <[e-mail]>
>> ---
>>   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 April 5, 2023, 8:15 a.m. UTC | #4

Hi Patrik

Am 05.04.23 um 10:05 schrieb Patrik Jakobsson:
> On Wed, Apr 5, 2023 at 9:49 AM Thomas Zimmermann <[e-mail]> 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 <[e-mail]>
>>> Cc: Patrik Jakobsson <[e-mail]>
>>> Cc: Thomas Zimmermann <[e-mail]>
>>> Cc: Javier Martinez Canillas <[e-mail]>
>>> ---
>>>    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 April 5, 2023, 8:19 a.m. UTC | #5

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.

I do own an Intel DN2800MT board with gma500 hardware. [1] It has a PCIe 
x1 slot. I never tried, but in principle, there could be another 
graphics card in the system. The linked spec say 'Discrete: None'. I 
don't know what that means exactly.

Best regards
Thomas

[1] 
https://ark.intel.com/content/www/us/en/ark/products/56455/intel-desktop-board-dn2800mt.html

> 
> 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 <[e-mail]>
> Cc: Patrik Jakobsson <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Javier Martinez Canillas <[e-mail]>
> ---
>   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);
>   	if (ret)
>   		return ret;
>

Daniel Vetter April 5, 2023, 8:59 a.m. UTC | #6

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 <[e-mail]>
> > > Cc: Patrik Jakobsson <[e-mail]>
> > > Cc: Thomas Zimmermann <[e-mail]>
> > > Cc: Javier Martinez Canillas <[e-mail]>
> > > ---
> > >   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 April 5, 2023, 9:09 a.m. UTC | #7

On Wed, Apr 05, 2023 at 10:19:55AM +0200, Thomas Zimmermann 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.
> 
> I do own an Intel DN2800MT board with gma500 hardware. [1] It has a PCIe x1
> slot. I never tried, but in principle, there could be another graphics card
> in the system. The linked spec say 'Discrete: None'. I don't know what that
> means exactly.

Well even if that's the case, I'm not making the situation worse. Because
the old code also nuked everything. The new one at least only nukes the
vga if gma500 is decoding that, and not the the discrete card. In practice
it won't help, because I don't think you'll boot this in legacy vga mode
with vga16fb :-)
-Daniel

> 
> Best regards
> Thomas
> 
> [1] https://ark.intel.com/content/www/us/en/ark/products/56455/intel-desktop-board-dn2800mt.html
> 
> > 
> > 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 <[e-mail]>
> > Cc: Patrik Jakobsson <[e-mail]>
> > Cc: Thomas Zimmermann <[e-mail]>
> > Cc: Javier Martinez Canillas <[e-mail]>
> > ---
> >   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);
> >   	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 April 5, 2023, 9:26 a.m. UTC | #8

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 <[e-mail]>
>>>> Cc: Patrik Jakobsson <[e-mail]>
>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>> Cc: Javier Martinez Canillas <[e-mail]>
>>>> ---
>>>>    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

Daniel Vetter April 5, 2023, 9:38 a.m. UTC | #9

On Wed, Apr 05, 2023 at 11:26:51AM +0200, Thomas Zimmermann wrote:
> 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 <[e-mail]>
> > > > > Cc: Patrik Jakobsson <[e-mail]>
> > > > > Cc: Thomas Zimmermann <[e-mail]>
> > > > > Cc: Javier Martinez Canillas <[e-mail]>
> > > > > ---
> > > > >    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.

I don't see why it can't be a pci vga card. There is no requirement that a
pci vga card must be also a non-vga card with real non-vga framebuffer. We
don't have a hole lot of them really.

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

The thing is, pci is self-describing. We don't need to open code every
variant in every driver, the pci code can figure out in a generic way
whether vga needs to be nuked or not. That's the entire point of this
refactoring.

Also note that we nuke all bars, and on most pci cards that will include a
bunch of mmio bars which will never ever hold a framebuffer. And the old
per-driver open-coded version ensured that we only nuked the pci bar that
could potentially contain the framebuffer.

Why is gma500 special and it needs to be the only pci driver where we
intentionally filter out all the bars that wont ever contain a
framebuffer? If this is your argument, the entire series is toast, not
just the gma500 part.

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

I didn't know about the dual-nature of gma500. Which is why I added a
comment to explain what's going on, since at first glance it just looked
like someone didn't bother to implement the open-coded pci conflicting
driver removal correctly. It's by far not the only driver that was sloppy,
a bunch did not compute the primary flag correctly at least. Maybe I
overlooked some really funny special case there too?

Also I think my goal fits, because if we'd have had the newly proposed
helpers, then gma500 would have needed to have the two calls + comments
from the start. Which would have helped me to realize that this is indeed
intentionally special and not accidentally buggy.

As-is, without the obvious special case in code or some comment or digging
around elsewhere it just looks buggy.
-Daniel

Thomas Zimmermann April 5, 2023, 11 a.m. UTC | #10

Hi

Am 05.04.23 um 11:38 schrieb Daniel Vetter:
> On Wed, Apr 05, 2023 at 11:26:51AM +0200, Thomas Zimmermann wrote:
>> 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 <[e-mail]>
>>>>>> Cc: Patrik Jakobsson <[e-mail]>
>>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>>> Cc: Javier Martinez Canillas <[e-mail]>
>>>>>> ---
>>>>>>     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.
> 
> I don't see why it can't be a pci vga card. There is no requirement that a
> pci vga card must be also a non-vga card with real non-vga framebuffer. We
> don't have a hole lot of them really.
> 
>>> 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.
> 
> The thing is, pci is self-describing. We don't need to open code every
> variant in every driver, the pci code can figure out in a generic way
> whether vga needs to be nuked or not. That's the entire point of this
> refactoring.
> 
> Also note that we nuke all bars, and on most pci cards that will include a
> bunch of mmio bars which will never ever hold a framebuffer. And the old
> per-driver open-coded version ensured that we only nuked the pci bar that
> could potentially contain the framebuffer.

I never talked about about PCI. I'm saying that the current code is not 
easy to understand.

> 
> Why is gma500 special and it needs to be the only pci driver where we
> intentionally filter out all the bars that wont ever contain a
> framebuffer? If this is your argument, the entire series is toast, not
> just the gma500 part.
> 
>>> 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.
> 
> I didn't know about the dual-nature of gma500. Which is why I added a
> comment to explain what's going on, since at first glance it just looked
> like someone didn't bother to implement the open-coded pci conflicting
> driver removal correctly. It's by far not the only driver that was sloppy,

I know. And I think you've added the same problem again in a different 
look. I expect that the next guy who looks at this code will again think 
that someone messed up the open coded PCI handling.

Your comment says that it calls a PCI function to clean up to vgacon. 
That comment explains what is happening, not why. And how the PCI and 
vgacon code work together is non-obvious.

Again, here's my proposal for gma500:

// call this from psb_pci_probe()
int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
					struct drm_driver *req_driver)
{
	resource_size_t base = 0;
	resource_size_t size = (resource_size_t)-1;
	const char *name = req_driver->name;
	int ret;

	/*
	 * We cannot yet easily find the framebuffer's location in
	 * memory. So remove all framebuffers here.
	 *
	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
	 *       we might be able to read the framebuffer range from the
	 *       device.
	 */
	ret = aperture_remove_conflicting_devices(base, size, name);
	if (ret)
		return ret;

	/*
	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
	 * otherwise the vga fbdev driver falls over.
	 */
	ret = vga_remove_vgacon(pdev);
	if (ret)
		return ret;

	return 0;
}

Best regards
Thomas

> a bunch did not compute the primary flag correctly at least. Maybe I
> overlooked some really funny special case there too?
> 
> Also I think my goal fits, because if we'd have had the newly proposed
> helpers, then gma500 would have needed to have the two calls + comments
> from the start. Which would have helped me to realize that this is indeed
> intentionally special and not accidentally buggy.
> 
> As-is, without the obvious special case in code or some comment or digging
> around elsewhere it just looks buggy.
> -Daniel

Javier Martinez Canillas April 5, 2023, 11:16 a.m. UTC | #11

Thomas Zimmermann <[e-mail]> writes:

[...]

>
> Your comment says that it calls a PCI function to clean up to vgacon. 
> That comment explains what is happening, not why. And how the PCI and 
> vgacon code work together is non-obvious.
>
> Again, here's my proposal for gma500:
>
> // call this from psb_pci_probe()
> int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
> 					struct drm_driver *req_driver)
> {
> 	resource_size_t base = 0;
> 	resource_size_t size = (resource_size_t)-1;
> 	const char *name = req_driver->name;
> 	int ret;
>
> 	/*
> 	 * We cannot yet easily find the framebuffer's location in
> 	 * memory. So remove all framebuffers here.
> 	 *
> 	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
> 	 *       we might be able to read the framebuffer range from the
> 	 *       device.
> 	 */
> 	ret = aperture_remove_conflicting_devices(base, size, name);
> 	if (ret)
> 		return ret;
>
> 	/*
> 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> 	 * otherwise the vga fbdev driver falls over.
> 	 */
> 	ret = vga_remove_vgacon(pdev);
> 	if (ret)
> 		return ret;
>
> 	return 0;
> }
>

If this is enough I agree that is much more easier code to understand.

Daniel Vetter April 5, 2023, 1:18 p.m. UTC | #12

On Wed, Apr 05, 2023 at 01:16:27PM +0200, Javier Martinez Canillas wrote:
> Thomas Zimmermann <[e-mail]> writes:
> 
> [...]
> 
> >
> > Your comment says that it calls a PCI function to clean up to vgacon. 
> > That comment explains what is happening, not why. And how the PCI and 
> > vgacon code work together is non-obvious.

Would a better comment help then:

	/*
	 * gma500 is a strange hybrid device, which both acts as a pci
	 * device (for legacy vga functionality) but also more like an
	 * integrated display on a SoC where the framebuffer simply
	 * resides in main memory and not in a special pci bar (that
	 * internally redirects to a stolen range of main memory) like all
	 * other integrated pci display devices have.
	 *
	 * To catch all cases we need to both remove conflicting fw
	 * drivers for the pci device and main memory.
	 */
> >
> > Again, here's my proposal for gma500:
> >
> > // call this from psb_pci_probe()
> > int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
> > 					struct drm_driver *req_driver)
> > {
> > 	resource_size_t base = 0;
> > 	resource_size_t size = (resource_size_t)-1;
> > 	const char *name = req_driver->name;
> > 	int ret;
> >
> > 	/*
> > 	 * We cannot yet easily find the framebuffer's location in
> > 	 * memory. So remove all framebuffers here.
> > 	 *
> > 	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
> > 	 *       we might be able to read the framebuffer range from the
> > 	 *       device.
> > 	 */
> > 	ret = aperture_remove_conflicting_devices(base, size, name);

Why can't this be a call to drm_aperture_remove_framebuffers? At least as
long as we don't implement the "read out actual fb base and size" code,
which also none of the other soc drivers bother with?

> > 	if (ret)
> > 		return ret;
> >
> > 	/*
> > 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > 	 * otherwise the vga fbdev driver falls over.
> > 	 */
> > 	ret = vga_remove_vgacon(pdev);

This isn't enough, we also nuke stuff that's mapping the vga fb range.
Which is really the reason I don't want to open code random stuff, pci is
self-describing, if it's decoding legacy vga it can figure this out and we
only have to implement the "how do I nuke legacy vga fw drivers from a pci
driver" once.

Not twice like this would result in, with the gma500 version being only
half the thing.

If it absolutely has to be a separate function for the gma500 pci legacy
vga (I still don't get why, it's just a pci vga device, there's absolutely
nothing special about that part at all) then I think it needs to be at
least a common "nuke a legacy vga device for me pls" function, which
shares the implementation with the pci one.

But not open-coding just half of it only.

> > 	if (ret)
> > 		return ret;
> >
> > 	return 0;
> > }
> >
> 
> If this is enough I agree that is much more easier code to understand.

It's still two calls and more code with more bugs? I'm not seeing the
point.
-Daniel

Thomas Zimmermann April 5, 2023, 2:32 p.m. UTC | #13

Hi

Am 05.04.23 um 15:18 schrieb Daniel Vetter:
> On Wed, Apr 05, 2023 at 01:16:27PM +0200, Javier Martinez Canillas wrote:
>> Thomas Zimmermann <[e-mail]> writes:
>>
>> [...]
>>
>>>
>>> Your comment says that it calls a PCI function to clean up to vgacon.
>>> That comment explains what is happening, not why. And how the PCI and
>>> vgacon code work together is non-obvious.
> 
> Would a better comment help then:
> 
> 	/*
> 	 * gma500 is a strange hybrid device, which both acts as a pci
> 	 * device (for legacy vga functionality) but also more like an
> 	 * integrated display on a SoC where the framebuffer simply
> 	 * resides in main memory and not in a special pci bar (that
> 	 * internally redirects to a stolen range of main memory) like all
> 	 * other integrated pci display devices have.
> 	 *
> 	 * To catch all cases we need to both remove conflicting fw
> 	 * drivers for the pci device and main memory.
> 	 */

Together with the existing comment, this should be the comment to 
describe gma_remove_conflicting_framebuffers().

>>>
>>> Again, here's my proposal for gma500:
>>>
>>> // call this from psb_pci_probe()
>>> int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
>>> 					struct drm_driver *req_driver)
>>> {
>>> 	resource_size_t base = 0;
>>> 	resource_size_t size = (resource_size_t)-1;
>>> 	const char *name = req_driver->name;
>>> 	int ret;
>>>
>>> 	/*
>>> 	 * We cannot yet easily find the framebuffer's location in
>>> 	 * memory. So remove all framebuffers here.
>>> 	 *
>>> 	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
>>> 	 *       we might be able to read the framebuffer range from the
>>> 	 *       device.
>>> 	 */
>>> 	ret = aperture_remove_conflicting_devices(base, size, name);
> 
> Why can't this be a call to drm_aperture_remove_framebuffers? At least as
> long as we don't implement the "read out actual fb base and size" code,
> which also none of the other soc drivers bother with?

It can. Feel free to use it.

But I have to say that those DRM helpers are somewhat empty and obsolete 
after the aperture code has been moved to drivers/video/. They exist 
mostly for convenience. As with other DRM helpers, if a driver needs 
something special, it can ignore them.

> 
>>> 	if (ret)
>>> 		return ret;
>>>
>>> 	/*
>>> 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
>>> 	 * otherwise the vga fbdev driver falls over.
>>> 	 */
>>> 	ret = vga_remove_vgacon(pdev);
> 
> This isn't enough, we also nuke stuff that's mapping the vga fb range.
> Which is really the reason I don't want to open code random stuff, pci is
> self-describing, if it's decoding legacy vga it can figure this out and we
> only have to implement the "how do I nuke legacy vga fw drivers from a pci
> driver" once.

Sure, but it's really just one additional line:

   aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);

as you mention below, this and vgacon can be exported in a single VGA 
aperture helper.

> 
> Not twice like this would result in, with the gma500 version being only
> half the thing.
> 
> If it absolutely has to be a separate function for the gma500 pci legacy
> vga (I still don't get why, it's just a pci vga device, there's absolutely
> nothing special about that part at all) then I think it needs to be at
> least a common "nuke a legacy vga device for me pls" function, which
> shares the implementation with the pci one.

Sure

/**
  * kerneldoc goes here
  *
  * WARNING: Apparently we must remove graphics drivers before calling
  *          this helper. Otherwise the vga fbdev driver falls over if
  *          we have vgacon configured.
  */
int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
{
	aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);

	return vga_remove_vgacon(pdev);
}

And that can be called from gma500 and the pci aperture helper.

Best regards
Thomas

> 
> But not open-coding just half of it only.
> 
>>> 	if (ret)
>>> 		return ret;
>>>
>>> 	return 0;
>>> }
>>>
>>
>> If this is enough I agree that is much more easier code to understand.
> 
> It's still two calls and more code with more bugs? I'm not seeing the
> point.
> -Daniel

Daniel Vetter April 5, 2023, 4:02 p.m. UTC | #14

On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 05.04.23 um 15:18 schrieb Daniel Vetter:
> > On Wed, Apr 05, 2023 at 01:16:27PM +0200, Javier Martinez Canillas wrote:
> > > Thomas Zimmermann <[e-mail]> writes:
> > > 
> > > [...]
> > > 
> > > > 
> > > > Your comment says that it calls a PCI function to clean up to vgacon.
> > > > That comment explains what is happening, not why. And how the PCI and
> > > > vgacon code work together is non-obvious.
> > 
> > Would a better comment help then:
> > 
> > 	/*
> > 	 * gma500 is a strange hybrid device, which both acts as a pci
> > 	 * device (for legacy vga functionality) but also more like an
> > 	 * integrated display on a SoC where the framebuffer simply
> > 	 * resides in main memory and not in a special pci bar (that
> > 	 * internally redirects to a stolen range of main memory) like all
> > 	 * other integrated pci display devices have.
> > 	 *
> > 	 * To catch all cases we need to both remove conflicting fw
> > 	 * drivers for the pci device and main memory.
> > 	 */
> 
> Together with the existing comment, this should be the comment to describe
> gma_remove_conflicting_framebuffers().
> 
> > > > 
> > > > Again, here's my proposal for gma500:
> > > > 
> > > > // call this from psb_pci_probe()
> > > > int gma_remove_conflicting_framebuffers(struct pci_dev *pdev, const
> > > > 					struct drm_driver *req_driver)
> > > > {
> > > > 	resource_size_t base = 0;
> > > > 	resource_size_t size = (resource_size_t)-1;
> > > > 	const char *name = req_driver->name;
> > > > 	int ret;
> > > > 
> > > > 	/*
> > > > 	 * We cannot yet easily find the framebuffer's location in
> > > > 	 * memory. So remove all framebuffers here.
> > > > 	 *
> > > > 	 * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
> > > > 	 *       we might be able to read the framebuffer range from the
> > > > 	 *       device.
> > > > 	 */
> > > > 	ret = aperture_remove_conflicting_devices(base, size, name);
> > 
> > Why can't this be a call to drm_aperture_remove_framebuffers? At least as
> > long as we don't implement the "read out actual fb base and size" code,
> > which also none of the other soc drivers bother with?
> 
> It can. Feel free to use it.
> 
> But I have to say that those DRM helpers are somewhat empty and obsolete
> after the aperture code has been moved to drivers/video/. They exist mostly
> for convenience. As with other DRM helpers, if a driver needs something
> special, it can ignore them.
> 
> > 
> > > > 	if (ret)
> > > > 		return ret;
> > > > 
> > > > 	/*
> > > > 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > > > 	 * otherwise the vga fbdev driver falls over.
> > > > 	 */
> > > > 	ret = vga_remove_vgacon(pdev);
> > 
> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
> > Which is really the reason I don't want to open code random stuff, pci is
> > self-describing, if it's decoding legacy vga it can figure this out and we
> > only have to implement the "how do I nuke legacy vga fw drivers from a pci
> > driver" once.
> 
> Sure, but it's really just one additional line:
> 
>   aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> 
> as you mention below, this and vgacon can be exported in a single VGA
> aperture helper.
> 
> > 
> > Not twice like this would result in, with the gma500 version being only
> > half the thing.
> > 
> > If it absolutely has to be a separate function for the gma500 pci legacy
> > vga (I still don't get why, it's just a pci vga device, there's absolutely
> > nothing special about that part at all) then I think it needs to be at
> > least a common "nuke a legacy vga device for me pls" function, which
> > shares the implementation with the pci one.
> 
> Sure
> 
> /**
>  * kerneldoc goes here
>  *
>  * WARNING: Apparently we must remove graphics drivers before calling
>  *          this helper. Otherwise the vga fbdev driver falls over if
>  *          we have vgacon configured.
>  */
> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> {
> 	aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> 
> 	return vga_remove_vgacon(pdev);
> }
> 
> And that can be called from gma500 and the pci aperture helper.

But you still pass a pci_dev to that helper. Which just doesn't make any
sense to me (assuming your entire point is that this isn't just a normal
pci device but some special legacy vga thing), but if we go with (void)
then there's more refactoring to do because the vga_remove_vgacon also
wants a pdev.

All so that we don't call aperture_detach_devices() on a bunch of pci
bars, which apparently is not problem for any other driver, but absolutely
is a huge problem for gma500 somehow.

I don't understand why.

Consider this me throwing in the towel. If you&Javier are convinced this
makes sense please type it up and merge it, but I'm not going to type
something that just doesn't make sense to me.
-Daniel

> Best regards
> Thomas
> 
> > 
> > But not open-coding just half of it only.
> > 
> > > > 	if (ret)
> > > > 		return ret;
> > > > 
> > > > 	return 0;
> > > > }
> > > > 
> > > 
> > > If this is enough I agree that is much more easier code to understand.
> > 
> > It's still two calls and more code with more bugs? I'm not seeing the
> > point.
> > -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

Javier Martinez Canillas April 5, 2023, 4:54 p.m. UTC | #15

Daniel Vetter <[e-mail]> writes:

> On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:

[...]

>> > > > 	/*
>> > > > 	 * WARNING: Apparently we must kick fbdev drivers before vgacon,
>> > > > 	 * otherwise the vga fbdev driver falls over.
>> > > > 	 */
>> > > > 	ret = vga_remove_vgacon(pdev);
>> > 
>> > This isn't enough, we also nuke stuff that's mapping the vga fb range.

Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.

[...]

>> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
>> {
>> 	aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
>> 
>> 	return vga_remove_vgacon(pdev);
>> }
>> 
>> And that can be called from gma500 and the pci aperture helper.
>
> But you still pass a pci_dev to that helper. Which just doesn't make any
> sense to me (assuming your entire point is that this isn't just a normal
> pci device but some special legacy vga thing), but if we go with (void)
> then there's more refactoring to do because the vga_remove_vgacon also
> wants a pdev.
>
> All so that we don't call aperture_detach_devices() on a bunch of pci
> bars, which apparently is not problem for any other driver, but absolutely
> is a huge problem for gma500 somehow.
>
> I don't understand why.
>

Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
is needed then starts to get a little silly. Maybe one option is to add a
3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
to iterate over PCI bars and call aperture_remove_conflicting_devices() ?

> Consider this me throwing in the towel. If you&Javier are convinced this
> makes sense please type it up and merge it, but I'm not going to type
> something that just doesn't make sense to me.

Honestly, I would just go with the double drm_aperture_remove_*() helper
calls (your original patch) unless that causes real issues. There is no
point on blocking all your series just for this IMO.

Then latter if Thomas has strong opinions can send a follow-up patch for
the gma500 driver and the aperture helpers.

> -Daniel
>

Daniel Vetter April 5, 2023, 5:14 p.m. UTC | #16

On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
<[e-mail]> wrote:
>
> Daniel Vetter <[e-mail]> writes:
>
> > On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
>
> [...]
>
> >> > > >        /*
> >> > > >         * WARNING: Apparently we must kick fbdev drivers before vgacon,
> >> > > >         * otherwise the vga fbdev driver falls over.
> >> > > >         */
> >> > > >        ret = vga_remove_vgacon(pdev);
> >> >
> >> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
>
> Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.
>
> [...]
>
> >> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> >> {
> >>      aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> >>
> >>      return vga_remove_vgacon(pdev);
> >> }
> >>
> >> And that can be called from gma500 and the pci aperture helper.
> >
> > But you still pass a pci_dev to that helper. Which just doesn't make any
> > sense to me (assuming your entire point is that this isn't just a normal
> > pci device but some special legacy vga thing), but if we go with (void)
> > then there's more refactoring to do because the vga_remove_vgacon also
> > wants a pdev.
> >
> > All so that we don't call aperture_detach_devices() on a bunch of pci
> > bars, which apparently is not problem for any other driver, but absolutely
> > is a huge problem for gma500 somehow.
> >
> > I don't understand why.
> >
>
> Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
> is needed then starts to get a little silly. Maybe one option is to add a
> 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
> to iterate over PCI bars and call aperture_remove_conflicting_devices() ?

The thing I don't get: Why does this matter for gma500 and not any of
the other pci devices? Look at your gpu, realize there's a lot more
than the one pci bar for vram or stolen memory, realize that we're
nuking bars that cannot possible contain the framebuffer for everyone
else too. Like the entire "gpus have a lot of bars" thing is the
reason why I pulled the sysfb_disable one level up, because we've been
doing that quite a few times before this patch (yes it's not the main
thing, but the side-effect cleanup is why I've gone down this rabbit
hole and wrote the entire series here):

https://lore.kernel.org/dri-devel/[e-mail]/

But somehow for gma500 it's a problem, while for everyone else it's
fine. That's the part I dont get, or Thomas&me have been talking past
each another and there's another issue that I'm missing.
-Daniel

> > Consider this me throwing in the towel. If you&Javier are convinced this
> > makes sense please type it up and merge it, but I'm not going to type
> > something that just doesn't make sense to me.
>
> Honestly, I would just go with the double drm_aperture_remove_*() helper
> calls (your original patch) unless that causes real issues. There is no
> point on blocking all your series just for this IMO.
>
> Then latter if Thomas has strong opinions can send a follow-up patch for
> the gma500 driver and the aperture helpers.
>
> > -Daniel
> >
>
> --
> Best regards,
>
> Javier Martinez Canillas
> Core Platforms
> Red Hat
>

Javier Martinez Canillas April 5, 2023, 5:43 p.m. UTC | #17

Daniel Vetter <[e-mail]> writes:

> On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
> <[e-mail]> wrote:
>>
>> Daniel Vetter <[e-mail]> writes:

[...]

>>
>> Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
>> is needed then starts to get a little silly. Maybe one option is to add a
>> 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
>> to iterate over PCI bars and call aperture_remove_conflicting_devices() ?
>
> The thing I don't get: Why does this matter for gma500 and not any of
> the other pci devices? Look at your gpu, realize there's a lot more

Yes, I don't know why gma500 is special in that sense but I'm not familiar
with that hardware to have an opinion on this.

Patrik Jakobsson April 5, 2023, 5:46 p.m. UTC | #18

On Wed, Apr 5, 2023 at 7:15 PM Daniel Vetter <[e-mail]> wrote:
>
> On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
> <[e-mail]> wrote:
> >
> > Daniel Vetter <[e-mail]> writes:
> >
> > > On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
> >
> > [...]
> >
> > >> > > >        /*
> > >> > > >         * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > >> > > >         * otherwise the vga fbdev driver falls over.
> > >> > > >         */
> > >> > > >        ret = vga_remove_vgacon(pdev);
> > >> >
> > >> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
> >
> > Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.
> >
> > [...]
> >
> > >> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> > >> {
> > >>      aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> > >>
> > >>      return vga_remove_vgacon(pdev);
> > >> }
> > >>
> > >> And that can be called from gma500 and the pci aperture helper.
> > >
> > > But you still pass a pci_dev to that helper. Which just doesn't make any
> > > sense to me (assuming your entire point is that this isn't just a normal
> > > pci device but some special legacy vga thing), but if we go with (void)
> > > then there's more refactoring to do because the vga_remove_vgacon also
> > > wants a pdev.
> > >
> > > All so that we don't call aperture_detach_devices() on a bunch of pci
> > > bars, which apparently is not problem for any other driver, but absolutely
> > > is a huge problem for gma500 somehow.
> > >
> > > I don't understand why.
> > >
> >
> > Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
> > is needed then starts to get a little silly. Maybe one option is to add a
> > 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
> > to iterate over PCI bars and call aperture_remove_conflicting_devices() ?
>
> The thing I don't get: Why does this matter for gma500 and not any of
> the other pci devices? Look at your gpu, realize there's a lot more
> than the one pci bar for vram or stolen memory, realize that we're
> nuking bars that cannot possible contain the framebuffer for everyone
> else too. Like the entire "gpus have a lot of bars" thing is the
> reason why I pulled the sysfb_disable one level up, because we've been
> doing that quite a few times before this patch (yes it's not the main
> thing, but the side-effect cleanup is why I've gone down this rabbit
> hole and wrote the entire series here):
>
> https://lore.kernel.org/dri-devel/[e-mail]/
>
> But somehow for gma500 it's a problem, while for everyone else it's
> fine. That's the part I dont get, or Thomas&me have been talking past
> each another and there's another issue that I'm missing.
> -Daniel

I'm also getting confused here.

AFAIK the stolen memory works the same for gma500 hardware as other
Intel GPUs. Are you saying that there is a difference in how gma500
hardware works? I always assumed that i915 got away with not dealing
much with stolen memory because it simply doesn't use it for
allocations. In gma500 we use it for fbdev and cursors. The actual
pages reserved by the bios can be accessed through a pci bar if you
map it so (which IIRC we do) but I suppose that doesn't help
identifying it as a range reserved by other drivers.

The reason I've kept the stolen allocation logic is because some
gma500 systems don't have a lot of memory. But that is mostly the old
Pouslbo systems. Perhaps it is time to ditch the stolen allocation
code?

-Patrik

>
> > > Consider this me throwing in the towel. If you&Javier are convinced this
> > > makes sense please type it up and merge it, but I'm not going to type
> > > something that just doesn't make sense to me.
> >
> > Honestly, I would just go with the double drm_aperture_remove_*() helper
> > calls (your original patch) unless that causes real issues. There is no
> > point on blocking all your series just for this IMO.
> >
> > Then latter if Thomas has strong opinions can send a follow-up patch for
> > the gma500 driver and the aperture helpers.
> >
> > > -Daniel
> > >
> >
> > --
> > Best regards,
> >
> > Javier Martinez Canillas
> > Core Platforms
> > Red Hat
> >
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Thomas Zimmermann April 6, 2023, 7:14 a.m. UTC | #19

Hi

Attached is the patch to provide gma500 with an interface to remove the 
VGA devices. Hopefully this, otherwise, I'd respin the whole series.

Best regards
Thomas

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 <[e-mail]>
> Cc: Patrik Jakobsson <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: Javier Martinez Canillas <[e-mail]>
> ---
>   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);
>   	if (ret)
>   		return ret;
>

Daniel Vetter April 6, 2023, 7:31 a.m. UTC | #20

On Wed, 5 Apr 2023 at 19:46, Patrik Jakobsson
<[e-mail]> wrote:
>
> On Wed, Apr 5, 2023 at 7:15 PM Daniel Vetter <[e-mail]> wrote:
> >
> > On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
> > <[e-mail]> wrote:
> > >
> > > Daniel Vetter <[e-mail]> writes:
> > >
> > > > On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
> > >
> > > [...]
> > >
> > > >> > > >        /*
> > > >> > > >         * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > > >> > > >         * otherwise the vga fbdev driver falls over.
> > > >> > > >         */
> > > >> > > >        ret = vga_remove_vgacon(pdev);
> > > >> >
> > > >> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
> > >
> > > Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.
> > >
> > > [...]
> > >
> > > >> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> > > >> {
> > > >>      aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> > > >>
> > > >>      return vga_remove_vgacon(pdev);
> > > >> }
> > > >>
> > > >> And that can be called from gma500 and the pci aperture helper.
> > > >
> > > > But you still pass a pci_dev to that helper. Which just doesn't make any
> > > > sense to me (assuming your entire point is that this isn't just a normal
> > > > pci device but some special legacy vga thing), but if we go with (void)
> > > > then there's more refactoring to do because the vga_remove_vgacon also
> > > > wants a pdev.
> > > >
> > > > All so that we don't call aperture_detach_devices() on a bunch of pci
> > > > bars, which apparently is not problem for any other driver, but absolutely
> > > > is a huge problem for gma500 somehow.
> > > >
> > > > I don't understand why.
> > > >
> > >
> > > Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
> > > is needed then starts to get a little silly. Maybe one option is to add a
> > > 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
> > > to iterate over PCI bars and call aperture_remove_conflicting_devices() ?
> >
> > The thing I don't get: Why does this matter for gma500 and not any of
> > the other pci devices? Look at your gpu, realize there's a lot more
> > than the one pci bar for vram or stolen memory, realize that we're
> > nuking bars that cannot possible contain the framebuffer for everyone
> > else too. Like the entire "gpus have a lot of bars" thing is the
> > reason why I pulled the sysfb_disable one level up, because we've been
> > doing that quite a few times before this patch (yes it's not the main
> > thing, but the side-effect cleanup is why I've gone down this rabbit
> > hole and wrote the entire series here):
> >
> > https://lore.kernel.org/dri-devel/[e-mail]/
> >
> > But somehow for gma500 it's a problem, while for everyone else it's
> > fine. That's the part I dont get, or Thomas&me have been talking past
> > each another and there's another issue that I'm missing.
> > -Daniel
>
> I'm also getting confused here.
>
> AFAIK the stolen memory works the same for gma500 hardware as other
> Intel GPUs. Are you saying that there is a difference in how gma500
> hardware works? I always assumed that i915 got away with not dealing
> much with stolen memory because it simply doesn't use it for
> allocations. In gma500 we use it for fbdev and cursors. The actual
> pages reserved by the bios can be accessed through a pci bar if you
> map it so (which IIRC we do) but I suppose that doesn't help
> identifying it as a range reserved by other drivers.

The other integrated gpu have their fw fb behind a pci bar, and stolen
is often entirely hidden from the cpu for direct access. gma500 seems
different with having stolen as just a specially marked up range of
normal system memory. That's why the usual pci helper doesn't catch
everything for gma500.

> The reason I've kept the stolen allocation logic is because some
> gma500 systems don't have a lot of memory. But that is mostly the old
> Pouslbo systems. Perhaps it is time to ditch the stolen allocation
> code?

Yeah that's all fine.
-Daniel

>
> -Patrik
>
> >
> > > > Consider this me throwing in the towel. If you&Javier are convinced this
> > > > makes sense please type it up and merge it, but I'm not going to type
> > > > something that just doesn't make sense to me.
> > >
> > > Honestly, I would just go with the double drm_aperture_remove_*() helper
> > > calls (your original patch) unless that causes real issues. There is no
> > > point on blocking all your series just for this IMO.
> > >
> > > Then latter if Thomas has strong opinions can send a follow-up patch for
> > > the gma500 driver and the aperture helpers.
> > >
> > > > -Daniel
> > > >
> > >
> > > --
> > > Best regards,
> > >
> > > Javier Martinez Canillas
> > > Core Platforms
> > > Red Hat
> > >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

Javier Martinez Canillas April 6, 2023, 8:38 a.m. UTC | #21

Thomas Zimmermann <[e-mail]> writes:

[...]

> Am 04.04.23 um 22:18 schrieb Daniel Vetter:
> Gma500 therefore calls both helpers to catch all cases. It's confusing
> as it implies that there's something about the PCI device that requires
> ownership management. The relationship between the PCI device and the
> VGA devices is non-obvious. At worst, readers might assume that calling
> two functions for aperture clearing ownership is a bug in the driver.
>

Yeah, or someone looking as the driver for reference may wrongly think
that calling both aperture helpers are needed for PCI drivers and that
is not the case.

> Hence, move the PCI removal helper's code for VGA functionality into
> a separate function and call this function from gma500. Documents the
> purpose of each call to aperture helpers. The change contains comments
> and example code form the discussion at [1].
>
> Signed-off-by: Thomas Zimmermann <[e-mail]>
> Link: https://patchwork.kernel.org/project/dri-devel/patch/[e-mail]/ # 1
> ---

Looks good to me and I agree that it makes the code easier to understand.

Reviewed-by: Javier Martinez Canillas <[e-mail]>

I've a couple of comments below though:

[...]

> + * Hardware for gma500 is a hybrid device, which both acts as a PCI
> + * device (for legacy vga functionality) but also more like an
> + * integrated display on a SoC where the framebuffer simply
> + * resides in main memory and not in a special PCI bar (that
> + * internally redirects to a stolen range of main memory) like all
> + * other integrated PCI display devices have.
> + *

Is "have" the correct word here or "do" ? Or maybe "are implemented" ?

> + * To catch all cases we need to remove conflicting firmware devices
> + * for the stolen system memory and for the VGA functionality. As we
> + * currently cannot easily find the framebuffer's location in stolen
> + * memory, we remove all framebuffers here.
> + *
> + * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
> + *       we might be able to read the framebuffer range from the
> + *       device.
> + */
> +static int gma_remove_conflicting_framebuffers(struct pci_dev *pdev,
> +					       const struct drm_driver *req_driver)
>  {
> -	struct drm_psb_private *dev_priv;
> -	struct drm_device *dev;
> +	resource_size_t base = 0;
> +	resource_size_t size = PHYS_ADDR_MAX;
> +	const char *name = req_driver->name;
>  	int ret;
>  
> -	/*
> -	 * We cannot yet easily find the framebuffer's location in memory. So
> -	 * 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(&driver);
> +	ret = aperture_remove_conflicting_devices(base, size, name);
>  	if (ret)
>  		return ret;
>  
> -	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
> +	return __aperture_remove_legacy_vga_devices(pdev);

I don't like the __ prefix for this function name, as it usually implies
that is a function that is only local to the compilation unit. But it is
an exported symbol from the aperture infrastructure.

[...]
  
> +/**
> + * __aperture_remove_legacy_vga_devices - remove legacy VGA devices of a PCI devices
> + * @pdev: PCI device
> + *
> + * This function removes VGA devices provided by @pdev, such as a VGA
> + * framebuffer or a console. This is useful if you have a VGA-compatible
> + * PCI graphics device with framebuffers in non-BAR locations. Drivers
> + * should acquire ownership of those memory areas and afterwards call
> + * this helper to release remaining VGA devices.
> + *
> + * If your hardware has its framebuffers accessible via PCI BARS, use
> + * aperture_remove_conflicting_pci_devices() instead. The function will
> + * release any VGA devices automatically.
> + *
> + * WARNING: Apparently we must remove graphics drivers before calling
> + *          this helper. Otherwise the vga fbdev driver falls over if
> + *          we have vgacon configured.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise
> + */
> +int __aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> +{
> +	/* VGA framebuffer */
> +	aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> +
> +	/* VGA textmode console */
> +	return vga_remove_vgacon(pdev);
> +}
> +EXPORT_SYMBOL(__aperture_remove_legacy_vga_devices);

I would just call this symbol aperture_remove_legacy_vga_devices() as
mentioned, the fact that aperture_remove_conflicting_pci_devices() use it
internally is an implementation detail IMO. But it's an exported symbol so
the naming should be consistent.

Thomas Zimmermann April 6, 2023, 8:49 a.m. UTC | #22

Hi

Am 06.04.23 um 10:38 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <[e-mail]> writes:
> 
> [...]
> 
>> Am 04.04.23 um 22:18 schrieb Daniel Vetter:
>> Gma500 therefore calls both helpers to catch all cases. It's confusing
>> as it implies that there's something about the PCI device that requires
>> ownership management. The relationship between the PCI device and the
>> VGA devices is non-obvious. At worst, readers might assume that calling
>> two functions for aperture clearing ownership is a bug in the driver.
>>
> 
> Yeah, or someone looking as the driver for reference may wrongly think
> that calling both aperture helpers are needed for PCI drivers and that
> is not the case.
> 
>> Hence, move the PCI removal helper's code for VGA functionality into
>> a separate function and call this function from gma500. Documents the
>> purpose of each call to aperture helpers. The change contains comments
>> and example code form the discussion at [1].
>>
>> Signed-off-by: Thomas Zimmermann <[e-mail]>
>> Link: https://patchwork.kernel.org/project/dri-devel/patch/[e-mail]/ # 1
>> ---
> 
> Looks good to me and I agree that it makes the code easier to understand.
> 
> Reviewed-by: Javier Martinez Canillas <[e-mail]>

Thanks for the review.

> 
> I've a couple of comments below though:
> 
> [...]
> 
>> + * Hardware for gma500 is a hybrid device, which both acts as a PCI
>> + * device (for legacy vga functionality) but also more like an
>> + * integrated display on a SoC where the framebuffer simply
>> + * resides in main memory and not in a special PCI bar (that
>> + * internally redirects to a stolen range of main memory) like all
>> + * other integrated PCI display devices have.
>> + *
> 
> Is "have" the correct word here or "do" ? Or maybe "are implemented" ?

Right. I'll update this.

> 
>> + * To catch all cases we need to remove conflicting firmware devices
>> + * for the stolen system memory and for the VGA functionality. As we
>> + * currently cannot easily find the framebuffer's location in stolen
>> + * memory, we remove all framebuffers here.
>> + *
>> + * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then
>> + *       we might be able to read the framebuffer range from the
>> + *       device.
>> + */
>> +static int gma_remove_conflicting_framebuffers(struct pci_dev *pdev,
>> +					       const struct drm_driver *req_driver)
>>   {
>> -	struct drm_psb_private *dev_priv;
>> -	struct drm_device *dev;
>> +	resource_size_t base = 0;
>> +	resource_size_t size = PHYS_ADDR_MAX;
>> +	const char *name = req_driver->name;
>>   	int ret;
>>   
>> -	/*
>> -	 * We cannot yet easily find the framebuffer's location in memory. So
>> -	 * 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(&driver);
>> +	ret = aperture_remove_conflicting_devices(base, size, name);
>>   	if (ret)
>>   		return ret;
>>   
>> -	ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev, &driver);
>> +	return __aperture_remove_legacy_vga_devices(pdev);
> 
> I don't like the __ prefix for this function name, as it usually implies
> that is a function that is only local to the compilation unit. But it is
> an exported symbol from the aperture infrastructure.
> 
> [...]
>    
>> +/**
>> + * __aperture_remove_legacy_vga_devices - remove legacy VGA devices of a PCI devices
>> + * @pdev: PCI device
>> + *
>> + * This function removes VGA devices provided by @pdev, such as a VGA
>> + * framebuffer or a console. This is useful if you have a VGA-compatible
>> + * PCI graphics device with framebuffers in non-BAR locations. Drivers
>> + * should acquire ownership of those memory areas and afterwards call
>> + * this helper to release remaining VGA devices.
>> + *
>> + * If your hardware has its framebuffers accessible via PCI BARS, use
>> + * aperture_remove_conflicting_pci_devices() instead. The function will
>> + * release any VGA devices automatically.
>> + *
>> + * WARNING: Apparently we must remove graphics drivers before calling
>> + *          this helper. Otherwise the vga fbdev driver falls over if
>> + *          we have vgacon configured.
>> + *
>> + * Returns:
>> + * 0 on success, or a negative errno code otherwise
>> + */
>> +int __aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
>> +{
>> +	/* VGA framebuffer */
>> +	aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
>> +
>> +	/* VGA textmode console */
>> +	return vga_remove_vgacon(pdev);
>> +}
>> +EXPORT_SYMBOL(__aperture_remove_legacy_vga_devices);
> 
> I would just call this symbol aperture_remove_legacy_vga_devices() as
> mentioned, the fact that aperture_remove_conflicting_pci_devices() use it
> internally is an implementation detail IMO. But it's an exported symbol so
> the naming should be consistent.

That prefix __ is supposed to indicate that it's not a all-in-one 
solution. Most of all, it doesn't do sysfb_disable(). The helper is 
meant to be used as part of a larger function. I tried to outline this 
in the comment, where I say that drivers should first aquire framebuffer 
ownership and then call this helper. If naming isn't a showstopper, I'd 
like to keep the underscores.

Best regards
Thomas

>

Javier Martinez Canillas April 6, 2023, 9:05 a.m. UTC | #23

Thomas Zimmermann <[e-mail]> writes:

[...]

>>> +EXPORT_SYMBOL(__aperture_remove_legacy_vga_devices);
>> 
>> I would just call this symbol aperture_remove_legacy_vga_devices() as
>> mentioned, the fact that aperture_remove_conflicting_pci_devices() use it
>> internally is an implementation detail IMO. But it's an exported symbol so
>> the naming should be consistent.
>
> That prefix __ is supposed to indicate that it's not a all-in-one 
> solution. Most of all, it doesn't do sysfb_disable(). The helper is 
> meant to be used as part of a larger function. I tried to outline this 
> in the comment, where I say that drivers should first aquire framebuffer 
> ownership and then call this helper. If naming isn't a showstopper, I'd 
> like to keep the underscores.
>

Sure, I see that we have other symbols exported in DRM that have a __
prefix in their name. So maybe I am the one who was confused on the
meaning of it.

Patrik Jakobsson April 6, 2023, 11:16 a.m. UTC | #24

On Thu, Apr 6, 2023 at 9:32 AM Daniel Vetter <[e-mail]> wrote:
>
> On Wed, 5 Apr 2023 at 19:46, Patrik Jakobsson
> <[e-mail]> wrote:
> >
> > On Wed, Apr 5, 2023 at 7:15 PM Daniel Vetter <[e-mail]> wrote:
> > >
> > > On Wed, 5 Apr 2023 at 18:54, Javier Martinez Canillas
> > > <[e-mail]> wrote:
> > > >
> > > > Daniel Vetter <[e-mail]> writes:
> > > >
> > > > > On Wed, Apr 05, 2023 at 04:32:19PM +0200, Thomas Zimmermann wrote:
> > > >
> > > > [...]
> > > >
> > > > >> > > >        /*
> > > > >> > > >         * WARNING: Apparently we must kick fbdev drivers before vgacon,
> > > > >> > > >         * otherwise the vga fbdev driver falls over.
> > > > >> > > >         */
> > > > >> > > >        ret = vga_remove_vgacon(pdev);
> > > > >> >
> > > > >> > This isn't enough, we also nuke stuff that's mapping the vga fb range.
> > > >
> > > > Ah, also need aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE) then.
> > > >
> > > > [...]
> > > >
> > > > >> int aperture_remove_legacy_vga_devices(struct pci_dev *pdev)
> > > > >> {
> > > > >>      aperture_detach_devices(VGA_FB_PHYS_BASE, VGA_FB_PHYS_SIZE);
> > > > >>
> > > > >>      return vga_remove_vgacon(pdev);
> > > > >> }
> > > > >>
> > > > >> And that can be called from gma500 and the pci aperture helper.
> > > > >
> > > > > But you still pass a pci_dev to that helper. Which just doesn't make any
> > > > > sense to me (assuming your entire point is that this isn't just a normal
> > > > > pci device but some special legacy vga thing), but if we go with (void)
> > > > > then there's more refactoring to do because the vga_remove_vgacon also
> > > > > wants a pdev.
> > > > >
> > > > > All so that we don't call aperture_detach_devices() on a bunch of pci
> > > > > bars, which apparently is not problem for any other driver, but absolutely
> > > > > is a huge problem for gma500 somehow.
> > > > >
> > > > > I don't understand why.
> > > > >
> > > >
> > > > Yeah, agreed that if vga_remove_vgacon() isn't enough and another helper
> > > > is needed then starts to get a little silly. Maybe one option is to add a
> > > > 3rd param to aperture_remove_conflicting_pci_devices() and skip the logic
> > > > to iterate over PCI bars and call aperture_remove_conflicting_devices() ?
> > >
> > > The thing I don't get: Why does this matter for gma500 and not any of
> > > the other pci devices? Look at your gpu, realize there's a lot more
> > > than the one pci bar for vram or stolen memory, realize that we're
> > > nuking bars that cannot possible contain the framebuffer for everyone
> > > else too. Like the entire "gpus have a lot of bars" thing is the
> > > reason why I pulled the sysfb_disable one level up, because we've been
> > > doing that quite a few times before this patch (yes it's not the main
> > > thing, but the side-effect cleanup is why I've gone down this rabbit
> > > hole and wrote the entire series here):
> > >
> > > https://lore.kernel.org/dri-devel/[e-mail]/
> > >
> > > But somehow for gma500 it's a problem, while for everyone else it's
> > > fine. That's the part I dont get, or Thomas&me have been talking past
> > > each another and there's another issue that I'm missing.
> > > -Daniel
> >
> > I'm also getting confused here.
> >
> > AFAIK the stolen memory works the same for gma500 hardware as other
> > Intel GPUs. Are you saying that there is a difference in how gma500
> > hardware works? I always assumed that i915 got away with not dealing
> > much with stolen memory because it simply doesn't use it for
> > allocations. In gma500 we use it for fbdev and cursors. The actual
> > pages reserved by the bios can be accessed through a pci bar if you
> > map it so (which IIRC we do) but I suppose that doesn't help
> > identifying it as a range reserved by other drivers.
>
> The other integrated gpu have their fw fb behind a pci bar, and stolen
> is often entirely hidden from the cpu for direct access. gma500 seems
> different with having stolen as just a specially marked up range of
> normal system memory. That's why the usual pci helper doesn't catch
> everything for gma500.

Right, now I get it. You don't have the GATT on some systems so the
range can never be inside the aperture on those systems anyway.

The GATT probably went away because there is no need for it since you
don't get coherency with the PowerVR parts anyway.

Thanks for explaining

>
> > The reason I've kept the stolen allocation logic is because some
> > gma500 systems don't have a lot of memory. But that is mostly the old
> > Pouslbo systems. Perhaps it is time to ditch the stolen allocation
> > code?
>
> Yeah that's all fine.
> -Daniel
>
> >
> > -Patrik
> >
> > >
> > > > > Consider this me throwing in the towel. If you&Javier are convinced this
> > > > > makes sense please type it up and merge it, but I'm not going to type
> > > > > something that just doesn't make sense to me.
> > > >
> > > > Honestly, I would just go with the double drm_aperture_remove_*() helper
> > > > calls (your original patch) unless that causes real issues. There is no
> > > > point on blocking all your series just for this IMO.
> > > >
> > > > Then latter if Thomas has strong opinions can send a follow-up patch for
> > > > the gma500 driver and the aperture helpers.
> > > >
> > > > > -Daniel
> > > > >
> > > >
> > > > --
> > > > Best regards,
> > > >
> > > > Javier Martinez Canillas
> > > > Core Platforms
> > > > Red Hat
> > > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Patch

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);
 	if (ret)
 		return ret;