This patch series adds fdinfo support to the Panfrost DRM driver. It will
display a series of key:value pairs under /proc/pid/fdinfo/fd for render
processes that open the Panfrost DRM file.
The pairs contain basic drm gpu engine and memory region information that
can either be cat by a privileged user or accessed with IGT's gputop
utility.
Changelog:
v1: https://lore.kernel.org/lkml/[email protected]/T/
v2: https://lore.kernel.org/lkml/[email protected]/T/
- Changed the way gpu cycles and engine time are calculated, using GPU
registers and taking into account potential resets.
- Split render engine values into fragment and vertex/tiler ones.
- Added more fine-grained calculation of RSS size for BO's.
- Implemente selection of drm-memory region size units
- Removed locking of shrinker's mutex in GEM obj status function
v3: https://lore.kernel.org/lkml/[email protected]/
- Changed fdinfo engine names to something more descriptive
- Mentioned GPU cycle counts aren't an exact measure
- Handled the case when job->priv might be NULL
- Handled 32 bit overflow of cycle register
- Kept fdinfo drm memory stats size unit display within 10k times the
previous multiplier for more accurate BO size numbers
- Removed special handling of Prime imported BO RSS
- Use rss_size only for heap objects
- Use bo->base.madv instead of specific purgeable flag
- Fixed kernel test robot warnings
v4: https://lore.kernel.org/lkml/[email protected]/
- Move cycle counter get and put to panfrost_job_hw_submit and
panfrost_job_handle_{err,done} for more accuracy
- Make sure cycle counter refs are released in reset path
- Drop the model param for toggling cycle counting and do
leave it down to the debugfs file
- Don't disable cycle counter when togglint debugfs file,
let refcounting logic handle it instead.
- Remove fdinfo data nested structure definion and 'names' field
- When incrementing BO RSS size in GPU MMU page fault IRQ handler, assume
granuality of 2MiB for every successful mapping.
- drm-file picks an fdinfo memory object size unit that doesn't lose precision.
v5: https://lore.kernel.org/lkml/[email protected]/
- Removed explicit initialisation of atomic variable for profiling mode,
as it's allocated with kzalloc.
- Pass engine utilisation structure to jobs rather than the file context, to avoid
future misusage of the latter.
- Remove double reading of cycle counter register and ktime in job deqeueue function,
as the scheduler will make sure these values are read over in case of requeuing.
- Moved putting of cycle counting refcnt into panfrost job dequeue
function to avoid repetition.
v6:
- Fix wrong swapped-round engine time and cycle values in fdinfo
drm print statements.
Adrián Larumbe (6):
drm/panfrost: Add cycle count GPU register definitions
drm/panfrost: Add fdinfo support GPU load metrics
drm/panfrost: Add fdinfo support for memory stats
drm/drm_file: Add DRM obj's RSS reporting function for fdinfo
drm/panfrost: Implement generic DRM object RSS reporting function
drm/drm-file: Show finer-grained BO sizes in drm_show_memory_stats
drivers/gpu/drm/drm_file.c | 10 +++-
drivers/gpu/drm/panfrost/Makefile | 2 +
drivers/gpu/drm/panfrost/panfrost_debugfs.c | 20 +++++++
drivers/gpu/drm/panfrost/panfrost_debugfs.h | 13 +++++
drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 +++
drivers/gpu/drm/panfrost/panfrost_devfreq.h | 3 ++
drivers/gpu/drm/panfrost/panfrost_device.c | 2 +
drivers/gpu/drm/panfrost/panfrost_device.h | 13 +++++
drivers/gpu/drm/panfrost/panfrost_drv.c | 59 ++++++++++++++++++++-
drivers/gpu/drm/panfrost/panfrost_gem.c | 29 ++++++++++
drivers/gpu/drm/panfrost/panfrost_gem.h | 5 ++
drivers/gpu/drm/panfrost/panfrost_gpu.c | 41 ++++++++++++++
drivers/gpu/drm/panfrost/panfrost_gpu.h | 4 ++
drivers/gpu/drm/panfrost/panfrost_job.c | 24 +++++++++
drivers/gpu/drm/panfrost/panfrost_job.h | 5 ++
drivers/gpu/drm/panfrost/panfrost_mmu.c | 1 +
drivers/gpu/drm/panfrost/panfrost_regs.h | 5 ++
include/drm/drm_gem.h | 9 ++++
18 files changed, 250 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
create mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
base-commit: f45acf7acf75921c0409d452f0165f51a19a74fd
--
2.42.0
Some BO's might be mapped onto physical memory chunkwise and on demand,
like Panfrost's tiler heap. In this case, even though the
drm_gem_shmem_object page array might already be allocated, only a very
small fraction of the BO is currently backed by system memory, but
drm_show_memory_stats will then proceed to add its entire virtual size to
the file's total resident size regardless.
This led to very unrealistic RSS sizes being reckoned for Panfrost, where
said tiler heap buffer is initially allocated with a virtual size of 128
MiB, but only a small part of it will eventually be backed by system memory
after successive GPU page faults.
Provide a new DRM object generic function that would allow drivers to
return a more accurate RSS size for their BOs.
Signed-off-by: Adrián Larumbe <[email protected]>
Reviewed-by: Boris Brezillon <[email protected]>
Reviewed-by: Steven Price <[email protected]>
---
drivers/gpu/drm/drm_file.c | 5 ++++-
include/drm/drm_gem.h | 9 +++++++++
2 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
index 883d83bc0e3d..762965e3d503 100644
--- a/drivers/gpu/drm/drm_file.c
+++ b/drivers/gpu/drm/drm_file.c
@@ -944,7 +944,10 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
}
if (s & DRM_GEM_OBJECT_RESIDENT) {
- status.resident += obj->size;
+ if (obj->funcs && obj->funcs->rss)
+ status.resident += obj->funcs->rss(obj);
+ else
+ status.resident += obj->size;
} else {
/* If already purged or not yet backed by pages, don't
* count it as purgeable:
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
index bc9f6aa2f3fe..16364487fde9 100644
--- a/include/drm/drm_gem.h
+++ b/include/drm/drm_gem.h
@@ -208,6 +208,15 @@ struct drm_gem_object_funcs {
*/
enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
+ /**
+ * @rss:
+ *
+ * Return resident size of the object in physical memory.
+ *
+ * Called by drm_show_memory_stats().
+ */
+ size_t (*rss)(struct drm_gem_object *obj);
+
/**
* @vm_ops:
*
--
2.42.0
On 20/09/2023 00:34, Adrián Larumbe wrote:
> Some BO's might be mapped onto physical memory chunkwise and on demand,
> like Panfrost's tiler heap. In this case, even though the
> drm_gem_shmem_object page array might already be allocated, only a very
> small fraction of the BO is currently backed by system memory, but
> drm_show_memory_stats will then proceed to add its entire virtual size to
> the file's total resident size regardless.
>
> This led to very unrealistic RSS sizes being reckoned for Panfrost, where
> said tiler heap buffer is initially allocated with a virtual size of 128
> MiB, but only a small part of it will eventually be backed by system memory
> after successive GPU page faults.
>
> Provide a new DRM object generic function that would allow drivers to
> return a more accurate RSS size for their BOs.
>
> Signed-off-by: Adrián Larumbe <[email protected]>
> Reviewed-by: Boris Brezillon <[email protected]>
> Reviewed-by: Steven Price <[email protected]>
> ---
> drivers/gpu/drm/drm_file.c | 5 ++++-
> include/drm/drm_gem.h | 9 +++++++++
> 2 files changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
> index 883d83bc0e3d..762965e3d503 100644
> --- a/drivers/gpu/drm/drm_file.c
> +++ b/drivers/gpu/drm/drm_file.c
> @@ -944,7 +944,10 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
> }
>
> if (s & DRM_GEM_OBJECT_RESIDENT) {
> - status.resident += obj->size;
> + if (obj->funcs && obj->funcs->rss)
> + status.resident += obj->funcs->rss(obj);
> + else
> + status.resident += obj->size;
Presumably you'd want the same smaller size in both active and
purgeable? Or you can end up with more in those two than in rss which
would look odd.
Also, alternative to adding a new callback could be adding multiple
output parameters to the existing obj->func->status() which maybe ends
up simpler due fewer callbacks?
Like:
s = obj->funcs->status(obj, &supported_status, &rss)
And adjust the code flow to pick up the rss if driver signaled it
supports reporting it.
Regards,
Tvrtko
> } else {
> /* If already purged or not yet backed by pages, don't
> * count it as purgeable:
> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
> index bc9f6aa2f3fe..16364487fde9 100644
> --- a/include/drm/drm_gem.h
> +++ b/include/drm/drm_gem.h
> @@ -208,6 +208,15 @@ struct drm_gem_object_funcs {
> */
> enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
>
> + /**
> + * @rss:
> + *
> + * Return resident size of the object in physical memory.
> + *
> + * Called by drm_show_memory_stats().
> + */
> + size_t (*rss)(struct drm_gem_object *obj);
> +
> /**
> * @vm_ops:
> *
On 20.09.2023 16:53, Tvrtko Ursulin wrote:
>
>On 20/09/2023 00:34, Adrián Larumbe wrote:
>> Some BO's might be mapped onto physical memory chunkwise and on demand,
>> like Panfrost's tiler heap. In this case, even though the
>> drm_gem_shmem_object page array might already be allocated, only a very
>> small fraction of the BO is currently backed by system memory, but
>> drm_show_memory_stats will then proceed to add its entire virtual size to
>> the file's total resident size regardless.
>>
>> This led to very unrealistic RSS sizes being reckoned for Panfrost, where
>> said tiler heap buffer is initially allocated with a virtual size of 128
>> MiB, but only a small part of it will eventually be backed by system memory
>> after successive GPU page faults.
>>
>> Provide a new DRM object generic function that would allow drivers to
>> return a more accurate RSS size for their BOs.
>>
>> Signed-off-by: Adrián Larumbe <[email protected]>
>> Reviewed-by: Boris Brezillon <[email protected]>
>> Reviewed-by: Steven Price <[email protected]>
>> ---
>> drivers/gpu/drm/drm_file.c | 5 ++++-
>> include/drm/drm_gem.h | 9 +++++++++
>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>> index 883d83bc0e3d..762965e3d503 100644
>> --- a/drivers/gpu/drm/drm_file.c
>> +++ b/drivers/gpu/drm/drm_file.c
>> @@ -944,7 +944,10 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>> }
>> if (s & DRM_GEM_OBJECT_RESIDENT) {
>> - status.resident += obj->size;
>> + if (obj->funcs && obj->funcs->rss)
>> + status.resident += obj->funcs->rss(obj);
>> + else
>> + status.resident += obj->size;
>
>Presumably you'd want the same smaller size in both active and purgeable? Or
>you can end up with more in those two than in rss which would look odd.
I didn't think of this. I guess when an object is both resident and purgeable,
then its RSS and purgeable sizes should be the same.
>Also, alternative to adding a new callback could be adding multiple output
>parameters to the existing obj->func->status() which maybe ends up simpler due
>fewer callbacks?
>
>Like:
>
> s = obj->funcs->status(obj, &supported_status, &rss)
>
>And adjust the code flow to pick up the rss if driver signaled it supports
>reporting it.
I personally find having a separate object callback more readable in this case.
There's also the question of what output parameter value would be used as a token
that the relevant BO doesn't have an RSS different from its virtual
size. I guess '0' would be alright, but this is on the assumption that this
could never be a legitimate BO virtual size across all DRM drivers. I guess
most of them round the size up to the nearest page multiple at BO creation
time.
>
>Regards,
>
>Tvrtko
>
>> } else {
>> /* If already purged or not yet backed by pages, don't
>> * count it as purgeable:
>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>> index bc9f6aa2f3fe..16364487fde9 100644
>> --- a/include/drm/drm_gem.h
>> +++ b/include/drm/drm_gem.h
>> @@ -208,6 +208,15 @@ struct drm_gem_object_funcs {
>> */
>> enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
>> + /**
>> + * @rss:
>> + *
>> + * Return resident size of the object in physical memory.
>> + *
>> + * Called by drm_show_memory_stats().
>> + */
>> + size_t (*rss)(struct drm_gem_object *obj);
>> +
>> /**
>> * @vm_ops:
>> *
On 22/09/2023 11:58, Adrián Larumbe wrote:
> On 20.09.2023 16:53, Tvrtko Ursulin wrote:
>>
>> On 20/09/2023 00:34, Adrián Larumbe wrote:
>>> Some BO's might be mapped onto physical memory chunkwise and on demand,
>>> like Panfrost's tiler heap. In this case, even though the
>>> drm_gem_shmem_object page array might already be allocated, only a very
>>> small fraction of the BO is currently backed by system memory, but
>>> drm_show_memory_stats will then proceed to add its entire virtual size to
>>> the file's total resident size regardless.
>>>
>>> This led to very unrealistic RSS sizes being reckoned for Panfrost, where
>>> said tiler heap buffer is initially allocated with a virtual size of 128
>>> MiB, but only a small part of it will eventually be backed by system memory
>>> after successive GPU page faults.
>>>
>>> Provide a new DRM object generic function that would allow drivers to
>>> return a more accurate RSS size for their BOs.
>>>
>>> Signed-off-by: Adrián Larumbe <[email protected]>
>>> Reviewed-by: Boris Brezillon <[email protected]>
>>> Reviewed-by: Steven Price <[email protected]>
>>> ---
>>> drivers/gpu/drm/drm_file.c | 5 ++++-
>>> include/drm/drm_gem.h | 9 +++++++++
>>> 2 files changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_file.c b/drivers/gpu/drm/drm_file.c
>>> index 883d83bc0e3d..762965e3d503 100644
>>> --- a/drivers/gpu/drm/drm_file.c
>>> +++ b/drivers/gpu/drm/drm_file.c
>>> @@ -944,7 +944,10 @@ void drm_show_memory_stats(struct drm_printer *p, struct drm_file *file)
>>> }
>>> if (s & DRM_GEM_OBJECT_RESIDENT) {
>>> - status.resident += obj->size;
>>> + if (obj->funcs && obj->funcs->rss)
>>> + status.resident += obj->funcs->rss(obj);
>>> + else
>>> + status.resident += obj->size;
>>
>> Presumably you'd want the same smaller size in both active and purgeable? Or
>> you can end up with more in those two than in rss which would look odd.
>
> I didn't think of this. I guess when an object is both resident and purgeable,
> then its RSS and purgeable sizes should be the same.
>
>> Also, alternative to adding a new callback could be adding multiple output
>> parameters to the existing obj->func->status() which maybe ends up simpler due
>> fewer callbacks?
>>
>> Like:
>>
>> s = obj->funcs->status(obj, &supported_status, &rss)
>>
>> And adjust the code flow to pick up the rss if driver signaled it supports
>> reporting it.
>
> I personally find having a separate object callback more readable in this case.
> There's also the question of what output parameter value would be used as a token
> that the relevant BO doesn't have an RSS different from its virtual
> size. I guess '0' would be alright, but this is on the assumption that this
> could never be a legitimate BO virtual size across all DRM drivers. I guess
> most of them round the size up to the nearest page multiple at BO creation
> time.
Okay. See how it will look once you need to apply it to resident and
purgeable. I wonder if "driver knows better" will end up a dominant case
and we do end up considering reversing the scheme (like ask the driver
to fill in the meminfo record). TBH I do not remember all the flavours
both Rob and I proposed at this point.
Regards,
Tvrtko
>
>>
>> Regards,
>>
>> Tvrtko
>>
>>> } else {
>>> /* If already purged or not yet backed by pages, don't
>>> * count it as purgeable:
>>> diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h
>>> index bc9f6aa2f3fe..16364487fde9 100644
>>> --- a/include/drm/drm_gem.h
>>> +++ b/include/drm/drm_gem.h
>>> @@ -208,6 +208,15 @@ struct drm_gem_object_funcs {
>>> */
>>> enum drm_gem_object_status (*status)(struct drm_gem_object *obj);
>>> + /**
>>> + * @rss:
>>> + *
>>> + * Return resident size of the object in physical memory.
>>> + *
>>> + * Called by drm_show_memory_stats().
>>> + */
>>> + size_t (*rss)(struct drm_gem_object *obj);
>>> +
>>> /**
>>> * @vm_ops:
>>> *