[PATCH 1/8] drm/gma500: Use drm_aperture_remove_conflicting_pci_framebuffers
Thomas Zimmermann
tzimmermann at suse.de
Wed Apr 5 11:00:41 UTC 2023
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 <daniel.vetter at intel.com>
>>>>>> Cc: Patrik Jakobsson <patrik.r.jakobsson at gmail.com>
>>>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
>>>>>> Cc: Javier Martinez Canillas <javierm at redhat.com>
>>>>>> ---
>>>>>> drivers/gpu/drm/gma500/psb_drv.c | 9 +++++++--
>>>>>> 1 file changed, 7 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/gma500/psb_drv.c
>>>>>> b/drivers/gpu/drm/gma500/psb_drv.c
>>>>>> index 2ce96b1b9c74..f1e0eed8fea4 100644
>>>>>> --- a/drivers/gpu/drm/gma500/psb_drv.c
>>>>>> +++ b/drivers/gpu/drm/gma500/psb_drv.c
>>>>>> @@ -422,12 +422,17 @@ static int psb_pci_probe(struct pci_dev *pdev,
>>>>>> const struct pci_device_id *ent)
>>>>>> /*
>>>>>> * We cannot yet easily find the framebuffer's location in
>>>>>> memory. So
>>>>>> - * remove all framebuffers here.
>>>>>> + * remove all framebuffers here. Note that we still want the
>>>>>> pci special
>>>>>> + * handling to kick out vgacon.
>>>>>> *
>>>>>> * TODO: Refactor psb_driver_load() to map vdc_reg earlier. Then we
>>>>>> * might be able to read the framebuffer range from the
>>>>>> device.
>>>>>> */
>>>>>> - ret = drm_aperture_remove_framebuffers(true, &driver);
>>>>>> + ret = drm_aperture_remove_framebuffers(false, &driver);
>>>>>> + if (ret)
>>>>>> + return ret;
>>>>>> +
>>>>>> + ret = drm_aperture_remove_conflicting_pci_framebuffers(pdev,
>>>>>> &driver);
>>>>>
>>>>> This simply isn't it. If you have to work around your own API, it's time
>>>>> to rethink the API.
>>>>
>>>> Here's a proposal:
>>>>
>>>> 1) As you're changing aperture_remove_conflicting_devices() anyway, rename
>>>> it to aperture_remove_conflicting_devices_at(), so it's clear that it takes
>>>> a memory range.
>>>>
>>>> 2) Introduce aperture_remove_conflicting_pci_devices_at(), which takes a
>>>> PCI device and a memory range. It should do the is_primary and vgacon stuff,
>>>> but kick out the range.
>>>>
>>>> 3) Call aperture_remove_conflicting_pci_devices_at() from gma500 with the
>>>> full range. Even if we can restructure gma500 to detect the firmware
>>>> framebuffer from its registers (there's this TODO item), we'd still need
>>>> aperture_remove_conflicting_pci_devices_at() to do something useful with it.
>>>>
>>>> 4) With that, aperture_remove_conflicting_devices_at() can drop the primary
>>>> argument.
>>>>
>>>> Of course, the DRM-related interface should be adapted as well. There's a
>>>> bit of overlap in the implementation of both PCI aperture helpers, but the
>>>> overall interface is clear.
>>>
>>> This essentially just gives us a helper which does the above two
>>> open-coded steps but all wrapped up. For gma500 only. Also I really don't
>>> think I'm working around the api here, it's gma500 which is special:
>>>
>>> - Normal pci devices all have their fw framebuffer within the memory bars,
>>> never outside. So for those the pci version is the right interface.
>>>
>>> - If the framebuffer is outside of the pci bar then it's just not really a
>>> pci vga device anymore, but looks a lot more like a SoC design.
>>>
>>> gma500 is somehow both at the same time, so it gets two calls. And having
>>
>> It's not "both at the same time." It like an SoC that can act as VGA. But
>> it's not really a PCI graphics card on its own. Maybe that's just
>> nitpicking, though.
>
> 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
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: OpenPGP_signature
Type: application/pgp-signature
Size: 840 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230405/39941bc8/attachment.sig>
More information about the dri-devel
mailing list