| Message ID | 20230411225725.2032862-1-robdclark@gmail.com |
|---|---|
| Headers | show |
| Series | drm: fdinfo memory stats | expand |
On Tue, Apr 11, 2023 at 03:56:08PM -0700, Rob Clark wrote: > From: Rob Clark <robdclark@chromium.org> > > Signed-off-by: Rob Clark <robdclark@chromium.org> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 16 ++++++---------- > drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h | 2 +- > 3 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > index f5ffca24def4..3611cfd5f076 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c > @@ -2752,7 +2752,7 @@ static const struct file_operations amdgpu_driver_kms_fops = { > .compat_ioctl = amdgpu_kms_compat_ioctl, > #endif > #ifdef CONFIG_PROC_FS > - .show_fdinfo = amdgpu_show_fdinfo > + .show_fdinfo = drm_fop_show_fdinfo, > #endif > }; > > @@ -2807,6 +2807,7 @@ static const struct drm_driver amdgpu_kms_driver = { > .dumb_map_offset = amdgpu_mode_dumb_mmap, > .fops = &amdgpu_driver_kms_fops, > .release = &amdgpu_driver_release_kms, > + .show_fdinfo = amdgpu_show_fdinfo, > > .prime_handle_to_fd = drm_gem_prime_handle_to_fd, > .prime_fd_to_handle = drm_gem_prime_fd_to_handle, > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c > index 99a7855ab1bc..c2fdd5e448d1 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c > @@ -53,9 +53,8 @@ static const char *amdgpu_ip_name[AMDGPU_HW_IP_NUM] = { > [AMDGPU_HW_IP_VCN_JPEG] = "jpeg", > }; > > -void amdgpu_show_fdinfo(struct seq_file *m, struct file *f) > +void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file) > { > - struct drm_file *file = f->private_data; > struct amdgpu_device *adev = drm_to_adev(file->minor->dev); > struct amdgpu_fpriv *fpriv = file->driver_priv; > struct amdgpu_vm *vm = &fpriv->vm; > @@ -86,18 +85,15 @@ void amdgpu_show_fdinfo(struct seq_file *m, struct file *f) > * ****************************************************************** > */ > > - seq_printf(m, "pasid:\t%u\n", fpriv->vm.pasid); > - seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name); > - seq_printf(m, "drm-pdev:\t%04x:%02x:%02x.%d\n", domain, bus, dev, fn); > - seq_printf(m, "drm-client-id:\t%Lu\n", vm->immediate.fence_context); > - seq_printf(m, "drm-memory-vram:\t%llu KiB\n", vram_mem/1024UL); > - seq_printf(m, "drm-memory-gtt: \t%llu KiB\n", gtt_mem/1024UL); > - seq_printf(m, "drm-memory-cpu: \t%llu KiB\n", cpu_mem/1024UL); > + drm_printf(p, "pasid:\t%u\n", fpriv->vm.pasid); > + drm_printf(p, "drm-memory-vram:\t%llu KiB\n", vram_mem/1024UL); > + drm_printf(p, "drm-memory-gtt: \t%llu KiB\n", gtt_mem/1024UL); > + drm_printf(p, "drm-memory-cpu: \t%llu KiB\n", cpu_mem/1024UL); random aside, but we're not super consistent here, some of these have an additional ' ' space. I guess a next step would be a drm_fdinfo_printf(drm_printer *p, const char *name, const char *printf, ...) and maybe some specialized ones that dtrt for specific parameters, like drm_fdinfo_llu(). But that's for next one I guess :-) -Daniel > for (hw_ip = 0; hw_ip < AMDGPU_HW_IP_NUM; ++hw_ip) { > if (!usage[hw_ip]) > continue; > > - seq_printf(m, "drm-engine-%s:\t%Ld ns\n", amdgpu_ip_name[hw_ip], > + drm_printf(p, "drm-engine-%s:\t%Ld ns\n", amdgpu_ip_name[hw_ip], > ktime_to_ns(usage[hw_ip])); > } > } > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h > index e86834bfea1d..0398f5a159ef 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h > @@ -37,6 +37,6 @@ > #include "amdgpu_ids.h" > > uint32_t amdgpu_get_ip_count(struct amdgpu_device *adev, int id); > -void amdgpu_show_fdinfo(struct seq_file *m, struct file *f); > +void amdgpu_show_fdinfo(struct drm_printer *p, struct drm_file *file); > > #endif > -- > 2.39.2 >
Am 12.04.23 um 00:56 schrieb Rob Clark: > From: Rob Clark <robdclark@chromium.org> > > Similar motivation to other similar recent attempt[1]. But with an > attempt to have some shared code for this. As well as documentation. > > It is probably a bit UMA-centric, I guess devices with VRAM might want > some placement stats as well. But this seems like a reasonable start. > > Basic gputop support: https://patchwork.freedesktop.org/series/116236/ > And already nvtop support: https://github.com/Syllo/nvtop/pull/204 > > [1] https://patchwork.freedesktop.org/series/112397/ I think the extra client id looks a bit superfluous since the ino of the file should already be unique and IIRC we have been already using that one. Apart from that looks good to me, Christian. PS: For some reason only the two patches I was CCed on ended up in my inbox, dri-devel swallowed all the rest and hasn't spit it out yet. Had to dig up the rest from patchwork. > > Rob Clark (7): > drm: Add common fdinfo helper > drm/msm: Switch to fdinfo helper > drm/amdgpu: Switch to fdinfo helper > drm/i915: Switch to fdinfo helper > drm/etnaviv: Switch to fdinfo helper > drm: Add fdinfo memory stats > drm/msm: Add memory stats to fdinfo > > Documentation/gpu/drm-usage-stats.rst | 21 ++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 16 ++- > drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h | 2 +- > drivers/gpu/drm/drm_file.c | 115 +++++++++++++++++++++ > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +- > drivers/gpu/drm/i915/i915_driver.c | 3 +- > drivers/gpu/drm/i915/i915_drm_client.c | 18 +--- > drivers/gpu/drm/i915/i915_drm_client.h | 2 +- > drivers/gpu/drm/msm/msm_drv.c | 11 +- > drivers/gpu/drm/msm/msm_gem.c | 15 +++ > drivers/gpu/drm/msm/msm_gpu.c | 2 - > include/drm/drm_drv.h | 7 ++ > include/drm/drm_file.h | 5 + > include/drm/drm_gem.h | 19 ++++ > 15 files changed, 208 insertions(+), 41 deletions(-) >
On 12/04/2023 10:34, Christian König wrote: > Am 12.04.23 um 00:56 schrieb Rob Clark: >> From: Rob Clark <robdclark@chromium.org> >> >> Similar motivation to other similar recent attempt[1]. But with an >> attempt to have some shared code for this. As well as documentation. >> >> It is probably a bit UMA-centric, I guess devices with VRAM might want >> some placement stats as well. But this seems like a reasonable start. >> >> Basic gputop support: https://patchwork.freedesktop.org/series/116236/ >> And already nvtop support: https://github.com/Syllo/nvtop/pull/204 >> >> [1] https://patchwork.freedesktop.org/series/112397/ > > I think the extra client id looks a bit superfluous since the ino of the > file should already be unique and IIRC we have been already using that one. Do you mean file_inode(struct drm_file->filp)->i_ino ? That one would be the same number for all clients which open the same device node so wouldn't work. I also don't think the atomic_add_return for client id works either, since it can alias on overflow. In i915 I use an xarray and __xa_alloc_cyclic. Regards, Tvrtko
Am 12.04.23 um 14:10 schrieb Tvrtko Ursulin: > > On 12/04/2023 10:34, Christian König wrote: >> Am 12.04.23 um 00:56 schrieb Rob Clark: >>> From: Rob Clark <robdclark@chromium.org> >>> >>> Similar motivation to other similar recent attempt[1]. But with an >>> attempt to have some shared code for this. As well as documentation. >>> >>> It is probably a bit UMA-centric, I guess devices with VRAM might want >>> some placement stats as well. But this seems like a reasonable start. >>> >>> Basic gputop support: https://patchwork.freedesktop.org/series/116236/ >>> And already nvtop support: https://github.com/Syllo/nvtop/pull/204 >>> >>> [1] https://patchwork.freedesktop.org/series/112397/ >> >> I think the extra client id looks a bit superfluous since the ino of >> the file should already be unique and IIRC we have been already using >> that one. > > Do you mean file_inode(struct drm_file->filp)->i_ino ? That one would > be the same number for all clients which open the same device node so > wouldn't work. Ah, right. DMA-buf used a separate ino per buffer, but we don't do that for the drm_file. > > I also don't think the atomic_add_return for client id works either, > since it can alias on overflow. Yeah, we might want to use a 64bit number here if any. Christian. > > In i915 I use an xarray and __xa_alloc_cyclic. > > Regards, > > Tvrtko
On Wed, Apr 12, 2023 at 12:59 AM Daniel Vetter <daniel@ffwll.ch> wrote: > > On Tue, Apr 11, 2023 at 03:56:10PM -0700, Rob Clark wrote: > > From: Rob Clark <robdclark@chromium.org> > > > > Signed-off-by: Rob Clark <robdclark@chromium.org> > > You're on an old tree, this got reverted. But I'm kinda wondering whether > another patch on top shouldn't just includ the drm_show_fdinfo in > DRM_GEM_FOPS macro ... There's really no good reasons for drivers to not > have this I think? oh, I'm roughly on msm-next, so didn't see the revert.. I'll drop this one. But with things in flux, this is why I decided against adding it to DRM_GEM_FOPS. Ie. we should do that as a followup cleanup step once everyone is moved over to the new helpers to avoid conflicts or build breaks when merging things via different driver trees BR, -R > -Daniel > > > --- > > drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 ++++------ > > 1 file changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.c b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > index 44ca803237a5..170000d6af94 100644 > > --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.c > > @@ -476,9 +476,8 @@ static const struct drm_ioctl_desc etnaviv_ioctls[] = { > > ETNA_IOCTL(PM_QUERY_SIG, pm_query_sig, DRM_RENDER_ALLOW), > > }; > > > > -static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) > > +static void etnaviv_fop_show_fdinfo(struct drm_printer *p, struct drm_file *file) > > { > > - struct drm_file *file = f->private_data; > > struct drm_device *dev = file->minor->dev; > > struct etnaviv_drm_private *priv = dev->dev_private; > > struct etnaviv_file_private *ctx = file->driver_priv; > > @@ -487,8 +486,6 @@ static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) > > * For a description of the text output format used here, see > > * Documentation/gpu/drm-usage-stats.rst. > > */ > > - seq_printf(m, "drm-driver:\t%s\n", dev->driver->name); > > - seq_printf(m, "drm-client-id:\t%u\n", ctx->id); > > > > for (int i = 0; i < ETNA_MAX_PIPES; i++) { > > struct etnaviv_gpu *gpu = priv->gpu[i]; > > @@ -507,7 +504,7 @@ static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) > > cur = snprintf(engine + cur, sizeof(engine) - cur, > > "%sNN", cur ? "/" : ""); > > > > - seq_printf(m, "drm-engine-%s:\t%llu ns\n", engine, > > + drm_printf(p, "drm-engine-%s:\t%llu ns\n", engine, > > ctx->sched_entity[i].elapsed_ns); > > } > > } > > @@ -515,7 +512,7 @@ static void etnaviv_fop_show_fdinfo(struct seq_file *m, struct file *f) > > static const struct file_operations fops = { > > .owner = THIS_MODULE, > > DRM_GEM_FOPS, > > - .show_fdinfo = etnaviv_fop_show_fdinfo, > > + .show_fdinfo = drm_fop_show_fdinfo, > > }; > > > > static const struct drm_driver etnaviv_drm_driver = { > > @@ -529,6 +526,7 @@ static const struct drm_driver etnaviv_drm_driver = { > > #ifdef CONFIG_DEBUG_FS > > .debugfs_init = etnaviv_debugfs_init, > > #endif > > + .show_fdinfo = etnaviv_fop_show_fdinfo, > > .ioctls = etnaviv_ioctls, > > .num_ioctls = DRM_ETNAVIV_NUM_IOCTLS, > > .fops = &fops, > > -- > > 2.39.2 > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch
From: Rob Clark <robdclark@chromium.org> Similar motivation to other similar recent attempt[1]. But with an attempt to have some shared code for this. As well as documentation. It is probably a bit UMA-centric, I guess devices with VRAM might want some placement stats as well. But this seems like a reasonable start. Basic gputop support: https://patchwork.freedesktop.org/series/116236/ And already nvtop support: https://github.com/Syllo/nvtop/pull/204 [1] https://patchwork.freedesktop.org/series/112397/ Rob Clark (7): drm: Add common fdinfo helper drm/msm: Switch to fdinfo helper drm/amdgpu: Switch to fdinfo helper drm/i915: Switch to fdinfo helper drm/etnaviv: Switch to fdinfo helper drm: Add fdinfo memory stats drm/msm: Add memory stats to fdinfo Documentation/gpu/drm-usage-stats.rst | 21 ++++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 3 +- drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.c | 16 ++- drivers/gpu/drm/amd/amdgpu/amdgpu_fdinfo.h | 2 +- drivers/gpu/drm/drm_file.c | 115 +++++++++++++++++++++ drivers/gpu/drm/etnaviv/etnaviv_drv.c | 10 +- drivers/gpu/drm/i915/i915_driver.c | 3 +- drivers/gpu/drm/i915/i915_drm_client.c | 18 +--- drivers/gpu/drm/i915/i915_drm_client.h | 2 +- drivers/gpu/drm/msm/msm_drv.c | 11 +- drivers/gpu/drm/msm/msm_gem.c | 15 +++ drivers/gpu/drm/msm/msm_gpu.c | 2 - include/drm/drm_drv.h | 7 ++ include/drm/drm_file.h | 5 + include/drm/drm_gem.h | 19 ++++ 15 files changed, 208 insertions(+), 41 deletions(-)