Re: [PATCH 8/9] drm: Add infrastructure for platform devices

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 11796 bytes --]

Hi

Am 28.09.20 um 10:50 schrieb Daniel Vetter:
> On Mon, Sep 28, 2020 at 10:40 AM Thomas Zimmermann <tzimmermann@suse.de> 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@suse.de>
>>>
>>> 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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel