[PATCH 03/51] drm: add managed resources tied to drm_device

[Andrzej Hajda]

On 25.02.2020 16:03, Daniel Vetter wrote:
> On Tue, Feb 25, 2020 at 11:27 AM Andrzej Hajda <a.hajda at samsung.com> wrote:
>> Hi Daniel,
>>
>>
>> The patchset looks interesting.
>>
>>
>> On 21.02.2020 22:02, Daniel Vetter wrote:
>>> We have lots of these. And the cleanup code tends to be of dubious
>>> quality. The biggest wrong pattern is that developers use devm_, which
>>> ties the release action to the underlying struct device, whereas
>>> all the userspace visible stuff attached to a drm_device can long
>>> outlive that one (e.g. after a hotunplug while userspace has open
>>> files and mmap'ed buffers). Give people what they want, but with more
>>> correctness.
>>
>> I am not familiar with this stuff, so forgive me stupid questions.
>>
>> Is it documented how uapi should behave in such case?
>>
>> I guess the general rule is to return errors on most ioctls (ENODEV,
>> EIO?), and wait until userspace releases everything, as there is not
>> much more to do.
>>
>> If that is true what is the point of keeping these structs anyway -
>> trivial functions with small context data should do the job.
>>
>> I suspect I am missing something but I do not know what :)
> We could do the above (also needs unmapping of all mmaps, so userspace
> then gets SIGSEGV everywhere) and watch userspace crash&burn.
> Essentially if the kernel can't do this properly, then there's no hope
> that userspace will be any better.


We do not want to crash userspace. We just need to tell userspace that
the kernel objects userspace has references to are not valid.

For this two mechanism should be enough:

- signal hot-unplug,

- report error (ENODEV for example) on any userspace requests (ioctls)
on invalid objects.

Expecting from userspace properly handling ioctl errors seems to be fair.

Regarding mmap I am not sure how to properly handle disappearing
devices, but this is common problem regardless which solution we use.


>
> Hence the idea is that we keep everything userspace facing still
> around, except it doesn't do much anymore. So connectors still there,
> but they look disconnected.


It looks like lying to userspace that physical connectors still exists.
If we want to lie we need good reason for that. What is that reason?

Why not just tell connectors are gone?


> Userspace can then hopefully eventually
> get around to processing the sysfs hotunplug event and remove the
> device from all its list. So the long-term idea is that a lot of stuff
> keeps working, except the driver doesn't talk to the hardware anymore.
> And we just sit around waiting for userspace to clean things up.


What does it mean "lot of stuff keeps working"? What drm driver can do
without hardware? Could you show some examples?


>
> I guess once we have a bunch of the panel/usb drivers converted over
> we could indeed document how this is all supposed to work from an uapi
> pov. But right now a lot of this is all rather aspirational, I think
> only the recent simple display pipe based drivers implement this as
> described above.
>
>>> Mostly copied from devres.c, with types adjusted to fit drm_device and
>>> a few simplifications - I didn't (yet) copy over everything. Since
>>> the types don't match code sharing looked like a hopeless endeavour.
>>>
>>> For now it's only super simplified, no groups, you can't remove
>>> actions (but kfree exists, we'll need that soon). Plus all specific to
>>> drm_device ofc, including the logging. Which I didn't bother to make
>>> compile-time optional, since none of the other drm logging is compile
>>> time optional either.
>>
>> I saw in v1 thread that copy/paste is OK and merging back devres and
>> drmres can be done later, but experience shows that after short time
>> things get de-synchronized and merging process becomes quite painful.
>>
>> On the other side I guess it shouldn't be difficult to split devres into
>> consumer agnostic core and "struct device" helpers and then use the core
>> in drm.
>>
>> For example currently devres uses two fields from struct device:
>>
>>     spinlock_t        devres_lock;
>>     struct list_head    devres_head;
>>
>> Lets put it into separate struct:
>>
>> struct devres {
>>
>>     spinlock_t        lock;
>>     struct list_head    head;
>>
>> };
>>
>> And embed this struct into "struct device".
>>
>> Then convert all core devres functions to take "struct devres *"
>> argument instead of "struct device *" and then these core functions can
>> be usable in drm.
>>
>> Looks quite simple separation of abstraction (devres) and its consumer
>> (struct device).
>>
>> After such split one could think about changing name devres to something
>> more reliable.
> There was a long discussion on v1 exactly about this, Greg's
> suggestion was to "just share a struct device". So we're not going to
> do this here, and the struct device seems like slight overkill and not
> a good enough fit here.


But my proposition is different, I want to get rid of "struct device"
from devres core - devres has nothing to do with device, it was bound to
it probably because it was convenient as device was the only client of
devres (I guess). Now if we want to have more devres clients abstracting
out devres from device seems quite natural. This way we will have proper
abstractions without code duplication.

Examples of devres related code according to my proposition:

// devres core

void devres_add(struct devres_head *dh, void *res)
{

   struct devres *dr = container_of(res, struct devres, data);

    unsigned long flags;

    spin_lock_irqsave(&dh->lock, flags);
    add_dr(dev, &dr->node);
    spin_unlock_irqrestore(&dh->lock, flags);
}

// device devres helper (non core)

struct clk *devm_clk_get(struct device *dev, const char *id)
{
    struct clk **ptr, *clk;

    ptr = devres_alloc(devm_clk_release, sizeof(*ptr), GFP_KERNEL);
    if (!ptr)
        return ERR_PTR(-ENOMEM);

    clk = clk_get(dev, id);
    if (!IS_ERR(clk)) {
        *ptr = clk;
        devres_add(&dev->devres, ptr);
    } else {
        devres_free(ptr);
    }

    return clk;
}


Changes are cosmetic. But then you can easily add devres to drmdev:

struct drm_device {

   ...

+   struct devres_head devres;

};

// then copy/modify from your patch:

+void *drmm_kmalloc(struct drm_device *dev, size_t size, gfp_t gfp)
+{
+	struct drmres *dr;
+
+	dr = alloc_dr(NULL, size, gfp, dev_to_node(dev->dev));
+	if (!dr)
+		return NULL;
+	dr->node.name = "kmalloc";
+
+	devres_add(&dev->devres, dr); // the only change is here
+
+	return dr->data;
+}


Btw, reimplemented add_dr is different of original add_dr and is similar
to original devres_add, so your implementation differs already from
original one, merging back these two will be painfull :)


Regards

Andrzej