[PATCH 0/6] Properly detect swiotlb.

[PATCH 0/6] Properly detect swiotlb.

[jglisse]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[jglisse]

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
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/6] drm/radeon: Use swiotlb_in_use() to know if swiotlb is enabled or not.

[jglisse]

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(&gtt->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(&gtt->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
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/6] drm/nouveau: Use swiotlb_in_use() to know if swiotlb is enabled or not.

[jglisse]

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]) {
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 4/6] drm/vmwgfx: Use swiotlb_in_use() to know if swiotlb is enabled.

[jglisse]

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:
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 5/6] drm/i915: Use swiotlb_in_use() to know if swiotlb is enabled.

[jglisse]

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;
 
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 6/6] drm/amdgpu: Use swiotlb_in_use() to know if swiotlb is enabled.

[jglisse]

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(&gtt->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(&gtt->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
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[Konrad Rzeszutek Wilk]

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
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[Jerome Glisse]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[Konrad Rzeszutek Wilk]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[Jerome Glisse]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[Konrad Rzeszutek Wilk]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] swiotlb: Add helper to know if it is in use for a specific device.

[Jerome Glisse]

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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] Properly detect swiotlb.

[Alex Deucher]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

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

[-- Attachment #2: 0001-drm-amdgpu-use-ttm_pool-instead-of-ttm_dma.patch --]
[-- Type: text/x-patch, Size: 1320 bytes --]

From 054d47be0545ef13350222a5cbdbcbdc4a3d3609 Mon Sep 17 00:00:00 2001
From: "monk.liu" <monk.liu@amd.com>
Date: Fri, 21 Aug 2015 15:36:38 +0800
Subject: [PATCH 1/2] drm/amdgpu: use ttm_pool instead of ttm_dma

ttm_dma_populate is very time costing,
this could rasie around 20% fps of Nexuiz test.

Signed-off-by: monk.liu <monk.liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 2b6aa65..2346a6e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -699,12 +699,6 @@ static int amdgpu_ttm_tt_populate(struct ttm_tt *ttm)
 
 	adev = amdgpu_get_adev(ttm->bdev);
 
-#ifdef CONFIG_SWIOTLB
-	if (swiotlb_nr_tbl()) {
-		return ttm_dma_populate(&gtt->ttm, adev->dev);
-	}
-#endif
-
 	r = ttm_pool_populate(ttm);
 	if (r) {
 		return r;
@@ -745,13 +739,6 @@ static void amdgpu_ttm_tt_unpopulate(struct ttm_tt *ttm)
 
 	adev = amdgpu_get_adev(ttm->bdev);
 
-#ifdef CONFIG_SWIOTLB
-	if (swiotlb_nr_tbl()) {
-		ttm_dma_unpopulate(&gtt->ttm, adev->dev);
-		return;
-	}
-#endif
-
 	for (i = 0; i < ttm->num_pages; i++) {
 		if (gtt->ttm.dma_address[i]) {
 			pci_unmap_page(adev->pdev, gtt->ttm.dma_address[i],
-- 
2.0.0


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel