All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
@ 2017-01-26 20:19 Paulo Zanoni
  2017-01-26 21:21 ` Chris Wilson
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Paulo Zanoni @ 2017-01-26 20:19 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni

The i915_stolen_to_physical() function has 'unsigned long' as its
return type but it returns the 'base' variable, which is of type
'u32'. The only place where this function is called assigns the
returned value to dev_priv->mm.stolen_base, which is of type
'phys_addr_t'. The return value is actually a physical address and
everything else in the stolen memory code seems to be using
phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 127d698..0816ebd 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 unsigned long i915_stolen_to_physical(struct drm_i915_private *dev_priv)
+static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 {
 	struct pci_dev *pdev = dev_priv->drm.pdev;
 	struct i915_ggtt *ggtt = &dev_priv->ggtt;
 	struct resource *r;
-	u32 base;
+	phys_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 unsigned long 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 {
-			u32 start, end;
+			phys_addr_t start, end;
 		} stolen[2] = {
 			{ .start = base, .end = base + ggtt->stolen_size, },
 			{ .start = base, .end = base + ggtt->stolen_size, },
@@ -228,11 +228,12 @@ static unsigned long 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;
 			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 0x%x-0x%x\n",
-				      base, base + (u32)ggtt->stolen_size - 1);
+			DRM_DEBUG_KMS("Stolen memory adjusted to %pa - %pa\n",
+				      &base, &end);
 		}
 	}
 
@@ -261,8 +262,9 @@ static unsigned long i915_stolen_to_physical(struct drm_i915_private *dev_priv)
 		 * range. Apparently this works.
 		 */
 		if (r == NULL && !IS_GEN3(dev_priv)) {
-			DRM_ERROR("conflict detected with stolen region: [0x%08x - 0x%08x]\n",
-				  base, base + (uint32_t)ggtt->stolen_size);
+			phys_addr_t end = base + ggtt->stolen_size;
+			DRM_ERROR("conflict detected with stolen region: [%pa - %pa]\n",
+				  &base, &end);
 			base = 0;
 		}
 	}
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-26 20:19 [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t Paulo Zanoni
@ 2017-01-26 21:21 ` Chris Wilson
  2017-01-27  0:54 ` ✓ Fi.CI.BAT: success for " Patchwork
  2017-01-27 13:59 ` [PATCH] " Ville Syrjälä
  2 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2017-01-26 21:21 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> The i915_stolen_to_physical() function has 'unsigned long' as its
> return type but it returns the 'base' variable, which is of type
> 'u32'. The only place where this function is called assigns the
> returned value to dev_priv->mm.stolen_base, which is of type
> 'phys_addr_t'. The return value is actually a physical address and
> everything else in the stolen memory code seems to be using
> phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.

My fault for missing this in my recent round of switching to
phys_addr_t.

> @@ -228,11 +228,12 @@ static unsigned long 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;
>  			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 0x%x-0x%x\n",
> -				      base, base + (u32)ggtt->stolen_size - 1);
> +			DRM_DEBUG_KMS("Stolen memory adjusted to %pa - %pa\n",
> +				      &base, &end);
>  		}
>  	}
>  
> @@ -261,8 +262,9 @@ static unsigned long i915_stolen_to_physical(struct drm_i915_private *dev_priv)
>  		 * range. Apparently this works.
>  		 */
>  		if (r == NULL && !IS_GEN3(dev_priv)) {
> -			DRM_ERROR("conflict detected with stolen region: [0x%08x - 0x%08x]\n",
> -				  base, base + (uint32_t)ggtt->stolen_size);
> +			phys_addr_t end = base + ggtt->stolen_size;

Could make checkpatch happy by leaving a blank line here (and above). Maybe
overkill.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-26 20:19 [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t Paulo Zanoni
  2017-01-26 21:21 ` Chris Wilson
@ 2017-01-27  0:54 ` Patchwork
  2017-01-27 13:59 ` [PATCH] " Ville Syrjälä
  2 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-01-27  0:54 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: make i915_stolen_to_physical() return phys_addr_t
URL   : https://patchwork.freedesktop.org/series/18640/
State : success

== Summary ==

Series 18640v1 drm/i915: make i915_stolen_to_physical() return phys_addr_t
https://patchwork.freedesktop.org/api/1.0/series/18640/revisions/1/mbox/

Test gem_close_race:
        Subgroup basic-threads:
                incomplete -> PASS       (fi-bxt-j4205)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (fi-skl-6700k)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:225  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:222  dwarn:4   dfail:0   fail:0   skip:21 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

48cf644b4781ce8345c7506b719601c563982802 drm-tip: 2017y-01m-26d-18h-59m-50s UTC integration manifest
ada2e3e drm/i915: make i915_stolen_to_physical() return phys_addr_t

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3617/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-26 20:19 [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t Paulo Zanoni
  2017-01-26 21:21 ` Chris Wilson
  2017-01-27  0:54 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-01-27 13:59 ` Ville Syrjälä
  2017-01-27 14:20   ` Chris Wilson
  2017-01-27 15:44   ` Paulo Zanoni
  2 siblings, 2 replies; 10+ messages in thread
From: Ville Syrjälä @ 2017-01-27 13:59 UTC (permalink / raw)
  To: Paulo Zanoni; +Cc: intel-gfx

On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> The i915_stolen_to_physical() function has 'unsigned long' as its
> return type but it returns the 'base' variable, which is of type
> 'u32'. The only place where this function is called assigns the
> returned value to dev_priv->mm.stolen_base, which is of type
> 'phys_addr_t'. The return value is actually a physical address and
> everything else in the stolen memory code seems to be using
> phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.

Size of phys_addr_t depends on PAE no? So what if someone were to boot
a !PAE kernel on a machine where stolen lives somewhere >4GiB?

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 127d698..0816ebd 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 unsigned long i915_stolen_to_physical(struct drm_i915_private *dev_priv)
> +static phys_addr_t i915_stolen_to_physical(struct drm_i915_private *dev_priv)
>  {
>  	struct pci_dev *pdev = dev_priv->drm.pdev;
>  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
>  	struct resource *r;
> -	u32 base;
> +	phys_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 unsigned long 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 {
> -			u32 start, end;
> +			phys_addr_t start, end;
>  		} stolen[2] = {
>  			{ .start = base, .end = base + ggtt->stolen_size, },
>  			{ .start = base, .end = base + ggtt->stolen_size, },
> @@ -228,11 +228,12 @@ static unsigned long 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;
>  			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 0x%x-0x%x\n",
> -				      base, base + (u32)ggtt->stolen_size - 1);
> +			DRM_DEBUG_KMS("Stolen memory adjusted to %pa - %pa\n",
> +				      &base, &end);
>  		}
>  	}
>  
> @@ -261,8 +262,9 @@ static unsigned long i915_stolen_to_physical(struct drm_i915_private *dev_priv)
>  		 * range. Apparently this works.
>  		 */
>  		if (r == NULL && !IS_GEN3(dev_priv)) {
> -			DRM_ERROR("conflict detected with stolen region: [0x%08x - 0x%08x]\n",
> -				  base, base + (uint32_t)ggtt->stolen_size);
> +			phys_addr_t end = base + ggtt->stolen_size;
> +			DRM_ERROR("conflict detected with stolen region: [%pa - %pa]\n",
> +				  &base, &end);
>  			base = 0;
>  		}
>  	}
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-27 13:59 ` [PATCH] " Ville Syrjälä
@ 2017-01-27 14:20   ` Chris Wilson
  2017-01-27 14:38     ` Ville Syrjälä
  2017-01-27 15:44   ` Paulo Zanoni
  1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-01-27 14:20 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni

On Fri, Jan 27, 2017 at 03:59:19PM +0200, Ville Syrjälä wrote:
> On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> > The i915_stolen_to_physical() function has 'unsigned long' as its
> > return type but it returns the 'base' variable, which is of type
> > 'u32'. The only place where this function is called assigns the
> > returned value to dev_priv->mm.stolen_base, which is of type
> > 'phys_addr_t'. The return value is actually a physical address and
> > everything else in the stolen memory code seems to be using
> > phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.
> 
> Size of phys_addr_t depends on PAE no? So what if someone were to boot
> a !PAE kernel on a machine where stolen lives somewhere >4GiB?

dma_addr_t should be correct there, right? And in effect we do regard
this as only dma accessible, so the white lie would have some nice
semantic benefits.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-27 14:20   ` Chris Wilson
@ 2017-01-27 14:38     ` Ville Syrjälä
  2017-01-27 14:42       ` Chris Wilson
  0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2017-01-27 14:38 UTC (permalink / raw)
  To: Chris Wilson, Paulo Zanoni, intel-gfx

On Fri, Jan 27, 2017 at 02:20:52PM +0000, Chris Wilson wrote:
> On Fri, Jan 27, 2017 at 03:59:19PM +0200, Ville Syrjälä wrote:
> > On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> > > The i915_stolen_to_physical() function has 'unsigned long' as its
> > > return type but it returns the 'base' variable, which is of type
> > > 'u32'. The only place where this function is called assigns the
> > > returned value to dev_priv->mm.stolen_base, which is of type
> > > 'phys_addr_t'. The return value is actually a physical address and
> > > everything else in the stolen memory code seems to be using
> > > phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.
> > 
> > Size of phys_addr_t depends on PAE no? So what if someone were to boot
> > a !PAE kernel on a machine where stolen lives somewhere >4GiB?
> 
> dma_addr_t should be correct there, right? And in effect we do regard
> this as only dma accessible, so the white lie would have some nice
> semantic benefits.

config ARCH_PHYS_ADDR_T_64BIT
        def_bool y
        depends on X86_64 || X86_PAE

config ARCH_DMA_ADDR_T_64BIT
        def_bool y
        depends on X86_64 || HIGHMEM64G

So looks like the size of dma_addr_t also depends on the config.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-27 14:38     ` Ville Syrjälä
@ 2017-01-27 14:42       ` Chris Wilson
  2017-01-27 15:06         ` Ville Syrjälä
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-01-27 14:42 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni

On Fri, Jan 27, 2017 at 04:38:47PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 27, 2017 at 02:20:52PM +0000, Chris Wilson wrote:
> > On Fri, Jan 27, 2017 at 03:59:19PM +0200, Ville Syrjälä wrote:
> > > On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> > > > The i915_stolen_to_physical() function has 'unsigned long' as its
> > > > return type but it returns the 'base' variable, which is of type
> > > > 'u32'. The only place where this function is called assigns the
> > > > returned value to dev_priv->mm.stolen_base, which is of type
> > > > 'phys_addr_t'. The return value is actually a physical address and
> > > > everything else in the stolen memory code seems to be using
> > > > phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.
> > > 
> > > Size of phys_addr_t depends on PAE no? So what if someone were to boot
> > > a !PAE kernel on a machine where stolen lives somewhere >4GiB?
> > 
> > dma_addr_t should be correct there, right? And in effect we do regard
> > this as only dma accessible, so the white lie would have some nice
> > semantic benefits.
> 
> config ARCH_PHYS_ADDR_T_64BIT
>         def_bool y
>         depends on X86_64 || X86_PAE
> 
> config ARCH_DMA_ADDR_T_64BIT
>         def_bool y
>         depends on X86_64 || HIGHMEM64G
> 
> So looks like the size of dma_addr_t also depends on the config.

We are dependent upon dma_addr_t (for transporting the addresses to the
GTT), so use it and stick a warn or build bug if it ever comes up short?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-27 14:42       ` Chris Wilson
@ 2017-01-27 15:06         ` Ville Syrjälä
  2017-01-27 15:16           ` Chris Wilson
  0 siblings, 1 reply; 10+ messages in thread
From: Ville Syrjälä @ 2017-01-27 15:06 UTC (permalink / raw)
  To: Chris Wilson, Paulo Zanoni, intel-gfx

On Fri, Jan 27, 2017 at 02:42:40PM +0000, Chris Wilson wrote:
> On Fri, Jan 27, 2017 at 04:38:47PM +0200, Ville Syrjälä wrote:
> > On Fri, Jan 27, 2017 at 02:20:52PM +0000, Chris Wilson wrote:
> > > On Fri, Jan 27, 2017 at 03:59:19PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> > > > > The i915_stolen_to_physical() function has 'unsigned long' as its
> > > > > return type but it returns the 'base' variable, which is of type
> > > > > 'u32'. The only place where this function is called assigns the
> > > > > returned value to dev_priv->mm.stolen_base, which is of type
> > > > > 'phys_addr_t'. The return value is actually a physical address and
> > > > > everything else in the stolen memory code seems to be using
> > > > > phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.
> > > > 
> > > > Size of phys_addr_t depends on PAE no? So what if someone were to boot
> > > > a !PAE kernel on a machine where stolen lives somewhere >4GiB?
> > > 
> > > dma_addr_t should be correct there, right? And in effect we do regard
> > > this as only dma accessible, so the white lie would have some nice
> > > semantic benefits.
> > 
> > config ARCH_PHYS_ADDR_T_64BIT
> >         def_bool y
> >         depends on X86_64 || X86_PAE
> > 
> > config ARCH_DMA_ADDR_T_64BIT
> >         def_bool y
> >         depends on X86_64 || HIGHMEM64G
> > 
> > So looks like the size of dma_addr_t also depends on the config.
> 
> We are dependent upon dma_addr_t (for transporting the addresses to the
> GTT), so use it and stick a warn or build bug if it ever comes up short?

Needs a runtime check since the address comes from the firmware.
But I guess we could just check whether it'll fit, and if not we
simply don't use stolen?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-27 15:06         ` Ville Syrjälä
@ 2017-01-27 15:16           ` Chris Wilson
  0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2017-01-27 15:16 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx, Paulo Zanoni

On Fri, Jan 27, 2017 at 05:06:53PM +0200, Ville Syrjälä wrote:
> On Fri, Jan 27, 2017 at 02:42:40PM +0000, Chris Wilson wrote:
> > On Fri, Jan 27, 2017 at 04:38:47PM +0200, Ville Syrjälä wrote:
> > > On Fri, Jan 27, 2017 at 02:20:52PM +0000, Chris Wilson wrote:
> > > > On Fri, Jan 27, 2017 at 03:59:19PM +0200, Ville Syrjälä wrote:
> > > > > On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> > > > > > The i915_stolen_to_physical() function has 'unsigned long' as its
> > > > > > return type but it returns the 'base' variable, which is of type
> > > > > > 'u32'. The only place where this function is called assigns the
> > > > > > returned value to dev_priv->mm.stolen_base, which is of type
> > > > > > 'phys_addr_t'. The return value is actually a physical address and
> > > > > > everything else in the stolen memory code seems to be using
> > > > > > phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.
> > > > > 
> > > > > Size of phys_addr_t depends on PAE no? So what if someone were to boot
> > > > > a !PAE kernel on a machine where stolen lives somewhere >4GiB?
> > > > 
> > > > dma_addr_t should be correct there, right? And in effect we do regard
> > > > this as only dma accessible, so the white lie would have some nice
> > > > semantic benefits.
> > > 
> > > config ARCH_PHYS_ADDR_T_64BIT
> > >         def_bool y
> > >         depends on X86_64 || X86_PAE
> > > 
> > > config ARCH_DMA_ADDR_T_64BIT
> > >         def_bool y
> > >         depends on X86_64 || HIGHMEM64G
> > > 
> > > So looks like the size of dma_addr_t also depends on the config.
> > 
> > We are dependent upon dma_addr_t (for transporting the addresses to the
> > GTT), so use it and stick a warn or build bug if it ever comes up short?
> 
> Needs a runtime check since the address comes from the firmware.
> But I guess we could just check whether it'll fit, and if not we
> simply don't use stolen?

That's what I was thinking as well. System comes up and the user may
never even notice the error message.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t
  2017-01-27 13:59 ` [PATCH] " Ville Syrjälä
  2017-01-27 14:20   ` Chris Wilson
@ 2017-01-27 15:44   ` Paulo Zanoni
  1 sibling, 0 replies; 10+ messages in thread
From: Paulo Zanoni @ 2017-01-27 15:44 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

Em Sex, 2017-01-27 às 15:59 +0200, Ville Syrjälä escreveu:
> On Thu, Jan 26, 2017 at 06:19:07PM -0200, Paulo Zanoni wrote:
> > 
> > The i915_stolen_to_physical() function has 'unsigned long' as its
> > return type but it returns the 'base' variable, which is of type
> > 'u32'. The only place where this function is called assigns the
> > returned value to dev_priv->mm.stolen_base, which is of type
> > 'phys_addr_t'. The return value is actually a physical address and
> > everything else in the stolen memory code seems to be using
> > phys_addr_t, so fix i915_stolen_to_physical() to use phys_addr_t.
> 
> Size of phys_addr_t depends on PAE no? So what if someone were to
> boot
> a !PAE kernel on a machine where stolen lives somewhere >4GiB?

Notice that this should not be possible today since we read the stolen
memory address from a 32 bit register. Of course, things may change in
the future, so having future-proof code is obviously good.


> 
> > 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 127d698..0816ebd 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 unsigned long i915_stolen_to_physical(struct
> > drm_i915_private *dev_priv)
> > +static phys_addr_t i915_stolen_to_physical(struct drm_i915_private
> > *dev_priv)
> >  {
> >  	struct pci_dev *pdev = dev_priv->drm.pdev;
> >  	struct i915_ggtt *ggtt = &dev_priv->ggtt;
> >  	struct resource *r;
> > -	u32 base;
> > +	phys_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 unsigned long
> > 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 {
> > -			u32 start, end;
> > +			phys_addr_t start, end;
> >  		} stolen[2] = {
> >  			{ .start = base, .end = base + ggtt-
> > >stolen_size, },
> >  			{ .start = base, .end = base + ggtt-
> > >stolen_size, },
> > @@ -228,11 +228,12 @@ static unsigned long
> > 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;
> >  			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
> > 0x%x-0x%x\n",
> > -				      base, base + (u32)ggtt-
> > >stolen_size - 1);
> > +			DRM_DEBUG_KMS("Stolen memory adjusted to
> > %pa - %pa\n",
> > +				      &base, &end);
> >  		}
> >  	}
> >  
> > @@ -261,8 +262,9 @@ static unsigned long
> > i915_stolen_to_physical(struct drm_i915_private *dev_priv)
> >  		 * range. Apparently this works.
> >  		 */
> >  		if (r == NULL && !IS_GEN3(dev_priv)) {
> > -			DRM_ERROR("conflict detected with stolen
> > region: [0x%08x - 0x%08x]\n",
> > -				  base, base + (uint32_t)ggtt-
> > >stolen_size);
> > +			phys_addr_t end = base + ggtt-
> > >stolen_size;
> > +			DRM_ERROR("conflict detected with stolen
> > region: [%pa - %pa]\n",
> > +				  &base, &end);
> >  			base = 0;
> >  		}
> >  	}
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-01-27 15:44 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-26 20:19 [PATCH] drm/i915: make i915_stolen_to_physical() return phys_addr_t Paulo Zanoni
2017-01-26 21:21 ` Chris Wilson
2017-01-27  0:54 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-01-27 13:59 ` [PATCH] " Ville Syrjälä
2017-01-27 14:20   ` Chris Wilson
2017-01-27 14:38     ` Ville Syrjälä
2017-01-27 14:42       ` Chris Wilson
2017-01-27 15:06         ` Ville Syrjälä
2017-01-27 15:16           ` Chris Wilson
2017-01-27 15:44   ` Paulo Zanoni

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.