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

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx