| Message ID | 20190829060533.32315-2-Kenny.Ho@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | new cgroup controller for gpu/drm subsystem | expand |
On Thu, Aug 29, 2019 at 02:05:18AM -0400, Kenny Ho wrote: > To allow other subsystems to iterate through all stored DRM minors and > act upon them. > > Also exposes drm_minor_acquire and drm_minor_release for other subsystem > to handle drm_minor. DRM cgroup controller is the initial consumer of > this new features. > > Change-Id: I7c4b67ce6b31f06d1037b03435386ff5b8144ca5 > Signed-off-by: Kenny Ho <Kenny.Ho@amd.com> 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). -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 >
On Tue, Sep 3, 2019 at 3:57 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Thu, Aug 29, 2019 at 02:05:18AM -0400, Kenny Ho wrote: > > To allow other subsystems to iterate through all stored DRM minors and > > act upon them. > > > > Also exposes drm_minor_acquire and drm_minor_release for other subsystem > > to handle drm_minor. DRM cgroup controller is the initial consumer of > > this new features. > > > > Change-Id: I7c4b67ce6b31f06d1037b03435386ff5b8144ca5 > > Signed-off-by: Kenny Ho <Kenny.Ho@amd.com> > > 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.) 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
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: > > > > On Thu, Aug 29, 2019 at 02:05:18AM -0400, Kenny Ho wrote: > > > To allow other subsystems to iterate through all stored DRM minors and > > > act upon them. > > > > > > Also exposes drm_minor_acquire and drm_minor_release for other subsystem > > > to handle drm_minor. DRM cgroup controller is the initial consumer of > > > this new features. > > > > > > Change-Id: I7c4b67ce6b31f06d1037b03435386ff5b8144ca5 > > > Signed-off-by: Kenny Ho <Kenny.Ho@amd.com> > > > > 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. -Daniel > > 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
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? 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
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
Hi Daniel, This is the previous patch relevant to this discussion: https://patchwork.freedesktop.org/patch/314343/ 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. 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 >
(resent in plain text mode) Hi Daniel, This is the previous patch relevant to this discussion: https://patchwork.freedesktop.org/patch/314343/ 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. 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
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. 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. -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
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? > 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? 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
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
On Thu, Sep 5, 2019 at 4:32 PM Daniel Vetter <daniel@ffwll.ch> wrote: > *snip* > 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. Um... I think I get what you are saying, but isn't this a matter of the cgroup controller doing a drm_dev_get when using the drm_minor? Or that won't work because it's possible to have a valid drm_minor but invalid drm_device in it? I understand it's an extra level of indirection but since the convention for addressing device in cgroup is using $major:$minor I don't see a way to escape this. (Tejun actually already made a comment on my earlier RFC where I didn't follow the major:minor convention strictly.) Kenny > > > 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
On Thu, Sep 05, 2019 at 05:26:08PM -0400, Kenny Ho wrote: > On Thu, Sep 5, 2019 at 4:32 PM Daniel Vetter <daniel@ffwll.ch> wrote: > > > *snip* > > 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. > > Um... I think I get what you are saying, but isn't this a matter of > the cgroup controller doing a drm_dev_get when using the drm_minor? > Or that won't work because it's possible to have a valid drm_minor but > invalid drm_device in it? I understand it's an extra level of > indirection but since the convention for addressing device in cgroup > is using $major:$minor I don't see a way to escape this. (Tejun > actually already made a comment on my earlier RFC where I didn't > follow the major:minor convention strictly.) drm_device is the object that controls lifetime and everything, that's why you need to do a drm_dev_get and all that in some places. Going through the minor really feels like a distraction. And yes we have a bit a mess between cgroups insisting on using the minor, and drm_device having more than 1 minor for the same underlying physical resource. Just because the uapi is a bit a mess in that regard doesn't mean we should pull that mess into the kernel implementation imo. -Daniel > > Kenny > > > > > 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
Hello, On Wed, Sep 04, 2019 at 10:54:34AM +0200, Daniel Vetter wrote: > 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? I'm not extremely attached to maj:min. It's nice in that it'd be consistent with blkcg but it already isn't the nicest of identifiers for block devices. If using maj:min is reasonably straight forward for gpus even if not perfect, I'd prefer going with maj:min. Otherwise, please feel free to use the ID best for GPUs - hopefully something which is easy to understand, consistent with IDs used elsewhere and easy to build tooling around. Thanks.
On Fri, Sep 6, 2019 at 5:29 PM Tejun Heo <tj@kernel.org> wrote: > > Hello, > > On Wed, Sep 04, 2019 at 10:54:34AM +0200, Daniel Vetter wrote: > > 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? > > I'm not extremely attached to maj:min. It's nice in that it'd be > consistent with blkcg but it already isn't the nicest of identifiers > for block devices. If using maj:min is reasonably straight forward > for gpus even if not perfect, I'd prefer going with maj:min. > Otherwise, please feel free to use the ID best for GPUs - hopefully > something which is easy to understand, consistent with IDs used > elsewhere and easy to build tooling around. Block devices are a great example I think. How do you handle the partitions on that? For drm we also have a main minor interface, and then the render-only interface on drivers that support it. So if blkcg handles that by only exposing the primary maj:min pair, I think we can go with that and it's all nicely consistent. -Daniel
Hello, Daniel. On Fri, Sep 06, 2019 at 05:36:02PM +0200, Daniel Vetter wrote: > Block devices are a great example I think. How do you handle the > partitions on that? For drm we also have a main minor interface, and cgroup IO controllers only distribute hardware IO capacity and are blind to partitions. As there's always the whole device MAJ:MIN for block devices, we only use that. > then the render-only interface on drivers that support it. So if blkcg > handles that by only exposing the primary maj:min pair, I think we can > go with that and it's all nicely consistent. Ah yeah, that sounds equivalent. Great. Thanks.
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
To allow other subsystems to iterate through all stored DRM minors and act upon them. Also exposes drm_minor_acquire and drm_minor_release for other subsystem to handle drm_minor. DRM cgroup controller is the initial consumer of this new features. Change-Id: I7c4b67ce6b31f06d1037b03435386ff5b8144ca5 Signed-off-by: Kenny Ho <Kenny.Ho@amd.com> --- 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(-)