Re: [PATCH v4] drm/virtio: Track total GPU memory for virtio driver
On Thu, Jan 21, 2021 at 9:40 PM Yiwei Zhang wrote:
>
> On the success of virtio_gpu_object_create, add size of newly allocated
> bo to the tracked total_mem. In drm_gem_object_funcs.free, after the gem
> bo loses its last refcount, subtract the bo size from the tracked
> total_mem if the original underlying memory allocation is successful.
>
> It's more accurate to do this in device driver layer to best match when
> the underlying resource gets allocated and destroyed during tracing.
>
> Signed-off-by: Yiwei Zhang
> ---
> drivers/gpu/drm/virtio/Kconfig | 1 +
> drivers/gpu/drm/virtio/virtgpu_drv.h| 2 ++
> drivers/gpu/drm/virtio/virtgpu_object.c | 11 +++
> 3 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> index b925b8b1da16..e103b7e883b1 100644
> --- a/drivers/gpu/drm/virtio/Kconfig
> +++ b/drivers/gpu/drm/virtio/Kconfig
> @@ -5,6 +5,7 @@ config DRM_VIRTIO_GPU
> select DRM_KMS_HELPER
> select DRM_GEM_SHMEM_HELPER
> select VIRTIO_DMA_SHARED_BUFFER
> + select TRACE_GPU_MEM
> help
>This is the virtual GPU driver for virtio. It can be used with
>QEMU based VMMs (like KVM or Xen).
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 6a232553c99b..c5622f9b591f 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -249,6 +249,8 @@ struct virtio_gpu_device {
> spinlock_t resource_export_lock;
> /* protects map state and host_visible_mm */
> spinlock_t host_visible_lock;
> +
> + atomic64_t total_mem;
> };
>
> struct virtio_gpu_fpriv {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index d69a5b6da553..e2251fc41509 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -25,12 +25,21 @@
>
> #include
> #include
> +#include
>
> #include "virtgpu_drv.h"
>
> static int virtio_gpu_virglrenderer_workaround = 1;
> module_param_named(virglhack, virtio_gpu_virglrenderer_workaround, int,
> 0400);
>
> +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device
> *vgdev,
> + s64 delta)
> +{
> + u64 total_mem = atomic64_add_return(delta, >total_mem);
> +
> + trace_gpu_mem_total(vgdev->ddev->primary->index, 0, total_mem);
> +}
> +
> int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev, uint32_t
> *resid)
> {
> if (virtio_gpu_virglrenderer_workaround) {
> @@ -104,6 +113,7 @@ static void virtio_gpu_free_object(struct drm_gem_object
> *obj)
> struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
>
> if (bo->created) {
> + virtio_gpu_trace_total_mem(vgdev, -(obj->size));
> virtio_gpu_cmd_unref_resource(vgdev, bo);
> virtio_gpu_notify(vgdev);
> /* completion handler calls virtio_gpu_cleanup_object() */
> @@ -265,6 +275,7 @@ int virtio_gpu_object_create(struct virtio_gpu_device
> *vgdev,
> virtio_gpu_object_attach(vgdev, bo, ents, nents);
> }
>
> + virtio_gpu_trace_total_mem(vgdev, shmem_obj->base.size);
> *bo_ptr = bo;
> return 0;
>
> --
> 2.30.0.280.ga3ce27912f-goog
>
Re Gerd and Daniel:
I'm not sure why we want to couple this patch too much with the
dma-bufs tracking. The tracepoint added here itself is pretty useful
for tracking gem bo total usage in virtio gpu upon tracing. The
original purpose for integrating this tracepoint in all Android gpu
kernel drivers is to just track total gpu memory usage and serve the
accurate data to game developers in a much easier way. It's something
they can rely on for robust testing and regression monitoring.
The only overlap with the dma-buf side is when we export a bo via
prime to a dma-buf. But still, the total here is already useful for
this particular device. Using which approach to account for the
overlap wouldn't block this small integration from my understanding.
Besides, there's no plan for adding per-process gem total tracking in
virtio-gpu at this moment. This patch should be light enough to carry
without worrying about tech debt I believe.
Many thanks!
Yiwei
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/virtio: Track total GPU memory for virtio driver
On Wed, Jan 20, 2021 at 1:11 AM Daniel Vetter wrote:
>
> On Tue, Jan 19, 2021 at 11:08:12AM -0800, Yiwei Zhang wrote:
> > On Mon, Jan 18, 2021 at 11:03 PM Daniel Vetter wrote:
> > >
> > > On Tue, Jan 19, 2021 at 12:41 AM Yiwei Zhang wrote:
> > > >
> > > > On the success of virtio_gpu_object_create, add size of newly allocated
> > > > bo to the tracled total_mem. In drm_gem_object_funcs.free, after the gem
> > > > bo lost its last refcount, subtract the bo size from the tracked
> > > > total_mem if the original underlying memory allocation is successful.
> > > >
> > > > Signed-off-by: Yiwei Zhang
> > >
> > > Isn't this something that ideally we'd for everyone? Also tracepoint
> > > for showing the total feels like tracepoint abuse, usually we show
> > > totals somewhere in debugfs or similar, and tracepoint just for what's
> > > happening (i.e. which object got deleted/created).
> > >
> > > What is this for exactly?
> > > -Daniel
> > >
> > > > ---
> > > > drivers/gpu/drm/virtio/Kconfig | 1 +
> > > > drivers/gpu/drm/virtio/virtgpu_drv.h| 4
> > > > drivers/gpu/drm/virtio/virtgpu_object.c | 19 +++
> > > > 3 files changed, 24 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/virtio/Kconfig
> > > > b/drivers/gpu/drm/virtio/Kconfig
> > > > index b925b8b1da16..e103b7e883b1 100644
> > > > --- a/drivers/gpu/drm/virtio/Kconfig
> > > > +++ b/drivers/gpu/drm/virtio/Kconfig
> > > > @@ -5,6 +5,7 @@ config DRM_VIRTIO_GPU
> > > > select DRM_KMS_HELPER
> > > > select DRM_GEM_SHMEM_HELPER
> > > > select VIRTIO_DMA_SHARED_BUFFER
> > > > + select TRACE_GPU_MEM
> > > > help
> > > >This is the virtual GPU driver for virtio. It can be used
> > > > with
> > > >QEMU based VMMs (like KVM or Xen).
> > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > > b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > > index 6a232553c99b..7c60e7486bc4 100644
> > > > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > > > @@ -249,6 +249,10 @@ struct virtio_gpu_device {
> > > > spinlock_t resource_export_lock;
> > > > /* protects map state and host_visible_mm */
> > > > spinlock_t host_visible_lock;
> > > > +
> > > > +#ifdef CONFIG_TRACE_GPU_MEM
> > > > + atomic64_t total_mem;
> > > > +#endif
> > > > };
> > > >
> > > > struct virtio_gpu_fpriv {
> > > > diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> > > > b/drivers/gpu/drm/virtio/virtgpu_object.c
> > > > index d69a5b6da553..1e16226cebbe 100644
> > > > --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> > > > +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> > > > @@ -25,12 +25,29 @@
> > > >
> > > > #include
> > > > #include
> > > > +#ifdef CONFIG_TRACE_GPU_MEM
> > > > +#include
> > > > +#endif
> > > >
> > > > #include "virtgpu_drv.h"
> > > >
> > > > static int virtio_gpu_virglrenderer_workaround = 1;
> > > > module_param_named(virglhack, virtio_gpu_virglrenderer_workaround,
> > > > int, 0400);
> > > >
> > > > +#ifdef CONFIG_TRACE_GPU_MEM
> > > > +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device
> > > > *vgdev,
> > > > + s64 delta)
> > > > +{
> > > > + u64 total_mem = atomic64_add_return(delta, >total_mem);
> > > > +
> > > > + trace_gpu_mem_total(0, 0, total_mem);
> > > > +}
> > > > +#else
> > > > +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device
> > > > *, s64)
> > > > +{
> > > > +}
> > > > +#endif
> > > > +
> > > > int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev,
> > > > uint32_t *resid)
> > > > {
> > > > if (virtio_gpu_virglrenderer_workaround) {
> > > > @@ -104,6 +121,7 @@ static void virtio_gpu_free_object(struct
> > > > drm_gem_object *obj)
> > > > struct virtio_gpu_device *vgdev =
> > > > bo->base.base.dev->dev_private;
> > > >
> > > > if (bo->created) {
> > > > + virtio_gpu_trace_total_mem(vgdev, -(obj->size));
> > > > virtio_gpu_cmd_unref_resource(vgdev, bo);
> > > > virtio_gpu_notify(vgdev);
> > > > /* completion handler calls virtio_gpu_cleanup_object()
> > > > */
> > > > @@ -265,6 +283,7 @@ int virtio_gpu_object_create(struct
> > > > virtio_gpu_device *vgdev,
> > > > virtio_gpu_object_attach(vgdev, bo, ents, nents);
> > > > }
> > > >
> > > > + virtio_gpu_trace_total_mem(vgdev, shmem_obj->base.size);
> > > > *bo_ptr = bo;
> > > > return 0;
> > > >
> > > > --
> > > > 2.30.0.284.gd98b1dd5eaa7-goog
> > > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> >
> > Thanks for your reply! Android Cuttlefish virtual platform is using
> > the virtio-gpu driver, and we currently are carrying this small patch
> > at the downstream side. This is essential for us because:
> > (1)
Re: [PATCH v2] drm/virtio: Track total GPU memory for virtio driver
On Mon, Jan 18, 2021 at 11:03 PM Daniel Vetter wrote:
>
> On Tue, Jan 19, 2021 at 12:41 AM Yiwei Zhang wrote:
> >
> > On the success of virtio_gpu_object_create, add size of newly allocated
> > bo to the tracled total_mem. In drm_gem_object_funcs.free, after the gem
> > bo lost its last refcount, subtract the bo size from the tracked
> > total_mem if the original underlying memory allocation is successful.
> >
> > Signed-off-by: Yiwei Zhang
>
> Isn't this something that ideally we'd for everyone? Also tracepoint
> for showing the total feels like tracepoint abuse, usually we show
> totals somewhere in debugfs or similar, and tracepoint just for what's
> happening (i.e. which object got deleted/created).
>
> What is this for exactly?
> -Daniel
>
> > ---
> > drivers/gpu/drm/virtio/Kconfig | 1 +
> > drivers/gpu/drm/virtio/virtgpu_drv.h| 4
> > drivers/gpu/drm/virtio/virtgpu_object.c | 19 +++
> > 3 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> > index b925b8b1da16..e103b7e883b1 100644
> > --- a/drivers/gpu/drm/virtio/Kconfig
> > +++ b/drivers/gpu/drm/virtio/Kconfig
> > @@ -5,6 +5,7 @@ config DRM_VIRTIO_GPU
> > select DRM_KMS_HELPER
> > select DRM_GEM_SHMEM_HELPER
> > select VIRTIO_DMA_SHARED_BUFFER
> > + select TRACE_GPU_MEM
> > help
> >This is the virtual GPU driver for virtio. It can be used with
> >QEMU based VMMs (like KVM or Xen).
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > index 6a232553c99b..7c60e7486bc4 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> > +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> > @@ -249,6 +249,10 @@ struct virtio_gpu_device {
> > spinlock_t resource_export_lock;
> > /* protects map state and host_visible_mm */
> > spinlock_t host_visible_lock;
> > +
> > +#ifdef CONFIG_TRACE_GPU_MEM
> > + atomic64_t total_mem;
> > +#endif
> > };
> >
> > struct virtio_gpu_fpriv {
> > diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> > b/drivers/gpu/drm/virtio/virtgpu_object.c
> > index d69a5b6da553..1e16226cebbe 100644
> > --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> > +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> > @@ -25,12 +25,29 @@
> >
> > #include
> > #include
> > +#ifdef CONFIG_TRACE_GPU_MEM
> > +#include
> > +#endif
> >
> > #include "virtgpu_drv.h"
> >
> > static int virtio_gpu_virglrenderer_workaround = 1;
> > module_param_named(virglhack, virtio_gpu_virglrenderer_workaround, int,
> > 0400);
> >
> > +#ifdef CONFIG_TRACE_GPU_MEM
> > +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device
> > *vgdev,
> > + s64 delta)
> > +{
> > + u64 total_mem = atomic64_add_return(delta, >total_mem);
> > +
> > + trace_gpu_mem_total(0, 0, total_mem);
> > +}
> > +#else
> > +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device *,
> > s64)
> > +{
> > +}
> > +#endif
> > +
> > int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev, uint32_t
> > *resid)
> > {
> > if (virtio_gpu_virglrenderer_workaround) {
> > @@ -104,6 +121,7 @@ static void virtio_gpu_free_object(struct
> > drm_gem_object *obj)
> > struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
> >
> > if (bo->created) {
> > + virtio_gpu_trace_total_mem(vgdev, -(obj->size));
> > virtio_gpu_cmd_unref_resource(vgdev, bo);
> > virtio_gpu_notify(vgdev);
> > /* completion handler calls virtio_gpu_cleanup_object() */
> > @@ -265,6 +283,7 @@ int virtio_gpu_object_create(struct virtio_gpu_device
> > *vgdev,
> > virtio_gpu_object_attach(vgdev, bo, ents, nents);
> > }
> >
> > + virtio_gpu_trace_total_mem(vgdev, shmem_obj->base.size);
> > *bo_ptr = bo;
> > return 0;
> >
> > --
> > 2.30.0.284.gd98b1dd5eaa7-goog
> >
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Thanks for your reply! Android Cuttlefish virtual platform is using
the virtio-gpu driver, and we currently are carrying this small patch
at the downstream side. This is essential for us because:
(1) Android has deprecated debugfs on production devices already
(2) Android GPU drivers are not DRM based, and this won't change in a
short term.
Android relies on this tracepoint + eBPF to make the GPU memory totals
available at runtime on production devices, which has been enforced
already. Not only game developers can have a reliable kernel total GPU
memory to look at, but also Android leverages this to take GPU memory
usage out from the system lost ram.
I'm not sure whether the other DRM drivers would like to integrate
this tracepoint(maybe upstream drivers will move away from debugfs
later as well?), but at least we hope virtio-gpu can take
Re: [PATCH] Track total GPU memory for virtio driver
On Mon, Jan 18, 2021 at 2:47 PM Yiwei Zhang wrote:
>
> On the success of virtio_gpu_object_create, add size of newly allocated
> bo to the tracled total_mem. In drm_gem_object_funcs.free, after the gem
> bo lost its last refcount, subtract the bo size from the tracked
> total_mem if the original underlying memory allocation is successful.
>
> Signed-off-by: Yiwei Zhang
> ---
> drivers/gpu/drm/virtio/Kconfig | 1 +
> drivers/gpu/drm/virtio/virtgpu_drv.h| 4
> drivers/gpu/drm/virtio/virtgpu_object.c | 19 +++
> 3 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/virtio/Kconfig b/drivers/gpu/drm/virtio/Kconfig
> index b925b8b1da16..e103b7e883b1 100644
> --- a/drivers/gpu/drm/virtio/Kconfig
> +++ b/drivers/gpu/drm/virtio/Kconfig
> @@ -5,6 +5,7 @@ config DRM_VIRTIO_GPU
> select DRM_KMS_HELPER
> select DRM_GEM_SHMEM_HELPER
> select VIRTIO_DMA_SHARED_BUFFER
> + select TRACE_GPU_MEM
> help
>This is the virtual GPU driver for virtio. It can be used with
>QEMU based VMMs (like KVM or Xen).
> diff --git a/drivers/gpu/drm/virtio/virtgpu_drv.h
> b/drivers/gpu/drm/virtio/virtgpu_drv.h
> index 6a232553c99b..7c60e7486bc4 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_drv.h
> +++ b/drivers/gpu/drm/virtio/virtgpu_drv.h
> @@ -249,6 +249,10 @@ struct virtio_gpu_device {
> spinlock_t resource_export_lock;
> /* protects map state and host_visible_mm */
> spinlock_t host_visible_lock;
> +
> +#ifdef CONFIG_TRACE_GPU_MEM
> + atomic64_t total_mem;
> +#endif
> };
>
> struct virtio_gpu_fpriv {
> diff --git a/drivers/gpu/drm/virtio/virtgpu_object.c
> b/drivers/gpu/drm/virtio/virtgpu_object.c
> index d69a5b6da553..1e16226cebbe 100644
> --- a/drivers/gpu/drm/virtio/virtgpu_object.c
> +++ b/drivers/gpu/drm/virtio/virtgpu_object.c
> @@ -25,12 +25,29 @@
>
> #include
> #include
> +#ifdef CONFIG_TRACE_GPU_MEM
> +#include
> +#endif
>
> #include "virtgpu_drv.h"
>
> static int virtio_gpu_virglrenderer_workaround = 1;
> module_param_named(virglhack, virtio_gpu_virglrenderer_workaround, int,
> 0400);
>
> +#ifdef CONFIG_TRACE_GPU_MEM
> +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device
> *vgdev,
> + s64 delta)
> +{
> + u64 total_mem = atomic64_add_return(delta, >total_mem);
> +
> + trace_gpu_mem_total(0, 0, total_mem);
> +}
> +#else
> +static inline void virtio_gpu_trace_total_mem(struct virtio_gpu_device *,
> s64)
> +{
> +}
> +#endif
> +
> int virtio_gpu_resource_id_get(struct virtio_gpu_device *vgdev, uint32_t
> *resid)
> {
> if (virtio_gpu_virglrenderer_workaround) {
> @@ -104,6 +121,7 @@ static void virtio_gpu_free_object(struct drm_gem_object
> *obj)
> struct virtio_gpu_device *vgdev = bo->base.base.dev->dev_private;
>
> if (bo->created) {
> + virtio_gpu_trace_total_mem(vgdev, -(obj->size));
> virtio_gpu_cmd_unref_resource(vgdev, bo);
> virtio_gpu_notify(vgdev);
> /* completion handler calls virtio_gpu_cleanup_object() */
> @@ -265,6 +283,7 @@ int virtio_gpu_object_create(struct virtio_gpu_device
> *vgdev,
> virtio_gpu_object_attach(vgdev, bo, ents, nents);
> }
>
> + virtio_gpu_trace_total_mem(vgdev, shmem_obj->base.size);
> *bo_ptr = bo;
> return 0;
>
> --
> 2.30.0.284.gd98b1dd5eaa7-goog
>
-CC android-kernel-t...@google.com
+CC kernel-t...@android.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
