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

From: Andrzej Hajda <a.hajda@samsung.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>,
	DRI Development <dri-devel@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	"Rafael J. Wysocki" <rafael@kernel.org>
Subject: Re: [PATCH 03/51] drm: add managed resources tied to drm_device
Date: Tue, 25 Feb 2020 11:27:01 +0100	[thread overview]
Message-ID: <c088ce3b-d409-3a66-1f45-72f8ee9d9784@samsung.com> (raw)
In-Reply-To: <20200221210319.2245170-4-daniel.vetter@ffwll.ch>

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 :)

>
> 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.

Regards

Andrzej

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