* [PATCH v7 2/4] drm: make drm-active- stats optional [not found] <20241110154152.592-1-Yunxiang.Li@amd.com> @ 2024-11-10 15:41 ` Yunxiang Li 2024-11-11 9:14 ` Jani Nikula 2024-11-11 10:29 ` [PATCH v7 " Tvrtko Ursulin 0 siblings, 2 replies; 6+ messages in thread From: Yunxiang Li @ 2024-11-10 15:41 UTC (permalink / raw) To: amd-gfx, christian.koenig, tvrtko.ursulin Cc: Alexander.Deucher, Yunxiang Li, dri-devel, intel-gfx Make drm-active- optional just like drm-resident- and drm-purgeable-. Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com> CC: dri-devel@lists.freedesktop.org CC: intel-gfx@lists.freedesktop.org CC: amd-gfx@lists.freedesktop.org --- 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; } - 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; - } 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; /* 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 + * 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), }; /** -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/4] drm: make drm-active- stats optional 2024-11-10 15:41 ` [PATCH v7 2/4] drm: make drm-active- stats optional Yunxiang Li @ 2024-11-11 9:14 ` Jani Nikula 2024-11-11 14:28 ` [PATCH v8 " Yunxiang Li 2024-11-11 10:29 ` [PATCH v7 " Tvrtko Ursulin 1 sibling, 1 reply; 6+ messages in thread From: Jani Nikula @ 2024-11-11 9:14 UTC (permalink / raw) To: Yunxiang Li, amd-gfx, christian.koenig, tvrtko.ursulin Cc: Alexander.Deucher, Yunxiang Li, dri-devel, intel-gfx On Sun, 10 Nov 2024, Yunxiang Li <Yunxiang.Li@amd.com> wrote: > Make drm-active- optional just like drm-resident- and drm-purgeable-. Why? What does it mean? This is what the commit message should answer. > > Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com> > CC: dri-devel@lists.freedesktop.org > CC: intel-gfx@lists.freedesktop.org > CC: amd-gfx@lists.freedesktop.org > --- > 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; > } > > - 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; > - } > > 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; > > /* 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 > + * 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), > }; > > /** -- Jani Nikula, Intel ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v8 2/4] drm: make drm-active- stats optional 2024-11-11 9:14 ` Jani Nikula @ 2024-11-11 14:28 ` Yunxiang Li 0 siblings, 0 replies; 6+ messages in thread From: Yunxiang Li @ 2024-11-11 14:28 UTC (permalink / raw) To: dri-devel; +Cc: jani.nikula, Yunxiang Li, intel-gfx, amd-gfx When memory stats is generated fresh everytime by going though all the BOs, their active information is quite easy to get. But if the stats are tracked alongside BO's state changes this becomes harder since the job scheduling part doesn't really deal with individual buffers. Make drm-active- optional to enable amdgpu to switch to the second method. Signed-off-by: Yunxiang Li <Yunxiang.Li@amd.com> CC: dri-devel@lists.freedesktop.org CC: intel-gfx@lists.freedesktop.org CC: amd-gfx@lists.freedesktop.org --- 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; } - 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; - } 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; /* 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 + * 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), }; /** -- 2.34.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/4] drm: make drm-active- stats optional 2024-11-10 15:41 ` [PATCH v7 2/4] drm: make drm-active- stats optional Yunxiang Li 2024-11-11 9:14 ` Jani Nikula @ 2024-11-11 10:29 ` Tvrtko Ursulin 2024-11-18 15:17 ` Li, Yunxiang (Teddy) 1 sibling, 1 reply; 6+ messages in thread From: Tvrtko Ursulin @ 2024-11-11 10:29 UTC (permalink / raw) To: Yunxiang Li, amd-gfx, christian.koenig Cc: Alexander.Deucher, dri-devel, intel-gfx, Rob Clark 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@amd.com> > CC: dri-devel@lists.freedesktop.org > CC: intel-gfx@lists.freedesktop.org > CC: amd-gfx@lists.freedesktop.org > --- > 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. > > /* 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* RE: [PATCH v7 2/4] drm: make drm-active- stats optional 2024-11-11 10:29 ` [PATCH v7 " Tvrtko Ursulin @ 2024-11-18 15:17 ` Li, Yunxiang (Teddy) 2024-11-18 15:42 ` Tvrtko Ursulin 0 siblings, 1 reply; 6+ messages in thread From: Li, Yunxiang (Teddy) @ 2024-11-18 15:17 UTC (permalink / raw) To: Tvrtko Ursulin, amd-gfx@lists.freedesktop.org, Koenig, Christian Cc: Deucher, Alexander, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Rob Clark [Public] > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > 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@amd.com> > > CC: dri-devel@lists.freedesktop.org > > CC: intel-gfx@lists.freedesktop.org > > CC: amd-gfx@lists.freedesktop.org > > --- > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v7 2/4] drm: make drm-active- stats optional 2024-11-18 15:17 ` Li, Yunxiang (Teddy) @ 2024-11-18 15:42 ` Tvrtko Ursulin 0 siblings, 0 replies; 6+ messages in thread From: Tvrtko Ursulin @ 2024-11-18 15:42 UTC (permalink / raw) To: Li, Yunxiang (Teddy), amd-gfx@lists.freedesktop.org, Koenig, Christian Cc: Deucher, Alexander, dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, Rob Clark On 18/11/2024 15:17, Li, Yunxiang (Teddy) wrote: > [Public] > >> From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> >> 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@amd.com> >>> CC: dri-devel@lists.freedesktop.org >>> CC: intel-gfx@lists.freedesktop.org >>> CC: amd-gfx@lists.freedesktop.org >>> --- >>> 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. Ah yes, good point. I actually initially thought the same (that we would need additional "supports active reporting" flag) but then for some reason convinced myself it is possible without it. I agree it works as is. Regards, Tvrtko > > 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-11-18 15:47 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20241110154152.592-1-Yunxiang.Li@amd.com>
2024-11-10 15:41 ` [PATCH v7 2/4] drm: make drm-active- stats optional Yunxiang Li
2024-11-11 9:14 ` Jani Nikula
2024-11-11 14:28 ` [PATCH v8 " Yunxiang Li
2024-11-11 10:29 ` [PATCH v7 " Tvrtko Ursulin
2024-11-18 15:17 ` Li, Yunxiang (Teddy)
2024-11-18 15:42 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).