Re: [PATCH v5] drm/i915: stop using swiotlb

[Tvrtko Ursulin]

On 08/08/2022 16:48, Hellstrom, Thomas wrote:
> Hi, [back from vacation]
>
> On Tue, 2022-07-26 at 16:39 +0100, Robert Beckett wrote:
> > Calling swiotlb functions directly is nowadays considered harmful.
> > See
> > https://lore.kernel.org/intel-gfx/20220711082614.GA29487@xxxxxx/
> >
> > Replace swiotlb_max_segment() calls with dma_max_mapping_size().
> > In i915_gem_object_get_pages_internal() no longer consider
> > max_segment
> > only if CONFIG_SWIOTLB is enabled. There can be other (iommu related)
> > causes of specific max segment sizes.
> >
> > Cc: Christoph Hellwig <hch@xxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Cc: Thomas Hellstrom <thomas.hellstrom@xxxxxxxxx>
> > Cc: Matthew Auld <matthew.auld@xxxxxxxxx>
> >
> > v2: - restore UINT_MAX clamp in i915_sg_segment_size()
> >      - drop PAGE_SIZE check as it will always be >= PAGE_SIZE
> > v3: - actually clamp to UINT_MAX in i915_sg_segment_size()
> > v4: - round down max segment size to PAGE_SIZE
> > v5: - fix checkpatch whitespace issue
> >
> > Reviewed-by: Christoph Hellwig <hch@xxxxxx>
> > Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Signed-off-by: Robert Beckett <bob.beckett@xxxxxxxxxxxxx>
>
> Hmm,
>
> This whole thing looks a bit strange to me since with SWIOTLB actually
> used for i915, the driver should malfunction anyway as it doesn't do
> any dma_sync_sg_for_cpu() or dma_sync_sg_for_device(), and the driver
> assumes all coherent dma. Is that SWIOTLB=force kernel option still
> available?

Don't know about these - but pretty sure in the past we had i915 break 
if we did not respect swiotlb_max_segment.

Digging through git history at least running as Xen dom0 looks to have 
been impacted, but commits such as abb0deacb5a6 ("drm/i915: Fallback to 
single PAGE_SIZE segments for DMA remapping") are older and suggest 
problem was generic. 1625e7e549c5 ("drm/i915: make compact dma scatter 
lists creation work with SWIOTLB backend.") as well. So it looks it did 
work behind swiotlb despite those missing calls you highlighted.

> Also, correct me if I'm wrong, but the original driver segment size
> appears to mean "the largest contiguous area that can be handled either
> by the device or the dma mapping layer" rather than the total space
> available for dma mappings? Not completely sure what
> dma_max_mapping_size() is returning, though?

AFAIU looks to be compatible on paper at least.:

dma_max_mapping_size -> "Returns the maximum size of a mapping for the 
device."

So an individual mapping.

But then in case of swiotlb is implemented in swiotlb_max_mapping_size, 
and not the same code as swiotlb_max_segment. I agree, ideally if 
someone could clarify they are returning the same thing or there is a 
miss somewhere.

Regards,

Tvrtko