[Public]
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
> Sent: Monday, November 11, 2024 5:30
> On 10/11/2024 15:41, Yunxiang Li wrote:
> > Make drm-active- optional just like drm-resident- and drm-purgeable-.
>
> As Jani has already commented the commit message needs some work.
>
> > Signed-off-by: Yunxiang Li <Yunxiang.Li@xxxxxxx>
> > CC: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > CC: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > CC: amd-gfx@xxxxxxxxxxxxxxxxxxxxx
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 1 +
> > drivers/gpu/drm/drm_file.c | 13 +++++++------
> > drivers/gpu/drm/i915/i915_drm_client.c | 1 +
> > drivers/gpu/drm/xe/xe_drm_client.c | 1 +
> > include/drm/drm_gem.h | 14 ++++++++------
> > 5 files changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > index df2cf5c339255..7717e3e4f05b5 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c
> > @@ -97,6 +97,7 @@ void amdgpu_show_fdinfo(struct drm_printer *p,
> > struct drm_file *file)
> >
> > drm_print_memory_stats(p,
> > &stats[i].drm,
> > + DRM_GEM_OBJECT_ACTIVE |
> > DRM_GEM_OBJECT_RESIDENT |
> > DRM_GEM_OBJECT_PURGEABLE,
> > pl_name[i]);
> > diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> > index e285fcc28c59c..fd06671054723 100644
> > --- a/drivers/gpu/drm/drm_file.c
> > +++ b/drivers/gpu/drm/drm_file.c
> > @@ -884,7 +884,9 @@ void drm_print_memory_stats(struct drm_printer *p,
> > {
> > print_size(p, "total", region, stats->private + stats->shared);
> > print_size(p, "shared", region, stats->shared);
> > - print_size(p, "active", region, stats->active);
> > +
> > + if (supported_status & DRM_GEM_OBJECT_ACTIVE)
> > + print_size(p, "active", region, stats->active);
> >
> > if (supported_status & DRM_GEM_OBJECT_RESIDENT)
> > print_size(p, "resident", region, stats->resident); @@ -917,15
> > +919,13 @@ void drm_show_memory_stats(struct drm_printer *p, struct
> > drm_file *file)
> >
> > if (obj->funcs && obj->funcs->status) {
> > s = obj->funcs->status(obj);
> > - supported_status = DRM_GEM_OBJECT_RESIDENT |
> > - DRM_GEM_OBJECT_PURGEABLE;
> > + supported_status |= s;
>
> I think this is correct and I think I've raised that it should be like this when the code
> was originally added. I only don't remember what was the argument to keep it
> hardcoded, if there was any. Adding Rob in case he can remember.
>
> > }
> >
> > - if (drm_gem_object_is_shared_for_memory_stats(obj)) {
> > + if (drm_gem_object_is_shared_for_memory_stats(obj))
> > status.shared += obj->size;
> > - } else {
> > + else
> > status.private += obj->size;
> > - }
>
> Drive by cleanup, okay.
>
> >
> > if (s & DRM_GEM_OBJECT_RESIDENT) {
> > status.resident += add_size;
> > @@ -938,6 +938,7 @@ void drm_show_memory_stats(struct drm_printer *p,
> > struct drm_file *file)
> >
> > if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
> > status.active += add_size;
> > + supported_status |= DRM_GEM_OBJECT_ACTIVE;
>
> I wonder what behaviour we should have here if the driver has reported
> DRM_GEM_OBJECT_ACTIVE via its status vfunc. Like should it be like this:
>
> if ((s & DRM_GEM_OBJECT_ACTIVE) ||
> !dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true))) {
> ...
>
> ?
>
> So if some driver starts reporting this flag _and_ is still calling
> drm_show_memory_stats(), it's version of activity tracking is used instead of the
> the dma_resv based test.
I don't think that is feasible with the current API, because there's no way to differentiate "driver thinks a BO is not active" and "driver does not implement activity tracking". I think it's probably okay to keep it like this until someone wants to do it differently and refactor then.
Teddy
> >
> > /* If still active, don't count as purgeable: */
> > s &= ~DRM_GEM_OBJECT_PURGEABLE;
> > diff --git a/drivers/gpu/drm/i915/i915_drm_client.c
> > b/drivers/gpu/drm/i915/i915_drm_client.c
> > index f586825054918..168d7375304bc 100644
> > --- a/drivers/gpu/drm/i915/i915_drm_client.c
> > +++ b/drivers/gpu/drm/i915/i915_drm_client.c
> > @@ -102,6 +102,7 @@ static void show_meminfo(struct drm_printer *p, struct
> drm_file *file)
> > for_each_memory_region(mr, i915, id)
> > drm_print_memory_stats(p,
> > &stats[id],
> > + DRM_GEM_OBJECT_ACTIVE |
> > DRM_GEM_OBJECT_RESIDENT |
> > DRM_GEM_OBJECT_PURGEABLE,
> > mr->uabi_name);
> > diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
> > b/drivers/gpu/drm/xe/xe_drm_client.c
> > index 6a26923fa10e0..54941b4e850c4 100644
> > --- a/drivers/gpu/drm/xe/xe_drm_client.c
> > +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> > @@ -229,6 +229,7 @@ static void show_meminfo(struct drm_printer *p, struct
> drm_file *file)
> > if (man) {
> > drm_print_memory_stats(p,
> > &stats[mem_type],
> > + DRM_GEM_OBJECT_ACTIVE |
> > DRM_GEM_OBJECT_RESIDENT |
> > (mem_type != XE_PL_SYSTEM ? 0 :
> > DRM_GEM_OBJECT_PURGEABLE),
> diff --git
> > a/include/drm/drm_gem.h b/include/drm/drm_gem.h index
> > bae4865b2101a..584ffdf5c2542 100644
> > --- a/include/drm/drm_gem.h
> > +++ b/include/drm/drm_gem.h
> > @@ -48,19 +48,21 @@ struct drm_gem_object;
> > * enum drm_gem_object_status - bitmask of object state for fdinfo reporting
> > * @DRM_GEM_OBJECT_RESIDENT: object is resident in memory (ie. not
> unpinned)
> > * @DRM_GEM_OBJECT_PURGEABLE: object marked as purgeable by
> > userspace
> > + * @DRM_GEM_OBJECT_ACTIVE: object is currently used by an active
> > + submission
> > *
> > * Bitmask of status used for fdinfo memory stats, see
> > &drm_gem_object_funcs.status
> > - * and drm_show_fdinfo(). Note that an object can
> > DRM_GEM_OBJECT_PURGEABLE if
> > - * it still active or not resident, in which case drm_show_fdinfo()
> > will not
> > + * and drm_show_fdinfo(). Note that an object can report
> > + DRM_GEM_OBJECT_PURGEABLE
> > + * and be active or not resident, in which case drm_show_fdinfo()
> > + will not
> > * account for it as purgeable. So drivers do not need to check if
> > the buffer
> > - * is idle and resident to return this bit. (Ie. userspace can mark
> > a buffer
> > - * as purgeable even while it is still busy on the GPU.. it does not
> > _actually_
> > - * become puregeable until it becomes idle. The status gem object
> > func does
> > - * not need to consider this.)
> > + * is idle and resident to return this bit, i.e. userspace can mark a
> > + buffer as
> > + * purgeable even while it is still busy on the GPU. It whill not get
> > + reported
>
> Good cleanup.
>
> s/whill/will/
>
> > + * in the puregeable stats until it becomes idle. The status gem
> > + object func
> > + * does not need to consider this.
> > */
> > enum drm_gem_object_status {
> > DRM_GEM_OBJECT_RESIDENT = BIT(0),
> > DRM_GEM_OBJECT_PURGEABLE = BIT(1),
> > + DRM_GEM_OBJECT_ACTIVE = BIT(2),
> > };
> >
> > /**
>
> Regards,
>
> Tvrtko
