Re: [Intel-gfx] [PATCH 1/5] drm/i915: Track buffer objects belonging to clients

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 09/06/2023 05:16, Iddamsetty, Aravind wrote:
> On 08-06-2023 20:21, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> In order to show per client memory usage lets start tracking which
>> objects belong to which clients.
>>
>> We start with objects explicitly created by object creation UAPI and
>> track it on a new per client lists, protected by a new per client lock.
>> In order for delayed destruction (post client exit), we make tracked
>> objects hold references to the owning client.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/gem/i915_gem_create.c    | 32 ++++++++++++++--
>>   drivers/gpu/drm/i915/gem/i915_gem_object.c    |  6 +++
>>   .../gpu/drm/i915/gem/i915_gem_object_types.h  | 12 ++++++
>>   drivers/gpu/drm/i915/i915_drm_client.c        | 36 +++++++++++++++++-
>>   drivers/gpu/drm/i915/i915_drm_client.h        | 37 ++++++++++++++++++-
>>   drivers/gpu/drm/i915/i915_gem.c               |  2 +-
>>   6 files changed, 119 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_create.c b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> index d24c0ce8805c..4f1957638207 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_create.c
>> @@ -11,6 +11,7 @@
>>   #include "gem/i915_gem_region.h"
>>   #include "pxp/intel_pxp.h"
>>   
>> +#include "i915_drm_client.h"
>>   #include "i915_drv.h"
>>   #include "i915_gem_create.h"
>>   #include "i915_trace.h"
>> @@ -164,6 +165,14 @@ __i915_gem_object_create_user(struct drm_i915_private *i915, u64 size,
>>   						 n_placements, 0);
>>   }
>>   
>> +static void add_file_obj(struct drm_file *file,
>> +			 struct drm_i915_gem_object *obj)
>> +{
>> +	struct drm_i915_file_private *fpriv = file->driver_priv;
>> +
>> +	i915_drm_client_add_object(fpriv->client, obj);
>> +}
>> +
>>   int
>>   i915_gem_dumb_create(struct drm_file *file,
>>   		     struct drm_device *dev,
>> @@ -174,6 +183,7 @@ i915_gem_dumb_create(struct drm_file *file,
>>   	enum intel_memory_type mem_type;
>>   	int cpp = DIV_ROUND_UP(args->bpp, 8);
>>   	u32 format;
>> +	int ret;
>>   
>>   	switch (cpp) {
>>   	case 1:
>> @@ -212,7 +222,12 @@ i915_gem_dumb_create(struct drm_file *file,
>>   	if (IS_ERR(obj))
>>   		return PTR_ERR(obj);
>>   
>> -	return i915_gem_publish(obj, file, &args->size, &args->handle);
>> +	ret = i915_gem_publish(obj, file, &args->size, &args->handle);
>> +
>> +	if (!ret)
>> +		add_file_obj(file, obj);
>> +
>> +	return ret;
>>   }
>>   
>>   /**
>> @@ -229,6 +244,7 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
>>   	struct drm_i915_gem_create *args = data;
>>   	struct drm_i915_gem_object *obj;
>>   	struct intel_memory_region *mr;
>> +	int ret;
>>   
>>   	mr = intel_memory_region_by_type(i915, INTEL_MEMORY_SYSTEM);
>>   
>> @@ -236,7 +252,12 @@ i915_gem_create_ioctl(struct drm_device *dev, void *data,
>>   	if (IS_ERR(obj))
>>   		return PTR_ERR(obj);
> 
> Do we intend to track only client created objects and not imported ?
> or is that taken care by this "obj->base.handle_count > 1"

I missed the imports, now added in v3 of the series.

They wouldn't have been handled by the above check in the importer - 
only the exporter would have seen (sometimes) changes in the ratio of 
total vs shared.

Regards,

Tvrtko