Hi
Am 28.09.20 um 10:50 schrieb Daniel Vetter:
> On Mon, Sep 28, 2020 at 10:40 AM Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
>>
>> Hi
>>
>> Am 29.06.20 um 11:27 schrieb Daniel Vetter:
>>> On Thu, Jun 25, 2020 at 02:00:10PM +0200, Thomas Zimmermann wrote:
>>>> Platform devices might operate on firmware framebuffers, such as VESA or
>>>> EFI. Before a native driver for the graphics hardware can take over the
>>>> device, it has to remove any platform driver that operates on the firmware
>>>> framebuffer. Platform helpers provide the infrastructure for platform
>>>> drivers to acquire firmware framebuffers, and for native drivers to remove
>>>> them lateron.
>>>>
>>>> It works similar to the related fbdev mechanism. During initialization, the
>>>> platform driver acquires the firmware framebuffer's I/O memory and provides
>>>> a callback to be removed. The native driver later uses this inforamtion to
>>>> remove any platform driver for it's framebuffer I/O memory.
>>>>
>>>> The platform helper's removal code is integrated into the existing code for
>>>> removing conflicting fraembuffers, so native drivers use it automatically.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>>
>>> I have some ideas for how to do this a notch cleaner in the next patch.
>>> Maybe best to discuss the actual implmenentation stuff there.
>>>
>>> Aside from that usual nits:
>>> - kerneldoc for these please, pulled into drm-kms.rst.
>>> - naming isn't super ocd with drm_platform.c but that prefix not used, but
>>> I also don't have better ideas.
>>
>> I was never really happy with the naming either. Maybe call it aperture
>> helpers with drm_aperture_ and CONFIG_DRM_APERTURE_HELPER?
>
> +1
>
>>> - I think the functions from drm_fb_helper.h for removing other
>>> framebuffers should be moved here, and function name prefix adjusted
>>> acoordingly
>>>
>>> I'm wondering about the locking and deadlock potential here, is lockdep
>>> all happy with this?
>>
>> I haven't seen any complains, but I'll double check.
>
> Might be worth it to cc Greg on this, since the device driver model
> has a lot of corner cases to make sure it doesn't get stuck here with
> hiararchical drivers/device getting removed while something else is
> probing them at the same time.
Oh, OK. I'll keep that in mind when sending out v2 of these patches.
Best regards
Thomas
> -Daniel
>
>> Best regards
>> Thomas
>>
>>>
>>> Cheers, Daniel
>>>
>>>> ---
>>>> drivers/gpu/drm/Kconfig | 6 ++
>>>> drivers/gpu/drm/Makefile | 1 +
>>>> drivers/gpu/drm/drm_platform.c | 118 +++++++++++++++++++++++++++++++++
>>>> include/drm/drm_fb_helper.h | 18 ++++-
>>>> include/drm/drm_platform.h | 42 ++++++++++++
>>>> 5 files changed, 184 insertions(+), 1 deletion(-)
>>>> create mode 100644 drivers/gpu/drm/drm_platform.c
>>>> create mode 100644 include/drm/drm_platform.h
>>>>
>>>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>>>> index c4fd57d8b717..e9d6892f9d38 100644
>>>> --- a/drivers/gpu/drm/Kconfig
>>>> +++ b/drivers/gpu/drm/Kconfig
>>>> @@ -229,6 +229,12 @@ config DRM_SCHED
>>>> tristate
>>>> depends on DRM
>>>>
>>>> +config DRM_PLATFORM_HELPER
>>>> + bool
>>>> + depends on DRM
>>>> + help
>>>> + Helpers for DRM platform devices
>>>> +
>>>> source "drivers/gpu/drm/i2c/Kconfig"
>>>>
>>>> source "drivers/gpu/drm/arm/Kconfig"
>>>> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
>>>> index 2c0e5a7e5953..8ceb21d0770a 100644
>>>> --- a/drivers/gpu/drm/Makefile
>>>> +++ b/drivers/gpu/drm/Makefile
>>>> @@ -32,6 +32,7 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o
>>>> drm-$(CONFIG_PCI) += drm_pci.o
>>>> drm-$(CONFIG_DEBUG_FS) += drm_debugfs.o drm_debugfs_crc.o
>>>> drm-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o
>>>> +drm-$(CONFIG_DRM_PLATFORM_HELPER) += drm_platform.o
>>>>
>>>> drm_vram_helper-y := drm_gem_vram_helper.o
>>>> obj-$(CONFIG_DRM_VRAM_HELPER) += drm_vram_helper.o
>>>> diff --git a/drivers/gpu/drm/drm_platform.c b/drivers/gpu/drm/drm_platform.c
>>>> new file mode 100644
>>>> index 000000000000..09a2f2a31aa5
>>>> --- /dev/null
>>>> +++ b/drivers/gpu/drm/drm_platform.c
>>>> @@ -0,0 +1,118 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>>> +
>>>> +#include <linux/mutex.h>
>>>> +#include <linux/slab.h>
>>>> +
>>>> +#include <drm/drm_drv.h>
>>>> +#include <drm/drm_managed.h>
>>>> +#include <drm/drm_platform.h>
>>>> +
>>>> +static LIST_HEAD(drm_apertures);
>>>> +
>>>> +static DEFINE_MUTEX(drm_apertures_lock);
>>>> +
>>>> +static bool overlap(resource_size_t base1, resource_size_t end1,
>>>> + resource_size_t base2, resource_size_t end2)
>>>> +{
>>>> + return (base1 < end2) && (end1 > base2);
>>>> +}
>>>> +
>>>> +static struct drm_aperture *
>>>> +drm_aperture_acquire(struct drm_device *dev,
>>>> + resource_size_t base, resource_size_t size,
>>>> + const struct drm_aperture_funcs *funcs)
>>>> +{
>>>> + size_t end = base + size;
>>>> + struct list_head *pos;
>>>> + struct drm_aperture *ap;
>>>> +
>>>> + mutex_lock(&drm_apertures_lock);
>>>> +
>>>> + list_for_each(pos, &drm_apertures) {
>>>> + ap = container_of(pos, struct drm_aperture, lh);
>>>> + if (overlap(base, end, ap->base, ap->base + ap->size))
>>>> + return ERR_PTR(-EBUSY);
>>>> + }
>>>> +
>>>> + ap = drmm_kzalloc(dev, sizeof(*ap), GFP_KERNEL);
>>>> + if (!ap)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + ap->dev = dev;
>>>> + ap->base = base;
>>>> + ap->size = size;
>>>> + ap->funcs = funcs;
>>>> + INIT_LIST_HEAD(&ap->lh);
>>>> +
>>>> + list_add(&ap->lh, &drm_apertures);
>>>> +
>>>> + mutex_unlock(&drm_apertures_lock);
>>>> +
>>>> + return ap;
>>>> +}
>>>> +
>>>> +static void drm_aperture_release(struct drm_aperture *ap)
>>>> +{
>>>> + bool kicked_out = ap->kicked_out;
>>>> +
>>>> + if (!kicked_out)
>>>> + mutex_lock(&drm_apertures_lock);
>>>> +
>>>> + list_del(&ap->lh);
>>>> + if (ap->funcs->release)
>>>> + ap->funcs->release(ap);
>>>> +
>>>> + if (!kicked_out)
>>>> + mutex_unlock(&drm_apertures_lock);
>>>> +}
>>>> +
>>>> +static void drm_aperture_acquire_release(struct drm_device *dev, void *ptr)
>>>> +{
>>>> + struct drm_aperture *ap = ptr;
>>>> +
>>>> + drm_aperture_release(ap);
>>>> +}
>>>> +
>>>> +struct drm_aperture *
>>>> +drmm_aperture_acquire(struct drm_device *dev,
>>>> + resource_size_t base, resource_size_t size,
>>>> + const struct drm_aperture_funcs *funcs)
>>>> +{
>>>> + struct drm_aperture *ap;
>>>> + int ret;
>>>> +
>>>> + ap = drm_aperture_acquire(dev, base, size, funcs);
>>>> + if (IS_ERR(ap))
>>>> + return ap;
>>>> + ret = drmm_add_action_or_reset(dev, drm_aperture_acquire_release, ap);
>>>> + if (ret)
>>>> + return ERR_PTR(ret);
>>>> +
>>>> + return ap;
>>>> +}
>>>> +EXPORT_SYMBOL(drmm_aperture_acquire);
>>>> +
>>>> +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size)
>>>> +{
>>>> + resource_size_t end = base + size;
>>>> + struct list_head *pos, *n;
>>>> +
>>>> + mutex_lock(&drm_apertures_lock);
>>>> +
>>>> + list_for_each_safe(pos, n, &drm_apertures) {
>>>> + struct drm_aperture *ap =
>>>> + container_of(pos, struct drm_aperture, lh);
>>>> +
>>>> + if (!overlap(base, end, ap->base, ap->base + ap->size))
>>>> + continue;
>>>> +
>>>> + ap->kicked_out = true;
>>>> + if (ap->funcs->kickout)
>>>> + ap->funcs->kickout(ap);
>>>> + else
>>>> + drm_dev_put(ap->dev);
>>>> + }
>>>> +
>>>> + mutex_unlock(&drm_apertures_lock);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_kickout_apertures_at);
>>>> diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
>>>> index 306aa3a60be9..a919b78b1961 100644
>>>> --- a/include/drm/drm_fb_helper.h
>>>> +++ b/include/drm/drm_fb_helper.h
>>>> @@ -35,7 +35,9 @@ struct drm_fb_helper;
>>>> #include <drm/drm_client.h>
>>>> #include <drm/drm_crtc.h>
>>>> #include <drm/drm_device.h>
>>>> +#include <drm/drm_platform.h>
>>>> #include <linux/kgdb.h>
>>>> +#include <linux/pci.h>
>>>> #include <linux/vgaarb.h>
>>>>
>>>> enum mode_set_atomic {
>>>> @@ -465,6 +467,11 @@ static inline int
>>>> drm_fb_helper_remove_conflicting_framebuffers(struct apertures_struct *a,
>>>> const char *name, bool primary)
>>>> {
>>>> + int i;
>>>> +
>>>> + for (i = 0; i < a->count; ++i)
>>>> + drm_kickout_apertures_at(a->ranges[i].base, a->ranges[i].size);
>>>> +
>>>> #if IS_REACHABLE(CONFIG_FB)
>>>> return remove_conflicting_framebuffers(a, name, primary);
>>>> #else
>>>> @@ -487,7 +494,16 @@ static inline int
>>>> drm_fb_helper_remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
>>>> const char *name)
>>>> {
>>>> - int ret = 0;
>>>> + resource_size_t base, size;
>>>> + int bar, ret = 0;
>>>> +
>>>> + for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
>>>> + if (!(pci_resource_flags(pdev, bar) & IORESOURCE_MEM))
>>>> + continue;
>>>> + base = pci_resource_start(pdev, bar);
>>>> + size = pci_resource_len(pdev, bar);
>>>> + drm_kickout_apertures_at(base, size);
>>>> + }
>>>>
>>>> /*
>>>> * WARNING: Apparently we must kick fbdev drivers before vgacon,
>>>> diff --git a/include/drm/drm_platform.h b/include/drm/drm_platform.h
>>>> new file mode 100644
>>>> index 000000000000..475e88ee1fbd
>>>> --- /dev/null
>>>> +++ b/include/drm/drm_platform.h
>>>> @@ -0,0 +1,42 @@
>>>> +// SPDX-License-Identifier: GPL-2.0 OR MIT
>>>> +
>>>> +#ifndef _DRM_PLATFORM_H_
>>>> +#define _DRM_PLATFORM_H_
>>>> +
>>>> +#include <linux/list.h>
>>>> +#include <linux/types.h>
>>>> +
>>>> +struct drm_aperture;
>>>> +struct drm_device;
>>>> +
>>>> +struct drm_aperture_funcs {
>>>> + void (*kickout)(struct drm_aperture *ap);
>>>> + void (*release)(struct drm_aperture *ap);
>>>> +};
>>>> +
>>>> +struct drm_aperture {
>>>> + struct drm_device *dev;
>>>> + resource_size_t base;
>>>> + resource_size_t size;
>>>> +
>>>> + const struct drm_aperture_funcs *funcs;
>>>> +
>>>> + struct list_head lh;
>>>> + bool kicked_out;
>>>> +};
>>>> +
>>>> +struct drm_aperture *
>>>> +drmm_aperture_acquire(struct drm_device *dev,
>>>> + resource_size_t base, resource_size_t size,
>>>> + const struct drm_aperture_funcs *funcs);
>>>> +
>>>> +#if defined (CONFIG_DRM_PLATFORM_HELPER)
>>>> +void drm_kickout_apertures_at(resource_size_t base, resource_size_t size);
>>>> +#else
>>>> +static inline void
>>>> +drm_kickout_apertures_at(resource_size_t base, resource_size_t size)
>>>> +{
>>>> +}
>>>> +#endif
>>>> +
>>>> +#endif
>>>> --
>>>> 2.27.0
>>>>
>>>
>>
>> --
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>> (HRB 36809, AG Nürnberg)
>> Geschäftsführer: Felix Imendörffer
>>
>
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
