Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Fri, 22 Jan 2021 11:03:01 +0100
From:   Daniel Vetter <daniel@ffwll.ch>
To:     Yiwei Zhang <zzyiwei@android.com>
Cc:     David Airlie <airlied@linux.ie>, Gerd Hoffmann <kraxel@redhat.com>,
        Daniel Vetter <daniel@ffwll.ch>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        "open list:VIRTIO CORE, NET..." 
        <virtualization@lists.linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        kernel-team <kernel-team@android.com>
Subject: Re: [PATCH v4] drm/virtio: Track total GPU memory for virtio driver
Message-ID: <YAqi1fKM8Vy8ocay@phenom.ffwll.local>
Mail-Followup-To: Yiwei Zhang <zzyiwei@android.com>,
        David Airlie <airlied@linux.ie>, Gerd Hoffmann <kraxel@redhat.com>,
        dri-devel <dri-devel@lists.freedesktop.org>,
        "open list:VIRTIO CORE, NET..." <virtualization@lists.linux-foundation.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        kernel-team <kernel-team@android.com>
References: <20210122053159.1720274-1-zzyiwei@android.com>
 <20210122054011.1722954-1-zzyiwei@android.com>
 <CAKB3++bLzn5YVaR3iUTHtNYwBeG6Z27NjtWZ1q-xmjOkApAQwg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAKB3++bLzn5YVaR3iUTHtNYwBeG6Z27NjtWZ1q-xmjOkApAQwg@mail.gmail.com>
Precedence: bulk

On Thu, Jan 21, 2021 at 11:58:22PM -0800, Yiwei Zhang wrote:
> On Thu, Jan 21, 2021 at 9:40 PM Yiwei Zhang <zzyiwei@android.com> 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 <zzyiwei@android.com>
> > ---
> >  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 <linux/dma-mapping.h>
> >  #include <linux/moduleparam.h>
> > +#include <trace/events/gpu_mem.h>
> >
> >  #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, &vgdev->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 needs to be properly thought out, properly implemented (not
just on the virtual demo stack but something that looks like actual
production stack), with open drivers, proper alignment with other efforts
like tracking memory with cgroups, and the interactions with dma-buf
tracking resolved, and igt testcases (this is meant to be generic after
all), and at least solid proposals for rolling this out across the drm
drivers, and ...

In other words, new uapi needs to be done right.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch