| Message ID | 20230824013604.466224-4-adrian.larumbe@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add fdinfo support to Panfrost | expand |
On Thu, 24 Aug 2023 02:34:46 +0100 Adrián Larumbe <adrian.larumbe@collabora.com> wrote: > A new DRM GEM object function is added so that drm_show_memory_stats can > provider more accurate memory usage numbers. s/provider/provide/ > > Ideally, in panfrost_gem_status, the BO's purgeable flag would be checked > after locking the driver's shrinker mutex, but drm_show_memory_stats takes > over the drm file's object handle database spinlock, so there's potential > for a race condition here. Yeah, I don't think it matters much if we report a BO non-purgeable, and this BO becomes purgeable in the meantime. You'd have the same problem > > Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> > --- > drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++++++-- > drivers/gpu/drm/panfrost/panfrost_gem.c | 12 ++++++++++++ > drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + > 3 files changed, 20 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c > index 3fd372301019..93d5f5538c0b 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c > @@ -440,11 +440,14 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, > args->retained = drm_gem_shmem_madvise(&bo->base, args->madv); > > if (args->retained) { > - if (args->madv == PANFROST_MADV_DONTNEED) > + if (args->madv == PANFROST_MADV_DONTNEED) { > list_move_tail(&bo->base.madv_list, > &pfdev->shrinker_list); > - else if (args->madv == PANFROST_MADV_WILLNEED) > + bo->is_purgable = true; > + } else if (args->madv == PANFROST_MADV_WILLNEED) { > list_del_init(&bo->base.madv_list); > + bo->is_purgable = false; Should we really flag the BO as purgeable if it's already been evicted (args->retained == false)? > + } > } > > out_unlock_mappings: > @@ -559,6 +562,8 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file) > struct panfrost_device *pfdev = dev->dev_private; > > panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p); > + > + drm_show_memory_stats(p, file); > } > > static const struct file_operations panfrost_drm_driver_fops = { > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c > index 3c812fbd126f..aea16b0e4dda 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.c > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c > @@ -195,6 +195,17 @@ static int panfrost_gem_pin(struct drm_gem_object *obj) > return drm_gem_shmem_pin(&bo->base); > } > > +static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj) > +{ > + struct panfrost_gem_object *bo = to_panfrost_bo(obj); > + enum drm_gem_object_status res = 0; > + > + res |= (bo->is_purgable) ? DRM_GEM_OBJECT_PURGEABLE : 0; Why not checking bo->base.madv here instead of adding an is_purgeable field? > + > + res |= (bo->base.pages) ? DRM_GEM_OBJECT_RESIDENT : 0; Does it make sense to have DRM_GEM_OBJECT_PURGEABLE set when DRM_GEM_OBJECT_RESIDENT is not? > + > + return res; > +} > static const struct drm_gem_object_funcs panfrost_gem_funcs = { > .free = panfrost_gem_free_object, > .open = panfrost_gem_open, > @@ -206,6 +217,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = { > .vmap = drm_gem_shmem_object_vmap, > .vunmap = drm_gem_shmem_object_vunmap, > .mmap = drm_gem_shmem_object_mmap, > + .status = panfrost_gem_status, > .vm_ops = &drm_gem_shmem_vm_ops, > }; > > diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h > index ad2877eeeccd..e06f7ceb8f73 100644 > --- a/drivers/gpu/drm/panfrost/panfrost_gem.h > +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h > @@ -38,6 +38,7 @@ struct panfrost_gem_object { > > bool noexec :1; > bool is_heap :1; > + bool is_purgable :1; > }; > > struct panfrost_gem_mapping {
On 30.08.2023 12:31, Boris Brezillon wrote: >On Thu, 24 Aug 2023 02:34:46 +0100 >Adrián Larumbe <adrian.larumbe@collabora.com> wrote: > >> A new DRM GEM object function is added so that drm_show_memory_stats can >> provider more accurate memory usage numbers. > > s/provider/provide/ > >> >> Ideally, in panfrost_gem_status, the BO's purgeable flag would be checked >> after locking the driver's shrinker mutex, but drm_show_memory_stats takes >> over the drm file's object handle database spinlock, so there's potential >> for a race condition here. > >Yeah, I don't think it matters much if we report a BO non-purgeable, >and this BO becomes purgeable in the meantime. You'd have the same >problem > >> >> Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> >> --- >> drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++++++-- >> drivers/gpu/drm/panfrost/panfrost_gem.c | 12 ++++++++++++ >> drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + >> 3 files changed, 20 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c >> index 3fd372301019..93d5f5538c0b 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c >> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c >> @@ -440,11 +440,14 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, >> args->retained = drm_gem_shmem_madvise(&bo->base, args->madv); >> >> if (args->retained) { >> - if (args->madv == PANFROST_MADV_DONTNEED) >> + if (args->madv == PANFROST_MADV_DONTNEED) { >> list_move_tail(&bo->base.madv_list, >> &pfdev->shrinker_list); >> - else if (args->madv == PANFROST_MADV_WILLNEED) >> + bo->is_purgable = true; >> + } else if (args->madv == PANFROST_MADV_WILLNEED) { >> list_del_init(&bo->base.madv_list); >> + bo->is_purgable = false; > >Should we really flag the BO as purgeable if it's already been evicted >(args->retained == false)? I checked what msm is doing, and I guess it shouldn't be marked as purgeable after eviction. I didn't catch this at first because Freedreno isn't using drm_gem_shmem_madvise, but apparently tracking whether a BO was already purged through an additional MADV state. >> + } >> } >> >> out_unlock_mappings: >> @@ -559,6 +562,8 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file) >> struct panfrost_device *pfdev = dev->dev_private; >> >> panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p); >> + >> + drm_show_memory_stats(p, file); >> } >> >> static const struct file_operations panfrost_drm_driver_fops = { >> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c >> index 3c812fbd126f..aea16b0e4dda 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_gem.c >> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c >> @@ -195,6 +195,17 @@ static int panfrost_gem_pin(struct drm_gem_object *obj) >> return drm_gem_shmem_pin(&bo->base); >> } >> >> +static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj) >> +{ >> + struct panfrost_gem_object *bo = to_panfrost_bo(obj); >> + enum drm_gem_object_status res = 0; >> + >> + res |= (bo->is_purgable) ? DRM_GEM_OBJECT_PURGEABLE : 0; > >Why not checking bo->base.madv here instead of adding an is_purgeable >field? I thought it would make the meaning more clear, but I guess there's no point in duplicating information. >> + >> + res |= (bo->base.pages) ? DRM_GEM_OBJECT_RESIDENT : 0; > >Does it make sense to have DRM_GEM_OBJECT_PURGEABLE set when >DRM_GEM_OBJECT_RESIDENT is not? Freedreno's msm_gem_status seems not to care about this because drm_show_memory_stats is already handling this situation: if (s & DRM_GEM_OBJECT_RESIDENT) { 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: */ s &= ~DRM_GEM_OBJECT_PURGEABLE; } >> + >> + return res; >> +} >> static const struct drm_gem_object_funcs panfrost_gem_funcs = { >> .free = panfrost_gem_free_object, >> .open = panfrost_gem_open, >> @@ -206,6 +217,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = { >> .vmap = drm_gem_shmem_object_vmap, >> .vunmap = drm_gem_shmem_object_vunmap, >> .mmap = drm_gem_shmem_object_mmap, >> + .status = panfrost_gem_status, >> .vm_ops = &drm_gem_shmem_vm_ops, >> }; >> >> diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h >> index ad2877eeeccd..e06f7ceb8f73 100644 >> --- a/drivers/gpu/drm/panfrost/panfrost_gem.h >> +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h >> @@ -38,6 +38,7 @@ struct panfrost_gem_object { >> >> bool noexec :1; >> bool is_heap :1; >> + bool is_purgable :1; >> }; >> >> struct panfrost_gem_mapping {
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 3fd372301019..93d5f5538c0b 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -440,11 +440,14 @@ static int panfrost_ioctl_madvise(struct drm_device *dev, void *data, args->retained = drm_gem_shmem_madvise(&bo->base, args->madv); if (args->retained) { - if (args->madv == PANFROST_MADV_DONTNEED) + if (args->madv == PANFROST_MADV_DONTNEED) { list_move_tail(&bo->base.madv_list, &pfdev->shrinker_list); - else if (args->madv == PANFROST_MADV_WILLNEED) + bo->is_purgable = true; + } else if (args->madv == PANFROST_MADV_WILLNEED) { list_del_init(&bo->base.madv_list); + bo->is_purgable = false; + } } out_unlock_mappings: @@ -559,6 +562,8 @@ static void panfrost_show_fdinfo(struct drm_printer *p, struct drm_file *file) struct panfrost_device *pfdev = dev->dev_private; panfrost_gpu_show_fdinfo(pfdev, file->driver_priv, p); + + drm_show_memory_stats(p, file); } static const struct file_operations panfrost_drm_driver_fops = { diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.c b/drivers/gpu/drm/panfrost/panfrost_gem.c index 3c812fbd126f..aea16b0e4dda 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.c +++ b/drivers/gpu/drm/panfrost/panfrost_gem.c @@ -195,6 +195,17 @@ static int panfrost_gem_pin(struct drm_gem_object *obj) return drm_gem_shmem_pin(&bo->base); } +static enum drm_gem_object_status panfrost_gem_status(struct drm_gem_object *obj) +{ + struct panfrost_gem_object *bo = to_panfrost_bo(obj); + enum drm_gem_object_status res = 0; + + res |= (bo->is_purgable) ? DRM_GEM_OBJECT_PURGEABLE : 0; + + res |= (bo->base.pages) ? DRM_GEM_OBJECT_RESIDENT : 0; + + return res; +} static const struct drm_gem_object_funcs panfrost_gem_funcs = { .free = panfrost_gem_free_object, .open = panfrost_gem_open, @@ -206,6 +217,7 @@ static const struct drm_gem_object_funcs panfrost_gem_funcs = { .vmap = drm_gem_shmem_object_vmap, .vunmap = drm_gem_shmem_object_vunmap, .mmap = drm_gem_shmem_object_mmap, + .status = panfrost_gem_status, .vm_ops = &drm_gem_shmem_vm_ops, }; diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h index ad2877eeeccd..e06f7ceb8f73 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gem.h +++ b/drivers/gpu/drm/panfrost/panfrost_gem.h @@ -38,6 +38,7 @@ struct panfrost_gem_object { bool noexec :1; bool is_heap :1; + bool is_purgable :1; }; struct panfrost_gem_mapping {
A new DRM GEM object function is added so that drm_show_memory_stats can provider more accurate memory usage numbers. Ideally, in panfrost_gem_status, the BO's purgeable flag would be checked after locking the driver's shrinker mutex, but drm_show_memory_stats takes over the drm file's object handle database spinlock, so there's potential for a race condition here. Signed-off-by: Adrián Larumbe <adrian.larumbe@collabora.com> --- drivers/gpu/drm/panfrost/panfrost_drv.c | 9 +++++++-- drivers/gpu/drm/panfrost/panfrost_gem.c | 12 ++++++++++++ drivers/gpu/drm/panfrost/panfrost_gem.h | 1 + 3 files changed, 20 insertions(+), 2 deletions(-)