[Intel-gfx] [PATCH] drm/i915: Fix DMA mapped scatterlist walks

[Logan Gunthorpe]

On 2020-09-09 6:44 a.m., Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> When walking DMA mapped scatterlists sg_dma_len has to be used since it
> can be different (coalesced) from the backing store entry.
> 
> This also means we have to end the walk when encountering a zero length
> DMA entry and cannot rely on the normal sg list end marker.
> 
> Both issues were there in theory for some time but were hidden by the fact
> Intel IOMMU driver was never coalescing entries. As there are ongoing
> efforts to change this we need to start handling it.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> References: 85d1225ec066 ("drm/i915: Introduce & use new lightweight SGL iterators")
> References: b31144c0daa8 ("drm/i915: Micro-optimise gen6_ppgtt_insert_entries()")
> Reported-by: Tom Murphy <murphyt7 at tcd.ie>
> Suggested-by: Tom Murphy <murphyt7 at tcd.ie> # __sgt_iter
> Suggested-by: Logan Gunthorpe <logang at deltatee.com> # __sgt_iter
> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.auld at intel.com>
> Cc: Lu Baolu <baolu.lu at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/gt/gen6_ppgtt.c    |  6 +++---
>  drivers/gpu/drm/i915/gt/gen8_ppgtt.c    | 17 ++++++++++-------
>  drivers/gpu/drm/i915/gt/intel_gtt.h     |  2 +-
>  drivers/gpu/drm/i915/i915_scatterlist.h | 12 ++++++++----
>  4 files changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> index fd0d24d28763..c0d17f87b00f 100644
> --- a/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen6_ppgtt.c
> @@ -131,17 +131,17 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
>  
>  	vaddr = kmap_atomic_px(i915_pt_entry(pd, act_pt));
>  	do {
> -		GEM_BUG_ON(iter.sg->length < I915_GTT_PAGE_SIZE);
> +		GEM_BUG_ON(sg_dma_len(iter.sg) < I915_GTT_PAGE_SIZE);
>  		vaddr[act_pte] = pte_encode | GEN6_PTE_ADDR_ENCODE(iter.dma);
>  
>  		iter.dma += I915_GTT_PAGE_SIZE;
>  		if (iter.dma == iter.max) {
>  			iter.sg = __sg_next(iter.sg);
> -			if (!iter.sg)
> +			if (!iter.sg || sg_dma_len(iter.sg) == 0)
>  				break;
>  
>  			iter.dma = sg_dma_address(iter.sg);
> -			iter.max = iter.dma + iter.sg->length;
> +			iter.max = iter.dma + sg_dma_len(iter.sg);
>  		}
>  
>  		if (++act_pte == GEN6_PTES) {
> diff --git a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> index eb64f474a78c..0361b3dfdc72 100644
> --- a/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> +++ b/drivers/gpu/drm/i915/gt/gen8_ppgtt.c
> @@ -372,19 +372,19 @@ gen8_ppgtt_insert_pte(struct i915_ppgtt *ppgtt,
>  	pd = i915_pd_entry(pdp, gen8_pd_index(idx, 2));
>  	vaddr = kmap_atomic_px(i915_pt_entry(pd, gen8_pd_index(idx, 1)));
>  	do {
> -		GEM_BUG_ON(iter->sg->length < I915_GTT_PAGE_SIZE);
> +		GEM_BUG_ON(sg_dma_len(iter->sg) < I915_GTT_PAGE_SIZE);
>  		vaddr[gen8_pd_index(idx, 0)] = pte_encode | iter->dma;
>  
>  		iter->dma += I915_GTT_PAGE_SIZE;
>  		if (iter->dma >= iter->max) {
>  			iter->sg = __sg_next(iter->sg);
> -			if (!iter->sg) {
> +			if (!iter->sg || sg_dma_len(iter->sg) == 0) {
>  				idx = 0;
>  				break;
>  			}
>  
>  			iter->dma = sg_dma_address(iter->sg);
> -			iter->max = iter->dma + iter->sg->length;
> +			iter->max = iter->dma + sg_dma_len(iter->sg);
>  		}
>  
>  		if (gen8_pd_index(++idx, 0) == 0) {
> @@ -414,7 +414,7 @@ static void gen8_ppgtt_insert_huge(struct i915_vma *vma,
>  {
>  	const gen8_pte_t pte_encode = gen8_pte_encode(0, cache_level, flags);
>  	u64 start = vma->node.start;
> -	dma_addr_t rem = iter->sg->length;
> +	dma_addr_t rem = sg_dma_len(iter->sg);

Seems a little odd to me to be storing a length in a dma_addr_t. But
besides that small nit, this all makes sense to me.

Reviewed-by: Logan Gunthorpe <logang at deltatee.com>

Thanks,

Logan