[Intel-gfx] [PATCH] drm/i915: Remove insert-page shortcut from execbuf relocate_iomap()

[Tvrtko Ursulin]

On 28/10/16 15:27, Chris Wilson wrote:
> We are not allowed to touch the GTT entries underneath an atomic section,
> as they take a rpm wakelock (which is illegal from atomic context) and
> in the near future acquiring the DMA address for a page within an object
> may sleep for an allocation. This makes the current shortcircuit in
> relocation_iomap() for performing a second relocation on an adjacent page
> illegal, and we need to release the atomic iomapping, lookup the DMA,
> insert it into the GTT before reentering the atomic iomap section.
>
> As it happens, this is precisely what we do on if we are using an
> iomapping over the full object and not just a single page and by
> removing the shortcut, we do the right thing.
>
> Fixes: 9c870d03674f ("drm/i915: Use RPM as the barrier for controlling...")
> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c | 10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> index dfa6dbb2cf85..83533fb6cf46 100644
> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
> @@ -418,15 +418,6 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
>  	unsigned long offset;
>  	void *vaddr;
>
> -	if (cache->node.allocated) {
> -		wmb();
> -		ggtt->base.insert_page(&ggtt->base,
> -				       i915_gem_object_get_dma_address(obj, page),
> -				       cache->node.start, I915_CACHE_NONE, 0);
> -		cache->page = page;
> -		return unmask_page(cache->vaddr);
> -	}
> -
>  	if (cache->vaddr) {
>  		io_mapping_unmap_atomic((void __force __iomem *) unmask_page(cache->vaddr));
>  	} else {
> @@ -466,6 +457,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
>
>  	offset = cache->node.start;
>  	if (cache->node.allocated) {
> +		wmb();
>  		ggtt->base.insert_page(&ggtt->base,
>  				       i915_gem_object_get_dma_address(obj, page),
>  				       offset, I915_CACHE_NONE, 0);
>

Looks correct to me. We lose the optimisation of not having to remap, 
but since we are not allowed to do that there is little other choice.

And we still keep the some part of this recent optimisation so it looks 
fine to me from that angle as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko