[PATCH v3 2/3] drm/vram: Add infrastructure for move_notify()

[Daniel Vetter]

On Fri, Sep 6, 2019 at 12:24 PM Thomas Zimmermann <tzimmermann at suse.de> wrote:
>
> Hi
>
> Am 06.09.19 um 11:28 schrieb Daniel Vetter:
> > On Fri, Sep 06, 2019 at 10:52:13AM +0200, Thomas Zimmermann wrote:
> >> This patch prepares VRAM helpers for lazy unmapping of buffer objects.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann at suse.de>
> >> ---
> >>  drivers/gpu/drm/drm_vram_mm_helper.c | 12 ++++++++++++
> >>  include/drm/drm_vram_mm_helper.h     |  4 ++++
> >>  2 files changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_vram_mm_helper.c b/drivers/gpu/drm/drm_vram_mm_helper.c
> >> index c911781d6728..31984690d5f3 100644
> >> --- a/drivers/gpu/drm/drm_vram_mm_helper.c
> >> +++ b/drivers/gpu/drm/drm_vram_mm_helper.c
> >> @@ -98,6 +98,17 @@ static int bo_driver_verify_access(struct ttm_buffer_object *bo,
> >>      return vmm->funcs->verify_access(bo, filp);
> >>  }
> >>
> >> +static void bo_driver_move_notify(struct ttm_buffer_object *bo,
> >> +                              bool evict,
> >> +                              struct ttm_mem_reg *new_mem)
> >> +{
> >> +    struct drm_vram_mm *vmm = drm_vram_mm_of_bdev(bo->bdev);
> >> +
> >> +    if (!vmm->funcs || !vmm->funcs->move_notify)
> >> +            return;
> >> +    vmm->funcs->move_notify(bo, evict, new_mem);
> >> +}
> >> +
> >>  static int bo_driver_io_mem_reserve(struct ttm_bo_device *bdev,
> >>                                  struct ttm_mem_reg *mem)
> >>  {
> >> @@ -140,6 +151,7 @@ static struct ttm_bo_driver bo_driver = {
> >>      .eviction_valuable = ttm_bo_eviction_valuable,
> >>      .evict_flags = bo_driver_evict_flags,
> >>      .verify_access = bo_driver_verify_access,
> >> +    .move_notify = bo_driver_move_notify,
> >>      .io_mem_reserve = bo_driver_io_mem_reserve,
> >>      .io_mem_free = bo_driver_io_mem_free,
> >>  };
> >> diff --git a/include/drm/drm_vram_mm_helper.h b/include/drm/drm_vram_mm_helper.h
> >> index 2aacfb1ccfae..7fb8700f45fe 100644
> >> --- a/include/drm/drm_vram_mm_helper.h
> >> +++ b/include/drm/drm_vram_mm_helper.h
> >> @@ -15,6 +15,8 @@ struct drm_device;
> >>      &ttm_bo_driver.evict_flags
> >>   * @verify_access:  Provides an implementation for \
> >>      struct &ttm_bo_driver.verify_access
> >> + * @move_notify:    Provides an implementation for
> >> + *                  struct &ttm_bo_driver.move_notify
> >>   *
> >>   * These callback function integrate VRAM MM with TTM buffer objects. New
> >>   * functions can be added if necessary.
> >> @@ -23,6 +25,8 @@ struct drm_vram_mm_funcs {
> >>      void (*evict_flags)(struct ttm_buffer_object *bo,
> >>                          struct ttm_placement *placement);
> >>      int (*verify_access)(struct ttm_buffer_object *bo, struct file *filp);
> >> +    void (*move_notify)(struct ttm_buffer_object *bo, bool evict,
> >> +                        struct ttm_mem_reg *new_mem);
> >
> > Is this indirection really worth it? We have a grand total of 1 driver
> > which isn't using gem (vmwgfx), and I don't think that one will ever
> > switch over to vram helpers.
> >
> > I'd fold that all in. Helpers don't need to be flexible enough to support
> > every possible use-case (that's just the job of the core), they can be
> > opinionated to get cleaner code for most cases.
> >
>
> The original idea was to make this as flexible as possible; probably
> more flexible than necessary. I wouldn't mind merging everything into
> one file, but please not in this patch set, can we keep it for now and I
> send you a cleanup next?

Oh sure, I just wondered about why this flexibility exists and
realized there's not really a user for it. And pondering this more I
didn't come up with a use-case where it might reasonably be needed,
and you don't want to roll your own complete, and couldn't come up
with anything. Aside from the locking question on patch 1 I think this
looks like a really tidy solution for the fbdev mapping issue, happy
to ack once we've figured out the locking thing.
-Daniel

>
> Best regards
> Thomas
>
> > For this case here if you fold this in you can unexport 3 EXPORT_SYMBOLs
> > (4 with this patch here), which I think is a neat simplification of the
> > complexity exposed to driver writers. Just put the necessary declarations
> > into a drm_vram_helper_internal.h so that drivers don't get at it by
> > accident (like the other drm*internal.h helpers we have).
> > -Daniel
> >
> >>  };
> >>
> >>  /**
> >> --
> >> 2.23.0
> >>
> >
>
> --
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Linux GmbH, Maxfeldstrasse 5, 90409 Nuernberg, Germany
> GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
> HRB 21284 (AG Nürnberg)
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch