On Tue, Feb 25, 2020 at 11:27 AM Andrzej Hajda <a.hajda@xxxxxxxxxxx> 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.
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. 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.
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.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
