* [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.