On Thu, Sep 5, 2019 at 10:21 PM Kenny Ho y2kenny@gmail.com wrote:
On Thu, Sep 5, 2019 at 4:06 PM Daniel Vetter daniel@ffwll.ch wrote:
On Thu, Sep 5, 2019 at 8:28 PM Kenny Ho y2kenny@gmail.com wrote:
(resent in plain text mode)
Hi Daniel,
This is the previous patch relevant to this discussion: https://patchwork.freedesktop.org/patch/314343/
Ah yes, thanks for finding that.
So before I refactored the code to leverage drm_minor, I kept my own list of "known" drm_device inside the controller and have explicit register and unregister function to init per device cgroup defaults. For v4, I refactored the per device cgroup properties and embedded them into the drm_device and continue to only use the primary minor as a way to index the device as v3.
I didn't really like the explicit registration step, at least for the basic cgroup controls (like gem buffer limits), and suggested that should happen automatically at drm_dev_register/unregister time. I also talked about picking a consistent minor (if we have to use minors, still would like Tejun to confirm what we should do here), but that was an unrelated comment. So doing auto-registration on drm_minor was one step too far.
How about your comments on embedding properties into drm_device? I am actually still not clear on the downside of using drm_minor this way. With this implementation in v4, there isn't additional state that can go out of sync with the ground truth of drm_device from the perspective of drm_minor. Wouldn't the issue with hotplugging drm device you described earlier get worsen if the cgroup controller keep its own list?
drm_dev_unregister gets called on hotunplug, so your cgroup-internal tracking won't get out of sync any more than the drm_minor list gets out of sync with drm_devices. The trouble with drm_minor is just that cgroup doesn't track allocations on drm_minor (that's just the uapi flavour), but on the underlying drm_device. So really doesn't make much sense to attach cgroup tracking to the drm_minor.
Just doing a drm_cg_register/unregister pair that's called from drm_dev_register/unregister, and then if you want, looking up the right minor (I think always picking the render node makes sense for this, and skipping if there's no render node) would make most sense. At least for the basic cgroup controllers which are generic across drivers.
Why do we want to skip drm devices that does not have a render node and not just use the primary instead?
I guess we could also take the primary node, but drivers with only primary node are generaly display-only drm drivers. Not sure we want cgroups on those (but I guess can't hurt, and more consistent). But then we'd always need to pick the primary node for cgroup identification purposes. -Daniel
Kenny
-Daniel
Regards, Kenny
On Wed, Sep 4, 2019 at 4:54 AM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Sep 03, 2019 at 04:43:45PM -0400, Kenny Ho wrote:
On Tue, Sep 3, 2019 at 4:12 PM Daniel Vetter daniel@ffwll.ch wrote:
On Tue, Sep 3, 2019 at 9:45 PM Kenny Ho y2kenny@gmail.com wrote: > On Tue, Sep 3, 2019 at 3:57 AM Daniel Vetter daniel@ffwll.ch wrote: > > Iterating over minors for cgroups sounds very, very wrong. Why do we care > > whether a buffer was allocated through kms dumb vs render nodes? > > > > I'd expect all the cgroup stuff to only work on drm_device, if it does > > care about devices. > > > > (I didn't look through the patch series to find out where exactly you're > > using this, so maybe I'm off the rails here). > > I am exposing this to remove the need to keep track of a separate list > of available drm_device in the system (to remove the registering and > unregistering of drm_device to the cgroup subsystem and just use > drm_minor as the single source of truth.) I am only filtering out the > render nodes minor because they point to the same drm_device and is > confusing. > > Perhaps I missed an obvious way to list the drm devices without > iterating through the drm_minors? (I probably jumped to the minors > because $major:$minor is the convention to address devices in cgroup.)
Create your own if there's nothing, because you need to anyway:
- You need special locking anyway, we can't just block on the idr lock
for everything.
- This needs to refcount drm_device, no the minors.
Iterating over stuff still feels kinda wrong still, because normally the way we register/unregister userspace api (and cgroups isn't anything else from a drm driver pov) is by adding more calls to drm_dev_register/unregister. If you put a drm_cg_register/unregister call in there we have a clean separation, and you can track all the currently active devices however you want. Iterating over objects that can be hotunplugged any time tends to get really complicated really quickly.
Um... I thought this is what I had previously. Did I misunderstood your feedback from v3? Doesn't drm_minor already include all these facilities so isn't creating my own kind of reinventing the wheel? (as I did previously?) drm_minor_register is called inside drm_dev_register so isn't leveraging existing drm_minor facilities much better solution?
Hm the previous version already dropped out of my inbox, so hard to find it again. And I couldn't find this in archieves. Do you have pointers?
I thought the previous version did cgroup init separately from drm_device setup, and I guess I suggested that it should be moved int drm_dev_register/unregister?
Anyway, I don't think reusing the drm_minor registration makes sense, since we want to be on the drm_device, not on the minor. Which is a bit awkward for cgroups, which wants to identify devices using major.minor pairs. But I guess drm is the first subsystem where 1 device can be exposed through multiple minors ...
Tejun, any suggestions on this?
Anyway, I think just leveraging existing code because it can be abused to make it fit for us doesn't make sense. E.g. for the kms side we also don't piggy-back on top of drm_minor_register (it would be technically possible), but instead we have drm_modeset_register_all(). -Daniel
Kenny
> > Kenny > > > -Daniel > > > > > --- > > > drivers/gpu/drm/drm_drv.c | 19 +++++++++++++++++++ > > > drivers/gpu/drm/drm_internal.h | 4 ---- > > > include/drm/drm_drv.h | 4 ++++ > > > 3 files changed, 23 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > > > index 862621494a93..000cddabd970 100644 > > > --- a/drivers/gpu/drm/drm_drv.c > > > +++ b/drivers/gpu/drm/drm_drv.c > > > @@ -254,11 +254,13 @@ struct drm_minor *drm_minor_acquire(unsigned int minor_id) > > > > > > return minor; > > > } > > > +EXPORT_SYMBOL(drm_minor_acquire); > > > > > > void drm_minor_release(struct drm_minor *minor) > > > { > > > drm_dev_put(minor->dev); > > > } > > > +EXPORT_SYMBOL(drm_minor_release); > > > > > > /** > > > * DOC: driver instance overview > > > @@ -1078,6 +1080,23 @@ int drm_dev_set_unique(struct drm_device *dev, const char *name) > > > } > > > EXPORT_SYMBOL(drm_dev_set_unique); > > > > > > +/** > > > + * drm_minor_for_each - Iterate through all stored DRM minors > > > + * @fn: Function to be called for each pointer. > > > + * @data: Data passed to callback function. > > > + * > > > + * The callback function will be called for each @drm_minor entry, passing > > > + * the minor, the entry and @data. > > > + * > > > + * If @fn returns anything other than %0, the iteration stops and that > > > + * value is returned from this function. > > > + */ > > > +int drm_minor_for_each(int (*fn)(int id, void *p, void *data), void *data) > > > +{ > > > + return idr_for_each(&drm_minors_idr, fn, data); > > > +} > > > +EXPORT_SYMBOL(drm_minor_for_each); > > > + > > > /* > > > * DRM Core > > > * The DRM core module initializes all global DRM objects and makes them > > > diff --git a/drivers/gpu/drm/drm_internal.h b/drivers/gpu/drm/drm_internal.h > > > index e19ac7ca602d..6bfad76f8e78 100644 > > > --- a/drivers/gpu/drm/drm_internal.h > > > +++ b/drivers/gpu/drm/drm_internal.h > > > @@ -54,10 +54,6 @@ void drm_prime_destroy_file_private(struct drm_prime_file_private *prime_fpriv); > > > void drm_prime_remove_buf_handle_locked(struct drm_prime_file_private *prime_fpriv, > > > struct dma_buf *dma_buf); > > > > > > -/* drm_drv.c */ > > > -struct drm_minor *drm_minor_acquire(unsigned int minor_id); > > > -void drm_minor_release(struct drm_minor *minor); > > > - > > > /* drm_vblank.c */ > > > void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe); > > > void drm_vblank_cleanup(struct drm_device *dev); > > > diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h > > > index 68ca736c548d..24f8d054c570 100644 > > > --- a/include/drm/drm_drv.h > > > +++ b/include/drm/drm_drv.h > > > @@ -799,5 +799,9 @@ static inline bool drm_drv_uses_atomic_modeset(struct drm_device *dev) > > > > > > int drm_dev_set_unique(struct drm_device *dev, const char *name); > > > > > > +int drm_minor_for_each(int (*fn)(int id, void *p, void *data), void *data); > > > + > > > +struct drm_minor *drm_minor_acquire(unsigned int minor_id); > > > +void drm_minor_release(struct drm_minor *minor); > > > > > > #endif > > > -- > > > 2.22.0 > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
-- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch