So this is only build tested as i am away from hardware right now. Idea is to provide reliable way to check if swiotlb is in use for a device or not. It seems swiotlb_nr_tbl() is no longer reliable for that.
Please test.
Cheers, Jérôme Glisse
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com --- include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+ +#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{ + struct dma_map_ops *ops = get_dma_ops(dev); + + return (ops->map_sg == swiotlb_map_sg_attrs || + ops->unmap_sg == swiotlb_unmap_sg_attrs || + ops->map_page == swiotlb_map_page); +} +#else +static inline bool swiotlb_in_use(struct device *dev) +{ + return false; +} +#endif + + #endif
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse@redhat.com wrote:
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com
include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{
- struct dma_map_ops *ops = get_dma_ops(dev);
- return (ops->map_sg == swiotlb_map_sg_attrs ||
ops->unmap_sg == swiotlb_unmap_sg_attrs ||ops->map_page == swiotlb_map_page);
That won't work. What if we use xen-swiotlb which has different function names?
+} +#else +static inline bool swiotlb_in_use(struct device *dev) +{
- return false;
+} +#endif
#endif
2.1.0
On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse@redhat.com wrote:
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f which cleanup swiotlb init and always allocate default size. Which i believe is a waste of memory. So we need to add a real helper to know if swiotlb is in use or not and we should not rely on expectation of some swiotlb value.
the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com
include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{
- struct dma_map_ops *ops = get_dma_ops(dev);
- return (ops->map_sg == swiotlb_map_sg_attrs ||
ops->unmap_sg == swiotlb_unmap_sg_attrs ||ops->map_page == swiotlb_map_page);That won't work. What if we use xen-swiotlb which has different function names?
I didn't thought about xen, always doing things differently, i think xen is just a matter of also testing for the xen function. I just wanted to have the helper in common code and only rely on common things, instead of having to add a per arch helper.
Cheers, Jérôme
On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse@redhat.com wrote:
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f which cleanup swiotlb init and always allocate default size. Which i believe is a waste of memory. So we need to add a real helper to know if swiotlb is in use or not and we should not rely on expectation of some swiotlb value.
Ah right, that patch. That should have been part of the description I believe.
the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com
include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{
- struct dma_map_ops *ops = get_dma_ops(dev);
- return (ops->map_sg == swiotlb_map_sg_attrs ||
ops->unmap_sg == swiotlb_unmap_sg_attrs ||ops->map_page == swiotlb_map_page);That won't work. What if we use xen-swiotlb which has different function names?
I didn't thought about xen, always doing things differently, i think xen is just a matter of also testing for the xen function. I just wanted to have the helper in common code and only rely on common things, instead of having to add a per arch helper.
There has to be a better way. Perhaps you can expand SWIOTLB to actually check if it is in use?
Cheers, Jérôme
On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse@redhat.com wrote:
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f which cleanup swiotlb init and always allocate default size. Which i believe is a waste of memory. So we need to add a real helper to know if swiotlb is in use or not and we should not rely on expectation of some swiotlb value.
Ah right, that patch. That should have been part of the description I believe.
the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com
include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{
- struct dma_map_ops *ops = get_dma_ops(dev);
- return (ops->map_sg == swiotlb_map_sg_attrs ||
ops->unmap_sg == swiotlb_unmap_sg_attrs ||ops->map_page == swiotlb_map_page);That won't work. What if we use xen-swiotlb which has different function names?
I didn't thought about xen, always doing things differently, i think xen is just a matter of also testing for the xen function. I just wanted to have the helper in common code and only rely on common things, instead of having to add a per arch helper.
There has to be a better way. Perhaps you can expand SWIOTLB to actually check if it is in use?
This would require per arch modifications which is what i was trying to avoid.
Cheers, Jérôme
On Wed, Aug 26, 2015 at 04:31:50PM -0400, Jerome Glisse wrote:
On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse@redhat.com wrote:
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f which cleanup swiotlb init and always allocate default size. Which i believe is a waste of memory. So we need to add a real helper to know if swiotlb is in use or not and we should not rely on expectation of some swiotlb value.
Ah right, that patch. That should have been part of the description I believe.
the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com
include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{
- struct dma_map_ops *ops = get_dma_ops(dev);
- return (ops->map_sg == swiotlb_map_sg_attrs ||
ops->unmap_sg == swiotlb_unmap_sg_attrs ||ops->map_page == swiotlb_map_page);That won't work. What if we use xen-swiotlb which has different function names?
I didn't thought about xen, always doing things differently, i think xen is just a matter of also testing for the xen function. I just wanted to have the helper in common code and only rely on common things, instead of having to add a per arch helper.
There has to be a better way. Perhaps you can expand SWIOTLB to actually check if it is in use?
This would require per arch modifications which is what i was trying to avoid.
How? If you modify 'swiotlb_nr_tbl' to return true only if it has been used then the modifications are only in the lib/swiotlb.c ?
Cheers, Jérôme
On Wed, Aug 26, 2015 at 04:38:14PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 04:31:50PM -0400, Jerome Glisse wrote:
On Wed, Aug 26, 2015 at 03:44:52PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 03:26:42PM -0400, Jerome Glisse wrote:
On Wed, Aug 26, 2015 at 03:02:31PM -0400, Konrad Rzeszutek Wilk wrote:
On Wed, Aug 26, 2015 at 02:52:02PM -0400, jglisse@redhat.com wrote:
From: Jérôme Glisse jglisse@redhat.com
Some device like GPU do things differently if swiotlb is in use. We use to rely on swiotlb_nr_tbl() to know if swiotlb was enabled or not but this is unreliable. Patch add a simple helpers to check if any of
Why is it unreliable?
Alex reported on irc that swiotlb_nr_tbl() returns non zero even if swiotlb is disabled. This seems to be due to ac2cbab21f318e19bc176a7f38a120cec835220f which cleanup swiotlb init and always allocate default size. Which i believe is a waste of memory. So we need to add a real helper to know if swiotlb is in use or not and we should not rely on expectation of some swiotlb value.
Ah right, that patch. That should have been part of the description I believe.
the dma_ops associated with a device points to the swiotlb functions, making swiotlb check reliable for a device.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com
include/linux/dma-mapping.h | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h index ac07ff0..eac911e 100644 --- a/include/linux/dma-mapping.h +++ b/include/linux/dma-mapping.h @@ -314,4 +314,22 @@ static inline int dma_mmap_writecombine(struct device *dev, #define dma_unmap_len_set(PTR, LEN_NAME, VAL) do { } while (0) #endif
+#ifdef CONFIG_SWIOTLB +static inline bool swiotlb_in_use(struct device *dev) +{
- struct dma_map_ops *ops = get_dma_ops(dev);
- return (ops->map_sg == swiotlb_map_sg_attrs ||
ops->unmap_sg == swiotlb_unmap_sg_attrs ||ops->map_page == swiotlb_map_page);That won't work. What if we use xen-swiotlb which has different function names?
I didn't thought about xen, always doing things differently, i think xen is just a matter of also testing for the xen function. I just wanted to have the helper in common code and only rely on common things, instead of having to add a per arch helper.
There has to be a better way. Perhaps you can expand SWIOTLB to actually check if it is in use?
This would require per arch modifications which is what i was trying to avoid.
How? If you modify 'swiotlb_nr_tbl' to return true only if it has been used then the modifications are only in the lib/swiotlb.c ?
I am not sure i follow swiotlb_nr_tbl is an internal value that so far have only be use by driver through calling swiotlb_nr_tbl() so calls to that functions does not reflect if swiotlb is enabled or not.
In fact i am pretty sure only arch specific code stores information on wether or not swiotlb is enabled. Beside i think some device overide dev->archdata.dma_ops which effectively disable swiotlb for the device.
But if you really dislike just testing dma_ops against swiotlb & xen_swiotlb i will respin with arch mod but i will only handle ppc/arm/x86.
Cheers, Jérôme
From: Jérôme Glisse jglisse@redhat.com
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/radeon/radeon_ttm.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c b/drivers/gpu/drm/radeon/radeon_ttm.c index 06ac59fe..5c9814a 100644 --- a/drivers/gpu/drm/radeon/radeon_ttm.c +++ b/drivers/gpu/drm/radeon/radeon_ttm.c @@ -38,7 +38,6 @@ #include <drm/radeon_drm.h> #include <linux/seq_file.h> #include <linux/slab.h> -#include <linux/swiotlb.h> #include <linux/swap.h> #include <linux/pagemap.h> #include <linux/debugfs.h> @@ -742,11 +741,9 @@ static int radeon_ttm_tt_populate(struct ttm_tt *ttm) } #endif
-#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(rdev->dev)) { return ttm_dma_populate(>t->ttm, rdev->dev); } -#endif
r = ttm_pool_populate(ttm); if (r) { @@ -794,12 +791,10 @@ static void radeon_ttm_tt_unpopulate(struct ttm_tt *ttm) } #endif
-#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(rdev->dev)) { ttm_dma_unpopulate(>t->ttm, rdev->dev); return; } -#endif
for (i = 0; i < ttm->num_pages; i++) { if (gtt->ttm.dma_address[i]) { @@ -1169,10 +1164,8 @@ static int radeon_ttm_debugfs_init(struct radeon_device *rdev)
count = ARRAY_SIZE(radeon_ttm_debugfs_list);
-#ifdef CONFIG_SWIOTLB - if (!swiotlb_nr_tbl()) + if (!swiotlb_in_use(rdev->dev)) --count; -#endif
return radeon_debugfs_add_files(rdev, radeon_ttm_debugfs_list, count); #else
From: Jérôme Glisse jglisse@redhat.com
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/nouveau/nouveau_bo.c | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c b/drivers/gpu/drm/nouveau/nouveau_bo.c index 6edcce1..f601049 100644 --- a/drivers/gpu/drm/nouveau/nouveau_bo.c +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c @@ -28,7 +28,6 @@ */
#include <linux/dma-mapping.h> -#include <linux/swiotlb.h>
#include "nouveau_drm.h" #include "nouveau_dma.h" @@ -1504,11 +1503,9 @@ nouveau_ttm_tt_populate(struct ttm_tt *ttm) } #endif
-#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(pdev)) { return ttm_dma_populate((void *)ttm, dev->dev); } -#endif
r = ttm_pool_populate(ttm); if (r) { @@ -1572,12 +1569,10 @@ nouveau_ttm_tt_unpopulate(struct ttm_tt *ttm) } #endif
-#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(pdev)) { ttm_dma_unpopulate((void *)ttm, dev->dev); return; } -#endif
for (i = 0; i < ttm->num_pages; i++) { if (ttm_dma->dma_address[i]) {
From: Jérôme Glisse jglisse@redhat.com
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c index 620bb5c..a2b0ec0 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c @@ -517,10 +517,8 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
if (dma_ops->sync_single_for_cpu) dev_priv->map_mode = vmw_dma_alloc_coherent; -#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl() == 0) + if (!swiotlb_in_use(dev_priv->dev->dev)) dev_priv->map_mode = vmw_dma_map_populate; -#endif
#ifdef CONFIG_INTEL_IOMMU out_fixup:
From: Jérôme Glisse jglisse@redhat.com
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/i915/i915_gem.c | 9 +++------ drivers/gpu/drm/i915/i915_gem_userptr.c | 14 +++++--------- 2 files changed, 8 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 52b446b..a67b649 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2251,14 +2251,13 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) goto err_pages; } } -#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(obj->base.dev->dev)) { st->nents++; sg_set_page(sg, page, PAGE_SIZE, 0); sg = sg_next(sg); continue; } -#endif + if (!i || page_to_pfn(page) != last_pfn + 1) { if (i) sg = sg_next(sg); @@ -2272,9 +2271,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj) /* Check that the i965g/gm workaround works. */ WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL)); } -#ifdef CONFIG_SWIOTLB - if (!swiotlb_nr_tbl()) -#endif + if (!swiotlb_in_use(obj->base.dev->dev)) sg_mark_end(sg); obj->pages = st;
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index 8fd431b..ecf03b7 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -506,14 +506,9 @@ struct get_pages_work { struct task_struct *task; };
-#if IS_ENABLED(CONFIG_SWIOTLB) -#define swiotlb_active() swiotlb_nr_tbl() -#else -#define swiotlb_active() 0 -#endif - static int -st_set_pages(struct sg_table **st, struct page **pvec, int num_pages) +st_set_pages(struct device *dev, struct sg_table **st, + struct page **pvec, int num_pages) { struct scatterlist *sg; int ret, n; @@ -522,7 +517,7 @@ st_set_pages(struct sg_table **st, struct page **pvec, int num_pages) if (*st == NULL) return -ENOMEM;
- if (swiotlb_active()) { + if (swiotlb_in_use(dev)) { ret = sg_alloc_table(*st, num_pages, GFP_KERNEL); if (ret) goto err; @@ -549,9 +544,10 @@ static int __i915_gem_userptr_set_pages(struct drm_i915_gem_object *obj, struct page **pvec, int num_pages) { + struct device *dev = obj->base.dev->dev; int ret;
- ret = st_set_pages(&obj->pages, pvec, num_pages); + ret = st_set_pages(dev, &obj->pages, pvec, num_pages); if (ret) return ret;
From: Jérôme Glisse jglisse@redhat.com
We can not rely on swiotlb_nr_tbl() to know if swiotlb is in use or not for our device. Use the new helper to determine that.
Signed-off-by: Jérôme Glisse jglisse@redhat.com Cc: Konrad Rzeszutek Wilk konrad.wilk@oracle.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Ben Skeggs bskeggs@redhat.com Cc: Dave Airlie airlied@redhat.com Cc: lkml@vger.kernel.org Cc: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index dd3415d..bb48fc5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -38,7 +38,6 @@ #include <drm/amdgpu_drm.h> #include <linux/seq_file.h> #include <linux/slab.h> -#include <linux/swiotlb.h> #include <linux/swap.h> #include <linux/pagemap.h> #include <linux/debugfs.h> @@ -692,11 +691,9 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm)
adev = amdgpu_get_adev(ttm->bdev);
-#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(adev->dev)) { return ttm_dma_populate(>t->ttm, adev->dev); } -#endif
r = ttm_pool_populate(ttm); if (r) { @@ -738,12 +735,10 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
adev = amdgpu_get_adev(ttm->bdev);
-#ifdef CONFIG_SWIOTLB - if (swiotlb_nr_tbl()) { + if (swiotlb_in_use(adev->dev)) { ttm_dma_unpopulate(>t->ttm, adev->dev); return; } -#endif
for (i = 0; i < ttm->num_pages; i++) { if (gtt->ttm.dma_address[i]) { @@ -1190,10 +1185,8 @@ static int amdgpu_ttm_debugfs_init(struct amdgpu_device *adev)
count = ARRAY_SIZE(amdgpu_ttm_debugfs_list);
-#ifdef CONFIG_SWIOTLB - if (!swiotlb_nr_tbl()) + if (!swiotlb_in_use(adev->dev)) --count; -#endif
return amdgpu_debugfs_add_files(adev, amdgpu_ttm_debugfs_list, count); #else
On Wed, Aug 26, 2015 at 2:52 PM, jglisse@redhat.com wrote:
So this is only build tested as i am away from hardware right now. Idea is to provide reliable way to check if swiotlb is in use for a device or not. It seems swiotlb_nr_tbl() is no longer reliable for that.
Please test.
Even with these patches applied, we are still ending up in the ttm_dma_populate() path in the amdgpu driver rather than ttm_pool_populate() even with a hw iommu enabled. Any idea?
The attached patch gives us a 20% performance boost in some apps, but obviously has potential issues if a hw iommu is not present.
Alex
Cheers, Jérôme Glisse _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Jerome Glisse -
jglisse＠redhat.com -
Konrad Rzeszutek Wilk