| Message ID | 20230922134700.235039-6-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fdinfo memory stats | expand |
On 22-09-2023 19:16, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > > At the moment memory region names are a bit too varied and too > inconsistent to be used for ABI purposes, like for upcoming fdinfo > memory stats. > > System memory can be either system or system-ttm. Local memory has the > instance number appended, others do not. Not only incosistent but thi > kind of implementation detail is uninteresting for intended users of > fdinfo memory stats. > > Add a stable name always formed as $type$instance. Could have chosen a > different stable scheme, but I think any consistent and stable scheme > should do just fine. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> > --- > drivers/gpu/drm/i915/intel_memory_region.c | 19 +++++++++++++++++++ > drivers/gpu/drm/i915/intel_memory_region.h | 1 + > 2 files changed, 20 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c > index 3d1fdea9811d..60a03340bbd4 100644 > --- a/drivers/gpu/drm/i915/intel_memory_region.c > +++ b/drivers/gpu/drm/i915/intel_memory_region.c > @@ -216,6 +216,22 @@ static int intel_memory_region_memtest(struct intel_memory_region *mem, > return err; > } > > +static const char *region_type_str(u16 type) > +{ > + switch (type) { > + case INTEL_MEMORY_SYSTEM: > + return "system"; > + case INTEL_MEMORY_LOCAL: > + return "local"; > + case INTEL_MEMORY_STOLEN_LOCAL: > + return "stolen-local"; > + case INTEL_MEMORY_STOLEN_SYSTEM: > + return "stolen-system"; > + default: > + return "unknown"; > + } > +} > + > struct intel_memory_region * > intel_memory_region_create(struct drm_i915_private *i915, > resource_size_t start, > @@ -244,6 +260,9 @@ intel_memory_region_create(struct drm_i915_private *i915, > mem->type = type; > mem->instance = instance; > > + snprintf(mem->uabi_name, sizeof(mem->uabi_name), "%s%u", > + region_type_str(type), instance); > + > mutex_init(&mem->objects.lock); > INIT_LIST_HEAD(&mem->objects.list); > > diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h > index 2953ed5c3248..9ba36454e51b 100644 > --- a/drivers/gpu/drm/i915/intel_memory_region.h > +++ b/drivers/gpu/drm/i915/intel_memory_region.h > @@ -80,6 +80,7 @@ struct intel_memory_region { > u16 instance; > enum intel_region_id id; > char name[16]; > + char uabi_name[16]; Just a thought instead of creating a new field, can't we derive this with name and instance? Thanks, Aravind. > bool private; /* not for userspace */ > > struct {
On 26/09/2023 16:29, Iddamsetty, Aravind wrote: > On 22-09-2023 19:16, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> >> At the moment memory region names are a bit too varied and too >> inconsistent to be used for ABI purposes, like for upcoming fdinfo >> memory stats. >> >> System memory can be either system or system-ttm. Local memory has the >> instance number appended, others do not. Not only incosistent but thi >> kind of implementation detail is uninteresting for intended users of >> fdinfo memory stats. >> >> Add a stable name always formed as $type$instance. Could have chosen a >> different stable scheme, but I think any consistent and stable scheme >> should do just fine. >> >> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >> --- >> drivers/gpu/drm/i915/intel_memory_region.c | 19 +++++++++++++++++++ >> drivers/gpu/drm/i915/intel_memory_region.h | 1 + >> 2 files changed, 20 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c >> index 3d1fdea9811d..60a03340bbd4 100644 >> --- a/drivers/gpu/drm/i915/intel_memory_region.c >> +++ b/drivers/gpu/drm/i915/intel_memory_region.c >> @@ -216,6 +216,22 @@ static int intel_memory_region_memtest(struct intel_memory_region *mem, >> return err; >> } >> >> +static const char *region_type_str(u16 type) >> +{ >> + switch (type) { >> + case INTEL_MEMORY_SYSTEM: >> + return "system"; >> + case INTEL_MEMORY_LOCAL: >> + return "local"; >> + case INTEL_MEMORY_STOLEN_LOCAL: >> + return "stolen-local"; >> + case INTEL_MEMORY_STOLEN_SYSTEM: >> + return "stolen-system"; >> + default: >> + return "unknown"; >> + } >> +} >> + >> struct intel_memory_region * >> intel_memory_region_create(struct drm_i915_private *i915, >> resource_size_t start, >> @@ -244,6 +260,9 @@ intel_memory_region_create(struct drm_i915_private *i915, >> mem->type = type; >> mem->instance = instance; >> >> + snprintf(mem->uabi_name, sizeof(mem->uabi_name), "%s%u", >> + region_type_str(type), instance); >> + >> mutex_init(&mem->objects.lock); >> INIT_LIST_HEAD(&mem->objects.list); >> >> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h >> index 2953ed5c3248..9ba36454e51b 100644 >> --- a/drivers/gpu/drm/i915/intel_memory_region.h >> +++ b/drivers/gpu/drm/i915/intel_memory_region.h >> @@ -80,6 +80,7 @@ struct intel_memory_region { >> u16 instance; >> enum intel_region_id id; >> char name[16]; >> + char uabi_name[16]; > > Just a thought instead of creating a new field, can't we derive this > with name and instance? I'd rather not snprintf on every fdinfo read - for every pid and every drm fd versus 2-3 strings kept around. I did briefly wonder if mr->name could be dropped, that is renamed to mr->uabi_name, but I guess there is some value to print the internal name in some log messages, to leave a trace of what underlying implementation is used. Although I am not too sure about the value of that either since it is implied from the kernel version. Then on top the usage in i915_gem_create/repr_name I could replace with mr->uabi_name and simplify. If there is any value in printing the name there, versus just uabi type:instance integers. Dunno. All I know is fdinfo should have stable names and not confuse with implementation details so I need something.. Regards, Tvrtko
On 26-09-2023 21:12, Tvrtko Ursulin wrote: > > On 26/09/2023 16:29, Iddamsetty, Aravind wrote: >> On 22-09-2023 19:16, Tvrtko Ursulin wrote: >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> >>> At the moment memory region names are a bit too varied and too >>> inconsistent to be used for ABI purposes, like for upcoming fdinfo >>> memory stats. >>> >>> System memory can be either system or system-ttm. Local memory has the >>> instance number appended, others do not. Not only incosistent but thi >>> kind of implementation detail is uninteresting for intended users of >>> fdinfo memory stats. >>> >>> Add a stable name always formed as $type$instance. Could have chosen a >>> different stable scheme, but I think any consistent and stable scheme >>> should do just fine. >>> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>> --- >>> drivers/gpu/drm/i915/intel_memory_region.c | 19 +++++++++++++++++++ >>> drivers/gpu/drm/i915/intel_memory_region.h | 1 + >>> 2 files changed, 20 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/intel_memory_region.c >>> b/drivers/gpu/drm/i915/intel_memory_region.c >>> index 3d1fdea9811d..60a03340bbd4 100644 >>> --- a/drivers/gpu/drm/i915/intel_memory_region.c >>> +++ b/drivers/gpu/drm/i915/intel_memory_region.c >>> @@ -216,6 +216,22 @@ static int intel_memory_region_memtest(struct >>> intel_memory_region *mem, >>> return err; >>> } >>> +static const char *region_type_str(u16 type) >>> +{ >>> + switch (type) { >>> + case INTEL_MEMORY_SYSTEM: >>> + return "system"; >>> + case INTEL_MEMORY_LOCAL: >>> + return "local"; >>> + case INTEL_MEMORY_STOLEN_LOCAL: >>> + return "stolen-local"; >>> + case INTEL_MEMORY_STOLEN_SYSTEM: >>> + return "stolen-system"; >>> + default: >>> + return "unknown"; >>> + } >>> +} >>> + >>> struct intel_memory_region * >>> intel_memory_region_create(struct drm_i915_private *i915, >>> resource_size_t start, >>> @@ -244,6 +260,9 @@ intel_memory_region_create(struct >>> drm_i915_private *i915, >>> mem->type = type; >>> mem->instance = instance; >>> + snprintf(mem->uabi_name, sizeof(mem->uabi_name), "%s%u", >>> + region_type_str(type), instance); >>> + >>> mutex_init(&mem->objects.lock); >>> INIT_LIST_HEAD(&mem->objects.list); >>> diff --git a/drivers/gpu/drm/i915/intel_memory_region.h >>> b/drivers/gpu/drm/i915/intel_memory_region.h >>> index 2953ed5c3248..9ba36454e51b 100644 >>> --- a/drivers/gpu/drm/i915/intel_memory_region.h >>> +++ b/drivers/gpu/drm/i915/intel_memory_region.h >>> @@ -80,6 +80,7 @@ struct intel_memory_region { >>> u16 instance; >>> enum intel_region_id id; >>> char name[16]; >>> + char uabi_name[16]; >> >> Just a thought instead of creating a new field, can't we derive this >> with name and instance? > > I'd rather not snprintf on every fdinfo read - for every pid and every > drm fd versus 2-3 strings kept around. ya agreed makes no sense. > > I did briefly wonder if mr->name could be dropped, that is renamed to > mr->uabi_name, but I guess there is some value to print the internal > name in some log messages, to leave a trace of what underlying > implementation is used. Although I am not too sure about the value of > that either since it is implied from the kernel version. > > Then on top the usage in i915_gem_create/repr_name I could replace with > mr->uabi_name and simplify. If there is any value in printing the name > there, versus just uabi type:instance integers. Dunno. All I know is > fdinfo should have stable names and not confuse with implementation > details so I need something.. Ok. LGTM Reviewed-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com> Thanks, Aravind. > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/intel_memory_region.c b/drivers/gpu/drm/i915/intel_memory_region.c index 3d1fdea9811d..60a03340bbd4 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.c +++ b/drivers/gpu/drm/i915/intel_memory_region.c @@ -216,6 +216,22 @@ static int intel_memory_region_memtest(struct intel_memory_region *mem, return err; } +static const char *region_type_str(u16 type) +{ + switch (type) { + case INTEL_MEMORY_SYSTEM: + return "system"; + case INTEL_MEMORY_LOCAL: + return "local"; + case INTEL_MEMORY_STOLEN_LOCAL: + return "stolen-local"; + case INTEL_MEMORY_STOLEN_SYSTEM: + return "stolen-system"; + default: + return "unknown"; + } +} + struct intel_memory_region * intel_memory_region_create(struct drm_i915_private *i915, resource_size_t start, @@ -244,6 +260,9 @@ intel_memory_region_create(struct drm_i915_private *i915, mem->type = type; mem->instance = instance; + snprintf(mem->uabi_name, sizeof(mem->uabi_name), "%s%u", + region_type_str(type), instance); + mutex_init(&mem->objects.lock); INIT_LIST_HEAD(&mem->objects.list); diff --git a/drivers/gpu/drm/i915/intel_memory_region.h b/drivers/gpu/drm/i915/intel_memory_region.h index 2953ed5c3248..9ba36454e51b 100644 --- a/drivers/gpu/drm/i915/intel_memory_region.h +++ b/drivers/gpu/drm/i915/intel_memory_region.h @@ -80,6 +80,7 @@ struct intel_memory_region { u16 instance; enum intel_region_id id; char name[16]; + char uabi_name[16]; bool private; /* not for userspace */ struct {