Re: [PATCH v4] drm/virtio: Track total GPU memory for virtio driver
On Thu, Jan 21, 2021 at 11:58:22PM -0800, Yiwei Zhang wrote:
> 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.
The tracepoint is clearly more generic than just what you implement here,
to support the full use cases on Android's closed stacks. And it is uapi.
Tech debt isn't measured in lines of code, but in how expensive it's going
to be to fix up the mess in the future. uapi is expensive no matter how
few lines are used to implement it.
So yeah this
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
[PATCH v4] drm/virtio: Track total GPU memory for virtio driver
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
