[Intel-gfx] [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/intel-gfx/attachments/20230405/39941bc8/attachment-0001.sig>


More information about the Intel-gfx mailing list