Re: [Intel-gfx] [PATCH 3/7] drm/i915/ttm, drm/ttm: Initialize the ttm device and memory managers.

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2021-05-12 at 09:57 +0100, Matthew Auld wrote:
> On Tue, 11 May 2021 at 14:26, Thomas Hellström
> <thomas.hellstrom@linux.intel.com> wrote:
> > 
> > Temporarily remove the buddy allocator and related selftests
> > and hook up the TTM range manager for i915 regions.
> > 
> > In order to support some of the mock region-related selftests, we
> > need to
> > be able to initialize the TTM range-manager standalone without a
> > struct
> > ttm_device. Add two functions to allow that to the TTM api.
> > 
> > Finally modify the mock region selftests somewhat to account for a
> > fragmenting manager.
> > 
> > Cc: Christian König <christian.koenig@amd.com>
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/Kconfig                  |   1 +
> >  drivers/gpu/drm/i915/Makefile                 |   2 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_lmem.c      |  58 +-
> >  .../gpu/drm/i915/gem/i915_gem_object_types.h  |   6 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_pages.c     |   3 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_region.c    | 120 ---
> >  drivers/gpu/drm/i915/gem/i915_gem_region.h    |   4 -
> >  drivers/gpu/drm/i915/gem/i915_gem_shmem.c     |   4 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_stolen.c    |  10 +-
> >  drivers/gpu/drm/i915/gem/i915_gem_stolen.h    |   9 +-
> >  drivers/gpu/drm/i915/gt/intel_gt.c            |   2 -
> >  drivers/gpu/drm/i915/gt/intel_region_lmem.c   |  27 +-
> >  drivers/gpu/drm/i915/i915_buddy.c             | 435 ----------
> >  drivers/gpu/drm/i915/i915_buddy.h             | 131 ---
> >  drivers/gpu/drm/i915/i915_drv.c               |   8 +
> >  drivers/gpu/drm/i915/i915_drv.h               |   7 +-
> >  drivers/gpu/drm/i915/i915_gem.c               |   1 +
> >  drivers/gpu/drm/i915/i915_globals.c           |   1 -
> >  drivers/gpu/drm/i915/i915_globals.h           |   1 -
> >  drivers/gpu/drm/i915/i915_scatterlist.c       |  70 ++
> >  drivers/gpu/drm/i915/i915_scatterlist.h       |  35 +
> >  drivers/gpu/drm/i915/intel_memory_region.c    | 180 ++--
> >  drivers/gpu/drm/i915/intel_memory_region.h    |  44 +-
> >  drivers/gpu/drm/i915/intel_region_ttm.c       | 246 ++++++
> >  drivers/gpu/drm/i915/intel_region_ttm.h       |  29 +
> >  drivers/gpu/drm/i915/selftests/i915_buddy.c   | 789 --------------
> > ----
> >  .../drm/i915/selftests/i915_mock_selftests.h  |   1 -
> >  .../drm/i915/selftests/intel_memory_region.c  | 133 +--
> >  drivers/gpu/drm/i915/selftests/mock_region.c  |  51 +-
> >  drivers/gpu/drm/ttm/ttm_range_manager.c       |  55 +-
> >  include/drm/ttm/ttm_bo_driver.h               |  23 +
> >  31 files changed, 715 insertions(+), 1771 deletions(-)
> >  delete mode 100644 drivers/gpu/drm/i915/i915_buddy.c
> >  delete mode 100644 drivers/gpu/drm/i915/i915_buddy.h
> >  create mode 100644 drivers/gpu/drm/i915/intel_region_ttm.c
> >  create mode 100644 drivers/gpu/drm/i915/intel_region_ttm.h
> >  delete mode 100644 drivers/gpu/drm/i915/selftests/i915_buddy.c
> > 
> > diff --git a/drivers/gpu/drm/i915/Kconfig
> > b/drivers/gpu/drm/i915/Kconfig
> > index 1e1cb245fca7..b63d374dff23 100644
> > --- a/drivers/gpu/drm/i915/Kconfig
> > +++ b/drivers/gpu/drm/i915/Kconfig
> > @@ -26,6 +26,7 @@ config DRM_I915
> >         select SND_HDA_I915 if SND_HDA_CORE
> >         select CEC_CORE if CEC_NOTIFIER
> >         select VMAP_PFN
> > +       select DRM_TTM
> >         help
> >           Choose this option if you have a system that has "Intel
> > Graphics
> >           Media Accelerator" or "HD Graphics" integrated graphics,
> > diff --git a/drivers/gpu/drm/i915/Makefile
> > b/drivers/gpu/drm/i915/Makefile
> > index d0d936d9137b..cb8823570996 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -50,6 +50,7 @@ i915-y += i915_drv.o \
> >           intel_memory_region.o \
> >           intel_pch.o \
> >           intel_pm.o \
> > +         intel_region_ttm.o \
> >           intel_runtime_pm.o \
> >           intel_sideband.o \
> >           intel_step.o \
> > @@ -160,7 +161,6 @@ gem-y += \
> >  i915-y += \
> >           $(gem-y) \
> >           i915_active.o \
> > -         i915_buddy.o \
> >           i915_cmd_parser.o \
> >           i915_gem_evict.o \
> >           i915_gem_gtt.o \
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> > index f44bdd08f7cb..f42803ea48f2 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_lmem.c
> > @@ -4,16 +4,70 @@
> >   */
> > 
> >  #include "intel_memory_region.h"
> > +#include "intel_region_ttm.h"
> >  #include "gem/i915_gem_region.h"
> >  #include "gem/i915_gem_lmem.h"
> >  #include "i915_drv.h"
> > 
> > +static void lmem_put_pages(struct drm_i915_gem_object *obj,
> > +                                 struct sg_table *pages)
> > +{
> > +       intel_region_ttm_node_free(obj->mm.region, obj-
> > >mm.st_mm_node);
> > +       obj->mm.dirty = false;
> > +       sg_free_table(pages);
> > +       kfree(pages);
> > +}
> > +
> > +static int lmem_get_pages(struct drm_i915_gem_object *obj)
> > +{
> > +       unsigned int flags;
> > +       struct sg_table *pages;
> > +
> > +       flags = I915_ALLOC_MIN_PAGE_SIZE;
> > +       if (obj->flags & I915_BO_ALLOC_CONTIGUOUS)
> > +               flags |= I915_ALLOC_CONTIGUOUS;
> > +
> > +       obj->mm.st_mm_node = intel_region_ttm_node_alloc(obj-
> > >mm.region,
> > +                                                        obj-
> > >base.size,
> > +                                                        flags);
> > +       if (IS_ERR(obj->mm.st_mm_node))
> > +               return PTR_ERR(obj->mm.st_mm_node);
> > +
> > +       /* Range manager is always contigous */
> > +       if (obj->mm.region->is_range_manager)
> > +               obj->flags |= I915_BO_ALLOC_CONTIGUOUS;
> > +       pages = intel_region_ttm_node_to_st(obj->mm.region, obj-
> > >mm.st_mm_node);
> > +       if (IS_ERR(pages))
> > +               return PTR_ERR(pages);
> 
> error unwind?
> 
> > +
> > +       __i915_gem_object_set_pages(obj, pages,
> > +                                   i915_sg_dma_page_sizes(pages-
> > >sgl));
> > +
> > +       if (obj->flags & I915_BO_ALLOC_CPU_CLEAR) {
> > +               void __iomem *vaddr =
> > +                       i915_gem_object_lmem_io_map(obj, 0, obj-
> > >base.size);
> > +
> > +               if (!vaddr) {
> > +                       struct sg_table *pages =
> > +                               __i915_gem_object_unset_pages(obj);
> > +
> > +                       if (!IS_ERR_OR_NULL(pages))
> > +                               lmem_put_pages(obj, pages);
> > +               }
> > +
> > +               memset_io(vaddr, 0, obj->base.size);
> > +               io_mapping_unmap(vaddr);
> > +       }
> > +
> > +       return 0;
> > +}
> 
> <snip>
> 
> > +/**
> > + * i915_sg_dma_page_sizes - Calculate page sizes from a
> > scatterlist
> > + * @sg: The scatterlist from which to calculate page sizes
> > + *
> > + * Return: a value with bits sets for relevant page sizes.
> > + */
> > +static inline unsigned int i915_sg_dma_page_sizes(struct
> > scatterlist *sg)
> > +{
> > +       unsigned int page_sizes;
> > +
> > +       page_sizes = 0;
> > +       while (sg) {
> > +               GEM_BUG_ON(sg->offset);
> > +               GEM_BUG_ON(!IS_ALIGNED(sg->dma_length, PAGE_SIZE));
> > +               page_sizes |= sg->dma_length;
> > +               sg = __sg_next(sg);
> > +       }
> > +
> > +       /*
> > +        * Is this necessary to support building large GPU pages
> > from
> > +        * even larger segments?
> > +        */
> 
> We just need to know the above sg page_sizes, which is the mask of
> sg->dma_length for each sg. Later in __i915_gem_object_set_pages() we
> use this to determine the potential GPU page sizes for the object,
> depending on what the device supports. So the below stuff shouldn't
> be
> needed. Also we can just use i915_sg_page_sizes() here.

I noticed that i915_sg_page_sizes() uses sg->length() instead of sg-
>dma_length(). That doesn't look like what we're after here. Do you
know if there is a reason for that?

Thanks,
Thomas