On Thu, Sep 5, 2019 at 10:21 PM Kenny Ho <y2kenny@xxxxxxxxx> wrote: > > On Thu, Sep 5, 2019 at 4:06 PM Daniel Vetter <daniel@xxxxxxxx> wrote: > > > > On Thu, Sep 5, 2019 at 8:28 PM Kenny Ho <y2kenny@xxxxxxxxx> 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@xxxxxxxx> 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@xxxxxxxx> wrote: > > > > > > On Tue, Sep 3, 2019 at 9:45 PM Kenny Ho <y2kenny@xxxxxxxxx> wrote: > > > > > > > On Tue, Sep 3, 2019 at 3:57 AM Daniel Vetter <daniel@xxxxxxxx> 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
