Re: [PATCH] drm/amdgpu: Place new CPU-accessbile BOs in GTT if visible VRAM is full

[John Brooks <john-xq/Ko7C6e2Bl57MIdRCFDg@public.gmane.org>]

On Fri, May 19, 2017 at 04:03:28PM +0900, Michel Dänzer wrote:
> On 19/05/17 12:04 PM, John Brooks wrote:
> > Set GTT as the busy placement for newly created BOs that have the
> > AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED flag, so that they don't cause
> > established BOs to be evicted from visible VRAM.
> 
> This is an interesting idea, but there are some issues with this patch.
> 
> 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 365883d..655718a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -392,6 +392,17 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
> >  #endif
> >  
> >  	amdgpu_fill_placement_to_bo(bo, placement);
> > +
> > +	/* This is a new BO that wants to be CPU-visible; set GTT as the busy
> > +	 * placement so that it does not evict established BOs from visible VRAM.
> > +	 */
> > +	if (domain & (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
> 
> Should be something like
> 
> 	if (domain == (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT) &&
> 
> otherwise it would also match e.g. BOs with domain ==
> AMDGPU_GEM_DOMAIN_GTT and the AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED
> flag set (not that this makes sense, but there's nothing to prevent it).
> 
> 
> > +	    flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> > +		bo->placement.num_placement = 1;
> > +		bo->placement.num_busy_placement = 1;
> > +		bo->placement.busy_placement = &bo->placement.placement[1];
> > +	}
> 
> The original placements set by amdgpu_fill_placement_to_bo need to be
> restored before returning from this function, otherwise I suspect such
> BOs which end up being created in GTT will stay there permanently.
> 

I'm curious, what makes you think that the BOs cannot move back to VRAM
otherwise? VRAM is still in the placements and prefered/allowed domains, just
not in the busy placements.

> Does the patch still help for Dying Light if you fix this?
> 
> The patch as is doesn't seem to make any difference for my dirt-rally
> test case.
> 
> 
> > @@ -484,6 +495,13 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> >  	memset(&placements, 0,
> >  	       (AMDGPU_GEM_DOMAIN_MAX + 1) * sizeof(struct ttm_place));
> >  
> > +
> > +	/* New CPU-visible BOs will have GTT set as their busy placement */
> > +	if (domain & AMDGPU_GEM_DOMAIN_VRAM &&
> > +	    flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) {
> 
> Make this
> 
> 	if ((domain & AMDGPU_GEM_DOMAIN_VRAM) &&
> 	    (flags & AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED)) {
> 
> to match the coding style.
> 
> 
> -- 
> Earthling Michel Dänzer               |               http://www.amd.com
> Libre software enthusiast             |             Mesa and X developer

Thanks very much for your feedback. I'll send an updated patch soon.

--
John Brooks
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx