[PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Apparently there are some machines that put semi-sensible looking values
into the stolen "reserved" base and size, except those values are actually
outside the stolen memory. There is a bit in the register which
supposedly could tell us whether the reserved area is even enabled or
not. Let's check for that before we go trusting the base and size.

Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 30 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h        |  2 ++
 2 files changed, 32 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 03e7abc7e043..44a5dbc8c23b 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -294,6 +294,12 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
 				     ELK_STOLEN_RESERVED);
 	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
 
+	if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0) {
+		*base = 0;
+		*size = 0;
+		return;
+	}
+
 	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
 
 	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
@@ -313,6 +319,12 @@ static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
+	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
+		*base = 0;
+		*size = 0;
+		return;
+	}
+
 	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
 
 	switch (reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK) {
@@ -339,6 +351,12 @@ static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
+	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
+		*base = 0;
+		*size = 0;
+		return;
+	}
+
 	*base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
 
 	switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
@@ -359,6 +377,12 @@ static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv,
 {
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 
+	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
+		*base = 0;
+		*size = 0;
+		return;
+	}
+
 	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
 
 	switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) {
@@ -387,6 +411,12 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
 	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
 	dma_addr_t stolen_top;
 
+	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
+		*base = 0;
+		*size = 0;
+		return;
+	}
+
 	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
 
 	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index f0f8f6059652..dc2cbc428d1b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -382,6 +382,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t reg)
 #define GEN8_STOLEN_RESERVED_2M		(1 << 7)
 #define GEN8_STOLEN_RESERVED_4M		(2 << 7)
 #define GEN8_STOLEN_RESERVED_8M		(3 << 7)
+#define GEN6_STOLEN_RESERVED_ENABLE	(1 << 0)
 
 /* VGA stuff */
 
@@ -3398,6 +3399,7 @@ enum i915_power_well_id {
 #define ELK_STOLEN_RESERVED		_MMIO(MCHBAR_MIRROR_BASE + 0x48)
 #define G4X_STOLEN_RESERVED_ADDR1_MASK	(0xFFFF << 16)
 #define G4X_STOLEN_RESERVED_ADDR2_MASK	(0xFFF << 4)
+#define G4X_STOLEN_RESERVED_ENABLE	(1 << 0)
 
 /* Memory controller frequency in MCHBAR for Haswell (possible SNB+) */
 #define DCLK _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5e04)
-- 
2.13.6

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

[PATCH 2/3] drm/i915: Make the report about a bogus stolen reserved area an error

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Now that we should be properly filtering out the cases when the stolen
reserved area is disabled, let's convert the debug message about a
misplaced reserved area into an error.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 44a5dbc8c23b..4f9377b5f4ae 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -503,9 +503,9 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 	if (reserved_base < dev_priv->mm.stolen_base ||
 	    reserved_base + reserved_size > stolen_top) {
 		dma_addr_t reserved_top = reserved_base + reserved_size;
-		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad] outside stolen memory [%pad - %pad]\n",
-			      &reserved_base, &reserved_top,
-			      &dev_priv->mm.stolen_base, &stolen_top);
+		DRM_ERROR("Stolen reserved area [%pad - %pad] outside stolen memory [%pad - %pad]\n",
+			  &reserved_base, &reserved_top,
+			  &dev_priv->mm.stolen_base, &stolen_top);
 		return 0;
 	}
 
-- 
2.13.6

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

[PATCH 3/3] drm/i915: Use ELK stolen memory reserved detection for ILK

[Ville Syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

While I have no solid proof that ILK follows the ELK path when it
comes to the stolen memory reserved area, there are some hints that
it might be the case. Unfortunately my ILK doesn't have this enabled,
and no way to enable it via the BIOS it seems.

So let's have ILK use the ELK code path, and let's toss in a WARN
into the code to see if we catch anyone with an ILK that has this
enabled to further analyze the situation.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 4f9377b5f4ae..1877ae9a1d9b 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -300,6 +300,12 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
 		return;
 	}
 
+	/*
+	 * Whether ILK really reuses the ELK register for this is unclear.
+	 * Let's see if we catch anyone with this supposedly enabled on ILK.
+	 */
+	WARN(IS_GEN5(dev_priv), "ILK stolen reserved found? 0x%08x\n", reg_val);
+
 	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
 
 	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
@@ -466,14 +472,12 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 	case 3:
 		break;
 	case 4:
-		if (IS_G4X(dev_priv))
-			g4x_get_stolen_reserved(dev_priv,
-						&reserved_base, &reserved_size);
-		break;
+		if (!IS_G4X(dev_priv))
+			break;
+		/* fall through */
 	case 5:
-		/* Assume the gen6 maximum for the older platforms. */
-		reserved_size = 1024 * 1024;
-		reserved_base = stolen_top - reserved_size;
+		g4x_get_stolen_reserved(dev_priv,
+					&reserved_base, &reserved_size);
 		break;
 	case 6:
 		gen6_get_stolen_reserved(dev_priv,
-- 
2.13.6

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

✓ Fi.CI.BAT: success for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
URL   : https://patchwork.freedesktop.org/series/33060/
State : success

== Summary ==

Series 33060v1 series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
https://patchwork.freedesktop.org/api/1.0/series/33060/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                fail       -> PASS       (fi-kbl-7500u) fdo#102514
Test kms_cursor_legacy:
        Subgroup basic-flip-before-cursor-varying-size:
                skip       -> PASS       (fi-hsw-4770r)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                dmesg-warn -> PASS       (fi-skl-6700k) fdo#103546 +1

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#103546 https://bugs.freedesktop.org/show_bug.cgi?id=103546

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:443s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:453s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:379s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:553s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:277s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:511s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:515s
fi-byt-j1900     total:289  pass:253  dwarn:1   dfail:0   fail:0   skip:35  time:512s
fi-byt-n2820     total:289  pass:249  dwarn:1   dfail:0   fail:0   skip:39  time:495s
fi-cnl-y         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:598s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:436s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:272s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:586s
fi-glk-dsi       total:289  pass:258  dwarn:0   dfail:0   fail:1   skip:30  time:492s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:430s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:428s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:431s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:500s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:466s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:501s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:577s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:478s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:588s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:571s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:457s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:599s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:651s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:518s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:507s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:462s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:571s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s

fca2506bc5d492609e3f1b6e59d667e376a1eb3f drm-tip: 2017y-11m-02d-13h-10m-58s UTC integration manifest
8c03bce335c0 drm/i915: Use ELK stolen memory reserved detection for ILK
cc31312966c2 drm/i915: Make the report about a bogus stolen reserved area an error
f1ec6e9db4ef drm/i915: Check if the stolen memory "reserved" area is enabled or not

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
URL   : https://patchwork.freedesktop.org/series/33060/
State : warning

== Summary ==

Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
                dmesg-warn -> PASS       (shard-hsw)
        Subgroup extended-modeset-hang-newfb-with-reset-render-B:
                pass       -> DMESG-WARN (shard-hsw)

shard-hsw        total:2539 pass:1432 dwarn:2   dfail:0   fail:8   skip:1097 time:9313s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjälä]

On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
> == Series Details ==
> 
> Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
> URL   : https://patchwork.freedesktop.org/series/33060/
> State : warning
> 
> == Summary ==
> 
> Test kms_busy:
>         Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
>                 dmesg-warn -> PASS       (shard-hsw)
>         Subgroup extended-modeset-hang-newfb-with-reset-render-B:
>                 pass       -> DMESG-WARN (shard-hsw)

Hmm. The warn was there already AFAICS. I wonder why this is claiming
things were passing?

Also shard-glkb didn't seem to get any results from this run. No idea
why, nor why this summary fails to mention that fact.

Oh and BTW the boot/dmesg links from the shard results don't seem to
work very well. Sometimes it just gets you an empty log and you have
to manually find a file that has some actual content in it.

> 
> shard-hsw        total:2539 pass:1432 dwarn:2   dfail:0   fail:8   skip:1097 time:9313s
> 
> == Logs ==
> 
> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html

Looking at the results we go from
<7>[    2.822784] [drm:i915_ggtt_probe_hw [i915]] GTT stolen size = 128M
<7>[    2.826115] [drm:i915_gem_init_stolen [i915]] Stolen reserved area
[0x0000000001100000 - 0x0000000001200000] outside stolen memory
[0x00000000c7a00000 - 0x00000000cfa00000]

to
<7>[    2.909693] [drm:i915_ggtt_probe_hw [i915]] GTT stolen size = 128M
<7>[    2.912226] [drm:i915_gem_init_stolen [i915]] Memory reserved for
graphics device: 131072K, usable: 131072K

on shard-snb6 at least.

After going through the dmesgs for all the other machines we have in ci,
it doesn't look like there were any other changes in the amount of stolen
memory we detect (well, couldn't check shard-glkb due to lack fo results).

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

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Saarinen, Jani]

Hi, 
> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> Ville Syrjälä
> Sent: torstai 2. marraskuuta 2017 19.08
> To: intel-gfx@lists.freedesktop.org
> Cc: Sarvela, Tomi P <tomi.p.sarvela@intel.com>
> Subject: Re: [Intel-gfx] ✗ Fi.CI.IGT: warning for series starting with [1/3]
> drm/i915: Check if the stolen memory "reserved" area is enabled or not
> 
> On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
> > == Series Details ==
> >
> > Series: series starting with [1/3] drm/i915: Check if the stolen memory
> "reserved" area is enabled or not
> > URL   : https://patchwork.freedesktop.org/series/33060/
> > State : warning
> >
> > == Summary ==
> >
> > Test kms_busy:
> >         Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
> >                 dmesg-warn -> PASS       (shard-hsw)
> >         Subgroup extended-modeset-hang-newfb-with-reset-render-B:
> >                 pass       -> DMESG-WARN (shard-hsw)
> 
> Hmm. The warn was there already AFAICS. I wonder why this is claiming
> things were passing?
Tomi, Arek? 
> 
> Also shard-glkb didn't seem to get any results from this run. No idea why, nor
> why this summary fails to mention that fact.
We are not getting runs from GLK for pre-merge, right Tomi
> 
> Oh and BTW the boot/dmesg links from the shard results don't seem to work
> very well. Sometimes it just gets you an empty log and you have to manually
> find a file that has some actual content in it.
> 
Tomi again.
> >
> > shard-hsw        total:2539 pass:1432 dwarn:2   dfail:0   fail:8   skip:1097
> time:9313s
> >


Jani Saarinen
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo


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

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Tomi Sarvela]

On 02/11/17 19:08, Ville Syrjälä wrote:
> On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
>> == Series Details ==
>>
>> Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
>> URL   : https://patchwork.freedesktop.org/series/33060/
>> State : warning
>>
>> == Summary ==
>>
>> Test kms_busy:
>>          Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
>>                  dmesg-warn -> PASS       (shard-hsw)
>>          Subgroup extended-modeset-hang-newfb-with-reset-render-B:
>>                  pass       -> DMESG-WARN (shard-hsw)
> 
> Hmm. The warn was there already AFAICS. I wonder why this is claiming
> things were passing?

The sharded result for the run is 'warning'. What do you mean?

> Also shard-glkb didn't seem to get any results from this run. No idea
> why, nor why this summary fails to mention that fact.

One GLK died, and 4 GLKs take nearly 90 minutes to run shards. It's not 
reasonable to wait for them on every run and queue everything else, so 
Patchwork/Trybot runs are skipped on GLK. Anything can be run and added 
to results manually, if needed.

As a comparison, SNB/HSW/APL all run shards in ~35 minutes. KBL is the 
only one that gets rebooted between shards (due to leaking context), and 
takes around 50 minutes.

> Oh and BTW the boot/dmesg links from the shard results don't seem to
> work very well. Sometimes it just gets you an empty log and you have
> to manually find a file that has some actual content in it.

If there is no bootlog for a run, the host has not booted between runs. 
It's maybe not intuitively clear. I've tried to communicate that the 
shard runs are shifting to conditional reboots (booted only if hung).

Tomi

>> shard-hsw        total:2539 pass:1432 dwarn:2   dfail:0   fail:8   skip:1097 time:9313s
>>
>> == Logs ==
>>
>> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
> 
> Looking at the results we go from
> <7>[    2.822784] [drm:i915_ggtt_probe_hw [i915]] GTT stolen size = 128M
> <7>[    2.826115] [drm:i915_gem_init_stolen [i915]] Stolen reserved area
> [0x0000000001100000 - 0x0000000001200000] outside stolen memory
> [0x00000000c7a00000 - 0x00000000cfa00000]
> 
> to
> <7>[    2.909693] [drm:i915_ggtt_probe_hw [i915]] GTT stolen size = 128M
> <7>[    2.912226] [drm:i915_gem_init_stolen [i915]] Memory reserved for
> graphics device: 131072K, usable: 131072K
> 
> on shard-snb6 at least.
> 
> After going through the dmesgs for all the other machines we have in ci,
> it doesn't look like there were any other changes in the amount of stolen
> memory we detect (well, couldn't check shard-glkb due to lack fo results).

There is GLK in Farm1, so you can check the bootlogs from fast-feedback 
run if that part is what you're interested in. All the other gens too.

Tomi
-- 
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjälä]

On Fri, Nov 03, 2017 at 10:18:32AM +0200, Tomi Sarvela wrote:
> On 02/11/17 19:08, Ville Syrjälä wrote:
> > On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
> >> == Series Details ==
> >>
> >> Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
> >> URL   : https://patchwork.freedesktop.org/series/33060/
> >> State : warning
> >>
> >> == Summary ==
> >>
> >> Test kms_busy:
> >>          Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
> >>                  dmesg-warn -> PASS       (shard-hsw)
> >>          Subgroup extended-modeset-hang-newfb-with-reset-render-B:
> >>                  pass       -> DMESG-WARN (shard-hsw)
> > 
> > Hmm. The warn was there already AFAICS. I wonder why this is claiming
> > things were passing?
> 
> The sharded result for the run is 'warning'. What do you mean?

This should read 'DMESG-WARN -> DMESG-WARN' rather than 'pass ->
DMESG-WARN'. If I click the link to open up the results in the browser
this test isn't shown on the shards.html, but I can see it as
orange->orange in shards-all.html.

> 
> > Also shard-glkb didn't seem to get any results from this run. No idea
> > why, nor why this summary fails to mention that fact.
> 
> One GLK died, and 4 GLKs take nearly 90 minutes to run shards. It's not 
> reasonable to wait for them on every run and queue everything else, so 
> Patchwork/Trybot runs are skipped on GLK. Anything can be run and added 
> to results manually, if needed.

If they aren't being run, then maybe they shouldn't be part of the
shards.html for pw/trybot runs? Would save me having to wonder why I'm
getting empty results. Or we should have some kind of indication why we
didn't get any results for a particular machine (ie. whether it was
expected or not).

> 
> As a comparison, SNB/HSW/APL all run shards in ~35 minutes. KBL is the 
> only one that gets rebooted between shards (due to leaking context), and 
> takes around 50 minutes.
> 
> > Oh and BTW the boot/dmesg links from the shard results don't seem to
> > work very well. Sometimes it just gets you an empty log and you have
> > to manually find a file that has some actual content in it.
> 
> If there is no bootlog for a run, the host has not booted between runs. 
> It's maybe not intuitively clear. I've tried to communicate that the 
> shard runs are shifting to conditional reboots (booted only if hung).

I don't know how that relates to the several dmesg/boot logs I see
in the directory. And why the web links often seems to point to the
empty files instead of ones with actual content in them.

Also the files are numbered in some way. Whether there's any significance
to those numbers I can't really tell.

> 
> Tomi
> 
> >> shard-hsw        total:2539 pass:1432 dwarn:2   dfail:0   fail:8   skip:1097 time:9313s
> >>
> >> == Logs ==
> >>
> >> For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
> > 
> > Looking at the results we go from
> > <7>[    2.822784] [drm:i915_ggtt_probe_hw [i915]] GTT stolen size = 128M
> > <7>[    2.826115] [drm:i915_gem_init_stolen [i915]] Stolen reserved area
> > [0x0000000001100000 - 0x0000000001200000] outside stolen memory
> > [0x00000000c7a00000 - 0x00000000cfa00000]
> > 
> > to
> > <7>[    2.909693] [drm:i915_ggtt_probe_hw [i915]] GTT stolen size = 128M
> > <7>[    2.912226] [drm:i915_gem_init_stolen [i915]] Memory reserved for
> > graphics device: 131072K, usable: 131072K
> > 
> > on shard-snb6 at least.
> > 
> > After going through the dmesgs for all the other machines we have in ci,
> > it doesn't look like there were any other changes in the amount of stolen
> > memory we detect (well, couldn't check shard-glkb due to lack fo results).
> 
> There is GLK in Farm1, so you can check the bootlogs from fast-feedback 
> run if that part is what you're interested in. All the other gens too.

I did check all the BAT runs. But I have no idea if the glks there are
the same board as what we have as gklb in the shard. So not sure if I
actually checked all the N machine types we have, or just N-1.

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

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Petri Latvala]

On Fri, Nov 03, 2017 at 10:18:32AM +0200, Tomi Sarvela wrote:
> On 02/11/17 19:08, Ville Syrjälä wrote:
> > On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
> > > == Series Details ==
> > > 
> > > Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
> > > URL   : https://patchwork.freedesktop.org/series/33060/
> > > State : warning
> > > 
> > > == Summary ==
> > > 
> > > Test kms_busy:
> > >          Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
> > >                  dmesg-warn -> PASS       (shard-hsw)
> > >          Subgroup extended-modeset-hang-newfb-with-reset-render-B:
> > >                  pass       -> DMESG-WARN (shard-hsw)
> > 
> > Hmm. The warn was there already AFAICS. I wonder why this is claiming
> > things were passing?
> 
> The sharded result for the run is 'warning'. What do you mean?

https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html

The comparison is done to CI_DRM_3309 according to this, and
kms_busy@extended-modeset-hang-newfb-with-reset-render-B was
dmesg-warn there. Why is the above diff showing pass -> DMESG-WARN?



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

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Tomi Sarvela]

On 03/11/17 12:41, Ville Syrjälä wrote:
> On Fri, Nov 03, 2017 at 10:18:32AM +0200, Tomi Sarvela wrote:
>> On 02/11/17 19:08, Ville Syrjälä wrote:
>>> On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
>>>>
>>>> Test kms_busy:
>>>>           Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
>>>>                   dmesg-warn -> PASS       (shard-hsw)
>>>>           Subgroup extended-modeset-hang-newfb-with-reset-render-B:
>>>>                   pass       -> DMESG-WARN (shard-hsw)
>>>
>>> Hmm. The warn was there already AFAICS. I wonder why this is claiming
>>> things were passing?
>>
>> The sharded result for the run is 'warning'. What do you mean?
> 
> This should read 'DMESG-WARN -> DMESG-WARN' rather than 'pass ->
> DMESG-WARN'. If I click the link to open up the results in the browser
> this test isn't shown on the shards.html, but I can see it as
> orange->orange in shards-all.html.

I'll see what has happened there. Clearly a bug.

>>> Also shard-glkb didn't seem to get any results from this run. No idea
>>> why, nor why this summary fails to mention that fact.
>>
>> One GLK died, and 4 GLKs take nearly 90 minutes to run shards. It's not
>> reasonable to wait for them on every run and queue everything else, so
>> Patchwork/Trybot runs are skipped on GLK. Anything can be run and added
>> to results manually, if needed.
> 
> If they aren't being run, then maybe they shouldn't be part of the
> shards.html for pw/trybot runs? Would save me having to wonder why I'm
> getting empty results. Or we should have some kind of indication why we
> didn't get any results for a particular machine (ie. whether it was
> expected or not).

Because of people, we can't always be sure if missing machine is 
expected or not. There is no hard reservation system, and machines are 
taken out of the farm for debugging, bios upgrades, that kind of stuff.

Dropping shard from shards.html if there's no comparable results 
(missing either baseline, or patchset run) would probably be a good thing.

>> As a comparison, SNB/HSW/APL all run shards in ~35 minutes. KBL is the
>> only one that gets rebooted between shards (due to leaking context), and
>> takes around 50 minutes.
>>
>>> Oh and BTW the boot/dmesg links from the shard results don't seem to
>>> work very well. Sometimes it just gets you an empty log and you have
>>> to manually find a file that has some actual content in it.
>>
>> If there is no bootlog for a run, the host has not booted between runs.
>> It's maybe not intuitively clear. I've tried to communicate that the
>> shard runs are shifting to conditional reboots (booted only if hung).
> 
> I don't know how that relates to the several dmesg/boot logs I see
> in the directory. And why the web links often seems to point to the
> empty files instead of ones with actual content in them.

I could link to the original bootlog, but that doesn't answer question 
"what happened just before this shard was run"

Sometimes there's leftover dmesg between shardruns, which gets recorded 
in boot.log. Earlier name was "dmesg_before_run.log" or something like that.

> Also the files are numbered in some way. Whether there's any significance
> to those numbers I can't really tell.

