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
