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
