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

[Tvrtko Ursulin]

On 09/09/2020 20:29, Logan Gunthorpe wrote:
> 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>

I did not spot that, thanks. I'll improve that in v2 since I need to add 
a 2nd patch to completely prepare i915 for this.

Regards,

Tvrtko