Shards are fed to testhosts from 0 to 34, linear order. Filenames have 
that run number. On one host smaller number has been run before larger 
number. On shardruns the number is used to choose shard testlist in our 
IGT package (./shards/x0000)

We could also do several runs with same kernel and testlist, but 
cibuglog can't yet handle the statistics for these kind of results.

>>> After going through the dmesgs for all the other machines we have in ci,
>>> it doesn't look like there were any other changes in the amount of stolen
>>> memory we detect (well, couldn't check shard-glkb due to lack fo results).
>>
>> There is GLK in Farm1, so you can check the bootlogs from fast-feedback
>> run if that part is what you're interested in. All the other gens too.
> 
> I did check all the BAT runs. But I have no idea if the glks there are
> the same board as what we have as gklb in the shard. So not sure if I
> actually checked all the N machine types we have, or just N-1.

Shards have two different boards, and one of those is in Farm1 too. 
Farm1 has different eDP panel, though.

Tomi
-- 
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Tomi Sarvela]

On 03/11/17 12:43, Petri Latvala wrote:
> On Fri, Nov 03, 2017 at 10:18:32AM +0200, Tomi Sarvela wrote:
>> On 02/11/17 19:08, Ville Syrjälä wrote:
>>> On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
>>>> == Series Details ==
>>>>
>>>> Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
>>>> URL   : https://patchwork.freedesktop.org/series/33060/
>>>> State : warning
>>>>
>>>> == Summary ==
>>>>
>>>> Test kms_busy:
>>>>           Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
>>>>                   dmesg-warn -> PASS       (shard-hsw)
>>>>           Subgroup extended-modeset-hang-newfb-with-reset-render-B:
>>>>                   pass       -> DMESG-WARN (shard-hsw)
>>>
>>> Hmm. The warn was there already AFAICS. I wonder why this is claiming
>>> things were passing?
>>
>> The sharded result for the run is 'warning'. What do you mean?
> 
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
> 
> The comparison is done to CI_DRM_3309 according to this, and
> kms_busy@extended-modeset-hang-newfb-with-reset-render-B was
> dmesg-warn there. Why is the above diff showing pass -> DMESG-WARN?

Found the reason: some of the shards have been run twice for CI_DRM_3309 
(my mistake, probably). This has the unintended effect on visualization 
/ comparison which create independently: results depends on loading 
order if there's flip-flops, and differences might show up.

I'll remove duplicate shards and re-run the html.

Tomi
-- 
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Patchwork]

== Series Details ==

Series: series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not
URL   : https://patchwork.freedesktop.org/series/33060/
State : success

== Summary ==

Test kms_busy:
        Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
                dmesg-warn -> PASS       (shard-hsw)

shard-hsw        total:2539 pass:1432 dwarn:2   dfail:0   fail:8   skip:1097 time:9313s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.IGT: warning for series starting with [1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Tomi Sarvela]

On 03/11/17 13:33, Tomi Sarvela wrote:
> On 03/11/17 12:43, Petri Latvala wrote:
>> On Fri, Nov 03, 2017 at 10:18:32AM +0200, Tomi Sarvela wrote:
>>> On 02/11/17 19:08, Ville Syrjälä wrote:
>>>> On Thu, Nov 02, 2017 at 04:34:26PM -0000, Patchwork wrote:
>>>>> == Series Details ==
>>>>>
>>>>> Series: series starting with [1/3] drm/i915: Check if the stolen 
>>>>> memory "reserved" area is enabled or not
>>>>> URL   : https://patchwork.freedesktop.org/series/33060/
>>>>> State : warning
>>>>>
>>>>> == Summary ==
>>>>>
>>>>> Test kms_busy:
>>>>>           Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
>>>>>                   dmesg-warn -> PASS       (shard-hsw)
>>>>>           Subgroup extended-modeset-hang-newfb-with-reset-render-B:
>>>>>                   pass       -> DMESG-WARN (shard-hsw)
>>>>
>>>> Hmm. The warn was there already AFAICS. I wonder why this is claiming
>>>> things were passing?
>>>
>>> The sharded result for the run is 'warning'. What do you mean?
>>
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
>>
>> The comparison is done to CI_DRM_3309 according to this, and
>> kms_busy@extended-modeset-hang-newfb-with-reset-render-B was
>> dmesg-warn there. Why is the above diff showing pass -> DMESG-WARN?
> 
> Found the reason: some of the shards have been run twice for CI_DRM_3309 
> (my mistake, probably). This has the unintended effect on visualization 
> / comparison which create independently: results depends on loading 
> order if there's flip-flops, and differences might show up.
> 
> I'll remove duplicate shards and re-run the html.

Cleaned up CI_DRM_3309 run. Sorry for the confusion.

Made the improvement of dropping hosts without needed runs when asking 
for changes: there is no comparable changes if one of two runs is 
missing. They can be seen in shards-all.html though.

Re-ran Patchwork_6930 as an example: (SUCCESS)

https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards.html
https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_6930/shards-all.html

Test kms_flip:
         Subgroup plain-flip-fb-recreate-interruptible:
                 pass       -> FAIL       (shard-hsw) fdo#100368
Test kms_busy:
         Subgroup extended-modeset-hang-oldfb-with-reset-render-A:
                 dmesg-warn -> PASS       (shard-hsw)

fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368

shard-hsw        total:2539 pass:1431 dwarn:2   dfail:0   fail:9 
skip:1097 time:9313s


Tomi
-- 
Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Paulo Zanoni]

Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Apparently there are some machines that put semi-sensible looking
> values
> into the stolen "reserved" base and size, except those values are
> actually
> outside the stolen memory. There is a bit in the register which
> supposedly could tell us whether the reserved area is even enabled or
> not. Let's check for that before we go trusting the base and size.

If this is such a problem since g4x, why didn't we spot it earlier? It
would be nice if you could explain in the commit message (or at least
in this email) what are the consequences you're seeing that made you
realize about this problem. Did something actually explode? I'm
genuinely curious.


Now talking about the patch itself:

If we're going to assume random bits instead of a full-zero in
(possibly) uninitialized stuff, shouldn't we first check bit 1, which
is supposed to tell us if the whole reg is locked or not?

if ((reg_val & 0x3) != 0x3)
	ignore stolen reserved stuff;

Anyway, this patch without my suggestions is probably better than the
current situation so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
but please feel investigate the bit 1 thing and CC me on v2 or a
possible follow-up patch with conclusions.


> Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 30
> ++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_reg.h        |  2 ++
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 03e7abc7e043..44a5dbc8c23b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -294,6 +294,12 @@ static void g4x_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  				     ELK_STOLEN_RESERVED);
>  	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> >stolen_size;
>  
> +	if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0) {
> +		*base = 0;
> +		*size = 0;
> +		return;
> +	}
> +
>  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
>  
>  	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
> @@ -313,6 +319,12 @@ static void gen6_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> +		*base = 0;
> +		*size = 0;
> +		return;
> +	}
> +
>  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
>  
>  	switch (reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK) {
> @@ -339,6 +351,12 @@ static void gen7_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> +		*base = 0;
> +		*size = 0;
> +		return;
> +	}
> +
>  	*base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
>  
>  	switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
> @@ -359,6 +377,12 @@ static void chv_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  {
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  
> +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> +		*base = 0;
> +		*size = 0;
> +		return;
> +	}
> +
>  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
>  
>  	switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) {
> @@ -387,6 +411,12 @@ static void bdw_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
>  	dma_addr_t stolen_top;
>  
> +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> +		*base = 0;
> +		*size = 0;
> +		return;
> +	}
> +
>  	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
>  
>  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index f0f8f6059652..dc2cbc428d1b 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -382,6 +382,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t
> reg)
>  #define GEN8_STOLEN_RESERVED_2M		(1 << 7)
>  #define GEN8_STOLEN_RESERVED_4M		(2 << 7)
>  #define GEN8_STOLEN_RESERVED_8M		(3 << 7)
> +#define GEN6_STOLEN_RESERVED_ENABLE	(1 << 0)
>  
>  /* VGA stuff */
>  
> @@ -3398,6 +3399,7 @@ enum i915_power_well_id {
>  #define ELK_STOLEN_RESERVED		_MMIO(MCHBAR_MIRROR_BASE
> + 0x48)
>  #define G4X_STOLEN_RESERVED_ADDR1_MASK	(0xFFFF << 16)
>  #define G4X_STOLEN_RESERVED_ADDR2_MASK	(0xFFF << 4)
> +#define G4X_STOLEN_RESERVED_ENABLE	(1 << 0)
>  
>  /* Memory controller frequency in MCHBAR for Haswell (possible SNB+)
> */
>  #define DCLK _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5e04)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Chris Wilson]

Quoting Paulo Zanoni (2017-11-14 20:12:41)
> Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Apparently there are some machines that put semi-sensible looking
> > values
> > into the stolen "reserved" base and size, except those values are
> > actually
> > outside the stolen memory. There is a bit in the register which
> > supposedly could tell us whether the reserved area is even enabled or
> > not. Let's check for that before we go trusting the base and size.
> 
> If this is such a problem since g4x, why didn't we spot it earlier? It
> would be nice if you could explain in the commit message (or at least
> in this email) what are the consequences you're seeing that made you
> realize about this problem. Did something actually explode? I'm
> genuinely curious.

The consequence is that we disable stolen; the machine keeps on working
quite happily. Only fbc actually depends on stolen allocation to
function, and no one complains if fbc is disabled. (There's a sketch out
there that we could use a contiguous allocation for fbc if we run out of
stolen.) Internal hw functions are oblivious to our qualms about the
location of stolen and whether some other device is using the same
physical address for its trampoline.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Paulo Zanoni]

Em Ter, 2017-11-14 às 20:19 +0000, Chris Wilson escreveu:
> Quoting Paulo Zanoni (2017-11-14 20:12:41)
> > Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Apparently there are some machines that put semi-sensible looking
> > > values
> > > into the stolen "reserved" base and size, except those values are
> > > actually
> > > outside the stolen memory. There is a bit in the register which
> > > supposedly could tell us whether the reserved area is even
> > > enabled or
> > > not. Let's check for that before we go trusting the base and
> > > size.
> > 
> > If this is such a problem since g4x, why didn't we spot it earlier?
> > It
> > would be nice if you could explain in the commit message (or at
> > least
> > in this email) what are the consequences you're seeing that made
> > you
> > realize about this problem. Did something actually explode? I'm
> > genuinely curious.
> 
> The consequence is that we disable stolen; the machine keeps on
> working
> quite happily.

Thanks!

>  Only fbc actually depends on stolen allocation to
> function, and no one complains if fbc is disabled. (There's a sketch
> out
> there that we could use a contiguous allocation for fbc if we run out
> of
> stolen.)

ILK_DPFC_CB_BASE (aka FBC_CFB_BASE these days) needs to be programmed
as an offset of the base of stolen memory, so you'll need to allocate
this memory in the region that comes right after stolen, or you'll run
out of bits to write to the register.

Also, things such as the "last 8mb bug" of BDW/SKL suggest that maybe
this wouldn't work.

Unless, of course, you have some plan to work around this.

>  Internal hw functions are oblivious to our qualms about the
> location of stolen and whether some other device is using the same
> physical address for its trampoline.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Make the report about a bogus stolen reserved area an error

[Paulo Zanoni]

Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Now that we should be properly filtering out the cases when the
> stolen
> reserved area is disabled, let's convert the debug message about a
> misplaced reserved area into an error.

Worst that could happen is that some unhappy user will notify us that
we may still be missing something.

Speaking of that, is CI already able to call our attention to boot
error messages like this one?

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 44a5dbc8c23b..4f9377b5f4ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -503,9 +503,9 @@ int i915_gem_init_stolen(struct drm_i915_private
> *dev_priv)
>  	if (reserved_base < dev_priv->mm.stolen_base ||
>  	    reserved_base + reserved_size > stolen_top) {
>  		dma_addr_t reserved_top = reserved_base +
> reserved_size;
> -		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad]
> outside stolen memory [%pad - %pad]\n",
> -			      &reserved_base, &reserved_top,
> -			      &dev_priv->mm.stolen_base,
> &stolen_top);
> +		DRM_ERROR("Stolen reserved area [%pad - %pad]
> outside stolen memory [%pad - %pad]\n",
> +			  &reserved_base, &reserved_top,
> +			  &dev_priv->mm.stolen_base, &stolen_top);
>  		return 0;
>  	}
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 06:12:41PM -0200, Paulo Zanoni wrote:
> Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Apparently there are some machines that put semi-sensible looking
> > values
> > into the stolen "reserved" base and size, except those values are
> > actually
> > outside the stolen memory. There is a bit in the register which
> > supposedly could tell us whether the reserved area is even enabled or
> > not. Let's check for that before we go trusting the base and size.
> 
> If this is such a problem since g4x, why didn't we spot it earlier? It
> would be nice if you could explain in the commit message (or at least
> in this email) what are the consequences you're seeing that made you
> realize about this problem. Did something actually explode? I'm
> genuinely curious.

CI is getting ping-ping on the SNB shards. Apparently the BIOS leaves
some garbage in the the base+size fields of the register, and sometimes
that results in the reserved area landing inside stolen (at which point
we just reduce the size of stolen and continue), and sometimes it lands
somewhere outside (at which point we just disable stolen entirely).
Hence the FBC tests sometimes find enough stolen, sometimes not.

> 
> 
> Now talking about the patch itself:
> 
> If we're going to assume random bits instead of a full-zero in
> (possibly) uninitialized stuff, shouldn't we first check bit 1, which
> is supposed to tell us if the whole reg is locked or not?

Not sure we can trust the BIOS to always set the lock bit. I suppose it
really should, but I don't recall seeing anything in the docs saying
that not setting the lock bit would somehow disable the reserved area.
So my understanding is that as long as the enable bit is set the
hardware will prevent us from accessing the are, regardless of whether
the settings have been locked down or not.

This wouldn't be the first lock bit some BIOS versions forget to set BTW.

> 
> if ((reg_val & 0x3) != 0x3)
> 	ignore stolen reserved stuff;
> 
> Anyway, this patch without my suggestions is probably better than the
> current situation so:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> but please feel investigate the bit 1 thing and CC me on v2 or a
> possible follow-up patch with conclusions.
> 
> 
> > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 30
> > ++++++++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_reg.h        |  2 ++
> >  2 files changed, 32 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 03e7abc7e043..44a5dbc8c23b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -294,6 +294,12 @@ static void g4x_get_stolen_reserved(struct
> > drm_i915_private *dev_priv,
> >  				     ELK_STOLEN_RESERVED);
> >  	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> > >stolen_size;
> >  
> > +	if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0) {
> > +		*base = 0;
> > +		*size = 0;
> > +		return;
> > +	}
> > +
> >  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
> >  
> >  	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
> > @@ -313,6 +319,12 @@ static void gen6_get_stolen_reserved(struct
> > drm_i915_private *dev_priv,
> >  {
> >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> >  
> > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > +		*base = 0;
> > +		*size = 0;
> > +		return;
> > +	}
> > +
> >  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> >  
> >  	switch (reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK) {
> > @@ -339,6 +351,12 @@ static void gen7_get_stolen_reserved(struct
> > drm_i915_private *dev_priv,
> >  {
> >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> >  
> > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > +		*base = 0;
> > +		*size = 0;
> > +		return;
> > +	}
> > +
> >  	*base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
> >  
> >  	switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
> > @@ -359,6 +377,12 @@ static void chv_get_stolen_reserved(struct
> > drm_i915_private *dev_priv,
> >  {
> >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> >  
> > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > +		*base = 0;
> > +		*size = 0;
> > +		return;
> > +	}
> > +
> >  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> >  
> >  	switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) {
> > @@ -387,6 +411,12 @@ static void bdw_get_stolen_reserved(struct
> > drm_i915_private *dev_priv,
> >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> >  	dma_addr_t stolen_top;
> >  
> > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > +		*base = 0;
> > +		*size = 0;
> > +		return;
> > +	}
> > +
> >  	stolen_top = dev_priv->mm.stolen_base + ggtt->stolen_size;
> >  
> >  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index f0f8f6059652..dc2cbc428d1b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -382,6 +382,7 @@ static inline bool i915_mmio_reg_valid(i915_reg_t
> > reg)
> >  #define GEN8_STOLEN_RESERVED_2M		(1 << 7)
> >  #define GEN8_STOLEN_RESERVED_4M		(2 << 7)
> >  #define GEN8_STOLEN_RESERVED_8M		(3 << 7)
> > +#define GEN6_STOLEN_RESERVED_ENABLE	(1 << 0)
> >  
> >  /* VGA stuff */
> >  
> > @@ -3398,6 +3399,7 @@ enum i915_power_well_id {
> >  #define ELK_STOLEN_RESERVED		_MMIO(MCHBAR_MIRROR_BASE
> > + 0x48)
> >  #define G4X_STOLEN_RESERVED_ADDR1_MASK	(0xFFFF << 16)
> >  #define G4X_STOLEN_RESERVED_ADDR2_MASK	(0xFFF << 4)
> > +#define G4X_STOLEN_RESERVED_ENABLE	(1 << 0)
> >  
> >  /* Memory controller frequency in MCHBAR for Haswell (possible SNB+)
> > */
> >  #define DCLK _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5e04)

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

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Chris Wilson]

Quoting Paulo Zanoni (2017-11-14 20:29:49)
> Em Ter, 2017-11-14 às 20:19 +0000, Chris Wilson escreveu:
> >  Only fbc actually depends on stolen allocation to
> > function, and no one complains if fbc is disabled. (There's a sketch
> > out
> > there that we could use a contiguous allocation for fbc if we run out
> > of
> > stolen.)
> 
> ILK_DPFC_CB_BASE (aka FBC_CFB_BASE these days) needs to be programmed
> as an offset of the base of stolen memory, so you'll need to allocate
> this memory in the region that comes right after stolen, or you'll run
> out of bits to write to the register.
> 
> Also, things such as the "last 8mb bug" of BDW/SKL suggest that maybe
> this wouldn't work.
> 
> Unless, of course, you have some plan to work around this.

Nah, just the stuff I was last looking at (powerctx) were physical.

Being able to disable stolen and recover the memory is still on the
wishlist.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Make the report about a bogus stolen reserved area an error

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 06:33:41PM -0200, Paulo Zanoni wrote:
> Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Now that we should be properly filtering out the cases when the
> > stolen
> > reserved area is disabled, let's convert the debug message about a
> > misplaced reserved area into an error.
> 
> Worst that could happen is that some unhappy user will notify us that
> we may still be missing something.
> 
> Speaking of that, is CI already able to call our attention to boot
> error messages like this one?

I guess module reload should hit it. Would be nice to have checks for
initial boot errors/warnings too though since there's no guarantee
everything is 100% virgin on module reload.

> 
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 44a5dbc8c23b..4f9377b5f4ae 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -503,9 +503,9 @@ int i915_gem_init_stolen(struct drm_i915_private
> > *dev_priv)
> >  	if (reserved_base < dev_priv->mm.stolen_base ||
> >  	    reserved_base + reserved_size > stolen_top) {
> >  		dma_addr_t reserved_top = reserved_base +
> > reserved_size;
> > -		DRM_DEBUG_KMS("Stolen reserved area [%pad - %pad]
> > outside stolen memory [%pad - %pad]\n",
> > -			      &reserved_base, &reserved_top,
> > -			      &dev_priv->mm.stolen_base,
> > &stolen_top);
> > +		DRM_ERROR("Stolen reserved area [%pad - %pad]
> > outside stolen memory [%pad - %pad]\n",
> > +			  &reserved_base, &reserved_top,
> > +			  &dev_priv->mm.stolen_base, &stolen_top);
> >  		return 0;
> >  	}
> >  

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

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Paulo Zanoni]

Em Ter, 2017-11-14 às 22:34 +0200, Ville Syrjälä escreveu:
> On Tue, Nov 14, 2017 at 06:12:41PM -0200, Paulo Zanoni wrote:
> > Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Apparently there are some machines that put semi-sensible looking
> > > values
> > > into the stolen "reserved" base and size, except those values are
> > > actually
> > > outside the stolen memory. There is a bit in the register which
> > > supposedly could tell us whether the reserved area is even
> > > enabled or
> > > not. Let's check for that before we go trusting the base and
> > > size.
> > 
> > If this is such a problem since g4x, why didn't we spot it earlier?
> > It
> > would be nice if you could explain in the commit message (or at
> > least
> > in this email) what are the consequences you're seeing that made
> > you
> > realize about this problem. Did something actually explode? I'm
> > genuinely curious.
> 
> CI is getting ping-ping on the SNB shards. Apparently the BIOS leaves
> some garbage in the the base+size fields of the register, and
> sometimes
> that results in the reserved area landing inside stolen (at which
> point
> we just reduce the size of stolen and continue), and sometimes it
> lands
> somewhere outside (at which point we just disable stolen entirely).
> Hence the FBC tests sometimes find enough stolen, sometimes not.

Couldn't it be that we're failing to read the "size of stolen"
registers instead of the "stolen reserved stuff" regs? All of the
stolen registers have a history of controversy...


> 
> > 
> > 
> > Now talking about the patch itself:
> > 
> > If we're going to assume random bits instead of a full-zero in
> > (possibly) uninitialized stuff, shouldn't we first check bit 1,
> > which
> > is supposed to tell us if the whole reg is locked or not?
> 
> Not sure we can trust the BIOS to always set the lock bit. I suppose
> it
> really should, but I don't recall seeing anything in the docs saying
> that not setting the lock bit would somehow disable the reserved
> area.

The way things are worded, it suggests that if the lock bit is zero,
everything else could be garbage. Bit 0 is only locked after bit 1 is
locked. It doesn't make sense to me to trust bit 0 without trusting bit
1.

So my understanding is that as long as the enable bit is set the
> hardware will prevent us from accessing the are, regardless of
> whether
> the settings have been locked down or not.
> 
> This wouldn't be the first lock bit some BIOS versions forget to set
> BTW.

But it could be worth checking this now that you have access to the
machines that have the random stuff...


> 
> > 
> > if ((reg_val & 0x3) != 0x3)
> > 	ignore stolen reserved stuff;
> > 
> > Anyway, this patch without my suggestions is probably better than
> > the
> > current situation so:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > but please feel investigate the bit 1 thing and CC me on v2 or a
> > possible follow-up patch with conclusions.
> > 
> > 
> > > Cc: Tomi Sarvela <tomi.p.sarvela@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_gem_stolen.c | 30
> > > ++++++++++++++++++++++++++++++
> > >  drivers/gpu/drm/i915/i915_reg.h        |  2 ++
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > index 03e7abc7e043..44a5dbc8c23b 100644
> > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > > @@ -294,6 +294,12 @@ static void g4x_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > >  				     ELK_STOLEN_RESERVED);
> > >  	dma_addr_t stolen_top = dev_priv->mm.stolen_base + ggtt-
> > > > stolen_size;
> > > 
> > >  
> > > +	if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0) {
> > > +		*base = 0;
> > > +		*size = 0;
> > > +		return;
> > > +	}
> > > +
> > >  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) <<
> > > 16;
> > >  
> > >  	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) <
> > > *base);
> > > @@ -313,6 +319,12 @@ static void gen6_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > >  {
> > >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >  
> > > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > +		*base = 0;
> > > +		*size = 0;
> > > +		return;
> > > +	}
> > > +
> > >  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > >  
> > >  	switch (reg_val & GEN6_STOLEN_RESERVED_SIZE_MASK) {
> > > @@ -339,6 +351,12 @@ static void gen7_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > >  {
> > >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >  
> > > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > +		*base = 0;
> > > +		*size = 0;
> > > +		return;
> > > +	}
> > > +
> > >  	*base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
> > >  
> > >  	switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
> > > @@ -359,6 +377,12 @@ static void chv_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > >  {
> > >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >  
> > > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > +		*base = 0;
> > > +		*size = 0;
> > > +		return;
> > > +	}
> > > +
> > >  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > >  
> > >  	switch (reg_val & GEN8_STOLEN_RESERVED_SIZE_MASK) {
> > > @@ -387,6 +411,12 @@ static void bdw_get_stolen_reserved(struct
> > > drm_i915_private *dev_priv,
> > >  	uint32_t reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> > >  	dma_addr_t stolen_top;
> > >  
> > > +	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> > > +		*base = 0;
> > > +		*size = 0;
> > > +		return;
> > > +	}
> > > +
> > >  	stolen_top = dev_priv->mm.stolen_base + ggtt-
> > > >stolen_size;
> > >  
> > >  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > b/drivers/gpu/drm/i915/i915_reg.h
> > > index f0f8f6059652..dc2cbc428d1b 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -382,6 +382,7 @@ static inline bool
> > > i915_mmio_reg_valid(i915_reg_t
> > > reg)
> > >  #define GEN8_STOLEN_RESERVED_2M		(1 << 7)
> > >  #define GEN8_STOLEN_RESERVED_4M		(2 << 7)
> > >  #define GEN8_STOLEN_RESERVED_8M		(3 << 7)
> > > +#define GEN6_STOLEN_RESERVED_ENABLE	(1 << 0)
> > >  
> > >  /* VGA stuff */
> > >  
> > > @@ -3398,6 +3399,7 @@ enum i915_power_well_id {
> > >  #define ELK_STOLEN_RESERVED		_MMIO(MCHBAR_MIRROR_B
> > > ASE
> > > + 0x48)
> > >  #define G4X_STOLEN_RESERVED_ADDR1_MASK	(0xFFFF << 16)
> > >  #define G4X_STOLEN_RESERVED_ADDR2_MASK	(0xFFF << 4)
> > > +#define G4X_STOLEN_RESERVED_ENABLE	(1 << 0)
> > >  
> > >  /* Memory controller frequency in MCHBAR for Haswell (possible
> > > SNB+)
> > > */
> > >  #define DCLK _MMIO(MCHBAR_MIRROR_BASE_SNB + 0x5e04)
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use ELK stolen memory reserved detection for ILK

[Paulo Zanoni]

Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> While I have no solid proof that ILK follows the ELK path when it
> comes to the stolen memory reserved area, there are some hints that
> it might be the case. Unfortunately my ILK doesn't have this enabled,
> and no way to enable it via the BIOS it seems.
> 
> So let's have ILK use the ELK code path, and let's toss in a WARN
> into the code to see if we catch anyone with an ILK that has this
> enabled to further analyze the situation.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 4f9377b5f4ae..1877ae9a1d9b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -300,6 +300,12 @@ static void g4x_get_stolen_reserved(struct
> drm_i915_private *dev_priv,
>  		return;
>  	}
>  
> +	/*
> +	 * Whether ILK really reuses the ELK register for this is
> unclear.
> +	 * Let's see if we catch anyone with this supposedly enabled
> on ILK.
> +	 */
> +	WARN(IS_GEN5(dev_priv), "ILK stolen reserved found?
> 0x%08x\n", reg_val);

Since we're going to introduce an unconditional WARN, we may as well
print the value of GEN6_STOLEN_RESERVED, just in case?

Also, this will probably scare a lot of users. Maybe minimizing it to
DRM_ERROR would help. We could also consider expanding the message a
little bit more and explain that it's there for debugging purposes and
should be reported back to us?

I'll let the Maintainers make the decision on whether it's fine to add
a WARN like that. Please ping them.

Anyway, just like you, I don't have the documents to back up the claims
of the patch, so giving a R-B tag is quite hard.

Acked-by: Paulo Zanoni <paulo.r.zanoni@intel.com>




> +
>  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
>  
>  	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
> @@ -466,14 +472,12 @@ int i915_gem_init_stolen(struct
> drm_i915_private *dev_priv)
>  	case 3:
>  		break;
>  	case 4:
> -		if (IS_G4X(dev_priv))
> -			g4x_get_stolen_reserved(dev_priv,
> -						&reserved_base,
> &reserved_size);
> -		break;
> +		if (!IS_G4X(dev_priv))
> +			break;
> +		/* fall through */
>  	case 5:
> -		/* Assume the gen6 maximum for the older platforms.
> */
> -		reserved_size = 1024 * 1024;
> -		reserved_base = stolen_top - reserved_size;
> +		g4x_get_stolen_reserved(dev_priv,
> +					&reserved_base,
> &reserved_size);
>  		break;
>  	case 6:
>  		gen6_get_stolen_reserved(dev_priv,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use ELK stolen memory reserved detection for ILK

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 06:59:21PM -0200, Paulo Zanoni wrote:
> Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > While I have no solid proof that ILK follows the ELK path when it
> > comes to the stolen memory reserved area, there are some hints that
> > it might be the case. Unfortunately my ILK doesn't have this enabled,
> > and no way to enable it via the BIOS it seems.
> > 
> > So let's have ILK use the ELK code path, and let's toss in a WARN
> > into the code to see if we catch anyone with an ILK that has this
> > enabled to further analyze the situation.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_stolen.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > index 4f9377b5f4ae..1877ae9a1d9b 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> > @@ -300,6 +300,12 @@ static void g4x_get_stolen_reserved(struct
> > drm_i915_private *dev_priv,
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * Whether ILK really reuses the ELK register for this is
> > unclear.
> > +	 * Let's see if we catch anyone with this supposedly enabled
> > on ILK.
> > +	 */
> > +	WARN(IS_GEN5(dev_priv), "ILK stolen reserved found?
> > 0x%08x\n", reg_val);
> 
> Since we're going to introduce an unconditional WARN, we may as well
> print the value of GEN6_STOLEN_RESERVED, just in case?

I ruled out the SNB and CTG registers already based on the results on
my ILK. The SNB register doesn't exist at all (gives all 1s) which
makes sense since that entire range of registers seems to have been
introduced with SNB. And the value in the CTG register didn't make
any sense for this. So the ELK register is the only viable candidate
really.

> 
> Also, this will probably scare a lot of users. Maybe minimizing it to
> DRM_ERROR would help. We could also consider expanding the message a
> little bit more and explain that it's there for debugging purposes and
> should be reported back to us?

If it looks too bening people might not bother reporting it ;)

While it is a little nasty to do it this way, I don't really have any
better ideas. Fortunately we can always kill it with cc:stable if/when
we get a report, so it should die off reasonably quickly.

And I guess it might also be the case that no ILK uses this anywhere.
On the ELK here a BIOS update seems to have locked this down entirely
and I can no longer test the modes where it was reserving stolen
for this :(

> 
> I'll let the Maintainers make the decision on whether it's fine to add
> a WARN like that. Please ping them.
> 
> Anyway, just like you, I don't have the documents to back up the claims
> of the patch, so giving a R-B tag is quite hard.
> 
> Acked-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Much appereciated.

> 
> 
> 
> 
> > +
> >  	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
> >  
> >  	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
> > @@ -466,14 +472,12 @@ int i915_gem_init_stolen(struct
> > drm_i915_private *dev_priv)
> >  	case 3:
> >  		break;
> >  	case 4:
> > -		if (IS_G4X(dev_priv))
> > -			g4x_get_stolen_reserved(dev_priv,
> > -						&reserved_base,
> > &reserved_size);
> > -		break;
> > +		if (!IS_G4X(dev_priv))
> > +			break;
> > +		/* fall through */
> >  	case 5:
> > -		/* Assume the gen6 maximum for the older platforms.
> > */
> > -		reserved_size = 1024 * 1024;
> > -		reserved_base = stolen_top - reserved_size;
> > +		g4x_get_stolen_reserved(dev_priv,
> > +					&reserved_base,
> > &reserved_size);
> >  		break;
> >  	case 6:
> >  		gen6_get_stolen_reserved(dev_priv,

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

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjälä]

On Tue, Nov 14, 2017 at 06:44:51PM -0200, Paulo Zanoni wrote:
> Em Ter, 2017-11-14 às 22:34 +0200, Ville Syrjälä escreveu:
> > On Tue, Nov 14, 2017 at 06:12:41PM -0200, Paulo Zanoni wrote:
> > > Em Qui, 2017-11-02 às 17:17 +0200, Ville Syrjala escreveu:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Apparently there are some machines that put semi-sensible looking
> > > > values
> > > > into the stolen "reserved" base and size, except those values are
> > > > actually
> > > > outside the stolen memory. There is a bit in the register which
> > > > supposedly could tell us whether the reserved area is even
> > > > enabled or
> > > > not. Let's check for that before we go trusting the base and
> > > > size.
> > > 
> > > If this is such a problem since g4x, why didn't we spot it earlier?
> > > It
> > > would be nice if you could explain in the commit message (or at
> > > least
> > > in this email) what are the consequences you're seeing that made
> > > you
> > > realize about this problem. Did something actually explode? I'm
> > > genuinely curious.
> > 
> > CI is getting ping-ping on the SNB shards. Apparently the BIOS leaves
> > some garbage in the the base+size fields of the register, and
> > sometimes
> > that results in the reserved area landing inside stolen (at which
> > point
> > we just reduce the size of stolen and continue), and sometimes it
> > lands
> > somewhere outside (at which point we just disable stolen entirely).
> > Hence the FBC tests sometimes find enough stolen, sometimes not.
> 
> Couldn't it be that we're failing to read the "size of stolen"
> registers instead of the "stolen reserved stuff" regs? All of the
> stolen registers have a history of controversy...

I think stolen itself looks correct. IIRC it was somewhere above
the 3 GiB mark and neatly inside a e820 reserved region.

The reserved region (at least in the logs I looked at) was somewhere
around the 17 MiB mark, right on top of normal RAM according to e820.
No sane BIOS would put it there.

> 
> 
> > 
> > > 
> > > 
> > > Now talking about the patch itself:
> > > 
> > > If we're going to assume random bits instead of a full-zero in
> > > (possibly) uninitialized stuff, shouldn't we first check bit 1,
> > > which
> > > is supposed to tell us if the whole reg is locked or not?
> > 
> > Not sure we can trust the BIOS to always set the lock bit. I suppose
> > it
> > really should, but I don't recall seeing anything in the docs saying
> > that not setting the lock bit would somehow disable the reserved
> > area.
> 
> The way things are worded, it suggests that if the lock bit is zero,
> everything else could be garbage. Bit 0 is only locked after bit 1 is
> locked. It doesn't make sense to me to trust bit 0 without trusting bit
> 1.

The hardware itself will trust bit 0 without bit 1. I think that's all
that really matters.

> 
> So my understanding is that as long as the enable bit is set the
> > hardware will prevent us from accessing the are, regardless of
> > whether
> > the settings have been locked down or not.
> > 
> > This wouldn't be the first lock bit some BIOS versions forget to set
> > BTW.
> 
> But it could be worth checking this now that you have access to the
> machines that have the random stuff...

Yeah. I guess it might be interesting to see the state of the lock bit
as well. I'll see if I can get someone to grab it for us (/me no longer
has any idea how to access those machines...).

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

Re: [PATCH 1/3] drm/i915: Check if the stolen memory "reserved" area is enabled or not

[Ville Syrjälä]

I pushed the series to dinq. Thanks for the review.

I think we can re-evaluate the case for the lock bit if and when someone
reports seeing the DRM_ERROR() in the wild.

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