[PATCH 0/4] Fix DMA ops layering violations in vmwgfx v2

[PATCH 0/4] Fix DMA ops layering violations in vmwgfx v2

[Thomas Hellstrom]

This is a patch set from Christoph Hellwig. Patch 2 has been updated by
me to not remove checks for TTM coherent pool presence. Ideally, we
should query TTM for this instead and I'll have a patch set intended for
5.1 so that we can do that. But I don't want to introduce cross-module API
additions outside of a merge window.

Original patch set description:
vmwgfx has been doing some odd checks based on DMA ops which rely
on deep DMA mapping layer internals, and I think the changes in
Linux 4.21 finally broke most of these implicit assumptions.

The real fix is in patch 3, but I think the others are important
to make it clear what is actually going on.


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

[PATCH 1/4] drm/vmwgfx: remove CONFIG_X86 ifdefs

[Thomas Hellstrom]

From: Christoph Hellwig <hch@lst.de>

The driver depends on CONFIG_X86 so these are dead code.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 25afb1d594e3..69e325b2d954 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -565,7 +565,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_alloc_coherent] = "Using coherent TTM pages.",
 		[vmw_dma_map_populate] = "Keeping DMA mappings.",
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
-#ifdef CONFIG_X86
 	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev->dev);
 
 #ifdef CONFIG_INTEL_IOMMU
@@ -607,11 +606,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
 		return -EINVAL;
 #endif
-
-#else /* CONFIG_X86 */
-	dev_priv->map_mode = vmw_dma_map_populate;
-#endif /* CONFIG_X86 */
-
 	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
 
 	return 0;
-- 
2.19.0.rc1

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

[PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs v2

[Thomas Hellstrom]

From: Christoph Hellwig <hch@lst.de>

intel_iommu_enabled is defined as always false for !CONFIG_INTEL_IOMMU,
so remove the ifdefs around it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Thomas Hellstrom <thellstrom@vmware.com>
---
v2: Retain the check for TTM dma page pool presence.
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 69e325b2d954..b7777b5b4a81 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -567,12 +567,10 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
 	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev->dev);
 
-#ifdef CONFIG_INTEL_IOMMU
 	if (intel_iommu_enabled) {
 		dev_priv->map_mode = vmw_dma_map_populate;
 		goto out_fixup;
 	}
-#endif
 
 	if (!(vmw_force_iommu || vmw_force_coherent)) {
 		dev_priv->map_mode = vmw_dma_phys;
@@ -589,9 +587,7 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		dev_priv->map_mode = vmw_dma_map_populate;
 #endif
 
-#ifdef CONFIG_INTEL_IOMMU
 out_fixup:
-#endif
 	if (dev_priv->map_mode == vmw_dma_map_populate &&
 	    vmw_restrict_iommu)
 		dev_priv->map_mode = vmw_dma_map_bind;
@@ -599,13 +595,11 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 	if (vmw_force_coherent)
 		dev_priv->map_mode = vmw_dma_alloc_coherent;
 
-#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
-	/*
-	 * No coherent page pool
-	 */
-	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
+	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
+	if (!(IS_ENABLED(CONFIG_SWIOTLB) || IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
+	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
 		return -EINVAL;
-#endif
+
 	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
 
 	return 0;
@@ -619,7 +613,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
  * With 32-bit we can only handle 32 bit PFNs. Optionally set that
  * restriction also for 64-bit systems.
  */
-#ifdef CONFIG_INTEL_IOMMU
 static int vmw_dma_masks(struct vmw_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -631,12 +624,6 @@ static int vmw_dma_masks(struct vmw_private *dev_priv)
 	}
 	return 0;
 }
-#else
-static int vmw_dma_masks(struct vmw_private *dev_priv)
-{
-	return 0;
-}
-#endif
 
 static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
 {
-- 
2.19.0.rc1

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

Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs v2

[Thomas Hellstrom]

On Mon, 2019-02-04 at 09:19 +0100, Christoph Hellwig wrote:
> On Fri, Jan 25, 2019 at 09:12:13AM +0100, Thomas Hellstrom wrote:
> > -#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
> > -	/*
> > -	 * No coherent page pool
> > -	 */
> > -	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
> > +	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
> > +	if (!(IS_ENABLED(CONFIG_SWIOTLB) ||
> > IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
> > +	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
> >  		return -EINVAL;
> > -#endif
> > +
> 
> I don't think this edited in change makes any sense.  The swiotlb vs
> dma-direct versions of dma_alloc_coherent are the same, so this check
> seems very obsfucating.

So this part of code is identical in functionality to the previous
version. It checks whether the TTM module has the coherent page pool
enabled. (an identical test is present in TTM). What we *really* need
to do here instead is to ask TTM whether it has enabled its coherent
page pool instead of trying to mimic TTM's test, and I have a changeset
under review for that. But as mentioned previously, I don't want to
change the TTM interface outside of a merge window, so we either have
to live with the above for 5.0 or keep the old defines. I'd prefer the
former so I don't have to respin the patch series once more.

Thanks,
Thoams

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

Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs v2

[Thomas Hellstrom]

On Mon, 2019-02-04 at 13:11 +0100, Thomas Hellström wrote:
> On Mon, 2019-02-04 at 09:19 +0100, Christoph Hellwig wrote:
> > On Fri, Jan 25, 2019 at 09:12:13AM +0100, Thomas Hellstrom wrote:
> > > -#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
> > > -	/*
> > > -	 * No coherent page pool
> > > -	 */
> > > -	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
> > > +	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
> > > +	if (!(IS_ENABLED(CONFIG_SWIOTLB) ||
> > > IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
> > > +	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
> > >  		return -EINVAL;
> > > -#endif
> > > +
> > 
> > I don't think this edited in change makes any sense.  The swiotlb
> > vs
> > dma-direct versions of dma_alloc_coherent are the same, so this
> > check
> > seems very obsfucating.
> 
> So this part of code is identical in functionality to the previous
> version. It checks whether the TTM module has the coherent page pool
> enabled. (an identical test is present in TTM). What we *really* need
> to do here instead is to ask TTM whether it has enabled its coherent
> page pool instead of trying to mimic TTM's test, and I have a
> changeset
> under review for that. But as mentioned previously, I don't want to
> change the TTM interface outside of a merge window, so we either have
> to live with the above for 5.0 or keep the old defines. I'd prefer
> the
> former so I don't have to respin the patch series once more.
> 
> Thanks,
> Thoams
> 

Hi, Christoph,

I need to get this merged this week. Could you please ack or ack
removing this hunk + updating the following patches for merge errors?

If no response, I'll add a Cc: tag on the patch and a #v1 to your s-o-
b.

Thanks,
Thomas

Re: [PATCH 2/4] drm/vmwgfx: remove CONFIG_INTEL_IOMMU ifdefs v2

[hch]

On Tue, Feb 05, 2019 at 07:59:22AM +0000, Thomas Hellstrom wrote:
> On Mon, 2019-02-04 at 13:11 +0100, Thomas Hellström wrote:
> > On Mon, 2019-02-04 at 09:19 +0100, Christoph Hellwig wrote:
> > > On Fri, Jan 25, 2019 at 09:12:13AM +0100, Thomas Hellstrom wrote:
> > > > -#if !defined(CONFIG_SWIOTLB) && !defined(CONFIG_INTEL_IOMMU)
> > > > -	/*
> > > > -	 * No coherent page pool
> > > > -	 */
> > > > -	if (dev_priv->map_mode == vmw_dma_alloc_coherent)
> > > > +	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
> > > > +	if (!(IS_ENABLED(CONFIG_SWIOTLB) ||
> > > > IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
> > > > +	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
> > > >  		return -EINVAL;
> > > > -#endif
> > > > +
> > > 
> > > I don't think this edited in change makes any sense.  The swiotlb
> > > vs
> > > dma-direct versions of dma_alloc_coherent are the same, so this
> > > check
> > > seems very obsfucating.
> > 
> > So this part of code is identical in functionality to the previous
> > version. It checks whether the TTM module has the coherent page pool
> > enabled. (an identical test is present in TTM). What we *really* need
> > to do here instead is to ask TTM whether it has enabled its coherent
> > page pool instead of trying to mimic TTM's test, and I have a
> > changeset
> > under review for that. But as mentioned previously, I don't want to
> > change the TTM interface outside of a merge window, so we either have
> > to live with the above for 5.0 or keep the old defines. I'd prefer
> > the
> > former so I don't have to respin the patch series once more.
> > 
> > Thanks,
> > Thoams
> > 
> 
> Hi, Christoph,
> 
> I need to get this merged this week. Could you please ack or ack
> removing this hunk + updating the following patches for merge errors?
> 
> If no response, I'll add a Cc: tag on the patch and a #v1 to your s-o-
> b.

Please go ahead.  I'll look into the fallout later.

[PATCH 3/4] drm/vmwgfx: fix the check when to use dma_alloc_coherent

[Thomas Hellstrom]

From: Christoph Hellwig <hch@lst.de>

Since Linux 4.21 we merged the swiotlb ops into the DMA direct ops,
so they would always have a the sync_single methods.  But late in
the cicle we also removed the direct ops entirely, so we'd see NULL
DMA ops.  Switch vmw_dma_select_mode to only detect swiotlb presence
using swiotlb_nr_tbl() instead.

Fixes: 55897af630 ("dma-direct: merge swiotlb_dma_ops into the dma_direct code")
Fixes: 356da6d0cd ("dma-mapping: bypass indirect calls for dma-direct")
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index b7777b5b4a81..1456101e67a9 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -565,7 +565,6 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_alloc_coherent] = "Using coherent TTM pages.",
 		[vmw_dma_map_populate] = "Keeping DMA mappings.",
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
-	const struct dma_map_ops *dma_ops = get_dma_ops(dev_priv->dev->dev);
 
 	if (intel_iommu_enabled) {
 		dev_priv->map_mode = vmw_dma_map_populate;
@@ -578,14 +577,12 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		return 0;
 	}
 
-	dev_priv->map_mode = vmw_dma_map_populate;
-
-	if (dma_ops && dma_ops->sync_single_for_cpu)
-		dev_priv->map_mode = vmw_dma_alloc_coherent;
 #ifdef CONFIG_SWIOTLB
-	if (swiotlb_nr_tbl() == 0)
-		dev_priv->map_mode = vmw_dma_map_populate;
+	if (swiotlb_nr_tbl())
+		dev_priv->map_mode = vmw_dma_alloc_coherent;
+	else
 #endif
+		dev_priv->map_mode = vmw_dma_map_populate;
 
 out_fixup:
 	if (dev_priv->map_mode == vmw_dma_map_populate &&
-- 
2.19.0.rc1

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

[PATCH 4/4] drm/vmwgfx: unwind spaghetti code in vmw_dma_select_mode

[Thomas Hellstrom]

From: Christoph Hellwig <hch@lst.de>

Just use a simple if/else chain to select the DMA mode.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 25 ++++++-------------------
 1 file changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 1456101e67a9..3e2bcff34032 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -566,39 +566,26 @@ static int vmw_dma_select_mode(struct vmw_private *dev_priv)
 		[vmw_dma_map_populate] = "Keeping DMA mappings.",
 		[vmw_dma_map_bind] = "Giving up DMA mappings early."};
 
-	if (intel_iommu_enabled) {
+	if (vmw_force_coherent)
+		dev_priv->map_mode = vmw_dma_alloc_coherent;
+	else if (intel_iommu_enabled)
 		dev_priv->map_mode = vmw_dma_map_populate;
-		goto out_fixup;
-	}
-
-	if (!(vmw_force_iommu || vmw_force_coherent)) {
+	else if (!vmw_force_iommu)
 		dev_priv->map_mode = vmw_dma_phys;
-		DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
-		return 0;
-	}
-
-#ifdef CONFIG_SWIOTLB
-	if (swiotlb_nr_tbl())
+	else if (IS_ENABLED(CONFIG_SWIOTLB) && swiotlb_nr_tbl())
 		dev_priv->map_mode = vmw_dma_alloc_coherent;
 	else
-#endif
 		dev_priv->map_mode = vmw_dma_map_populate;
 
-out_fixup:
-	if (dev_priv->map_mode == vmw_dma_map_populate &&
-	    vmw_restrict_iommu)
+	if (dev_priv->map_mode == vmw_dma_map_populate && vmw_restrict_iommu)
 		dev_priv->map_mode = vmw_dma_map_bind;
 
-	if (vmw_force_coherent)
-		dev_priv->map_mode = vmw_dma_alloc_coherent;
-
 	/* No TTM coherent page pool? FIXME: Ask TTM instead! */
         if (!(IS_ENABLED(CONFIG_SWIOTLB) || IS_ENABLED(CONFIG_INTEL_IOMMU)) &&
 	    (dev_priv->map_mode == vmw_dma_alloc_coherent))
 		return -EINVAL;
 
 	DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
-
 	return 0;
 }
 
-- 
2.19.0.rc1

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