On 2020-09-09 6:44 a.m., Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> 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@xxxxxx>
> Suggested-by: Tom Murphy <murphyt7@xxxxxx> # __sgt_iter
> Suggested-by: Logan Gunthorpe <logang@xxxxxxxxxxxx> # __sgt_iter
> Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> Cc: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxx>
Thanks,
Logan
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
