Re: [RFC 4/6] drm: Add simple fdinfo memory helpers

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Apr 20, 2023 at 6:14 AM Tvrtko Ursulin
<tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>
>
> On 19/04/2023 15:32, Rob Clark wrote:
> > On Wed, Apr 19, 2023 at 6:16 AM Tvrtko Ursulin
> > <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> >>
> >>
> >> On 18/04/2023 18:18, Rob Clark wrote:
> >>> On Mon, Apr 17, 2023 at 8:56 AM Tvrtko Ursulin
> >>> <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
> >>>>
> >>>> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >>>>
> >>>> 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@xxxxxxxxx>
> >>>> ---
> >>>>    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?

similar, danvet asked for something "more core" ;-)

https://patchwork.freedesktop.org/patch/531403/?series=116216&rev=1

BR,
-R

> 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
> >>>>




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux