| Message ID | 1485461947-16030-1-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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.
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
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?
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
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 >
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; } }
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(-)