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

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

Hi.

On Thu, Aug 29, 2019 at 02:05:28AM -0400, Kenny Ho Kenny.Ho@amd.com wrote:

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1256,6 +1257,12 @@ int ttm_bo_validate(struct ttm_buffer_object *bo, [...]

• move_delay /= 2000; /* check every half period in ms*/
  

[...] diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c [...] @@ -382,6 +548,25 @@ static ssize_t drmcg_limit_write(struct kernfs_open_file *of, char *buf, [...]

• 	if (rc || val < 2000) {
  

This just caught my eye and it may be simply caused by RFC-ness of the series but I'd suggest turning this into a constant with descriptive name.

My 2 cents, Michal

Thanks for the feedback Christian. I am still digging into this one. Daniel suggested leveraging the Shrinker API for the functionality of this commit in RFC v3 but I am still trying to figure it out how/if ttm fit with shrinker (though the idea behind the shrinker API seems fairly straightforward as far as I understand it currently.)

Regards, Kenny

On Thu, Aug 29, 2019 at 3:08 AM Koenig, Christian Christian.Koenig@amd.com wrote:

Am 29.08.19 um 08:05 schrieb Kenny Ho:

...

Allow DRM TTM memory manager to register a work_struct, such that, when a drmcgrp is under memory pressure, memory reclaiming can be triggered immediately.

Change-Id: I25ac04e2db9c19ff12652b88ebff18b44b2706d8 Signed-off-by: Kenny Ho Kenny.Ho@amd.com

---

drivers/gpu/drm/ttm/ttm_bo.c | 49 +++++++++++++++++++++++++++++++++ include/drm/drm_cgroup.h | 16 +++++++++++ include/drm/ttm/ttm_bo_driver.h | 2 ++ kernel/cgroup/drm.c | 30 ++++++++++++++++++++ 4 files changed, 97 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index d7e3d3128ebb..72efae694b7e 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1590,6 +1590,46 @@ int ttm_bo_evict_mm(struct ttm_bo_device *bdev,

unsigned mem_type)

...

} EXPORT_SYMBOL(ttm_bo_evict_mm);

+static void ttm_bo_reclaim_wq(struct work_struct *work) +{

• struct ttm_operation_ctx ctx = {
  

•         .interruptible = false,
  

•         .no_wait_gpu = false,
  

•         .flags = TTM_OPT_FLAG_FORCE_ALLOC
  

• };
  

• struct ttm_mem_type_manager *man =
  

•     container_of(work, struct ttm_mem_type_manager, reclaim_wq);
  

• struct ttm_bo_device *bdev = man->bdev;
  

• struct dma_fence *fence;
  

• int mem_type;
  

• int ret;
  

• for (mem_type = 0; mem_type < TTM_NUM_MEM_TYPES; mem_type++)
  

•         if (&bdev->man[mem_type] == man)
  

•                 break;
  

• WARN_ON(mem_type >= TTM_NUM_MEM_TYPES);
  

• if (mem_type >= TTM_NUM_MEM_TYPES)
  

•         return;
  

• if (!drmcg_mem_pressure_scan(bdev, mem_type))
  

•         return;
  

• ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx, NULL);
  

• if (ret)
  

•         return;
  

• spin_lock(&man->move_lock);
  

• fence = dma_fence_get(man->move);
  

• spin_unlock(&man->move_lock);
  

• if (fence) {
  

•         ret = dma_fence_wait(fence, false);
  

•         dma_fence_put(fence);
  

• }
  

Why do you want to block for the fence here? That is a rather bad idea and would break pipe-lining.

Apart from that I don't think we should put that into TTM.

Instead drmcg_register_device_mm() should get a function pointer which is called from a work item when the group is under pressure.

TTM can then provides the function which can be called, but the actually registration is job of the device and not TTM.

Regards, Christian.

...

+}

• int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type, unsigned long p_size) {

@@ -1624,6 +1664,13 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev,

unsigned type,

...

          INIT_LIST_HEAD(&man->lru[i]);
  man->move = NULL;

• pr_err("drmcg %p type %d\n", bdev->ddev, type);
  

• if (type <= TTM_PL_VRAM) {
  

•         INIT_WORK(&man->reclaim_wq, ttm_bo_reclaim_wq);
  

•         drmcg_register_device_mm(bdev->ddev, type,
  

&man->reclaim_wq);

...

• }
  

• return 0;
  

  } EXPORT_SYMBOL(ttm_bo_init_mm);

@@ -1701,6 +1748,8 @@ int ttm_bo_device_release(struct ttm_bo_device

*bdev)

...

          man = &bdev->man[i];
          if (man->has_type) {
                  man->use_type = false;

•                 drmcg_unregister_device_mm(bdev->ddev, i);
  

•                 cancel_work_sync(&man->reclaim_wq);
                  if ((i != TTM_PL_SYSTEM) && ttm_bo_clean_mm(bdev,
  

i)) {

...

                          ret = -EBUSY;
                          pr_err("DRM memory manager type %d is not

clean\n",

...

diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h index c11df388fdf2..6d9707e1eb72 100644 --- a/include/drm/drm_cgroup.h +++ b/include/drm/drm_cgroup.h @@ -5,6 +5,7 @@ #define __DRM_CGROUP_H__

#include <linux/cgroup_drm.h> +#include <linux/workqueue.h> #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h>

@@ -25,12 +26,17 @@ struct drmcg_props { s64 mem_bw_avg_bytes_per_us_default;

  s64                     mem_highs_default[TTM_PL_PRIV+1];

• struct work_struct      *mem_reclaim_wq[TTM_PL_PRIV];
  

  };

  #ifdef CONFIG_CGROUP_DRM

  void drmcg_device_update(struct drm_device *device); void drmcg_device_early_init(struct drm_device *device);

+void drmcg_register_device_mm(struct drm_device *dev, unsigned int type,

•         struct work_struct *wq);
  

+void drmcg_unregister_device_mm(struct drm_device *dev, unsigned int

type);

...

bool drmcg_try_chg_bo_alloc(struct drmcg *drmcg, struct drm_device

*dev,

...

          size_t size);

void drmcg_unchg_bo_alloc(struct drmcg *drmcg, struct drm_device *dev, @@ -53,6 +59,16 @@ static inline void drmcg_device_early_init(struct

drm_device *device)

...

{ }

+static inline void drmcg_register_device_mm(struct drm_device *dev,

•         unsigned int type, struct work_struct *wq)
  

+{ +}

+static inline void drmcg_unregister_device_mm(struct drm_device *dev,

•         unsigned int type)
  

+{ +}

• static inline void drmcg_try_chg_bo_alloc(struct drmcg *drmcg, struct drm_device *dev, size_t size) {

diff --git a/include/drm/ttm/ttm_bo_driver.h

b/include/drm/ttm/ttm_bo_driver.h

...

index e1a805d65b83..529cef92bcf6 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -205,6 +205,8 @@ struct ttm_mem_type_manager { * Protected by @move_lock. */ struct dma_fence *move;

• struct work_struct reclaim_wq;
  

  };

  /**

diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c index 04fb9a398740..0ea7f0619e25 100644 --- a/kernel/cgroup/drm.c +++ b/kernel/cgroup/drm.c @@ -804,6 +804,29 @@ void drmcg_device_early_init(struct drm_device *dev) } EXPORT_SYMBOL(drmcg_device_early_init);

+void drmcg_register_device_mm(struct drm_device *dev, unsigned int type,

•         struct work_struct *wq)
  

+{

• if (dev == NULL || type >= TTM_PL_PRIV)
  

•         return;
  

• mutex_lock(&drmcg_mutex);
  

• dev->drmcg_props.mem_reclaim_wq[type] = wq;
  

• mutex_unlock(&drmcg_mutex);
  

+} +EXPORT_SYMBOL(drmcg_register_device_mm);

+void drmcg_unregister_device_mm(struct drm_device *dev, unsigned int

type)

...

+{

• if (dev == NULL || type >= TTM_PL_PRIV)
  

•         return;
  

• mutex_lock(&drmcg_mutex);
  

• dev->drmcg_props.mem_reclaim_wq[type] = NULL;
  

• mutex_unlock(&drmcg_mutex);
  

+} +EXPORT_SYMBOL(drmcg_unregister_device_mm);

• /**
  • drmcg_try_chg_bo_alloc - charge GEM buffer usage for a device and

cgroup

...

• @drmcg: the DRM cgroup to be charged to

@@ -1013,6 +1036,13 @@ void drmcg_mem_track_move(struct

ttm_buffer_object *old_bo, bool evict,

...

          ddr->mem_bw_stats[DRMCG_MEM_BW_ATTR_BYTE_CREDIT]
                  -= move_in_bytes;

•         if (dev->drmcg_props.mem_reclaim_wq[new_mem_type]
  

•                 != NULL &&
  

•                 ddr->mem_stats[new_mem_type] >
  

•                         ddr->mem_highs[new_mem_type])
  

•                 schedule_work(dev->
  

•                         drmcg_props.mem_reclaim_wq[new_mem_type]);
  }
  mutex_unlock(&dev->drmcg_mutex);
  

  }

Yes, and I think it has quite a lot of coupling with mm's page and pressure mechanisms. My current thought is to just copy the API but have a separate implementation of "ttm_shrinker" and "ttm_shrinker_control" or something like that. I am certainly happy to listen to additional feedbacks and suggestions.

Regards, Kenny

On Thu, Aug 29, 2019 at 10:12 AM Koenig, Christian Christian.Koenig@amd.com wrote:

Yeah, that's also a really good idea as well.

The problem with the shrinker API is that it only applies to system memory currently.

So you won't have a distinction which domain you need to evict stuff from.

Regards, Christian.

Am 29.08.19 um 16:07 schrieb Kenny Ho:

Thanks for the feedback Christian. I am still digging into this one. Daniel suggested leveraging the Shrinker API for the functionality of this commit in RFC v3 but I am still trying to figure it out how/if ttm fit with shrinker (though the idea behind the shrinker API seems fairly straightforward as far as I understand it currently.)

Regards, Kenny

On Thu, Aug 29, 2019 at 3:08 AM Koenig, Christian Christian.Koenig@amd.com wrote:

...

Am 29.08.19 um 08:05 schrieb Kenny Ho:

...

Allow DRM TTM memory manager to register a work_struct, such that, when a drmcgrp is under memory pressure, memory reclaiming can be triggered immediately.

Change-Id: I25ac04e2db9c19ff12652b88ebff18b44b2706d8 Signed-off-by: Kenny Ho Kenny.Ho@amd.com

---

drivers/gpu/drm/ttm/ttm_bo.c | 49 +++++++++++++++++++++++++++++++++ include/drm/drm_cgroup.h | 16 +++++++++++ include/drm/ttm/ttm_bo_driver.h | 2 ++ kernel/cgroup/drm.c | 30 ++++++++++++++++++++ 4 files changed, 97 insertions(+)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index d7e3d3128ebb..72efae694b7e 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -1590,6 +1590,46 @@ int ttm_bo_evict_mm(struct ttm_bo_device *bdev, unsigned mem_type) } EXPORT_SYMBOL(ttm_bo_evict_mm);

+static void ttm_bo_reclaim_wq(struct work_struct *work) +{

• struct ttm_operation_ctx ctx = {
  

•         .interruptible = false,
  

•         .no_wait_gpu = false,
  

•         .flags = TTM_OPT_FLAG_FORCE_ALLOC
  

• };
  

• struct ttm_mem_type_manager *man =
  

•     container_of(work, struct ttm_mem_type_manager, reclaim_wq);
  

• struct ttm_bo_device *bdev = man->bdev;
  

• struct dma_fence *fence;
  

• int mem_type;
  

• int ret;
  

• for (mem_type = 0; mem_type < TTM_NUM_MEM_TYPES; mem_type++)
  

•         if (&bdev->man[mem_type] == man)
  

•                 break;
  

• WARN_ON(mem_type >= TTM_NUM_MEM_TYPES);
  

• if (mem_type >= TTM_NUM_MEM_TYPES)
  

•         return;
  

• if (!drmcg_mem_pressure_scan(bdev, mem_type))
  

•         return;
  

• ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx, NULL);
  

• if (ret)
  

•         return;
  

• spin_lock(&man->move_lock);
  

• fence = dma_fence_get(man->move);
  

• spin_unlock(&man->move_lock);
  

• if (fence) {
  

•         ret = dma_fence_wait(fence, false);
  

•         dma_fence_put(fence);
  

• }
  

Why do you want to block for the fence here? That is a rather bad idea and would break pipe-lining.

Apart from that I don't think we should put that into TTM.

Instead drmcg_register_device_mm() should get a function pointer which is called from a work item when the group is under pressure.

TTM can then provides the function which can be called, but the actually registration is job of the device and not TTM.

Regards, Christian.

...

+}

• int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type, unsigned long p_size) {

@@ -1624,6 +1664,13 @@ int ttm_bo_init_mm(struct ttm_bo_device *bdev, unsigned type, INIT_LIST_HEAD(&man->lru[i]); man->move = NULL;

• pr_err("drmcg %p type %d\n", bdev->ddev, type);
  

• if (type <= TTM_PL_VRAM) {
  

•         INIT_WORK(&man->reclaim_wq, ttm_bo_reclaim_wq);
  

•         drmcg_register_device_mm(bdev->ddev, type, &man->reclaim_wq);
  

• }
  

• return 0;
  

  } EXPORT_SYMBOL(ttm_bo_init_mm);

@@ -1701,6 +1748,8 @@ int ttm_bo_device_release(struct ttm_bo_device *bdev) man = &bdev->man[i]; if (man->has_type) { man->use_type = false;

•                 drmcg_unregister_device_mm(bdev->ddev, i);
  

•                 cancel_work_sync(&man->reclaim_wq);
                  if ((i != TTM_PL_SYSTEM) && ttm_bo_clean_mm(bdev, i)) {
                          ret = -EBUSY;
                          pr_err("DRM memory manager type %d is not clean\n",
  

diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h index c11df388fdf2..6d9707e1eb72 100644 --- a/include/drm/drm_cgroup.h +++ b/include/drm/drm_cgroup.h @@ -5,6 +5,7 @@ #define __DRM_CGROUP_H__

#include <linux/cgroup_drm.h> +#include <linux/workqueue.h> #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h>

@@ -25,12 +26,17 @@ struct drmcg_props { s64 mem_bw_avg_bytes_per_us_default;

  s64                     mem_highs_default[TTM_PL_PRIV+1];

• struct work_struct      *mem_reclaim_wq[TTM_PL_PRIV];
  

  };

  #ifdef CONFIG_CGROUP_DRM

  void drmcg_device_update(struct drm_device *device); void drmcg_device_early_init(struct drm_device *device);

+void drmcg_register_device_mm(struct drm_device *dev, unsigned int type,

•         struct work_struct *wq);
  

+void drmcg_unregister_device_mm(struct drm_device *dev, unsigned int type); bool drmcg_try_chg_bo_alloc(struct drmcg *drmcg, struct drm_device *dev, size_t size); void drmcg_unchg_bo_alloc(struct drmcg *drmcg, struct drm_device *dev, @@ -53,6 +59,16 @@ static inline void drmcg_device_early_init(struct drm_device *device) { }

+static inline void drmcg_register_device_mm(struct drm_device *dev,

•         unsigned int type, struct work_struct *wq)
  

+{ +}

+static inline void drmcg_unregister_device_mm(struct drm_device *dev,

•         unsigned int type)
  

+{ +}

• static inline void drmcg_try_chg_bo_alloc(struct drmcg *drmcg, struct drm_device *dev, size_t size) {

diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h index e1a805d65b83..529cef92bcf6 100644 --- a/include/drm/ttm/ttm_bo_driver.h +++ b/include/drm/ttm/ttm_bo_driver.h @@ -205,6 +205,8 @@ struct ttm_mem_type_manager { * Protected by @move_lock. */ struct dma_fence *move;

• struct work_struct reclaim_wq;
  

  };

  /**

diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c index 04fb9a398740..0ea7f0619e25 100644 --- a/kernel/cgroup/drm.c +++ b/kernel/cgroup/drm.c @@ -804,6 +804,29 @@ void drmcg_device_early_init(struct drm_device *dev) } EXPORT_SYMBOL(drmcg_device_early_init);

+void drmcg_register_device_mm(struct drm_device *dev, unsigned int type,

•         struct work_struct *wq)
  

+{

• if (dev == NULL || type >= TTM_PL_PRIV)
  

•         return;
  

• mutex_lock(&drmcg_mutex);
  

• dev->drmcg_props.mem_reclaim_wq[type] = wq;
  

• mutex_unlock(&drmcg_mutex);
  

+} +EXPORT_SYMBOL(drmcg_register_device_mm);

+void drmcg_unregister_device_mm(struct drm_device *dev, unsigned int type) +{

• if (dev == NULL || type >= TTM_PL_PRIV)
  

•         return;
  

• mutex_lock(&drmcg_mutex);
  

• dev->drmcg_props.mem_reclaim_wq[type] = NULL;
  

• mutex_unlock(&drmcg_mutex);
  

+} +EXPORT_SYMBOL(drmcg_unregister_device_mm);

• /**
  • drmcg_try_chg_bo_alloc - charge GEM buffer usage for a device and cgroup
  • @drmcg: the DRM cgroup to be charged to

@@ -1013,6 +1036,13 @@ void drmcg_mem_track_move(struct ttm_buffer_object *old_bo, bool evict,

          ddr->mem_bw_stats[DRMCG_MEM_BW_ATTR_BYTE_CREDIT]
                  -= move_in_bytes;

•         if (dev->drmcg_props.mem_reclaim_wq[new_mem_type]
  

•                 != NULL &&
  

•                 ddr->mem_stats[new_mem_type] >
  

•                         ddr->mem_highs[new_mem_type])
  

•                 schedule_work(dev->
  

•                         drmcg_props.mem_reclaim_wq[new_mem_type]);
  }
  mutex_unlock(&dev->drmcg_mutex);
  

  }

On 2019-08-29 2:05 a.m., Kenny Ho wrote:

drm.lgpu A read-write nested-keyed file which exists on all cgroups. Each entry is keyed by the DRM device's major:minor.

     lgpu stands for logical GPU, it is an abstraction used to
     subdivide a physical DRM device for the purpose of resource
     management.

     The lgpu is a discrete quantity that is device specific (i.e.
     some DRM devices may have 64 lgpus while others may have 100
     lgpus.)  The lgpu is a single quantity with two representations
     denoted by the following nested keys.

       =====     ========================================
       count     Representing lgpu as anonymous resource
       list      Representing lgpu as named resource
       =====     ========================================

     For example:
     226:0 count=256 list=0-255
     226:1 count=4 list=0,2,4,6
     226:2 count=32 list=32-63

     lgpu is represented by a bitmap and uses the bitmap_parselist
     kernel function so the list key input format is a
     comma-separated list of decimal numbers and ranges.

     Consecutively set bits are shown as two hyphen-separated decimal
     numbers, the smallest and largest bit numbers set in the range.
     Optionally each range can be postfixed to denote that only parts
     of it should be set.  The range will divided to groups of
     specific size.
     Syntax: range:used_size/group_size
     Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769

     The count key is the hamming weight / hweight of the bitmap.

     Both count and list accept the max and default keywords.

     Some DRM devices may only support lgpu as anonymous resources.
     In such case, the significance of the position of the set bits
     in list will be ignored.

     This lgpu resource supports the 'allocation' resource
     distribution model.

Change-Id: I1afcacf356770930c7f925df043e51ad06ceb98e Signed-off-by: Kenny Ho Kenny.Ho@amd.com

The description sounds reasonable to me and maps well to the CU masking feature in our GPUs.

It would also allow us to do more coarse-grained masking for example to guarantee balanced allocation of CUs across shader engines or partitioning of memory bandwidth or CP pipes (if that is supported by the hardware/firmware).

I can't comment on the code as I'm unfamiliar with the details of the cgroup code.

Acked-by: Felix Kuehling Felix.Kuehling@amd.com

---

Documentation/admin-guide/cgroup-v2.rst | 46 ++++++++ include/drm/drm_cgroup.h | 4 + include/linux/cgroup_drm.h | 6 ++ kernel/cgroup/drm.c | 135 ++++++++++++++++++++++++ 4 files changed, 191 insertions(+)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 87a195133eaa..57f18469bd76 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1958,6 +1958,52 @@ DRM Interface Files Set largest allocation for /dev/dri/card1 to 4MB echo "226:1 4m" > drm.buffer.peak.max

• drm.lgpu
• A read-write nested-keyed file which exists on all cgroups.
• Each entry is keyed by the DRM device's major:minor.
• lgpu stands for logical GPU, it is an abstraction used to
• subdivide a physical DRM device for the purpose of resource
• management.
• The lgpu is a discrete quantity that is device specific (i.e.
• some DRM devices may have 64 lgpus while others may have 100
• lgpus.) The lgpu is a single quantity with two representations
• denoted by the following nested keys.

•  =====     ========================================
  

•  count     Representing lgpu as anonymous resource
  

•  list      Representing lgpu as named resource
  

•  =====     ========================================
  

• For example:
• 226:0 count=256 list=0-255
• 226:1 count=4 list=0,2,4,6
• 226:2 count=32 list=32-63
• lgpu is represented by a bitmap and uses the bitmap_parselist
• kernel function so the list key input format is a
• comma-separated list of decimal numbers and ranges.
• Consecutively set bits are shown as two hyphen-separated decimal
• numbers, the smallest and largest bit numbers set in the range.
• Optionally each range can be postfixed to denote that only parts
• of it should be set. The range will divided to groups of
• specific size.
• Syntax: range:used_size/group_size
• Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
• The count key is the hamming weight / hweight of the bitmap.
• Both count and list accept the max and default keywords.
• Some DRM devices may only support lgpu as anonymous resources.
• In such case, the significance of the position of the set bits
• in list will be ignored.
• This lgpu resource supports the 'allocation' resource
• distribution model.
• GEM Buffer Ownership

diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h index 6d9707e1eb72..a8d6be0b075b 100644 --- a/include/drm/drm_cgroup.h +++ b/include/drm/drm_cgroup.h @@ -6,6 +6,7 @@

#include <linux/cgroup_drm.h> #include <linux/workqueue.h> +#include <linux/types.h> #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h>

@@ -28,6 +29,9 @@ struct drmcg_props { s64 mem_highs_default[TTM_PL_PRIV+1];

struct work_struct *mem_reclaim_wq[TTM_PL_PRIV];

• int lgpu_capacity;

•    DECLARE_BITMAP(lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

  };

  #ifdef CONFIG_CGROUP_DRM

diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h index c56cfe74d1a6..7b1cfc4ce4c3 100644 --- a/include/linux/cgroup_drm.h +++ b/include/linux/cgroup_drm.h @@ -14,6 +14,8 @@ /* limit defined per the way drm_minor_alloc operates */ #define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)

+#define MAX_DRMCG_LGPU_CAPACITY 256

• enum drmcg_mem_bw_attr { DRMCG_MEM_BW_ATTR_BYTE_MOVED, /* for calulating 'instantaneous' bw */ DRMCG_MEM_BW_ATTR_ACCUM_US, /* for calulating 'instantaneous' bw */

@@ -32,6 +34,7 @@ enum drmcg_res_type { DRMCG_TYPE_MEM_PEAK, DRMCG_TYPE_BANDWIDTH, DRMCG_TYPE_BANDWIDTH_PERIOD_BURST,

• DRMCG_TYPE_LGPU, __DRMCG_TYPE_LAST, };

@@ -58,6 +61,9 @@ struct drmcg_device_resource { s64 mem_bw_stats[__DRMCG_MEM_BW_ATTR_LAST]; s64 mem_bw_limits_bytes_in_period; s64 mem_bw_limits_avg_bytes_per_us;

• s64 lgpu_used;

• DECLARE_BITMAP(lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY); };

  /**

diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c index 0ea7f0619e25..18c4368e2c29 100644 --- a/kernel/cgroup/drm.c +++ b/kernel/cgroup/drm.c @@ -9,6 +9,7 @@ #include <linux/cgroup_drm.h> #include <linux/ktime.h> #include <linux/kernel.h> +#include <linux/bitmap.h> #include <drm/drm_file.h> #include <drm/drm_drv.h> #include <drm/ttm/ttm_bo_api.h> @@ -52,6 +53,9 @@ static char const *mem_bw_attr_names[] = { #define MEM_BW_LIMITS_NAME_AVG "avg_bytes_per_us" #define MEM_BW_LIMITS_NAME_BURST "bytes_in_period"

+#define LGPU_LIMITS_NAME_LIST "list" +#define LGPU_LIMITS_NAME_COUNT "count"

• static struct drmcg *root_drmcg __read_mostly;

  static int drmcg_css_free_fn(int id, void *ptr, void *data)

@@ -115,6 +119,10 @@ static inline int init_drmcg_single(struct drmcg *drmcg, struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) ddr->mem_highs[i] = dev->drmcg_props.mem_highs_default[i];

• bitmap_copy(ddr->lgpu_allocated, dev->drmcg_props.lgpu_slots,

• 	MAX_DRMCG_LGPU_CAPACITY);
  

• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex); return 0; }

@@ -280,6 +288,14 @@ static void drmcg_print_limits(struct drmcg_device_resource *ddr, MEM_BW_LIMITS_NAME_AVG, ddr->mem_bw_limits_avg_bytes_per_us); break;

• case DRMCG_TYPE_LGPU:

• seq_printf(sf, "%s=%lld %s=%*pbl\n",
  

• 		LGPU_LIMITS_NAME_COUNT,
  

• 		ddr->lgpu_used,
  

• 		LGPU_LIMITS_NAME_LIST,
  

• 		dev->drmcg_props.lgpu_capacity,
  

• 		ddr->lgpu_allocated);
  

• break;
  

  default: seq_puts(sf, "\n"); break;

@@ -314,6 +330,15 @@ static void drmcg_print_default(struct drmcg_props *props, MEM_BW_LIMITS_NAME_AVG, props->mem_bw_avg_bytes_per_us_default); break;

• case DRMCG_TYPE_LGPU:

• seq_printf(sf, "%s=%d %s=%*pbl\n",
  

• 		LGPU_LIMITS_NAME_COUNT,
  

• 		bitmap_weight(props->lgpu_slots,
  

• 			props->lgpu_capacity),
  

• 		LGPU_LIMITS_NAME_LIST,
  

• 		props->lgpu_capacity,
  

• 		props->lgpu_slots);
  

• break;
  

  default: seq_puts(sf, "\n"); break;

@@ -407,9 +432,21 @@ static void drmcg_value_apply(struct drm_device *dev, s64 *dst, s64 val) mutex_unlock(&dev->drmcg_mutex); }

+static void drmcg_lgpu_values_apply(struct drm_device *dev,

• struct drmcg_device_resource *ddr, unsigned long *val)
  

+{

• mutex_lock(&dev->drmcg_mutex);
• bitmap_copy(ddr->lgpu_allocated, val, MAX_DRMCG_LGPU_CAPACITY);
• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex);

+}

• static void drmcg_nested_limit_parse(struct kernfs_open_file *of, struct drm_device *dev, char *attrs) {
• DECLARE_BITMAP(tmp_bitmap, MAX_DRMCG_LGPU_CAPACITY);
• DECLARE_BITMAP(chk_bitmap, MAX_DRMCG_LGPU_CAPACITY); enum drmcg_res_type type = DRMCG_CTF_PRIV2RESTYPE(of_cft(of)->private); struct drmcg *drmcg = css_to_drmcg(of_css(of));

@@ -501,6 +538,83 @@ static void drmcg_nested_limit_parse(struct kernfs_open_file *of, continue; } break; /* DRMCG_TYPE_MEM */

• case DRMCG_TYPE_LGPU:
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) &&
  

• 		strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) )
  

• 		continue;
  

•                    if (!strcmp("max", sval) ||
  

• 			!strcmp("default", sval)) {
  

• 		if (parent != NULL)
  

• 			drmcg_lgpu_values_apply(dev, ddr,
  

• 				parent->dev_resources[minor]->
  

• 				lgpu_allocated);
  

• 		else
  

• 			drmcg_lgpu_values_apply(dev, ddr,
  

• 				props->lgpu_slots);
  

• 		continue;
  

• 	}
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) == 0) {
  

• 		p_max = parent == NULL ? props->lgpu_capacity:
  

• 			bitmap_weight(
  

• 			parent->dev_resources[minor]->
  

• 			lgpu_allocated, props->lgpu_capacity);
  

• 		rc = drmcg_process_limit_s64_val(sval,
  

• 			false, p_max, p_max, &val);
  

• 		if (rc || val < 0) {
  

• 			drmcg_pr_cft_err(drmcg, rc, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

• 		bitmap_zero(tmp_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY);
  

• 		bitmap_set(tmp_bitmap, 0, val);
  

• 	}
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) == 0) {
  

• 		rc = bitmap_parselist(sval, tmp_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY);
  

• 		if (rc) {
  

• 			drmcg_pr_cft_err(drmcg, rc, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

•                    	bitmap_andnot(chk_bitmap, tmp_bitmap,
  

• 			props->lgpu_slots,
  

• 			MAX_DRMCG_LGPU_CAPACITY);
  

•                    	if (!bitmap_empty(chk_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY)) {
  

• 			drmcg_pr_cft_err(drmcg, 0, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

• 	}
  

•                    if (parent != NULL) {
  

• 		bitmap_and(chk_bitmap, tmp_bitmap,
  

• 		parent->dev_resources[minor]->lgpu_allocated,
  

• 		props->lgpu_capacity);
  

• 		if (bitmap_empty(chk_bitmap,
  

• 				props->lgpu_capacity)) {
  

• 			drmcg_pr_cft_err(drmcg, 0,
  

• 					cft_name, minor);
  

• 			continue;
  

• 		}
  

• 	}
  

• 	drmcg_lgpu_values_apply(dev, ddr, tmp_bitmap);
  

• 	break; /* DRMCG_TYPE_LGPU */
  

  default: break; } /* switch (type) */

@@ -606,6 +720,7 @@ static ssize_t drmcg_limit_write(struct kernfs_open_file *of, char *buf, break; case DRMCG_TYPE_BANDWIDTH: case DRMCG_TYPE_MEM:

• case DRMCG_TYPE_LGPU:
  drmcg_nested_limit_parse(of, dm->dev, sattr);
  break;
  

  default:

@@ -731,6 +846,20 @@ struct cftype files[] = { .private = DRMCG_CTF_PRIV(DRMCG_TYPE_BANDWIDTH, DRMCG_FTYPE_DEFAULT), },

• {

• .name = "lgpu",
  

• .seq_show = drmcg_seq_show,
  

• .write = drmcg_limit_write,
  

• .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

• 				DRMCG_FTYPE_LIMIT),
  

• },
• {

• .name = "lgpu.default",
  

• .seq_show = drmcg_seq_show,
  

• .flags = CFTYPE_ONLY_ON_ROOT,
  

• .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

• 				DRMCG_FTYPE_DEFAULT),
  

• }, { } /* terminate */ };

@@ -744,6 +873,10 @@ struct cgroup_subsys drm_cgrp_subsys = {

static inline void drmcg_update_cg_tree(struct drm_device *dev) {

•    bitmap_zero(dev->drmcg_props.lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

•    bitmap_fill(dev->drmcg_props.lgpu_slots,
  

• 	dev->drmcg_props.lgpu_capacity);
  

• /* init cgroups created before registration (i.e. root cgroup) */ if (root_drmcg != NULL) { struct cgroup_subsys_state *pos;

@@ -800,6 +933,8 @@ void drmcg_device_early_init(struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) dev->drmcg_props.mem_highs_default[i] = S64_MAX;

• dev->drmcg_props.lgpu_capacity = MAX_DRMCG_LGPU_CAPACITY;
• drmcg_update_cg_tree(dev); } EXPORT_SYMBOL(drmcg_device_early_init);

Hi Daniel,

Can you elaborate what you mean in more details? The goal of lgpu is to provide the ability to subdivide a GPU device and give those slices to different users as needed. I don't think there is anything controversial or vendor specific here as requests for this are well documented. The underlying representation is just a bitmap, which is neither unprecedented nor vendor specific (bitmap is used in cpuset for instance.)

An implementation of this abstraction is not hardware specific either. For example, one can associate a virtual function in SRIOV as a lgpu. Alternatively, a device can also declare to have 100 lgpus and treat the lgpu quantity as a percentage representation of GPU subdivision. The fact that an abstraction works well with a vendor implementation does not make it a "prettification" of a vendor feature (by this logic, I hope you are not implying an abstraction is only valid if it does not work with amd CU masking because that seems fairly partisan.)

Did I misread your characterization of this patch?

Regards, Kenny

On Wed, Oct 9, 2019 at 6:31 AM Daniel Vetter daniel@ffwll.ch wrote:

On Tue, Oct 08, 2019 at 06:53:18PM +0000, Kuehling, Felix wrote:

...

On 2019-08-29 2:05 a.m., Kenny Ho wrote:

...

drm.lgpu A read-write nested-keyed file which exists on all cgroups. Each entry is keyed by the DRM device's major:minor.

     lgpu stands for logical GPU, it is an abstraction used to
     subdivide a physical DRM device for the purpose of resource
     management.

     The lgpu is a discrete quantity that is device specific (i.e.
     some DRM devices may have 64 lgpus while others may have 100
     lgpus.)  The lgpu is a single quantity with two representations
     denoted by the following nested keys.

       =====     ========================================
       count     Representing lgpu as anonymous resource
       list      Representing lgpu as named resource
       =====     ========================================

     For example:
     226:0 count=256 list=0-255
     226:1 count=4 list=0,2,4,6
     226:2 count=32 list=32-63

     lgpu is represented by a bitmap and uses the bitmap_parselist
     kernel function so the list key input format is a
     comma-separated list of decimal numbers and ranges.

     Consecutively set bits are shown as two hyphen-separated decimal
     numbers, the smallest and largest bit numbers set in the range.
     Optionally each range can be postfixed to denote that only parts
     of it should be set.  The range will divided to groups of
     specific size.
     Syntax: range:used_size/group_size
     Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769

     The count key is the hamming weight / hweight of the bitmap.

     Both count and list accept the max and default keywords.

     Some DRM devices may only support lgpu as anonymous resources.
     In such case, the significance of the position of the set bits
     in list will be ignored.

     This lgpu resource supports the 'allocation' resource
     distribution model.

Change-Id: I1afcacf356770930c7f925df043e51ad06ceb98e Signed-off-by: Kenny Ho Kenny.Ho@amd.com

The description sounds reasonable to me and maps well to the CU masking feature in our GPUs.

It would also allow us to do more coarse-grained masking for example to guarantee balanced allocation of CUs across shader engines or partitioning of memory bandwidth or CP pipes (if that is supported by the hardware/firmware).

Hm, so this sounds like the definition for how this cgroup is supposed to work is "amd CU masking" (whatever that exactly is). And the abstract description is just prettification on top, but not actually the real definition you guys want.

I think adding a cgroup which is that much depending upon the hw implementation of the first driver supporting it is not a good idea. -Daniel

...

I can't comment on the code as I'm unfamiliar with the details of the cgroup code.

Acked-by: Felix Kuehling Felix.Kuehling@amd.com

...

---

Documentation/admin-guide/cgroup-v2.rst | 46 ++++++++ include/drm/drm_cgroup.h | 4 + include/linux/cgroup_drm.h | 6 ++ kernel/cgroup/drm.c | 135 ++++++++++++++++++++++++ 4 files changed, 191 insertions(+)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 87a195133eaa..57f18469bd76 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1958,6 +1958,52 @@ DRM Interface Files Set largest allocation for /dev/dri/card1 to 4MB echo "226:1 4m" > drm.buffer.peak.max

• drm.lgpu
• A read-write nested-keyed file which exists on all cgroups.
• Each entry is keyed by the DRM device's major:minor.
• lgpu stands for logical GPU, it is an abstraction used to
• subdivide a physical DRM device for the purpose of resource
• management.
• The lgpu is a discrete quantity that is device specific (i.e.
• some DRM devices may have 64 lgpus while others may have 100
• lgpus.) The lgpu is a single quantity with two representations
• denoted by the following nested keys.

• =====     ========================================
  

• count     Representing lgpu as anonymous resource
  

• list      Representing lgpu as named resource
  

• =====     ========================================
  

• For example:
• 226:0 count=256 list=0-255
• 226:1 count=4 list=0,2,4,6
• 226:2 count=32 list=32-63
• lgpu is represented by a bitmap and uses the bitmap_parselist
• kernel function so the list key input format is a
• comma-separated list of decimal numbers and ranges.
• Consecutively set bits are shown as two hyphen-separated decimal
• numbers, the smallest and largest bit numbers set in the range.
• Optionally each range can be postfixed to denote that only parts
• of it should be set. The range will divided to groups of
• specific size.
• Syntax: range:used_size/group_size
• Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
• The count key is the hamming weight / hweight of the bitmap.
• Both count and list accept the max and default keywords.
• Some DRM devices may only support lgpu as anonymous resources.
• In such case, the significance of the position of the set bits
• in list will be ignored.
• This lgpu resource supports the 'allocation' resource
• distribution model.
• GEM Buffer Ownership

diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h index 6d9707e1eb72..a8d6be0b075b 100644 --- a/include/drm/drm_cgroup.h +++ b/include/drm/drm_cgroup.h @@ -6,6 +6,7 @@

#include <linux/cgroup_drm.h> #include <linux/workqueue.h> +#include <linux/types.h> #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h>

@@ -28,6 +29,9 @@ struct drmcg_props { s64 mem_highs_default[TTM_PL_PRIV+1];

struct work_struct      *mem_reclaim_wq[TTM_PL_PRIV];

• int lgpu_capacity;

•    DECLARE_BITMAP(lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

  };

  #ifdef CONFIG_CGROUP_DRM

diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h index c56cfe74d1a6..7b1cfc4ce4c3 100644 --- a/include/linux/cgroup_drm.h +++ b/include/linux/cgroup_drm.h @@ -14,6 +14,8 @@ /* limit defined per the way drm_minor_alloc operates */ #define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)

+#define MAX_DRMCG_LGPU_CAPACITY 256

• enum drmcg_mem_bw_attr { DRMCG_MEM_BW_ATTR_BYTE_MOVED, /* for calulating 'instantaneous' bw */ DRMCG_MEM_BW_ATTR_ACCUM_US, /* for calulating 'instantaneous' bw */

@@ -32,6 +34,7 @@ enum drmcg_res_type { DRMCG_TYPE_MEM_PEAK, DRMCG_TYPE_BANDWIDTH, DRMCG_TYPE_BANDWIDTH_PERIOD_BURST,

• DRMCG_TYPE_LGPU, __DRMCG_TYPE_LAST, };

@@ -58,6 +61,9 @@ struct drmcg_device_resource { s64 mem_bw_stats[__DRMCG_MEM_BW_ATTR_LAST]; s64 mem_bw_limits_bytes_in_period; s64 mem_bw_limits_avg_bytes_per_us;

• s64 lgpu_used;

• DECLARE_BITMAP(lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY); };

  /**

diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c index 0ea7f0619e25..18c4368e2c29 100644 --- a/kernel/cgroup/drm.c +++ b/kernel/cgroup/drm.c @@ -9,6 +9,7 @@ #include <linux/cgroup_drm.h> #include <linux/ktime.h> #include <linux/kernel.h> +#include <linux/bitmap.h> #include <drm/drm_file.h> #include <drm/drm_drv.h> #include <drm/ttm/ttm_bo_api.h> @@ -52,6 +53,9 @@ static char const *mem_bw_attr_names[] = { #define MEM_BW_LIMITS_NAME_AVG "avg_bytes_per_us" #define MEM_BW_LIMITS_NAME_BURST "bytes_in_period"

+#define LGPU_LIMITS_NAME_LIST "list" +#define LGPU_LIMITS_NAME_COUNT "count"

• static struct drmcg *root_drmcg __read_mostly;

  static int drmcg_css_free_fn(int id, void *ptr, void *data)

@@ -115,6 +119,10 @@ static inline int init_drmcg_single(struct drmcg *drmcg, struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) ddr->mem_highs[i] = dev->drmcg_props.mem_highs_default[i];

• bitmap_copy(ddr->lgpu_allocated, dev->drmcg_props.lgpu_slots,

•               MAX_DRMCG_LGPU_CAPACITY);
  

• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex); return 0; }

@@ -280,6 +288,14 @@ static void drmcg_print_limits(struct drmcg_device_resource *ddr, MEM_BW_LIMITS_NAME_AVG, ddr->mem_bw_limits_avg_bytes_per_us); break;

• case DRMCG_TYPE_LGPU:

•       seq_printf(sf, "%s=%lld %s=%*pbl\n",
  

•                       LGPU_LIMITS_NAME_COUNT,
  

•                       ddr->lgpu_used,
  

•                       LGPU_LIMITS_NAME_LIST,
  

•                       dev->drmcg_props.lgpu_capacity,
  

•                       ddr->lgpu_allocated);
  

•       break;
  

  default: seq_puts(sf, "\n"); break;

@@ -314,6 +330,15 @@ static void drmcg_print_default(struct drmcg_props *props, MEM_BW_LIMITS_NAME_AVG, props->mem_bw_avg_bytes_per_us_default); break;

• case DRMCG_TYPE_LGPU:

•       seq_printf(sf, "%s=%d %s=%*pbl\n",
  

•                       LGPU_LIMITS_NAME_COUNT,
  

•                       bitmap_weight(props->lgpu_slots,
  

•                               props->lgpu_capacity),
  

•                       LGPU_LIMITS_NAME_LIST,
  

•                       props->lgpu_capacity,
  

•                       props->lgpu_slots);
  

•       break;
  

  default: seq_puts(sf, "\n"); break;

@@ -407,9 +432,21 @@ static void drmcg_value_apply(struct drm_device *dev, s64 *dst, s64 val) mutex_unlock(&dev->drmcg_mutex); }

+static void drmcg_lgpu_values_apply(struct drm_device *dev,

•       struct drmcg_device_resource *ddr, unsigned long *val)
  

+{

• mutex_lock(&dev->drmcg_mutex);
• bitmap_copy(ddr->lgpu_allocated, val, MAX_DRMCG_LGPU_CAPACITY);
• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex);

+}

• static void drmcg_nested_limit_parse(struct kernfs_open_file *of, struct drm_device *dev, char *attrs) {
• DECLARE_BITMAP(tmp_bitmap, MAX_DRMCG_LGPU_CAPACITY);
• DECLARE_BITMAP(chk_bitmap, MAX_DRMCG_LGPU_CAPACITY); enum drmcg_res_type type = DRMCG_CTF_PRIV2RESTYPE(of_cft(of)->private); struct drmcg *drmcg = css_to_drmcg(of_css(of));

@@ -501,6 +538,83 @@ static void drmcg_nested_limit_parse(struct kernfs_open_file *of, continue; } break; /* DRMCG_TYPE_MEM */

•       case DRMCG_TYPE_LGPU:
  

•               if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) &&
  

•                       strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) )
  

•                       continue;
  

•                    if (!strcmp("max", sval) ||
  

•                               !strcmp("default", sval)) {
  

•                       if (parent != NULL)
  

•                               drmcg_lgpu_values_apply(dev, ddr,
  

•                                       parent->dev_resources[minor]->
  

•                                       lgpu_allocated);
  

•                       else
  

•                               drmcg_lgpu_values_apply(dev, ddr,
  

•                                       props->lgpu_slots);
  

•                       continue;
  

•               }
  

•               if (strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) == 0) {
  

•                       p_max = parent == NULL ? props->lgpu_capacity:
  

•                               bitmap_weight(
  

•                               parent->dev_resources[minor]->
  

•                               lgpu_allocated, props->lgpu_capacity);
  

•                       rc = drmcg_process_limit_s64_val(sval,
  

•                               false, p_max, p_max, &val);
  

•                       if (rc || val < 0) {
  

•                               drmcg_pr_cft_err(drmcg, rc, cft_name,
  

•                                               minor);
  

•                               continue;
  

•                       }
  

•                       bitmap_zero(tmp_bitmap,
  

•                                       MAX_DRMCG_LGPU_CAPACITY);
  

•                       bitmap_set(tmp_bitmap, 0, val);
  

•               }
  

•               if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) == 0) {
  

•                       rc = bitmap_parselist(sval, tmp_bitmap,
  

•                                       MAX_DRMCG_LGPU_CAPACITY);
  

•                       if (rc) {
  

•                               drmcg_pr_cft_err(drmcg, rc, cft_name,
  

•                                               minor);
  

•                               continue;
  

•                       }
  

•                       bitmap_andnot(chk_bitmap, tmp_bitmap,
  

•                               props->lgpu_slots,
  

•                               MAX_DRMCG_LGPU_CAPACITY);
  

•                       if (!bitmap_empty(chk_bitmap,
  

•                                       MAX_DRMCG_LGPU_CAPACITY)) {
  

•                               drmcg_pr_cft_err(drmcg, 0, cft_name,
  

•                                               minor);
  

•                               continue;
  

•                       }
  

•               }
  

•                    if (parent != NULL) {
  

•                       bitmap_and(chk_bitmap, tmp_bitmap,
  

•                       parent->dev_resources[minor]->lgpu_allocated,
  

•                       props->lgpu_capacity);
  

•                       if (bitmap_empty(chk_bitmap,
  

•                                       props->lgpu_capacity)) {
  

•                               drmcg_pr_cft_err(drmcg, 0,
  

•                                               cft_name, minor);
  

•                               continue;
  

•                       }
  

•               }
  

•               drmcg_lgpu_values_apply(dev, ddr, tmp_bitmap);
  

•               break; /* DRMCG_TYPE_LGPU */
        default:
                break;
        } /* switch (type) */
  

@@ -606,6 +720,7 @@ static ssize_t drmcg_limit_write(struct kernfs_open_file *of, char *buf, break; case DRMCG_TYPE_BANDWIDTH: case DRMCG_TYPE_MEM:

•       case DRMCG_TYPE_LGPU:
                drmcg_nested_limit_parse(of, dm->dev, sattr);
                break;
        default:
  

@@ -731,6 +846,20 @@ struct cftype files[] = { .private = DRMCG_CTF_PRIV(DRMCG_TYPE_BANDWIDTH, DRMCG_FTYPE_DEFAULT), },

• {

•       .name = "lgpu",
  

•       .seq_show = drmcg_seq_show,
  

•       .write = drmcg_limit_write,
  

•       .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

•                                       DRMCG_FTYPE_LIMIT),
  

• },
• {

•       .name = "lgpu.default",
  

•       .seq_show = drmcg_seq_show,
  

•       .flags = CFTYPE_ONLY_ON_ROOT,
  

•       .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

•                                       DRMCG_FTYPE_DEFAULT),
  

• }, { } /* terminate */ };

@@ -744,6 +873,10 @@ struct cgroup_subsys drm_cgrp_subsys = {

static inline void drmcg_update_cg_tree(struct drm_device *dev) {

•    bitmap_zero(dev->drmcg_props.lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

•    bitmap_fill(dev->drmcg_props.lgpu_slots,
  

•               dev->drmcg_props.lgpu_capacity);
  

• /* init cgroups created before registration (i.e. root cgroup) */ if (root_drmcg != NULL) { struct cgroup_subsys_state *pos;

@@ -800,6 +933,8 @@ void drmcg_device_early_init(struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) dev->drmcg_props.mem_highs_default[i] = S64_MAX;

• dev->drmcg_props.lgpu_capacity = MAX_DRMCG_LGPU_CAPACITY;
• drmcg_update_cg_tree(dev); } EXPORT_SYMBOL(drmcg_device_early_init);

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On 2019-10-09 6:31, Daniel Vetter wrote:

On Tue, Oct 08, 2019 at 06:53:18PM +0000, Kuehling, Felix wrote:

...

The description sounds reasonable to me and maps well to the CU masking feature in our GPUs.

It would also allow us to do more coarse-grained masking for example to guarantee balanced allocation of CUs across shader engines or partitioning of memory bandwidth or CP pipes (if that is supported by the hardware/firmware).

Hm, so this sounds like the definition for how this cgroup is supposed to work is "amd CU masking" (whatever that exactly is). And the abstract description is just prettification on top, but not actually the real definition you guys want.

I think you're reading this as the opposite of what I was trying to say. Using CU masking is one possible implementation of LGPUs on AMD hardware. It's the one that Kenny implemented at the end of this patch series, and I pointed out some problems with that approach. Other ways to partition the hardware into LGPUs are conceivable. For example we're considering splitting it along the lines of shader engines, which is more coarse-grain and would also affect memory bandwidth available to each partition.

We could also consider partitioning pipes in our command processor, although that is not supported by our current CP scheduler firmware.

The bottom line is, the LGPU model proposed by Kenny is quite abstract and allows drivers implementing it a lot of flexibility depending on the capability of their hardware and firmware. We haven't settled on a final implementation choice even for AMD.

Regards,   Felix

I think adding a cgroup which is that much depending upon the hw implementation of the first driver supporting it is not a good idea. -Daniel

...

I can't comment on the code as I'm unfamiliar with the details of the cgroup code.

Acked-by: Felix Kuehling Felix.Kuehling@amd.com

...

---

Documentation/admin-guide/cgroup-v2.rst | 46 ++++++++ include/drm/drm_cgroup.h | 4 + include/linux/cgroup_drm.h | 6 ++ kernel/cgroup/drm.c | 135 ++++++++++++++++++++++++ 4 files changed, 191 insertions(+)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 87a195133eaa..57f18469bd76 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1958,6 +1958,52 @@ DRM Interface Files Set largest allocation for /dev/dri/card1 to 4MB echo "226:1 4m" > drm.buffer.peak.max

• drm.lgpu
• A read-write nested-keyed file which exists on all cgroups.
• Each entry is keyed by the DRM device's major:minor.
• lgpu stands for logical GPU, it is an abstraction used to
• subdivide a physical DRM device for the purpose of resource
• management.
• The lgpu is a discrete quantity that is device specific (i.e.
• some DRM devices may have 64 lgpus while others may have 100
• lgpus.) The lgpu is a single quantity with two representations
• denoted by the following nested keys.

•  =====     ========================================
  

•  count     Representing lgpu as anonymous resource
  

•  list      Representing lgpu as named resource
  

•  =====     ========================================
  

• For example:
• 226:0 count=256 list=0-255
• 226:1 count=4 list=0,2,4,6
• 226:2 count=32 list=32-63
• lgpu is represented by a bitmap and uses the bitmap_parselist
• kernel function so the list key input format is a
• comma-separated list of decimal numbers and ranges.
• Consecutively set bits are shown as two hyphen-separated decimal
• numbers, the smallest and largest bit numbers set in the range.
• Optionally each range can be postfixed to denote that only parts
• of it should be set. The range will divided to groups of
• specific size.
• Syntax: range:used_size/group_size
• Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
• The count key is the hamming weight / hweight of the bitmap.
• Both count and list accept the max and default keywords.
• Some DRM devices may only support lgpu as anonymous resources.
• In such case, the significance of the position of the set bits
• in list will be ignored.
• This lgpu resource supports the 'allocation' resource
• distribution model.
• GEM Buffer Ownership

diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h index 6d9707e1eb72..a8d6be0b075b 100644 --- a/include/drm/drm_cgroup.h +++ b/include/drm/drm_cgroup.h @@ -6,6 +6,7 @@

#include <linux/cgroup_drm.h> #include <linux/workqueue.h> +#include <linux/types.h> #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h>

@@ -28,6 +29,9 @@ struct drmcg_props { s64 mem_highs_default[TTM_PL_PRIV+1];

struct work_struct	*mem_reclaim_wq[TTM_PL_PRIV];

• int lgpu_capacity;

•    DECLARE_BITMAP(lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

  };

  #ifdef CONFIG_CGROUP_DRM

diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h index c56cfe74d1a6..7b1cfc4ce4c3 100644 --- a/include/linux/cgroup_drm.h +++ b/include/linux/cgroup_drm.h @@ -14,6 +14,8 @@ /* limit defined per the way drm_minor_alloc operates */ #define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)

+#define MAX_DRMCG_LGPU_CAPACITY 256

• enum drmcg_mem_bw_attr { DRMCG_MEM_BW_ATTR_BYTE_MOVED, /* for calulating 'instantaneous' bw */ DRMCG_MEM_BW_ATTR_ACCUM_US, /* for calulating 'instantaneous' bw */

@@ -32,6 +34,7 @@ enum drmcg_res_type { DRMCG_TYPE_MEM_PEAK, DRMCG_TYPE_BANDWIDTH, DRMCG_TYPE_BANDWIDTH_PERIOD_BURST,

• DRMCG_TYPE_LGPU, __DRMCG_TYPE_LAST, };

@@ -58,6 +61,9 @@ struct drmcg_device_resource { s64 mem_bw_stats[__DRMCG_MEM_BW_ATTR_LAST]; s64 mem_bw_limits_bytes_in_period; s64 mem_bw_limits_avg_bytes_per_us;

• s64 lgpu_used;

• DECLARE_BITMAP(lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY); };

  /**

diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c index 0ea7f0619e25..18c4368e2c29 100644 --- a/kernel/cgroup/drm.c +++ b/kernel/cgroup/drm.c @@ -9,6 +9,7 @@ #include <linux/cgroup_drm.h> #include <linux/ktime.h> #include <linux/kernel.h> +#include <linux/bitmap.h> #include <drm/drm_file.h> #include <drm/drm_drv.h> #include <drm/ttm/ttm_bo_api.h> @@ -52,6 +53,9 @@ static char const *mem_bw_attr_names[] = { #define MEM_BW_LIMITS_NAME_AVG "avg_bytes_per_us" #define MEM_BW_LIMITS_NAME_BURST "bytes_in_period"

+#define LGPU_LIMITS_NAME_LIST "list" +#define LGPU_LIMITS_NAME_COUNT "count"

• static struct drmcg *root_drmcg __read_mostly;

  static int drmcg_css_free_fn(int id, void *ptr, void *data)

@@ -115,6 +119,10 @@ static inline int init_drmcg_single(struct drmcg *drmcg, struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) ddr->mem_highs[i] = dev->drmcg_props.mem_highs_default[i];

• bitmap_copy(ddr->lgpu_allocated, dev->drmcg_props.lgpu_slots,

• 	MAX_DRMCG_LGPU_CAPACITY);
  

• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex); return 0; }

@@ -280,6 +288,14 @@ static void drmcg_print_limits(struct drmcg_device_resource *ddr, MEM_BW_LIMITS_NAME_AVG, ddr->mem_bw_limits_avg_bytes_per_us); break;

• case DRMCG_TYPE_LGPU:

• seq_printf(sf, "%s=%lld %s=%*pbl\n",
  

• 		LGPU_LIMITS_NAME_COUNT,
  

• 		ddr->lgpu_used,
  

• 		LGPU_LIMITS_NAME_LIST,
  

• 		dev->drmcg_props.lgpu_capacity,
  

• 		ddr->lgpu_allocated);
  

• break;
  

  default: seq_puts(sf, "\n"); break;

@@ -314,6 +330,15 @@ static void drmcg_print_default(struct drmcg_props *props, MEM_BW_LIMITS_NAME_AVG, props->mem_bw_avg_bytes_per_us_default); break;

• case DRMCG_TYPE_LGPU:

• seq_printf(sf, "%s=%d %s=%*pbl\n",
  

• 		LGPU_LIMITS_NAME_COUNT,
  

• 		bitmap_weight(props->lgpu_slots,
  

• 			props->lgpu_capacity),
  

• 		LGPU_LIMITS_NAME_LIST,
  

• 		props->lgpu_capacity,
  

• 		props->lgpu_slots);
  

• break;
  

  default: seq_puts(sf, "\n"); break;

@@ -407,9 +432,21 @@ static void drmcg_value_apply(struct drm_device *dev, s64 *dst, s64 val) mutex_unlock(&dev->drmcg_mutex); }

+static void drmcg_lgpu_values_apply(struct drm_device *dev,

• struct drmcg_device_resource *ddr, unsigned long *val)
  

+{

• mutex_lock(&dev->drmcg_mutex);
• bitmap_copy(ddr->lgpu_allocated, val, MAX_DRMCG_LGPU_CAPACITY);
• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex);

+}

• static void drmcg_nested_limit_parse(struct kernfs_open_file *of, struct drm_device *dev, char *attrs) {
• DECLARE_BITMAP(tmp_bitmap, MAX_DRMCG_LGPU_CAPACITY);
• DECLARE_BITMAP(chk_bitmap, MAX_DRMCG_LGPU_CAPACITY); enum drmcg_res_type type = DRMCG_CTF_PRIV2RESTYPE(of_cft(of)->private); struct drmcg *drmcg = css_to_drmcg(of_css(of));

@@ -501,6 +538,83 @@ static void drmcg_nested_limit_parse(struct kernfs_open_file *of, continue; } break; /* DRMCG_TYPE_MEM */

• case DRMCG_TYPE_LGPU:
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) &&
  

• 		strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) )
  

• 		continue;
  

•                    if (!strcmp("max", sval) ||
  

• 			!strcmp("default", sval)) {
  

• 		if (parent != NULL)
  

• 			drmcg_lgpu_values_apply(dev, ddr,
  

• 				parent->dev_resources[minor]->
  

• 				lgpu_allocated);
  

• 		else
  

• 			drmcg_lgpu_values_apply(dev, ddr,
  

• 				props->lgpu_slots);
  

• 		continue;
  

• 	}
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) == 0) {
  

• 		p_max = parent == NULL ? props->lgpu_capacity:
  

• 			bitmap_weight(
  

• 			parent->dev_resources[minor]->
  

• 			lgpu_allocated, props->lgpu_capacity);
  

• 		rc = drmcg_process_limit_s64_val(sval,
  

• 			false, p_max, p_max, &val);
  

• 		if (rc || val < 0) {
  

• 			drmcg_pr_cft_err(drmcg, rc, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

• 		bitmap_zero(tmp_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY);
  

• 		bitmap_set(tmp_bitmap, 0, val);
  

• 	}
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) == 0) {
  

• 		rc = bitmap_parselist(sval, tmp_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY);
  

• 		if (rc) {
  

• 			drmcg_pr_cft_err(drmcg, rc, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

•                    	bitmap_andnot(chk_bitmap, tmp_bitmap,
  

• 			props->lgpu_slots,
  

• 			MAX_DRMCG_LGPU_CAPACITY);
  

•                    	if (!bitmap_empty(chk_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY)) {
  

• 			drmcg_pr_cft_err(drmcg, 0, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

• 	}
  

•                    if (parent != NULL) {
  

• 		bitmap_and(chk_bitmap, tmp_bitmap,
  

• 		parent->dev_resources[minor]->lgpu_allocated,
  

• 		props->lgpu_capacity);
  

• 		if (bitmap_empty(chk_bitmap,
  

• 				props->lgpu_capacity)) {
  

• 			drmcg_pr_cft_err(drmcg, 0,
  

• 					cft_name, minor);
  

• 			continue;
  

• 		}
  

• 	}
  

• 	drmcg_lgpu_values_apply(dev, ddr, tmp_bitmap);
  

• 	break; /* DRMCG_TYPE_LGPU */
  default:
  	break;
  } /* switch (type) */
  

@@ -606,6 +720,7 @@ static ssize_t drmcg_limit_write(struct kernfs_open_file *of, char *buf, break; case DRMCG_TYPE_BANDWIDTH: case DRMCG_TYPE_MEM:

• case DRMCG_TYPE_LGPU:
  	drmcg_nested_limit_parse(of, dm->dev, sattr);
  	break;
  default:
  

@@ -731,6 +846,20 @@ struct cftype files[] = { .private = DRMCG_CTF_PRIV(DRMCG_TYPE_BANDWIDTH, DRMCG_FTYPE_DEFAULT), },

• {

• .name = "lgpu",
  

• .seq_show = drmcg_seq_show,
  

• .write = drmcg_limit_write,
  

• .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

• 				DRMCG_FTYPE_LIMIT),
  

• },
• {

• .name = "lgpu.default",
  

• .seq_show = drmcg_seq_show,
  

• .flags = CFTYPE_ONLY_ON_ROOT,
  

• .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

• 				DRMCG_FTYPE_DEFAULT),
  

• }, { } /* terminate */ };

@@ -744,6 +873,10 @@ struct cgroup_subsys drm_cgrp_subsys = {

static inline void drmcg_update_cg_tree(struct drm_device *dev) {

•    bitmap_zero(dev->drmcg_props.lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

•    bitmap_fill(dev->drmcg_props.lgpu_slots,
  

• 	dev->drmcg_props.lgpu_capacity);
  

• /* init cgroups created before registration (i.e. root cgroup) */ if (root_drmcg != NULL) { struct cgroup_subsys_state *pos;

@@ -800,6 +933,8 @@ void drmcg_device_early_init(struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) dev->drmcg_props.mem_highs_default[i] = S64_MAX;

• dev->drmcg_props.lgpu_capacity = MAX_DRMCG_LGPU_CAPACITY;
• drmcg_update_cg_tree(dev); } EXPORT_SYMBOL(drmcg_device_early_init);

On 2019-10-09 11:34, Daniel Vetter wrote:

On Wed, Oct 09, 2019 at 03:25:22PM +0000, Kuehling, Felix wrote:

...

On 2019-10-09 6:31, Daniel Vetter wrote:

...

On Tue, Oct 08, 2019 at 06:53:18PM +0000, Kuehling, Felix wrote:

...

The description sounds reasonable to me and maps well to the CU masking feature in our GPUs.

It would also allow us to do more coarse-grained masking for example to guarantee balanced allocation of CUs across shader engines or partitioning of memory bandwidth or CP pipes (if that is supported by the hardware/firmware).

Hm, so this sounds like the definition for how this cgroup is supposed to work is "amd CU masking" (whatever that exactly is). And the abstract description is just prettification on top, but not actually the real definition you guys want.

I think you're reading this as the opposite of what I was trying to say. Using CU masking is one possible implementation of LGPUs on AMD hardware. It's the one that Kenny implemented at the end of this patch series, and I pointed out some problems with that approach. Other ways to partition the hardware into LGPUs are conceivable. For example we're considering splitting it along the lines of shader engines, which is more coarse-grain and would also affect memory bandwidth available to each partition.

If this is supposed to be useful for admins then "other ways to partition the hw are conceivable" is the problem. This should be unique&clear for admins/end-users. Reading the implementation details and realizing that the actual meaning is "amd CU masking" isn't good enough by far, since that's meaningless on any other hw.

And if there's other ways to implement this cgroup for amd, it's also meaningless (to sysadmins/users) for amd hw.

...

We could also consider partitioning pipes in our command processor, although that is not supported by our current CP scheduler firmware.

The bottom line is, the LGPU model proposed by Kenny is quite abstract and allows drivers implementing it a lot of flexibility depending on the capability of their hardware and firmware. We haven't settled on a final implementation choice even for AMD.

That abstract model of essentially "anything goes" is the problem here imo. E.g. for cpu cgroups this would be similar to allowing the bitmaks to mean "cpu core" on one machine "physical die" on the next and maybe "hyperthread unit" on the 3rd. Useless for admins.

So if we have a gpu bitmaks thing that might mean a command submissio pipe on one hw (maybe matching what vk exposed, maybe not), some compute unit mask on the next and something entirely different (e.g. intel has so called GT slices with compute cores + more stuff around) on the 3rd vendor then that's not useful for admins.

The goal is to partition GPU compute resources to eliminate as much resource contention as possible between different partitions. Different hardware will have different capabilities to implement this. No implementation will be perfect. For example, even with CPU cores that are supposedly well defined, you can still have different behaviours depending on CPU cache architectures, NUMA and thermal management across CPU cores. The admin will need some knowledge of their hardware architecture to understand those effects that are not described by the abstract model of cgroups.

The LGPU model is deliberately flexible, because GPU architectures are much less standardized than CPU architectures. Expecting a common model that is both very specific and applicable to to all GPUs is unrealistic, in my opinion.

Regards,   Felix

-Daniel

...

Regards,   Felix

...

I think adding a cgroup which is that much depending upon the hw implementation of the first driver supporting it is not a good idea. -Daniel

...

I can't comment on the code as I'm unfamiliar with the details of the cgroup code.

Acked-by: Felix Kuehling Felix.Kuehling@amd.com

...

---

Documentation/admin-guide/cgroup-v2.rst |  46 ++++++++
include/drm/drm_cgroup.h                |   4 +
include/linux/cgroup_drm.h              |   6 ++
kernel/cgroup/drm.c                     | 135 ++++++++++++++++++++++++
4 files changed, 191 insertions(+)

diff --git a/Documentation/admin-guide/cgroup-v2.rst b/Documentation/admin-guide/cgroup-v2.rst index 87a195133eaa..57f18469bd76 100644 --- a/Documentation/admin-guide/cgroup-v2.rst +++ b/Documentation/admin-guide/cgroup-v2.rst @@ -1958,6 +1958,52 @@ DRM Interface Files Set largest allocation for /dev/dri/card1 to 4MB echo "226:1 4m" > drm.buffer.peak.max

• drm.lgpu
• A read-write nested-keyed file which exists on all cgroups.
• Each entry is keyed by the DRM device's major:minor.
• lgpu stands for logical GPU, it is an abstraction used to
• subdivide a physical DRM device for the purpose of resource
• management.
• The lgpu is a discrete quantity that is device specific (i.e.
• some DRM devices may have 64 lgpus while others may have 100
• lgpus.) The lgpu is a single quantity with two representations
• denoted by the following nested keys.

•  =====     ========================================
  

•  count     Representing lgpu as anonymous resource
  

•  list      Representing lgpu as named resource
  

•  =====     ========================================
  

• For example:
• 226:0 count=256 list=0-255
• 226:1 count=4 list=0,2,4,6
• 226:2 count=32 list=32-63
• lgpu is represented by a bitmap and uses the bitmap_parselist
• kernel function so the list key input format is a
• comma-separated list of decimal numbers and ranges.
• Consecutively set bits are shown as two hyphen-separated decimal
• numbers, the smallest and largest bit numbers set in the range.
• Optionally each range can be postfixed to denote that only parts
• of it should be set. The range will divided to groups of
• specific size.
• Syntax: range:used_size/group_size
• Example: 0-1023:2/256 ==> 0,1,256,257,512,513,768,769
• The count key is the hamming weight / hweight of the bitmap.
• Both count and list accept the max and default keywords.
• Some DRM devices may only support lgpu as anonymous resources.
• In such case, the significance of the position of the set bits
• in list will be ignored.
• This lgpu resource supports the 'allocation' resource
• distribution model.
• GEM Buffer Ownership

diff --git a/include/drm/drm_cgroup.h b/include/drm/drm_cgroup.h index 6d9707e1eb72..a8d6be0b075b 100644 --- a/include/drm/drm_cgroup.h +++ b/include/drm/drm_cgroup.h @@ -6,6 +6,7 @@

#include <linux/cgroup_drm.h>
#include <linux/workqueue.h>

+#include <linux/types.h> #include <drm/ttm/ttm_bo_api.h> #include <drm/ttm/ttm_bo_driver.h>

@@ -28,6 +29,9 @@ struct drmcg_props { s64 mem_highs_default[TTM_PL_PRIV+1];

	struct work_struct	*mem_reclaim_wq[TTM_PL_PRIV];

• int lgpu_capacity;

•    DECLARE_BITMAP(lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

  };

  #ifdef CONFIG_CGROUP_DRM

diff --git a/include/linux/cgroup_drm.h b/include/linux/cgroup_drm.h index c56cfe74d1a6..7b1cfc4ce4c3 100644 --- a/include/linux/cgroup_drm.h +++ b/include/linux/cgroup_drm.h @@ -14,6 +14,8 @@ /* limit defined per the way drm_minor_alloc operates */ #define MAX_DRM_DEV (64 * DRM_MINOR_RENDER)

+#define MAX_DRMCG_LGPU_CAPACITY 256

• enum drmcg_mem_bw_attr { DRMCG_MEM_BW_ATTR_BYTE_MOVED, /* for calulating 'instantaneous' bw */ DRMCG_MEM_BW_ATTR_ACCUM_US, /* for calulating 'instantaneous' bw */

@@ -32,6 +34,7 @@ enum drmcg_res_type { DRMCG_TYPE_MEM_PEAK, DRMCG_TYPE_BANDWIDTH, DRMCG_TYPE_BANDWIDTH_PERIOD_BURST,

• DRMCG_TYPE_LGPU, __DRMCG_TYPE_LAST, };

@@ -58,6 +61,9 @@ struct drmcg_device_resource { s64 mem_bw_stats[__DRMCG_MEM_BW_ATTR_LAST]; s64 mem_bw_limits_bytes_in_period; s64 mem_bw_limits_avg_bytes_per_us;

• s64 lgpu_used;

• DECLARE_BITMAP(lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY); };

  /**

diff --git a/kernel/cgroup/drm.c b/kernel/cgroup/drm.c index 0ea7f0619e25..18c4368e2c29 100644 --- a/kernel/cgroup/drm.c +++ b/kernel/cgroup/drm.c @@ -9,6 +9,7 @@ #include <linux/cgroup_drm.h> #include <linux/ktime.h> #include <linux/kernel.h> +#include <linux/bitmap.h> #include <drm/drm_file.h> #include <drm/drm_drv.h> #include <drm/ttm/ttm_bo_api.h> @@ -52,6 +53,9 @@ static char const *mem_bw_attr_names[] = { #define MEM_BW_LIMITS_NAME_AVG "avg_bytes_per_us" #define MEM_BW_LIMITS_NAME_BURST "bytes_in_period"

+#define LGPU_LIMITS_NAME_LIST "list" +#define LGPU_LIMITS_NAME_COUNT "count"

• static struct drmcg *root_drmcg __read_mostly;

  static int drmcg_css_free_fn(int id, void *ptr, void *data)

@@ -115,6 +119,10 @@ static inline int init_drmcg_single(struct drmcg *drmcg, struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) ddr->mem_highs[i] = dev->drmcg_props.mem_highs_default[i];

• bitmap_copy(ddr->lgpu_allocated, dev->drmcg_props.lgpu_slots,

• 	MAX_DRMCG_LGPU_CAPACITY);
  

• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);

• mutex_unlock(&dev->drmcg_mutex);
  return 0;
  

  }

@@ -280,6 +288,14 @@ static void drmcg_print_limits(struct drmcg_device_resource *ddr, MEM_BW_LIMITS_NAME_AVG, ddr->mem_bw_limits_avg_bytes_per_us); break;

• case DRMCG_TYPE_LGPU:

• seq_printf(sf, "%s=%lld %s=%*pbl\n",
  

• 		LGPU_LIMITS_NAME_COUNT,
  

• 		ddr->lgpu_used,
  

• 		LGPU_LIMITS_NAME_LIST,
  

• 		dev->drmcg_props.lgpu_capacity,
  

• 		ddr->lgpu_allocated);
  

• break;
  default:
  	seq_puts(sf, "\n");
  	break;
  

@@ -314,6 +330,15 @@ static void drmcg_print_default(struct drmcg_props *props, MEM_BW_LIMITS_NAME_AVG, props->mem_bw_avg_bytes_per_us_default); break;

• case DRMCG_TYPE_LGPU:

• seq_printf(sf, "%s=%d %s=%*pbl\n",
  

• 		LGPU_LIMITS_NAME_COUNT,
  

• 		bitmap_weight(props->lgpu_slots,
  

• 			props->lgpu_capacity),
  

• 		LGPU_LIMITS_NAME_LIST,
  

• 		props->lgpu_capacity,
  

• 		props->lgpu_slots);
  

• break;
  default:
  	seq_puts(sf, "\n");
  	break;
  

@@ -407,9 +432,21 @@ static void drmcg_value_apply(struct drm_device *dev, s64 *dst, s64 val) mutex_unlock(&dev->drmcg_mutex); }

+static void drmcg_lgpu_values_apply(struct drm_device *dev,

• struct drmcg_device_resource *ddr, unsigned long *val)
  

+{

• mutex_lock(&dev->drmcg_mutex);
• bitmap_copy(ddr->lgpu_allocated, val, MAX_DRMCG_LGPU_CAPACITY);
• ddr->lgpu_used = bitmap_weight(ddr->lgpu_allocated, MAX_DRMCG_LGPU_CAPACITY);
• mutex_unlock(&dev->drmcg_mutex);

+}

• static void drmcg_nested_limit_parse(struct kernfs_open_file *of, struct drm_device *dev, char *attrs) {
• DECLARE_BITMAP(tmp_bitmap, MAX_DRMCG_LGPU_CAPACITY);
• DECLARE_BITMAP(chk_bitmap, MAX_DRMCG_LGPU_CAPACITY); enum drmcg_res_type type = DRMCG_CTF_PRIV2RESTYPE(of_cft(of)->private); struct drmcg *drmcg = css_to_drmcg(of_css(of));

@@ -501,6 +538,83 @@ static void drmcg_nested_limit_parse(struct kernfs_open_file *of, continue; } break; /* DRMCG_TYPE_MEM */

• case DRMCG_TYPE_LGPU:
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) &&
  

• 		strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) )
  

• 		continue;
  

•                    if (!strcmp("max", sval) ||
  

• 			!strcmp("default", sval)) {
  

• 		if (parent != NULL)
  

• 			drmcg_lgpu_values_apply(dev, ddr,
  

• 				parent->dev_resources[minor]->
  

• 				lgpu_allocated);
  

• 		else
  

• 			drmcg_lgpu_values_apply(dev, ddr,
  

• 				props->lgpu_slots);
  

• 		continue;
  

• 	}
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_COUNT, 256) == 0) {
  

• 		p_max = parent == NULL ? props->lgpu_capacity:
  

• 			bitmap_weight(
  

• 			parent->dev_resources[minor]->
  

• 			lgpu_allocated, props->lgpu_capacity);
  

• 		rc = drmcg_process_limit_s64_val(sval,
  

• 			false, p_max, p_max, &val);
  

• 		if (rc || val < 0) {
  

• 			drmcg_pr_cft_err(drmcg, rc, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

• 		bitmap_zero(tmp_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY);
  

• 		bitmap_set(tmp_bitmap, 0, val);
  

• 	}
  

• 	if (strncmp(sname, LGPU_LIMITS_NAME_LIST, 256) == 0) {
  

• 		rc = bitmap_parselist(sval, tmp_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY);
  

• 		if (rc) {
  

• 			drmcg_pr_cft_err(drmcg, rc, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

•                    	bitmap_andnot(chk_bitmap, tmp_bitmap,
  

• 			props->lgpu_slots,
  

• 			MAX_DRMCG_LGPU_CAPACITY);
  

•                    	if (!bitmap_empty(chk_bitmap,
  

• 				MAX_DRMCG_LGPU_CAPACITY)) {
  

• 			drmcg_pr_cft_err(drmcg, 0, cft_name,
  

• 					minor);
  

• 			continue;
  

• 		}
  

• 	}
  

•                    if (parent != NULL) {
  

• 		bitmap_and(chk_bitmap, tmp_bitmap,
  

• 		parent->dev_resources[minor]->lgpu_allocated,
  

• 		props->lgpu_capacity);
  

• 		if (bitmap_empty(chk_bitmap,
  

• 				props->lgpu_capacity)) {
  

• 			drmcg_pr_cft_err(drmcg, 0,
  

• 					cft_name, minor);
  

• 			continue;
  

• 		}
  

• 	}
  

• 	drmcg_lgpu_values_apply(dev, ddr, tmp_bitmap);
  

• 	break; /* DRMCG_TYPE_LGPU */
  	default:
  		break;
  	} /* switch (type) */
  

@@ -606,6 +720,7 @@ static ssize_t drmcg_limit_write(struct kernfs_open_file *of, char *buf, break; case DRMCG_TYPE_BANDWIDTH: case DRMCG_TYPE_MEM:

• case DRMCG_TYPE_LGPU:
  		drmcg_nested_limit_parse(of, dm->dev, sattr);
  		break;
  	default:
  

@@ -731,6 +846,20 @@ struct cftype files[] = { .private = DRMCG_CTF_PRIV(DRMCG_TYPE_BANDWIDTH, DRMCG_FTYPE_DEFAULT), },

• {

• .name = "lgpu",
  

• .seq_show = drmcg_seq_show,
  

• .write = drmcg_limit_write,
  

• .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

• 				DRMCG_FTYPE_LIMIT),
  

• },
• {

• .name = "lgpu.default",
  

• .seq_show = drmcg_seq_show,
  

• .flags = CFTYPE_ONLY_ON_ROOT,
  

• .private = DRMCG_CTF_PRIV(DRMCG_TYPE_LGPU,
  

• 				DRMCG_FTYPE_DEFAULT),
  

• }, { } /* terminate */ };

@@ -744,6 +873,10 @@ struct cgroup_subsys drm_cgrp_subsys = {

static inline void drmcg_update_cg_tree(struct drm_device *dev)
{

•    bitmap_zero(dev->drmcg_props.lgpu_slots, MAX_DRMCG_LGPU_CAPACITY);
  

•    bitmap_fill(dev->drmcg_props.lgpu_slots,
  

• 	dev->drmcg_props.lgpu_capacity);
  

• /* init cgroups created before registration (i.e. root cgroup) */
  if (root_drmcg != NULL) {
  	struct cgroup_subsys_state *pos;
  

@@ -800,6 +933,8 @@ void drmcg_device_early_init(struct drm_device *dev) for (i = 0; i <= TTM_PL_PRIV; i++) dev->drmcg_props.mem_highs_default[i] = S64_MAX;

• dev->drmcg_props.lgpu_capacity = MAX_DRMCG_LGPU_CAPACITY;

• drmcg_update_cg_tree(dev);
  

  } EXPORT_SYMBOL(drmcg_device_early_init);