Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Thu, 10 Sep 2020 at 14:33, Tom Murphy wrote:
>
> On Wed, 9 Sep 2020 at 13:56, Tvrtko Ursulin
> wrote:
> >
> >
> > On 09/09/2020 10:16, Tvrtko Ursulin wrote:
> > > On 08/09/2020 23:43, Tom Murphy wrote:
> > >> On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
> > >> wrote:
> > >>> On 08/09/2020 16:44, Logan Gunthorpe wrote:
> > On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
> > >>
> > >> 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))
> > >
> > > I'd extend the condition to be, just to be safe:
> > > if (dma && sgl && !sg_dma_len(s.sgp))
> > >
> >
> > Right, good catch, that's definitely necessary.
> >
> > >> + 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));
> > >> + }
> > >
> > > Otherwise has this been tested or alternatively how to test it?
> > > (How to
> > > repro the issue.)
> >
> > It has not been tested. To test it, you need Tom's patch set without
> > the
> > last "DO NOT MERGE" patch:
> >
> > https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
> > >>>
> > >>> Tom, do you have a branch somewhere I could pull from? (Just being lazy
> > >>> about downloading a bunch of messages from the archives.)
> > >>
> > >> I don't unfortunately. I'm working locally with poor internet.
> > >>
> > >>>
> > >>> What GPU is in your Lenovo x1 carbon 5th generation and what
> > >>> graphical/desktop setup I need to repro?
> > >>
> > >>
> > >> Is this enough info?:
> > >>
> > >> $ lspci -vnn | grep VGA -A 12
> > >> 00:02.0 VGA compatible controller [0300]: Intel Corporation HD
> > >> Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
> > >> Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
> > >> Flags: bus master, fast devsel, latency 0, IRQ 148
> > >> Memory at eb00 (64-bit, non-prefetchable) [size=16M]
> > >> Memory at 6000 (64-bit, prefetchable) [size=256M]
> > >> I/O ports at e000 [size=64]
> > >> [virtual] Expansion ROM at 000c [disabled] [size=128K]
> > >> Capabilities: [40] Vendor Specific Information: Len=0c
> > >> Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
> > >> Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
> > >> Capabilities: [d0] Power Management version 2
> > >> Capabilities: [100] Process Address Space ID (PASID)
> > >> Capabilities: [200] Address Translation Service (ATS)
> > >
> > > Works for a start. What about the steps to repro? Any desktop
> > > environment and it is just visual corruption, no hangs/stalls or such?
> > >
> > > I've submitted a series consisting of what I understood are the patches
> > > needed to repro the issue to our automated CI here:
> > >
> > > https://patchwork.freedesktop.org/series/81489/
> > >
> > > So will see if it will catch something, or more targeted testing will be
> > > required. Hopefully it does trip over in which case I can add the patch
> > > suggested by Logan on top and see if that fixes it. Or I'll need to
> > > write a new test case.
> > >
> > > If you could glance over my series to check I identified the patches
> > > correctly it would be appreciated.
> >
> > Our CI was more than capable at catching the breakage so I've copied you
> > on a patch (https://patchwork.freedesktop.org/series/81497/) which has a
> > good potential to fix this. (Or improve the robustness of our sg walks,
> > depends how you look at it.)
> >
> > Would you be able to test it in your environment by any chance? If it
> > works I understand it unblocks your IOMMU work, right?
And yes this does unblock the iommu work
>
> I tested your latest patch set ([PATCH 1/2] drm/i915: Fix DMA mapped
> scatterlist walks) and it fixes the issue. great work!
>
> >
> > Regards,
> >
> > Tvrtko
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Wed, 9 Sep 2020 at 13:56, Tvrtko Ursulin
wrote:
>
>
> On 09/09/2020 10:16, Tvrtko Ursulin wrote:
> > On 08/09/2020 23:43, Tom Murphy wrote:
> >> On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
> >> wrote:
> >>> On 08/09/2020 16:44, Logan Gunthorpe wrote:
> On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
> >>
> >> 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))
> >
> > I'd extend the condition to be, just to be safe:
> > if (dma && sgl && !sg_dma_len(s.sgp))
> >
>
> Right, good catch, that's definitely necessary.
>
> >> + 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));
> >> + }
> >
> > Otherwise has this been tested or alternatively how to test it?
> > (How to
> > repro the issue.)
>
> It has not been tested. To test it, you need Tom's patch set without
> the
> last "DO NOT MERGE" patch:
>
> https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
> >>>
> >>> Tom, do you have a branch somewhere I could pull from? (Just being lazy
> >>> about downloading a bunch of messages from the archives.)
> >>
> >> I don't unfortunately. I'm working locally with poor internet.
> >>
> >>>
> >>> What GPU is in your Lenovo x1 carbon 5th generation and what
> >>> graphical/desktop setup I need to repro?
> >>
> >>
> >> Is this enough info?:
> >>
> >> $ lspci -vnn | grep VGA -A 12
> >> 00:02.0 VGA compatible controller [0300]: Intel Corporation HD
> >> Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
> >> Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
> >> Flags: bus master, fast devsel, latency 0, IRQ 148
> >> Memory at eb00 (64-bit, non-prefetchable) [size=16M]
> >> Memory at 6000 (64-bit, prefetchable) [size=256M]
> >> I/O ports at e000 [size=64]
> >> [virtual] Expansion ROM at 000c [disabled] [size=128K]
> >> Capabilities: [40] Vendor Specific Information: Len=0c
> >> Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
> >> Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
> >> Capabilities: [d0] Power Management version 2
> >> Capabilities: [100] Process Address Space ID (PASID)
> >> Capabilities: [200] Address Translation Service (ATS)
> >
> > Works for a start. What about the steps to repro? Any desktop
> > environment and it is just visual corruption, no hangs/stalls or such?
> >
> > I've submitted a series consisting of what I understood are the patches
> > needed to repro the issue to our automated CI here:
> >
> > https://patchwork.freedesktop.org/series/81489/
> >
> > So will see if it will catch something, or more targeted testing will be
> > required. Hopefully it does trip over in which case I can add the patch
> > suggested by Logan on top and see if that fixes it. Or I'll need to
> > write a new test case.
> >
> > If you could glance over my series to check I identified the patches
> > correctly it would be appreciated.
>
> Our CI was more than capable at catching the breakage so I've copied you
> on a patch (https://patchwork.freedesktop.org/series/81497/) which has a
> good potential to fix this. (Or improve the robustness of our sg walks,
> depends how you look at it.)
>
> Would you be able to test it in your environment by any chance? If it
> works I understand it unblocks your IOMMU work, right?
I tested your latest patch set ([PATCH 1/2] drm/i915: Fix DMA mapped
scatterlist walks) and it fixes the issue. great work!
>
> Regards,
>
> Tvrtko
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 08/09/2020 23:43, Tom Murphy wrote:
On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
wrote:
On 08/09/2020 16:44, Logan Gunthorpe wrote:
On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
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))
I'd extend the condition to be, just to be safe:
if (dma && sgl && !sg_dma_len(s.sgp))
Right, good catch, that's definitely necessary.
+ 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));
+ }
Otherwise has this been tested or alternatively how to test it? (How to
repro the issue.)
It has not been tested. To test it, you need Tom's patch set without the
last "DO NOT MERGE" patch:
https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
Tom, do you have a branch somewhere I could pull from? (Just being lazy
about downloading a bunch of messages from the archives.)
I don't unfortunately. I'm working locally with poor internet.
What GPU is in your Lenovo x1 carbon 5th generation and what
graphical/desktop setup I need to repro?
Is this enough info?:
$ lspci -vnn | grep VGA -A 12
00:02.0 VGA compatible controller [0300]: Intel Corporation HD
Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
Flags: bus master, fast devsel, latency 0, IRQ 148
Memory at eb00 (64-bit, non-prefetchable) [size=16M]
Memory at 6000 (64-bit, prefetchable) [size=256M]
I/O ports at e000 [size=64]
[virtual] Expansion ROM at 000c [disabled] [size=128K]
Capabilities: [40] Vendor Specific Information: Len=0c
Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
Capabilities: [d0] Power Management version 2
Capabilities: [100] Process Address Space ID (PASID)
Capabilities: [200] Address Translation Service (ATS)
Works for a start. What about the steps to repro? Any desktop
environment and it is just visual corruption, no hangs/stalls or such?
I've submitted a series consisting of what I understood are the patches
needed to repro the issue to our automated CI here:
https://patchwork.freedesktop.org/series/81489/
So will see if it will catch something, or more targeted testing will be
required. Hopefully it does trip over in which case I can add the patch
suggested by Logan on top and see if that fixes it. Or I'll need to
write a new test case.
If you could glance over my series to check I identified the patches
correctly it would be appreciated.
Regards,
Tvrtko
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On Tue, 8 Sep 2020 at 16:56, Tvrtko Ursulin
wrote:
>
>
> On 08/09/2020 16:44, Logan Gunthorpe wrote:
> > On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
> >>>
> >>> 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))
> >>
> >> I'd extend the condition to be, just to be safe:
> >> if (dma && sgl && !sg_dma_len(s.sgp))
> >>
> >
> > Right, good catch, that's definitely necessary.
> >
> >>> + 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));
> >>> + }
> >>
> >> Otherwise has this been tested or alternatively how to test it? (How to
> >> repro the issue.)
> >
> > It has not been tested. To test it, you need Tom's patch set without the
> > last "DO NOT MERGE" patch:
> >
> > https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
>
> Tom, do you have a branch somewhere I could pull from? (Just being lazy
> about downloading a bunch of messages from the archives.)
I don't unfortunately. I'm working locally with poor internet.
>
> What GPU is in your Lenovo x1 carbon 5th generation and what
> graphical/desktop setup I need to repro?
Is this enough info?:
$ lspci -vnn | grep VGA -A 12
00:02.0 VGA compatible controller [0300]: Intel Corporation HD
Graphics 620 [8086:5916] (rev 02) (prog-if 00 [VGA controller])
Subsystem: Lenovo ThinkPad X1 Carbon 5th Gen [17aa:224f]
Flags: bus master, fast devsel, latency 0, IRQ 148
Memory at eb00 (64-bit, non-prefetchable) [size=16M]
Memory at 6000 (64-bit, prefetchable) [size=256M]
I/O ports at e000 [size=64]
[virtual] Expansion ROM at 000c [disabled] [size=128K]
Capabilities: [40] Vendor Specific Information: Len=0c
Capabilities: [70] Express Root Complex Integrated Endpoint, MSI 00
Capabilities: [ac] MSI: Enable+ Count=1/1 Maskable- 64bit-
Capabilities: [d0] Power Management version 2
Capabilities: [100] Process Address Space ID (PASID)
Capabilities: [200] Address Translation Service (ATS)
>
> Regards,
>
> Tvrtko
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
Hi,
On 27/08/2020 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?
This thread was brought to my attention and I initially missed this
reply. Essentially I came to the same conclusion about the need to use
sg_dma_len. One small correction below:
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))
I'd extend the condition to be, just to be safe:
if (dma && 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));
+ }
Otherwise has this been tested or alternatively how to test it? (How to
repro the issue.)
Regards,
Tvrtko
Re: [Intel-gfx] [PATCH 0/8] Convert the intel iommu driver to the dma-iommu api
On 2020-09-08 9:28 a.m., Tvrtko Ursulin wrote:
>>
>> 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))
>
> I'd extend the condition to be, just to be safe:
> if (dma && sgl && !sg_dma_len(s.sgp))
>
Right, good catch, that's definitely necessary.
>> + 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));
>> + }
>
> Otherwise has this been tested or alternatively how to test it? (How to
> repro the issue.)
It has not been tested. To test it, you need Tom's patch set without the
last "DO NOT MERGE" patch:
https://lkml.kernel.org/lkml/20200907070035.ga25...@infradead.org/T/
Thanks,
Logan
