[PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Chris Wilson]

So I made the mistake of missing that the desktop and mobile chipsets
have different layouts in their PCI configurations, and we were
incorrectly setting the wrong physical address for stolen memory on
mobile chipsets.

Since all gen3+ are actually consistent in the location of the GBSM
register in the PCI configuration space on device 2 (the GPU), use it.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 43 ++++++++++------------------------
 1 file changed, 12 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 8e02344..5a97b3c 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -44,45 +44,26 @@
 
 static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct pci_dev *pdev = dev_priv->bridge_dev;
 	u32 base;
 
-	/* On the machines I have tested the Graphics Base of Stolen Memory
-	 * is unreliable, so on those compute the base by subtracting the
-	 * stolen memory from the Top of Low Usable DRAM which is where the
-	 * BIOS places the graphics stolen memory.
-	 *
-	 * On gen2, the layout is slightly different with the Graphics Segment
-	 * immediately following Top of Memory (or Top of Usable DRAM). Note
-	 * it appears that TOUD is only reported by 865g, so we just use the
-	 * top of memory as determined by the e820 probe.
+	/* Almost universally we can find the Graphics Base of Stolen Memory
+	 * at offset 0x5c in the igfx configuration space. On a few (desktop)
+	 * machines this is also mirrored in the bridge device at different
+	 * locations, or in the MCHBAR. On gen2, the layout is again slightly
+	 * different with the Graphics Segment immediately following Top of
+	 * Memory (or Top of Usable DRAM). Note it appears that TOUD is only
+	 * reported by 865g, so we just use the top of memory as determined
+	 * by the e820 probe.
 	 *
-	 * XXX gen2 requires an unavailable symbol and 945gm fails with
-	 * its value of TOLUD.
+	 * XXX However gen2 requires an unavailable symbol.
 	 */
 	base = 0;
-	if (IS_VALLEYVIEW(dev)) {
+	if (INTEL_INFO(dev)->gen >= 3) {
+		/* Read Graphics Base of Stolen Memory directly */
 		pci_read_config_dword(dev->pdev, 0x5c, &base);
 		base &= ~((1<<20) - 1);
-	} else if (INTEL_INFO(dev)->gen >= 6) {
-		/* Read Base Data of Stolen Memory Register (BDSM) directly.
-		 * Note that there is also a MCHBAR miror at 0x1080c0 or
-		 * we could use device 2:0x5c instead.
-		*/
-		pci_read_config_dword(pdev, 0xB0, &base);
-		base &= ~4095; /* lower bits used for locking register */
-	} else if (INTEL_INFO(dev)->gen > 3 || IS_G33(dev)) {
-		/* Read Graphics Base of Stolen Memory directly */
-		pci_read_config_dword(pdev, 0xA4, &base);
+	} else { /* GEN2 */
 #if 0
-	} else if (IS_GEN3(dev)) {
-		u8 val;
-		/* Stolen is immediately below Top of Low Usable DRAM */
-		pci_read_config_byte(pdev, 0x9c, &val);
-		base = val >> 3 << 27;
-		base -= dev_priv->mm.gtt->stolen_size;
-	} else {
 		/* Stolen is immediately above Top of Memory */
 		base = max_low_pfn_mapped << PAGE_SHIFT;
 #endif
-- 
1.8.3.2

Re: [PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Daniel Vetter]

On Thu, Jul 4, 2013 at 1:23 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> So I made the mistake of missing that the desktop and mobile chipsets
> have different layouts in their PCI configurations, and we were
> incorrectly setting the wrong physical address for stolen memory on
> mobile chipsets.
>
> Since all gen3+ are actually consistent in the location of the GBSM
> register in the PCI configuration space on device 2 (the GPU), use it.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org

Nope, not cc: stable since the last time around the overlay blew up in
flames ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Chris Wilson]

On Thu, Jul 04, 2013 at 01:26:03AM +0200, Daniel Vetter wrote:
> On Thu, Jul 4, 2013 at 1:23 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > So I made the mistake of missing that the desktop and mobile chipsets
> > have different layouts in their PCI configurations, and we were
> > incorrectly setting the wrong physical address for stolen memory on
> > mobile chipsets.
> >
> > Since all gen3+ are actually consistent in the location of the GBSM
> > register in the PCI configuration space on device 2 (the GPU), use it.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: stable@vger.kernel.org
> 
> Nope, not cc: stable since the last time around the overlay blew up in
> flames ...

You can comment out gen3, but the dangerous part is that we are
overwriting random physical addresses.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Daniel Vetter]

On Thu, Jul 4, 2013 at 1:33 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Jul 04, 2013 at 01:26:03AM +0200, Daniel Vetter wrote:
>> On Thu, Jul 4, 2013 at 1:23 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > So I made the mistake of missing that the desktop and mobile chipsets
>> > have different layouts in their PCI configurations, and we were
>> > incorrectly setting the wrong physical address for stolen memory on
>> > mobile chipsets.
>> >
>> > Since all gen3+ are actually consistent in the location of the GBSM
>> > register in the PCI configuration space on device 2 (the GPU), use it.
>> >
>> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Cc: stable@vger.kernel.org
>>
>> Nope, not cc: stable since the last time around the overlay blew up in
>> flames ...
>
> You can comment out gen3, but the dangerous part is that we are
> overwriting random physical addresses.

Hm, should we do a request_region on the stolen range to double-check
that? Just for paranoia and in case the BIOS does something terrible
...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Chris Wilson]

On Thu, Jul 04, 2013 at 01:38:05AM +0200, Daniel Vetter wrote:
> On Thu, Jul 4, 2013 at 1:33 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Jul 04, 2013 at 01:26:03AM +0200, Daniel Vetter wrote:
> >> On Thu, Jul 4, 2013 at 1:23 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > So I made the mistake of missing that the desktop and mobile chipsets
> >> > have different layouts in their PCI configurations, and we were
> >> > incorrectly setting the wrong physical address for stolen memory on
> >> > mobile chipsets.
> >> >
> >> > Since all gen3+ are actually consistent in the location of the GBSM
> >> > register in the PCI configuration space on device 2 (the GPU), use it.
> >> >
> >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > Cc: stable@vger.kernel.org
> >>
> >> Nope, not cc: stable since the last time around the overlay blew up in
> >> flames ...
> >
> > You can comment out gen3, but the dangerous part is that we are
> > overwriting random physical addresses.
> 
> Hm, should we do a request_region on the stolen range to double-check
> that? Just for paranoia and in case the BIOS does something terrible
> ...

Afaict, request_region() is only being used to reserve and check for
conflicting io ranges. I don't know if that will give us protection
against overwriting user/kernel memory. Maybe it does - I haven't found
the documentation for it yet.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Daniel Vetter]

On Thu, Jul 04, 2013 at 12:55:08AM +0100, Chris Wilson wrote:
> On Thu, Jul 04, 2013 at 01:38:05AM +0200, Daniel Vetter wrote:
> > On Thu, Jul 4, 2013 at 1:33 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Thu, Jul 04, 2013 at 01:26:03AM +0200, Daniel Vetter wrote:
> > >> On Thu, Jul 4, 2013 at 1:23 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > >> > So I made the mistake of missing that the desktop and mobile chipsets
> > >> > have different layouts in their PCI configurations, and we were
> > >> > incorrectly setting the wrong physical address for stolen memory on
> > >> > mobile chipsets.
> > >> >
> > >> > Since all gen3+ are actually consistent in the location of the GBSM
> > >> > register in the PCI configuration space on device 2 (the GPU), use it.
> > >> >
> > >> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > >> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > >> > Cc: stable@vger.kernel.org
> > >>
> > >> Nope, not cc: stable since the last time around the overlay blew up in
> > >> flames ...
> > >
> > > You can comment out gen3, but the dangerous part is that we are
> > > overwriting random physical addresses.
> > 
> > Hm, should we do a request_region on the stolen range to double-check
> > that? Just for paranoia and in case the BIOS does something terrible
> > ...
> 
> Afaict, request_region() is only being used to reserve and check for
> conflicting io ranges. I don't know if that will give us protection
> against overwriting user/kernel memory. Maybe it does - I haven't found
> the documentation for it yet.

On a quick reading (I didn't really try it out in practice) the e820 code
inserts resource nodes for each memory segment into the resource tree. So
I think this should indeed work out. At least request_mem_region seems to
check for IORESOURCE_BUSY and e820_reserve_resources seems to set that on
all memory regions managed by the kernel (if I read the magic checks
correctly).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Verify that our stolen memory doesn't conflict

[Chris Wilson]

Sanity check that the memory region found through the Graphics Base
of Stolen Memory is reserved and hidden from the rest of the system
through the use of the resource API.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 5a97b3c..f110d3e 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -44,6 +44,8 @@
 
 static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct resource *r;
 	u32 base;
 
 	/* Almost universally we can find the Graphics Base of Stolen Memory
@@ -69,6 +71,21 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 #endif
 	}
 
+	if (base == 0)
+		return 0;
+
+	/* Verify that nothing else uses this physical address. Stolen
+	 * memory should be reserved by the BIOS and hidden from the
+	 * kernel. So if the region is already marked as busy, something
+	 * is seriously wrong.
+	 */
+	r = request_mem_region(base, dev_priv->gtt.stolen_size, "i915 stolen");
+	if (r == NULL) {
+		DRM_ERROR("conflict detected with stolen region: [%u - %u]\n",
+			  base, base + (uint32_t)dev_priv->gtt.stolen_size);
+		base = 0;
+	}
+
 	return base;
 }
 
@@ -162,6 +179,10 @@ void i915_gem_cleanup_stolen(struct drm_device *dev)
 
 	i915_gem_stolen_cleanup_compression(dev);
 	drm_mm_takedown(&dev_priv->mm.stolen);
+
+	if (dev_priv->mm.stolen_base != 0)
+		release_mem_region(dev_priv->mm.stolen_base,
+				   dev_priv->gtt.stolen_size);
 }
 
 int i915_gem_init_stolen(struct drm_device *dev)
-- 
1.8.3.2

Re: [PATCH] drm/i915: Verify that our stolen memory doesn't conflict

[Daniel Vetter]

On Thu, Jul 4, 2013 at 12:53 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Sanity check that the memory region found through the Graphics Base
> of Stolen Memory is reserved and hidden from the rest of the system
> through the use of the resource API.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Two little things:
- I think we should give this special resource block a proper name
(since it's rather elusive to the system), e.g. rename_region(r,
"Graphics Stolen Memory");
- This looks like a nice opportunity to start using the devres stuff
to bring some sanity to our teardown/cleanup code. It can only really
handle things which can be release exactly in reverse order than
they've been acquire. So it won't cover things with tricky ordering
constraints, but it should be a start. I.e.
s/request_mem_region/devm_request_mem_region/ and then drop the
cleanup code.

Cheers, Daniel

--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: Verify that our stolen memory doesn't conflict

[Chris Wilson]

Sanity check that the memory region found through the Graphics Base
of Stolen Memory is reserved and hidden from the rest of the system
through the use of the resource API.

v2: "Graphics Stolen Memory" is such a more bodacious name than the lame
    "i915 stolen", and convert to using devres for automagical cleanup of
    the resource. (danvet)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 5a97b3c..cc7dadf 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -44,6 +44,8 @@
 
 static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct resource *r;
 	u32 base;
 
 	/* Almost universally we can find the Graphics Base of Stolen Memory
@@ -69,6 +71,22 @@ static unsigned long i915_stolen_to_physical(struct drm_device *dev)
 #endif
 	}
 
+	if (base == 0)
+		return 0;
+
+	/* Verify that nothing else uses this physical address. Stolen
+	 * memory should be reserved by the BIOS and hidden from the
+	 * kernel. So if the region is already marked as busy, something
+	 * is seriously wrong.
+	 */
+	r = devm_request_mem_region(dev->dev, base, dev_priv->gtt.stolen_size,
+				    "Graphics Stolen Memory");
+	if (r == NULL) {
+		DRM_ERROR("conflict detected with stolen region: [%u - %u]\n",
+			  base, base + (uint32_t)dev_priv->gtt.stolen_size);
+		base = 0;
+	}
+
 	return base;
 }
 
-- 
1.8.3.2

Re: [PATCH] drm/i915: Verify that our stolen memory doesn't conflict

[Daniel Vetter]

On Thu, Jul 04, 2013 at 12:28:35PM +0100, Chris Wilson wrote:
> Sanity check that the memory region found through the Graphics Base
> of Stolen Memory is reserved and hidden from the rest of the system
> through the use of the resource API.
> 
> v2: "Graphics Stolen Memory" is such a more bodacious name than the lame
>     "i915 stolen", and convert to using devres for automagical cleanup of
>     the resource. (danvet)
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

Picked up for -fixes, thanks for the patch. Let's see what happens ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Verify that our stolen memory doesn't conflict

[Paulo Zanoni]

2013/7/4 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Jul 04, 2013 at 12:28:35PM +0100, Chris Wilson wrote:
>> Sanity check that the memory region found through the Graphics Base
>> of Stolen Memory is reserved and hidden from the rest of the system
>> through the use of the resource API.
>>
>> v2: "Graphics Stolen Memory" is such a more bodacious name than the lame
>>     "i915 stolen", and convert to using devres for automagical cleanup of
>>     the resource. (danvet)
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Picked up for -fixes, thanks for the patch. Let's see what happens ...

This happens:

pzanoni@sopa:~$ dmesg | grep stolen
[    2.025951] [drm:i915_gem_gtt_init], GTT stolen size = 128M
[    2.059755] [drm:i915_stolen_to_physical] *ERROR* conflict detected
with stolen region: [2812280832 - 2946498560]

As a consequence, FBC refuses to load. I'm not sure if this is a
bug... Is this expected? Is there any way I can help debug this?


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: Verify that our stolen memory doesn't conflict

[Daniel Vetter]

On Fri, Jul 05, 2013 at 06:34:46PM -0300, Paulo Zanoni wrote:
> 2013/7/4 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Jul 04, 2013 at 12:28:35PM +0100, Chris Wilson wrote:
> >> Sanity check that the memory region found through the Graphics Base
> >> of Stolen Memory is reserved and hidden from the rest of the system
> >> through the use of the resource API.
> >>
> >> v2: "Graphics Stolen Memory" is such a more bodacious name than the lame
> >>     "i915 stolen", and convert to using devres for automagical cleanup of
> >>     the resource. (danvet)
> >>
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > Picked up for -fixes, thanks for the patch. Let's see what happens ...
> 
> This happens:
> 
> pzanoni@sopa:~$ dmesg | grep stolen
> [    2.025951] [drm:i915_gem_gtt_init], GTT stolen size = 128M
> [    2.059755] [drm:i915_stolen_to_physical] *ERROR* conflict detected
> with stolen region: [2812280832 - 2946498560]
> 
> As a consequence, FBC refuses to load. I'm not sure if this is a
> bug... Is this expected? Is there any way I can help debug this?

I guess we need the e820 map from dmesg and compare which part is blocking
the stolen range ... Can you please attach your full dmesg?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Use Graphics Base of Stolen Memory on all gen3+

[Daniel Vetter]

On Thu, Jul 04, 2013 at 12:23:33AM +0100, Chris Wilson wrote:
> So I made the mistake of missing that the desktop and mobile chipsets
> have different layouts in their PCI configurations, and we were
> incorrectly setting the wrong physical address for stolen memory on
> mobile chipsets.
> 
> Since all gen3+ are actually consistent in the location of the GBSM
> register in the PCI configuration space on device 2 (the GPU), use it.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: stable@vger.kernel.org

Ok, with the quick overlay ducttape I've just submitted this should be
safe. Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch