Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Fri, 28 Aug 2020 at 00:34, Tom Murphy wrote:
>
> On Thu, 27 Aug 2020 at 22:36, Logan Gunthorpe wrote:
> >
> >
> >
> > On 2020-08-23 6:04 p.m., Tom Murphy wrote:
> > > I have added a check for the sg_dma_len == 0 :
> > > """
> > > } __sgt_iter(struct scatterlist *sgl, bool dma) {
> > > struct sgt_iter s = { .sgp = sgl };
> > >
> > > + if (sgl && sg_dma_len(sgl) == 0)
> > > + s.sgp = NULL;
> > >
> > > if (s.sgp) {
> > > .
> > > """
> > > at location [1].
> > > but it doens't fix the problem.
> >
> > Based on my read of the code, it looks like we also need to change usage
> > of sgl->length... Something like the rough patch below, maybe?
> >
> > Also, Tom, do you have an updated version of the patchset to convert the
> > Intel IOMMU to dma-iommu available? The last one I've found doesn't
> > apply cleanly (I'm assuming parts of it have been merged in slightly
> > modified forms).
> >
>
> I'll try and post one in the next 24hours
I have just posted this now:
The subject of the cover letter is:
"[PATCH V2 0/5] Convert the intel iommu driver to the dma-iommu api"
>
> > Thanks,
> >
> > Logan
> >
> > --
> >
> > diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
> > b/drivers/gpu/drm/i915/i915
> > index b7b59328cb76..9367ac801f0c 100644
> > --- a/drivers/gpu/drm/i915/i915_scatterlist.h
> > +++ b/drivers/gpu/drm/i915/i915_scatterlist.h
> > @@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
> > } __sgt_iter(struct scatterlist *sgl, bool dma) {
> > struct sgt_iter s = { .sgp = sgl };
> >
> > + if (sgl && !sg_dma_len(s.sgp))
> > + s.sgp = NULL;
> > +
> > if (s.sgp) {
> > s.max = s.curr = s.sgp->offset;
> > - s.max += s.sgp->length;
> > - if (dma)
> > +
> > + if (dma) {
> > + s.max += sg_dma_len(s.sgp);
> > s.dma = sg_dma_address(s.sgp);
> > - else
> > + } else {
> > + s.max += s.sgp->length;
> > s.pfn = page_to_pfn(sg_page(s.sgp));
> > + }
> > }
> >
> > return s;
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Thu, 27 Aug 2020 at 22:36, Logan Gunthorpe wrote:
>
>
>
> On 2020-08-23 6:04 p.m., Tom Murphy wrote:
> > I have added a check for the sg_dma_len == 0 :
> > """
> > } __sgt_iter(struct scatterlist *sgl, bool dma) {
> > struct sgt_iter s = { .sgp = sgl };
> >
> > + if (sgl && sg_dma_len(sgl) == 0)
> > + s.sgp = NULL;
> >
> > if (s.sgp) {
> > .
> > """
> > at location [1].
> > but it doens't fix the problem.
>
> Based on my read of the code, it looks like we also need to change usage
> of sgl->length... Something like the rough patch below, maybe?
>
> Also, Tom, do you have an updated version of the patchset to convert the
> Intel IOMMU to dma-iommu available? The last one I've found doesn't
> apply cleanly (I'm assuming parts of it have been merged in slightly
> modified forms).
>
I'll try and post one in the next 24hours
> Thanks,
>
> Logan
>
> --
>
> diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
> b/drivers/gpu/drm/i915/i915
> index b7b59328cb76..9367ac801f0c 100644
> --- a/drivers/gpu/drm/i915/i915_scatterlist.h
> +++ b/drivers/gpu/drm/i915/i915_scatterlist.h
> @@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
> } __sgt_iter(struct scatterlist *sgl, bool dma) {
> struct sgt_iter s = { .sgp = sgl };
>
> + if (sgl && !sg_dma_len(s.sgp))
> + s.sgp = NULL;
> +
> if (s.sgp) {
> s.max = s.curr = s.sgp->offset;
> - s.max += s.sgp->length;
> - if (dma)
> +
> + if (dma) {
> + s.max += sg_dma_len(s.sgp);
> s.dma = sg_dma_address(s.sgp);
> - else
> + } else {
> + s.max += s.sgp->length;
> s.pfn = page_to_pfn(sg_page(s.sgp));
> + }
> }
>
> return s;
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 2020-08-23 6:04 p.m., Tom Murphy wrote:
> I have added a check for the sg_dma_len == 0 :
> """
> } __sgt_iter(struct scatterlist *sgl, bool dma) {
> struct sgt_iter s = { .sgp = sgl };
>
> + if (sgl && sg_dma_len(sgl) == 0)
> + s.sgp = NULL;
>
> if (s.sgp) {
> .
> """
> at location [1].
> but it doens't fix the problem.
Based on my read of the code, it looks like we also need to change usage
of sgl->length... Something like the rough patch below, maybe?
Also, Tom, do you have an updated version of the patchset to convert the
Intel IOMMU to dma-iommu available? The last one I've found doesn't
apply cleanly (I'm assuming parts of it have been merged in slightly
modified forms).
Thanks,
Logan
--
diff --git a/drivers/gpu/drm/i915/i915_scatterlist.h
b/drivers/gpu/drm/i915/i915
index b7b59328cb76..9367ac801f0c 100644
--- a/drivers/gpu/drm/i915/i915_scatterlist.h
+++ b/drivers/gpu/drm/i915/i915_scatterlist.h
@@ -27,13 +27,19 @@ static __always_inline struct sgt_iter {
} __sgt_iter(struct scatterlist *sgl, bool dma) {
struct sgt_iter s = { .sgp = sgl };
+ if (sgl && !sg_dma_len(s.sgp))
+ s.sgp = NULL;
+
if (s.sgp) {
s.max = s.curr = s.sgp->offset;
- s.max += s.sgp->length;
- if (dma)
+
+ if (dma) {
+ s.max += sg_dma_len(s.sgp);
s.dma = sg_dma_address(s.sgp);
- else
+ } else {
+ s.max += s.sgp->length;
s.pfn = page_to_pfn(sg_page(s.sgp));
+ }
}
return s;
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Mon, Aug 24, 2020 at 2:56 AM Tom Murphy wrote:
>
> Hi Logan/All,
>
> I have added a check for the sg_dma_len == 0 :
> """
> } __sgt_iter(struct scatterlist *sgl, bool dma) {
> struct sgt_iter s = { .sgp = sgl };
>
> + if (sgl && sg_dma_len(sgl) == 0)
> + s.sgp = NULL;
>
> if (s.sgp) {
> .
> """
> at location [1].
> but it doens't fix the problem.
>
> You're right though, this change does need to be made, this code
> doesn't handle pages of sg_dma_len(sg) == 0 correctly
> So my guess is that we have more bugs in other parts of the i915
> driver (or there is a problem with my "sg_dma_len == 0" fix above).
> I have been trying to spot where else the code might be buggy but I
> haven't had any luck so far.
>
> I'm doing a microconfernce (at LPC 2020) this wednesdays [1] on this
> if you're interested in attending.
> I'm hoping I can chat about it with a few people and find how can
> reproduce and fix this issues. I don't have any more time I can give
> to this unfortunately and it would be a shame for the work to go to
> waste.
>
> [0]
> https://github.com/torvalds/linux/blob/d012a7190fc1fd72ed48911e77ca97ba4521bccd/drivers/gpu/drm/i915/i915_scatterlist.h#L28
> [1] https://linuxplumbersconf.org/event/7/contributions/846/
>
> On Fri, 29 May 2020 at 22:21, Logan Gunthorpe wrote:
> >
> >
> >
> > On 2020-05-29 3:11 p.m., Marek Szyprowski wrote:
> > > Patches are pending:
> > > https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprow...@samsung.com/T/
> >
> > Cool, nice! Though, I still don't think that fixes the issue in
> > i915_scatterlist.h given it still ignores sg_dma_len() and strictly
> > relies on sg_next()/sg_is_last() to stop iterating -- and I suspect this
> > is the bug that got in Tom's way.
> >
> > >> However, as Robin pointed out, there are other ugly tricks like stopping
> > >> iterating through the SGL when sg_dma_len() is zero. For example, the
> > >> AMD driver appears to use drm_prime_sg_to_page_addr_arrays() which does
> > >> this trick and thus likely isn't buggy (otherwise, I'd expect someone to
> > >> have complained by now seeing AMD has already switched to IOMMU-DMA.
We ran into the same issue with amdgpu and radeon when the AMD IOMMU
driver was converted and had to fix it as well. The relevant fixes
were:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=42e67b479eab6d26459b80b4867298232b0435e7
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0199172f933342d8b1011aae2054a695c25726f4
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=47f7826c520ecd92ffbffe59ecaa2fe61e42ec70
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c0f83d164fb8f3a2b7bc379a6c1e27d1123a9eab
Alex
> > >
> > > I'm not sure that this is a trick. Stopping at zero sg_dma_len() was
> > > somewhere documented.
> >
> > Well whatever you want to call it, it is ugly to have some drivers doing
> > one thing with the returned value and others assuming there's an extra
> > zero at the end. It just causes confusion for people reading/copying the
> > code. It would be better if they are all consistent. However, I concede
> > stopping at zero should not be broken, presently.
> >
> > Logan
> ___
> dri-devel mailing list
> dri-de...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
Hi Tom, On 2019-12-21 15:03, Tom Murphy wrote: This patchset converts the intel iommu driver to the dma-iommu api. While converting the driver I exposed a bug in the intel i915 driver which causes a huge amount of artifacts on the screen of my laptop. You can see a picture of it here: https://github.com/pippy360/kernelPatches/blob/master/IMG_20191219_225922.jpg This issue is most likely in the i915 driver and is most likely caused by the driver not respecting the return value of the dma_map_ops::map_sg function. You can see the driver ignoring the return value here: https://github.com/torvalds/linux/blob/7e0165b2f1a912a06e381e91f0f4e495f4ac3736/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c#L51 Previously this didn’t cause issues because the intel map_sg always returned the same number of elements as the input scatter gather list but with the change to this dma-iommu api this is no longer the case. I wasn’t able to track the bug down to a specific line of code unfortunately. Could someone from the intel team look at this? I have been testing on a lenovo x1 carbon 5th generation. Let me know if there’s any more information you need. To allow my patch set to be tested I have added a patch (patch 8/8) in this series to disable combining sg segments in the dma-iommu api which fixes the bug but it doesn't fix the actual problem. As part of this patch series I copied the intel bounce buffer code to the dma-iommu path. The addition of the bounce buffer code took me by surprise. I did most of my development on this patch series before the bounce buffer code was added and my reimplementation in the dma-iommu path is very rushed and not properly tested but I’m running out of time to work on this patch set. On top of that I also didn’t port over the intel tracing code from this commit: https://github.com/torvalds/linux/commit/3b53034c268d550d9e8522e613a14ab53b8840d8#diff-6b3e7c4993f05e76331e463ab1fc87e1 So all the work in that commit is now wasted. The code will need to be removed and reimplemented in the dma-iommu path. I would like to take the time to do this but I really don’t have the time at the moment and I want to get these changes out before the iommu code changes any more. Further to what we just discussed at LPC, I've realised that tracepoints are actually something I could do with *right now* for debugging my Arm DMA ops series, so if I'm going to hack something up anyway I may as well take responsibility for polishing it into a proper patch as well :) Robin. Tom Murphy (8): iommu/vt-d: clean up 32bit si_domain assignment iommu/vt-d: Use default dma_direct_* mapping functions for direct mapped devices iommu/vt-d: Remove IOVA handling code from non-dma_ops path iommu: Handle freelists when using deferred flushing in iommu drivers iommu: Add iommu_dma_free_cpu_cached_iovas function iommu: allow the dma-iommu api to use bounce buffers iommu/vt-d: Convert intel iommu driver to the iommu ops DO NOT MERGE: iommu: disable list appending in dma-iommu drivers/iommu/Kconfig | 1 + drivers/iommu/amd_iommu.c | 14 +- drivers/iommu/arm-smmu-v3.c | 3 +- drivers/iommu/arm-smmu.c| 3 +- drivers/iommu/dma-iommu.c | 183 +-- drivers/iommu/exynos-iommu.c| 3 +- drivers/iommu/intel-iommu.c | 936 drivers/iommu/iommu.c | 39 +- drivers/iommu/ipmmu-vmsa.c | 3 +- drivers/iommu/msm_iommu.c | 3 +- drivers/iommu/mtk_iommu.c | 3 +- drivers/iommu/mtk_iommu_v1.c| 3 +- drivers/iommu/omap-iommu.c | 3 +- drivers/iommu/qcom_iommu.c | 3 +- drivers/iommu/rockchip-iommu.c | 3 +- drivers/iommu/s390-iommu.c | 3 +- drivers/iommu/tegra-gart.c | 3 +- drivers/iommu/tegra-smmu.c | 3 +- drivers/iommu/virtio-iommu.c| 3 +- drivers/vfio/vfio_iommu_type1.c | 2 +- include/linux/dma-iommu.h | 3 + include/linux/intel-iommu.h | 1 - include/linux/iommu.h | 32 +- 23 files changed, 345 insertions(+), 908 deletions(-)
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
Hi Logan/All,
I have added a check for the sg_dma_len == 0 :
"""
} __sgt_iter(struct scatterlist *sgl, bool dma) {
struct sgt_iter s = { .sgp = sgl };
+ if (sgl && sg_dma_len(sgl) == 0)
+ s.sgp = NULL;
if (s.sgp) {
.
"""
at location [1].
but it doens't fix the problem.
You're right though, this change does need to be made, this code
doesn't handle pages of sg_dma_len(sg) == 0 correctly
So my guess is that we have more bugs in other parts of the i915
driver (or there is a problem with my "sg_dma_len == 0" fix above).
I have been trying to spot where else the code might be buggy but I
haven't had any luck so far.
I'm doing a microconfernce (at LPC 2020) this wednesdays [1] on this
if you're interested in attending.
I'm hoping I can chat about it with a few people and find how can
reproduce and fix this issues. I don't have any more time I can give
to this unfortunately and it would be a shame for the work to go to
waste.
[0]
https://github.com/torvalds/linux/blob/d012a7190fc1fd72ed48911e77ca97ba4521bccd/drivers/gpu/drm/i915/i915_scatterlist.h#L28
[1] https://linuxplumbersconf.org/event/7/contributions/846/
On Fri, 29 May 2020 at 22:21, Logan Gunthorpe wrote:
>
>
>
> On 2020-05-29 3:11 p.m., Marek Szyprowski wrote:
> > Patches are pending:
> > https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprow...@samsung.com/T/
>
> Cool, nice! Though, I still don't think that fixes the issue in
> i915_scatterlist.h given it still ignores sg_dma_len() and strictly
> relies on sg_next()/sg_is_last() to stop iterating -- and I suspect this
> is the bug that got in Tom's way.
>
> >> However, as Robin pointed out, there are other ugly tricks like stopping
> >> iterating through the SGL when sg_dma_len() is zero. For example, the
> >> AMD driver appears to use drm_prime_sg_to_page_addr_arrays() which does
> >> this trick and thus likely isn't buggy (otherwise, I'd expect someone to
> >> have complained by now seeing AMD has already switched to IOMMU-DMA.
> >
> > I'm not sure that this is a trick. Stopping at zero sg_dma_len() was
> > somewhere documented.
>
> Well whatever you want to call it, it is ugly to have some drivers doing
> one thing with the returned value and others assuming there's an extra
> zero at the end. It just causes confusion for people reading/copying the
> code. It would be better if they are all consistent. However, I concede
> stopping at zero should not be broken, presently.
>
> Logan
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 2020-05-29 3:11 p.m., Marek Szyprowski wrote: > Patches are pending: > https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprow...@samsung.com/T/ Cool, nice! Though, I still don't think that fixes the issue in i915_scatterlist.h given it still ignores sg_dma_len() and strictly relies on sg_next()/sg_is_last() to stop iterating -- and I suspect this is the bug that got in Tom's way. >> However, as Robin pointed out, there are other ugly tricks like stopping >> iterating through the SGL when sg_dma_len() is zero. For example, the >> AMD driver appears to use drm_prime_sg_to_page_addr_arrays() which does >> this trick and thus likely isn't buggy (otherwise, I'd expect someone to >> have complained by now seeing AMD has already switched to IOMMU-DMA. > > I'm not sure that this is a trick. Stopping at zero sg_dma_len() was > somewhere documented. Well whatever you want to call it, it is ugly to have some drivers doing one thing with the returned value and others assuming there's an extra zero at the end. It just causes confusion for people reading/copying the code. It would be better if they are all consistent. However, I concede stopping at zero should not be broken, presently. Logan
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
Hi Logan, On 29.05.2020 21:05, Logan Gunthorpe wrote: > On 2020-05-29 6:45 a.m., Christoph Hellwig wrote: >> On Thu, May 28, 2020 at 06:00:44PM -0600, Logan Gunthorpe wrote: This issue is most likely in the i915 driver and is most likely caused by the driver not respecting the return value of the dma_map_ops::map_sg function. You can see the driver ignoring the return value here: https://protect2.fireeye.com/url?k=ca25a34b-97f7b813-ca242804-0cc47a31c8b4-0ecdffc9f56851e1&q=1&u=https%3A%2F%2Fgithub.com%2Ftorvalds%2Flinux%2Fblob%2F7e0165b2f1a912a06e381e91f0f4e495f4ac3736%2Fdrivers%2Fgpu%2Fdrm%2Fi915%2Fgem%2Fi915_gem_dmabuf.c%23L51 Previously this didn’t cause issues because the intel map_sg always returned the same number of elements as the input scatter gather list but with the change to this dma-iommu api this is no longer the case. I wasn’t able to track the bug down to a specific line of code unfortunately. >> Mark did a big audit into the map_sg API abuse and initially had >> some i915 patches, but then gave up on them with this comment: >> >> "The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix >> it fully. The driver creatively uses sg_table->orig_nents to store the >> size of the allocate scatterlist and ignores the number of the entries >> returned by dma_map_sg function. In this patchset I only fixed the >> sg_table objects exported by dmabuf related functions. I hope that I >> didn't break anything there." >> >> it would be really nice if the i915 maintainers could help with sorting >> that API abuse out. >> > I agree completely that the API abuse should be sorted out, but I think > that's much larger than just the i915 driver. Pretty much every dma-buf > map_dma_buf implementation I looked at ignores the returned nents of > sg_attrs. This sucks, but I don't think it's the bug Tom ran into. See: > > amdgpu_dma_buf_map > armada_gem_prime_map_dma_buf > drm_gem_map_dma_buf > i915_gem_map_dma_buf > tegra_gem_prime_map_dma_buf > > So this should probably be addressed by the whole GPU community. Patches are pending: https://lore.kernel.org/linux-iommu/20200513132114.6046-1-m.szyprow...@samsung.com/T/ > However, as Robin pointed out, there are other ugly tricks like stopping > iterating through the SGL when sg_dma_len() is zero. For example, the > AMD driver appears to use drm_prime_sg_to_page_addr_arrays() which does > this trick and thus likely isn't buggy (otherwise, I'd expect someone to > have complained by now seeing AMD has already switched to IOMMU-DMA. I'm not sure that this is a trick. Stopping at zero sg_dma_len() was somewhere documented. > As I tried to point out in my previous email, i915 does not do this > trick. In fact, it completely ignores sg_dma_len() and is thus > completely broken. See i915_scatterlist.h and the __sgt_iter() function. > So it doesn't sound to me like Mark's fix would address the issue at > all. Per my previous email, I'd guess that it can be fixed simply by > adjusting the __sgt_iter() function to do something more sensible. > (Better yet, if possible, ditch __sgt_iter() and use the common DRM > function that AMD uses). > > This would at least allow us to make progress with Tom's IOMMU-DMA patch > set and once that gets in, it will be harder for other drivers to make > the same mistake. Best regards -- Marek Szyprowski, PhD Samsung R&D Institute Poland
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 2020-05-29 6:45 a.m., Christoph Hellwig wrote: > On Thu, May 28, 2020 at 06:00:44PM -0600, Logan Gunthorpe wrote: >>> This issue is most likely in the i915 driver and is most likely caused by >>> the driver not respecting the return value of the dma_map_ops::map_sg >>> function. You can see the driver ignoring the return value here: >>> https://github.com/torvalds/linux/blob/7e0165b2f1a912a06e381e91f0f4e495f4ac3736/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c#L51 >>> >>> Previously this didn’t cause issues because the intel map_sg always >>> returned the same number of elements as the input scatter gather list but >>> with the change to this dma-iommu api this is no longer the case. I wasn’t >>> able to track the bug down to a specific line of code unfortunately. > > Mark did a big audit into the map_sg API abuse and initially had > some i915 patches, but then gave up on them with this comment: > > "The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix > it fully. The driver creatively uses sg_table->orig_nents to store the > size of the allocate scatterlist and ignores the number of the entries > returned by dma_map_sg function. In this patchset I only fixed the > sg_table objects exported by dmabuf related functions. I hope that I > didn't break anything there." > > it would be really nice if the i915 maintainers could help with sorting > that API abuse out. > I agree completely that the API abuse should be sorted out, but I think that's much larger than just the i915 driver. Pretty much every dma-buf map_dma_buf implementation I looked at ignores the returned nents of sg_attrs. This sucks, but I don't think it's the bug Tom ran into. See: amdgpu_dma_buf_map armada_gem_prime_map_dma_buf drm_gem_map_dma_buf i915_gem_map_dma_buf tegra_gem_prime_map_dma_buf So this should probably be addressed by the whole GPU community. However, as Robin pointed out, there are other ugly tricks like stopping iterating through the SGL when sg_dma_len() is zero. For example, the AMD driver appears to use drm_prime_sg_to_page_addr_arrays() which does this trick and thus likely isn't buggy (otherwise, I'd expect someone to have complained by now seeing AMD has already switched to IOMMU-DMA. As I tried to point out in my previous email, i915 does not do this trick. In fact, it completely ignores sg_dma_len() and is thus completely broken. See i915_scatterlist.h and the __sgt_iter() function. So it doesn't sound to me like Mark's fix would address the issue at all. Per my previous email, I'd guess that it can be fixed simply by adjusting the __sgt_iter() function to do something more sensible. (Better yet, if possible, ditch __sgt_iter() and use the common DRM function that AMD uses). This would at least allow us to make progress with Tom's IOMMU-DMA patch set and once that gets in, it will be harder for other drivers to make the same mistake. Logan
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Thu, May 28, 2020 at 06:00:44PM -0600, Logan Gunthorpe wrote: > > This issue is most likely in the i915 driver and is most likely caused by > > the driver not respecting the return value of the dma_map_ops::map_sg > > function. You can see the driver ignoring the return value here: > > https://github.com/torvalds/linux/blob/7e0165b2f1a912a06e381e91f0f4e495f4ac3736/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c#L51 > > > > Previously this didn’t cause issues because the intel map_sg always > > returned the same number of elements as the input scatter gather list but > > with the change to this dma-iommu api this is no longer the case. I wasn’t > > able to track the bug down to a specific line of code unfortunately. Mark did a big audit into the map_sg API abuse and initially had some i915 patches, but then gave up on them with this comment: "The biggest TODO is DRM/i915 driver and I don't feel brave enough to fix it fully. The driver creatively uses sg_table->orig_nents to store the size of the allocate scatterlist and ignores the number of the entries returned by dma_map_sg function. In this patchset I only fixed the sg_table objects exported by dmabuf related functions. I hope that I didn't break anything there." it would be really nice if the i915 maintainers could help with sorting that API abuse out.
Re: [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
Hi Tom, On 2019-12-21 8:03 a.m., Tom Murphy wrote: > This patchset converts the intel iommu driver to the dma-iommu api. Just wanted to note that I've rebased your series on recent kernels and have done some testing on my old Sandybridge machine (without the DO NOT MERGE patch) and have found no issues. I hope this can make progress soon and get merged soon. If you like you can add: Tested-By: Logan Gunthorpe > While converting the driver I exposed a bug in the intel i915 driver which > causes a huge amount of artifacts on the screen of my laptop. You can see a > picture of it here: > https://github.com/pippy360/kernelPatches/blob/master/IMG_20191219_225922.jpg > > This issue is most likely in the i915 driver and is most likely caused by the > driver not respecting the return value of the dma_map_ops::map_sg function. > You can see the driver ignoring the return value here: > https://github.com/torvalds/linux/blob/7e0165b2f1a912a06e381e91f0f4e495f4ac3736/drivers/gpu/drm/i915/gem/i915_gem_dmabuf.c#L51 > > Previously this didn’t cause issues because the intel map_sg always returned > the same number of elements as the input scatter gather list but with the > change to this dma-iommu api this is no longer the case. I wasn’t able to > track the bug down to a specific line of code unfortunately. I did some digging into this myself and while I don't have full patch, I think I traced it closer to the problem. Sadly, ignoring the number of nents returned by map_sg() is endemic to dma-buf users, but AMD's GPU driver seems to do the same thing, presumably without issues. Digging a bit further, I found that the i915 has an "innovative" way of iterating through SGLs, see [1]. I suspect if __sgt_iter is changed to increment with sg_dma_len() and return NULL when there is no length left, it may fix the issue. But, sorry, I don't really have the means or time to fix and test this myself. Thanks, Logan [1] https://elixir.bootlin.com/linux/v5.7-rc7/source/drivers/gpu/drm/i915/i915_scatterlist.h#L76
