[Intel-gfx] [RFC 4/6] drm: Add simple fdinfo memory helpers

[Tvrtko Ursulin]

On 19/04/2023 15:32, Rob Clark wrote:
> On Wed, Apr 19, 2023 at 6:16 AM Tvrtko Ursulin
> <tvrtko.ursulin at linux.intel.com> wrote:
>>
>>
>> On 18/04/2023 18:18, Rob Clark wrote:
>>> On Mon, Apr 17, 2023 at 8:56 AM Tvrtko Ursulin
>>> <tvrtko.ursulin at linux.intel.com> wrote:
>>>>
>>>> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>>
>>>> For drivers who only wish to show one memory region called 'system,
>>>> and only account the GEM buffer object handles under it.
>>>>
>>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
>>>> ---
>>>>    drivers/gpu/drm/drm_file.c | 45 ++++++++++++++++++++++++++++++++++++++
>>>>    include/drm/drm_file.h     |  6 +++++
>>>>    2 files changed, 51 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>>> index e202f79e816d..1e70669dddf7 100644
>>>> --- a/drivers/gpu/drm/drm_file.c
>>>> +++ b/drivers/gpu/drm/drm_file.c
>>>> @@ -872,6 +872,51 @@ void drm_send_event(struct drm_device *dev, struct drm_pending_event *e)
>>>>    }
>>>>    EXPORT_SYMBOL(drm_send_event);
>>>>
>>>> +static void
>>>> +add_obj(struct drm_gem_object *obj, struct drm_fdinfo_memory_stat *stats)
>>>> +{
>>>> +       u64 sz = obj->size;
>>>> +
>>>> +       stats[0].size += sz;
>>>> +
>>>> +       if (obj->handle_count > 1)
>>>> +               stats[0].shared += sz;
>>>> +
>>>> +       if (!dma_resv_test_signaled(obj->resv, dma_resv_usage_rw(true)))
>>>> +               stats[0].active += sz;
>>>> +
>>>> +       /* Not supported. */
>>>> +       stats[0].resident = ~0ull;
>>>> +       stats[0].purgeable = ~0ull;
>>>
>>> Hmm, this kinda makes the simple helper not very useful.  In my
>>> version, you get something that is useful for all UMA drivers (which
>>> all, IIRC, have some form of madv ioctl).  I was kinda imagining that
>>> for ttm drivers, my print_memory_stats() would just become a helper
>>> and that TTM (or "multi-region") drivers would have their own thing.
>>
>> Hm how? Your version also needed a driver specific vfunc:
> 
> Sure, but there is a possibility for a driver specific vfunc ;-)

Indeed, they are there in both proposals! :)

> And arguably we could move madv to drm_gem_object, along with get/put
> pages tracking.. since all the UMA drivers pretty much do the same
> thing.  So maybe the vfunc is a temporary thing.
> 
> Anyways, I could go back to my original version where it was a helper
> called from the driver to print mem stats.  That way, TTM drivers
> could do their own thing.

I must have missed that. So it was the same idea as in my RFC?

Regards,

Tvrtko
  
> BR,
> -R
> 
>> +static enum drm_gem_object_status msm_gem_status(struct drm_gem_object *obj)
>> +{
>> +       struct msm_gem_object *msm_obj = to_msm_bo(obj);
>> +       enum drm_gem_object_status status = 0;
>> +
>> +       if (msm_obj->pages)
>> +               status |= DRM_GEM_OBJECT_RESIDENT;
>> +
>> +       if (msm_obj->madv == MSM_MADV_DONTNEED)
>> +               status |= DRM_GEM_OBJECT_PURGEABLE;
>> +
>> +       return status;
>> +}
>>
>> Regards,
>>
>> Tvrtko
>>
>>>
>>> BR,
>>> -R
>>>
>>>> +}
>>>> +
>>>> +char **
>>>> +drm_query_fdinfo_system_region(struct drm_device *dev, unsigned int *num)
>>>> +{
>>>> +       static char *region[] = {
>>>> +               "system",
>>>> +       };
>>>> +
>>>> +       *num = 1;
>>>> +
>>>> +       return region;
>>>> +}
>>>> +EXPORT_SYMBOL(drm_query_fdinfo_system_region);
>>>> +
>>>> +void
>>>> +drm_query_fdinfo_system_memory(struct drm_file *file,
>>>> +                              struct drm_fdinfo_memory_stat *stats)
>>>> +{
>>>> +       struct drm_gem_object *obj;
>>>> +       int id;
>>>> +
>>>> +       spin_lock(&file->table_lock);
>>>> +       idr_for_each_entry(&file->object_idr, obj, id)
>>>> +               add_obj(obj, stats);
>>>> +       spin_unlock(&file->table_lock);
>>>> +}
>>>> +EXPORT_SYMBOL(drm_query_fdinfo_system_memory);
>>>> +
>>>>    static void
>>>>    print_stat(struct drm_printer *p, const char *stat, const char *region, u64 sz)
>>>>    {
>>>> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
>>>> index 00d48beeac5c..dd7c6fb2c975 100644
>>>> --- a/include/drm/drm_file.h
>>>> +++ b/include/drm/drm_file.h
>>>> @@ -383,6 +383,12 @@ struct drm_fdinfo_memory_stat {
>>>>           u64 active;
>>>>    };
>>>>
>>>> +char **drm_query_fdinfo_system_region(struct drm_device *dev,
>>>> +                                     unsigned int *num);
>>>> +void drm_query_fdinfo_system_memory(struct drm_file *file,
>>>> +                                   struct drm_fdinfo_memory_stat *stats);
>>>> +
>>>> +
>>>>    /**
>>>>     * drm_is_primary_client - is this an open file of the primary node
>>>>     * @file_priv: DRM file
>>>> --
>>>> 2.37.2
>>>>