[1/2] drm/i915: Treat stolen memory as DMA addresses

Submitted by Chris Wilson on Jan. 27, 2017, 4:55 p.m.

Details

Message ID 20170127165531.28135-1-chris@chris-wilson.co.uk
State Accepted
Commit c88473878d47131ccfc67a00ba688d4d7d0f4519
Headers show
Series "Series without cover letter" ( rev: 4 3 2 1 ) in Intel GFX

Browsing this patch as part of:
"Series without cover letter" rev 1 in Intel GFX
<< prev patch [1/2] next patch >>

Commit Message

Chris Wilson Jan. 27, 2017, 4:55 p.m.
The conversion of stolen to use phys_addr_t (from essentially u32)
sparked an interesting discussion. We treat stolen memory as only
accessible from the GPU (the DMA device) - an attempt to use it from the
CPU will generate a MCE on gen6 onwards, although it is in theory a
physical address that can be dereferenced from the CPU as demonstrated
by earlier generations. As such, using phys_addr_t has the wrong
connotations and as we pass the address into the DMA device via
dma_addr_t (through the scatterlists used to program the GTT entries),
we should treat it as dma_addr_t throughout.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h        |  2 +-
 drivers/gpu/drm/i915/i915_gem_stolen.c | 36 +++++++++++++++++-----------------
 2 files changed, 19 insertions(+), 19 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d0a48cceb15b..a43ba7872d7c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1467,7 +1467,7 @@  struct i915_gem_mm {
 	struct work_struct free_work;
 
 	/** Usable portion of the GTT for GEM */
-	phys_addr_t stolen_base; /* limited to low memory (32-bit) */
+	dma_addr_t stolen_base; /* limited to low memory (32-bit) */
 
 	/** PPGTT used for aliasing the PPGTT with the GTT */
 	struct i915_hw_ppgtt *aliasing_ppgtt;
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 7226e4a21097..42bbc4b04fd6 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -79,12 +79,12 @@  void i915_gem_stolen_remove_node(struct drm_i915_private *dev_priv,
 	mutex_unlock(&dev_priv->mm.stolen_lock);
 }
 
-static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
+static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct resource *r;
-	phys_addr_t base;
+	dma_addr_t base;
 
 	/* Almost universally we can find the Graphics Base of Stolen Memory
 	 * at register BSM (0x5c) in the igfx configuration space. On a few
@@ -196,7 +196,7 @@  static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 	if (INTEL_GEN(dev_priv) <= 4 &&
 	    !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv) && !IS_G4X(dev_priv)) {
 		struct {
-			phys_addr_t start, end;
+			dma_addr_t start, end;
 		} stolen[2] = {
 			{ .start = base, .end = base + ggtt->stolen_size, },
 			{ .start = base, .end = base + ggtt->stolen_size, },
@@ -228,12 +228,12 @@  static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 
 		if (stolen[0].start != stolen[1].start ||
 		    stolen[0].end != stolen[1].end) {
-			phys_addr_t end = base + ggtt->stolen_size - 1;
+			dma_addr_t end = base + ggtt->stolen_size - 1;
 
 			DRM_DEBUG_KMS("GTT within stolen memory at 0x%llx-0x%llx\n",
 				      (unsigned long long)ggtt_start,
 				      (unsigned long long)ggtt_end - 1);
-			DRM_DEBUG_KMS("Stolen memory adjusted to %pa - %pa\n",
+			DRM_DEBUG_KMS("Stolen memory adjusted to %pad - %pad\n",
 				      &base, &end);
 		}
 	}
@@ -263,9 +263,9 @@  static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 		 * range. Apparently this works.
 		 */
 		if (r == NULL && !IS_GEN3(dev_priv)) {
-			phys_addr_t end = base + ggtt->stolen_size;
+			dma_addr_t end = base + ggtt->stolen_size;
 
-			DRM_ERROR("conflict detected with stolen region: [%pa - %pa]\n",
+			DRM_ERROR("conflict detected with stolen region: [%pad - %pad]\n",
 				  &base, &end);
 			base = 0;
 		}
@@ -285,13 +285,13 @@  void i915_gem_cleanup_stolen(struct drm_device *dev)
 }
 
 static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				    phys_addr_t *base, u32 *size)
+				    dma_addr_t *base, u32 *size)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	uint32_t reg_val = I915_READ(IS_GM45(dev_priv) ?
 				     CTG_STOLEN_RESERVED :
 				     ELK_STOLEN_RESERVED);
-	phys_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
+	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
 
 	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
 
@@ -308,7 +308,7 @@  static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				     phys_addr_t *base, u32 *size)
+				     dma_addr_t *base, u32 *size)
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
@@ -334,7 +334,7 @@  static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				     phys_addr_t *base, u32 *size)
+				     dma_addr_t *base, u32 *size)
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
@@ -354,7 +354,7 @@  static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				    phys_addr_t *base, u32 *size)
+				    dma_addr_t *base, u32 *size)
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
@@ -380,11 +380,11 @@  static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv,
 }
 
 static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
-				    phys_addr_t *base, u32 *size)
+				    dma_addr_t *base, u32 *size)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
-	phys_addr_t stolen_top;
+	dma_addr_t stolen_top;
 
 	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
 
@@ -403,7 +403,7 @@  static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
 int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 {
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
-	phys_addr_t reserved_base, stolen_top;
+	dma_addr_t reserved_base, stolen_top;
 	u32 reserved_total, reserved_size;
 	u32 stolen_usable_start;
 
@@ -424,7 +424,7 @@  int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 	if (ggtt->stolen_size == 0)
 		return 0;
 
-	dev_priv->mm.stolen_base = i915_stolen_to_physical(dev_priv);
+	dev_priv->mm.stolen_base = i915_stolen_to_dma(dev_priv);
 	if (dev_priv->mm.stolen_base == 0)
 		return 0;
 
@@ -473,8 +473,8 @@  int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 
 	if (reserved_base < dev_priv->mm.stolen_base ||
 	    reserved_base + reserved_size > stolen_top) {
-		phys_addr_t reserved_top = reserved_base + reserved_size;
-		DRM_DEBUG_KMS("Stolen reserved area [%pa - %pa] outside stolen memory [%pa - %pa]\n",
+		dma_addr_t reserved_top = reserved_base + reserved_size;
+		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad] outside stolen memory [%pad - %pad]\n",
 			      &reserved_base, &reserved_top,
 			      &dev_priv->mm.stolen_base, &stolen_top);
 		return 0;

Comments

Em Sex, 2017-01-27 às 16:55 +0000, Chris Wilson escreveu:
> The conversion of stolen to use phys_addr_t (from essentially u32)
> sparked an interesting discussion. We treat stolen memory as only
> accessible from the GPU (the DMA device) - an attempt to use it from
> the
> CPU will generate a MCE on gen6 onwards, although it is in theory a
> physical address that can be dereferenced from the CPU as
> demonstrated
> by earlier generations. As such, using phys_addr_t has the wrong
> connotations and as we pass the address into the DMA device via
> dma_addr_t (through the scatterlists used to program the GTT
> entries),
> we should treat it as dma_addr_t throughout.

I'm not a specialist here, but from what I could learn/understand, this
seems good. The patch seems to do what it says, so:

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

But now that I looked some more at the code, I started to think about
the following: shouldn't we convert our "stolen offset" variables from
u32 to dma_addr_t or some other appropriate type? I mean, if we ever
get 64 bit stolen pointers we may also get 64 bit stolen offsets... The
i915_ggtt struct contains 4 of such u32 pointers.

For example, i915_ggtt->stolen_reserved_base is another pointer to
stolen memory, it's generated directly from one of our drm_addr_t
variables.

Of course, this could be a separate patch.

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h        |  2 +-
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 36 +++++++++++++++++-------
> ----------
>  2 files changed, 19 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index d0a48cceb15b..a43ba7872d7c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1467,7 +1467,7 @@ struct i915_gem_mm {
>  	struct work_struct free_work;
>  
>  	/** Usable portion of the GTT for GEM */
> -	phys_addr_t stolen_base; /* limited to low memory (32-bit)
> */
> +	dma_addr_t stolen_base; /* limited to low memory (32-bit) */
>  
>  	/** PPGTT used for aliasing the PPGTT with the GTT */
>  	struct i915_hw_ppgtt *aliasing_ppgtt;
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 7226e4a21097..42bbc4b04fd6 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -79,12 +79,12 @@ void i915_gem_stolen_remove_node(struct
> drm_i915_private *dev_priv,
>  	mutex_unlock(&dev_priv->mm.stolen_lock);
>  }
>  
> -static phys_addr_t i915_stolen_to_physical(struct drm_i915_private
> *dev_priv)
> +static dma_addr_t i915_stolen_to_dma(struct drm_i915_private
> *dev_priv)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	struct resource *r;
> -	phys_addr_t base;
> +	dma_addr_t base;
>  
>  	/* Almost universally we can find the Graphics Base of
> Stolen Memory
>  	 * at register BSM (0x5c) in the igfx configuration space.
> On a few
> @@ -196,7 +196,7 @@ static phys_addr_t i915_stolen_to_physical(struct
> drm_i915_private *dev_priv)
>  	if (INTEL_GEN(dev_priv) <= 4 &&
>  	    !IS_G33(dev_priv) && !IS_PINEVIEW(dev_priv) &&
> !IS_G4X(dev_priv)) {
>  		struct {
> -			phys_addr_t start, end;
> +			dma_addr_t start, end;
>  		} stolen[2] = {
>  			{ .start = base, .end = base + ggtt-
> >stolen_size, },
>  			{ .start = base, .end = base + ggtt-
> >stolen_size, },
> @@ -228,12 +228,12 @@ static phys_addr_t
> i915_stolen_to_physical(struct drm_i915_private *dev_priv)
>  
>  		if (stolen[0].start != stolen[1].start ||
>  		    stolen[0].end != stolen[1].end) {
> -			phys_addr_t end = base + ggtt->stolen_size -
> 1;
> +			dma_addr_t end = base + ggtt->stolen_size -
> 1;
>  
>  			DRM_DEBUG_KMS("GTT within stolen memory at
> 0x%llx-0x%llx\n",
>  				      (unsigned long
> long)ggtt_start,
>  				      (unsigned long long)ggtt_end -
> 1);
> -			DRM_DEBUG_KMS("Stolen memory adjusted to %pa
> - %pa\n",
> +			DRM_DEBUG_KMS("Stolen memory adjusted to
> %pad - %pad\n",
>  				      &base, &end);
>  		}
>  	}
> @@ -263,9 +263,9 @@ static phys_addr_t i915_stolen_to_physical(struct
> drm_i915_private *dev_priv)
>  		 * range. Apparently this works.
>  		 */
>  		if (r == NULL && !IS_GEN3(dev_priv)) {
> -			phys_addr_t end = base + ggtt->stolen_size;
> +			dma_addr_t end = base + ggtt->stolen_size;
>  
> -			DRM_ERROR("conflict detected with stolen
> region: [%pa - %pa]\n",
> +			DRM_ERROR("conflict detected with stolen
> region: [%pad - %pad]\n",
>  				  &base, &end);
>  			base = 0;
>  		}
> @@ -285,13 +285,13 @@ void i915_gem_cleanup_stolen(struct drm_device
> *dev)
>  }
>  
>  static void g4x_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				    phys_addr_t *base, u32 *size)
> +				    dma_addr_t *base, u32 *size)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	uint32_t reg_val = I915_READ(IS_GM45(dev_priv) ?
>  				     CTG_STOLEN_RESERVED :
>  				     ELK_STOLEN_RESERVED);
> -	phys_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> >stolen_size;
> +	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> >stolen_size;
>  
>  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
>  
> @@ -308,7 +308,7 @@ static void g4x_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void gen6_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				     phys_addr_t *base, u32 *size)
> +				     dma_addr_t *base, u32 *size)
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> @@ -334,7 +334,7 @@ static void gen6_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void gen7_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				     phys_addr_t *base, u32 *size)
> +				     dma_addr_t *base, u32 *size)
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> @@ -354,7 +354,7 @@ static void gen7_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void chv_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				    phys_addr_t *base, u32 *size)
> +				    dma_addr_t *base, u32 *size)
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> @@ -380,11 +380,11 @@ static void chv_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  }
>  
>  static void bdw_get_stolen_reserved(struct drm_i915_private
> *dev_priv,
> -				    phys_addr_t *base, u32 *size)
> +				    dma_addr_t *base, u32 *size)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> -	phys_addr_t stolen_top;
> +	dma_addr_t stolen_top;
>  
>  	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
>  
> @@ -403,7 +403,7 @@ static void bdw_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  {
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> -	phys_addr_t reserved_base, stolen_top;
> +	dma_addr_t reserved_base, stolen_top;
>  	u32 reserved_total, reserved_size;
>  	u32 stolen_usable_start;
>  
> @@ -424,7 +424,7 @@ int i915_gem_init_stolen(struct drm_i915_private
> *dev_priv)
>  	if (ggtt->stolen_size == 0)
>  		return 0;
>  
> -	dev_priv->mm.stolen_base =
> i915_stolen_to_physical(dev_priv);
> +	dev_priv->mm.stolen_base = i915_stolen_to_dma(dev_priv);
>  	if (dev_priv->mm.stolen_base == 0)
>  		return 0;
>  
> @@ -473,8 +473,8 @@ int i915_gem_init_stolen(struct drm_i915_private
> *dev_priv)
>  
>  	if (reserved_base < dev_priv->mm.stolen_base ||
>  	    reserved_base + reserved_size > stolen_top) {
> -		phys_addr_t reserved_top = reserved_base +
> reserved_size;
> -		DRM_DEBUG_KMS("Stolen reserved area [%pa - %pa]
> outside stolen memory [%pa - %pa]\n",
> +		dma_addr_t reserved_top = reserved_base +
> reserved_size;
> +		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad]
> outside stolen memory [%pad - %pad]\n",
>  			      &reserved_base, &reserved_top,
>  			      &dev_priv->mm.stolen_base,
> &stolen_top);
>  		return 0;
On Fri, Jan 27, 2017 at 06:50:54PM -0200, Paulo Zanoni wrote:
> Em Sex, 2017-01-27 às 16:55 +0000, Chris Wilson escreveu:
> > The conversion of stolen to use phys_addr_t (from essentially u32)
> > sparked an interesting discussion. We treat stolen memory as only
> > accessible from the GPU (the DMA device) - an attempt to use it from
> > the
> > CPU will generate a MCE on gen6 onwards, although it is in theory a
> > physical address that can be dereferenced from the CPU as
> > demonstrated
> > by earlier generations. As such, using phys_addr_t has the wrong
> > connotations and as we pass the address into the DMA device via
> > dma_addr_t (through the scatterlists used to program the GTT
> > entries),
> > we should treat it as dma_addr_t throughout.
> 
> I'm not a specialist here, but from what I could learn/understand, this
> seems good. The patch seems to do what it says, so:
> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> But now that I looked some more at the code, I started to think about
> the following: shouldn't we convert our "stolen offset" variables from
> u32 to dma_addr_t or some other appropriate type? I mean, if we ever
> get 64 bit stolen pointers we may also get 64 bit stolen offsets... The
> i915_ggtt struct contains 4 of such u32 pointers.
> 
> For example, i915_ggtt->stolen_reserved_base is another pointer to
> stolen memory, it's generated directly from one of our drm_addr_t
> variables.
> 
> Of course, this could be a separate patch.

Oh yes, I advise you take gentle steps down the rabbit hole.

I thought the reserved_base was an offset, I may be wrong. Certainly the
limits and types are also good to review and be wary of for the future.
Something I've only really recently started trying to do is capture
these assumed limits in the code by checking for type overflows -
hopefully to catch the errors early. That would be a useful exercise
when reviewing the current code to annotate where we change types. Isn't
there a gcc flag for warning on type shrinkage?
-Chris