Re: [PATCH 5/6] drm/amdgpu: use TTM_PL_FLAG_CONTIGUOUS

[Christian König <deathsimple@xxxxxxxxxxx>]

Am 03.04.2017 um 18:22 schrieb Nicolai Hähnle:
> On 31.03.2017 11:47, Christian König wrote:
> > From: Christian König <christian.koenig@xxxxxxx>
> >
> > Implement AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS using TTM_PL_FLAG_CONTIGUOUS
> > instead of a placement limit. That allows us to better handle CPU
> > accessible placements.
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > Acked-by: Michel Dänzer <michel.daenzer@xxxxxxx>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c   | 11 +++++------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c | 14 ++++++++++----
> >  2 files changed, 15 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index d6b2de9..387d190 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -122,20 +122,19 @@ static void amdgpu_ttm_placement_init(struct 
> > amdgpu_device *adev,
> >
> >      if (domain & AMDGPU_GEM_DOMAIN_VRAM) {
> >          unsigned visible_pfn = adev->mc.visible_vram_size >> 
> > PAGE_SHIFT;
> > -        unsigned lpfn = 0;
> > -
> > -        /* This forces a reallocation if the flag wasn't set before */
> > -        if (flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)
> > -            lpfn = adev->mc.real_vram_size >> PAGE_SHIFT;
> >
> >          places[c].fpfn = 0;
> > -        places[c].lpfn = lpfn;
> > +        places[c].lpfn = 0;
> >          places[c].flags = TTM_PL_FLAG_WC | TTM_PL_FLAG_UNCACHED |
> >              TTM_PL_FLAG_VRAM;
> > +
> >          if (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)
> >              places[c].lpfn = visible_pfn;
> >          else
> >              places[c].flags |= TTM_PL_FLAG_TOPDOWN;
> > +
> > +        if (flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)
> > +            places[c].flags |= TTM_PL_FLAG_CONTIGUOUS;
> >          c++;
> >      }
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > index d710226..af2d172 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vram_mgr.c
> > @@ -93,7 +93,6 @@ static int amdgpu_vram_mgr_new(struct 
> > ttm_mem_type_manager *man,
> >                     const struct ttm_place *place,
> >                     struct ttm_mem_reg *mem)
> >  {
> > -    struct amdgpu_bo *bo = container_of(tbo, struct amdgpu_bo, tbo);
> >      struct amdgpu_vram_mgr *mgr = man->priv;
> >      struct drm_mm *mm = &mgr->mm;
> >      struct drm_mm_node *nodes;
> > @@ -107,8 +106,8 @@ static int amdgpu_vram_mgr_new(struct 
> > ttm_mem_type_manager *man,
> >      if (!lpfn)
> >          lpfn = man->size;
> >
> > -    if (bo->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS ||
> > -        place->lpfn || amdgpu_vram_page_split == -1) {
> > +    if (place->flags & TTM_PL_FLAG_CONTIGUOUS ||
> > +        amdgpu_vram_page_split == -1) {
> >          pages_per_node = ~0ul;
> >          num_nodes = 1;
> >      } else {
> > @@ -126,12 +125,14 @@ static int amdgpu_vram_mgr_new(struct 
> > ttm_mem_type_manager *man,
> >          aflags = DRM_MM_CREATE_TOP;
> >      }
> >
> > +    mem->start = 0;
> >      pages_left = mem->num_pages;
> >
> >      spin_lock(&mgr->lock);
> >      for (i = 0; i < num_nodes; ++i) {
> >          unsigned long pages = min(pages_left, pages_per_node);
> >          uint32_t alignment = mem->page_alignment;
> > +        unsigned long start;
> >
> >          if (pages == pages_per_node)
> >              alignment = pages_per_node;
> > @@ -145,11 +146,16 @@ static int amdgpu_vram_mgr_new(struct 
> > ttm_mem_type_manager *man,
> >          if (unlikely(r))
> >              goto error;
> >
> > +        /*
> > +         * Calculate a virtual BO start address to easily check if
> > +         * everything is CPU accessible.
> > +         */
> > +        start = nodes[i].start + nodes[i].size - mem->num_pages;
>
> This might wrap around (be a signed negative number), completely 
> breaking the max() logic below.

Good point, going to fix that.

> > +        mem->start = max(mem->start, start);
> >          pages_left -= pages;
> >      }
> >      spin_unlock(&mgr->lock);
> >
> > -    mem->start = num_nodes == 1 ? nodes[0].start : 
> > AMDGPU_BO_INVALID_OFFSET;
>
> If we're going to abuse mem->start anyway, might I suggest just 
> keeping track of max(nodes[i].start + nodes[i].size), and then setting 
> mem->start to a magic (macro'd) constant based on whether everything 
> is in visible VRAM or not?

No, that would break in kernel mappings.

> Then the check in amdgpu_ttm_io_mem_reserve could be simplified 
> accordingly.
>
> Also, I think patches #6 and #5 should be exchanged, otherwise there's 
> a temporary bug in handling split visible VRAM buffers.

Hui? Why? Patch #6 enables the whole thing by not making the contiguous 
flag mandatory for CPU mappings any more.

Switching those would cause problems with detecting when a BO is not in 
visible VRAM.

Regards,
Christian.

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel