diff mbox

[2/2] drm/i915/stolen: Intepret reserved_base=0 as deduce from top

Message ID 20180312125542.547-2-chris@chris-wilson.co.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Chris Wilson March 12, 2018, 12:55 p.m. UTC
On my j1900 Valleyview, and on both of CI's, if I allocate above the power
context (as allotted by PCBR) then the objects are not accessed. For
example, if it is a ringbuffer, then the commands are not executed,
causing a GPU hang with no breadcrumb advancement. On inspection,
GEN6_STOLEN_RESERVED is enabled, but has base==0 which we interpret a
being invalid. However, it fits neatly in after the powercontext if we
interpret base==0 as meaning allocate from top instead, the GPU doesn't
hang on boot!

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Ville Syrjala March 12, 2018, 2:55 p.m. UTC | #1
On Mon, Mar 12, 2018 at 12:55:42PM +0000, Chris Wilson wrote:
> On my j1900 Valleyview, and on both of CI's, if I allocate above the power
> context (as allotted by PCBR) then the objects are not accessed. For
> example, if it is a ringbuffer, then the commands are not executed,
> causing a GPU hang with no breadcrumb advancement. On inspection,
> GEN6_STOLEN_RESERVED is enabled, but has base==0 which we interpret a
> being invalid. However, it fits neatly in after the powercontext if we
> interpret base==0 as meaning allocate from top instead, the GPU doesn't
> hang on boot!
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index b04e2551bae6..1bd542977e4c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -386,12 +386,13 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  		break;
>  	}
>  
> -	/* It is possible for the reserved base to be zero, but the register
> -	 * field for size doesn't have a zero option. */
> -	if (reserved_base == 0) {
> -		reserved_size = 0;
> -		reserved_base = stolen_top;
> -	}
> +	/*
> +	 * It is possible for the reserved base to be zero, but the register
> +	 * field for size doesn't have a zero option. Experience says they
> +	 * mean allocate from top.
> +	 */
> +	if (reserved_base == 0)
> +		reserved_base = stolen_top - reserved_size;

I think on VLV and CHV the base is not used by the hardware. The docs
list the field as reserved, and there's a note that the hw just derives
the value as (top_of_stolen - size).

Although it does look like some BIOSen do still program the base.
My BSW has it set, but neither of my two VLVs have it.

So maybe we want to put this logic into the vlv/chv specific functions,
and maybe have the common code WARN if we get inconsistent values out?

>  
>  	dev_priv->dsm_reserved =
>  		(struct resource) DEFINE_RES_MEM(reserved_base, reserved_size);
> -- 
> 2.16.2
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index b04e2551bae6..1bd542977e4c 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -386,12 +386,13 @@  int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
 		break;
 	}
 
-	/* It is possible for the reserved base to be zero, but the register
-	 * field for size doesn't have a zero option. */
-	if (reserved_base == 0) {
-		reserved_size = 0;
-		reserved_base = stolen_top;
-	}
+	/*
+	 * It is possible for the reserved base to be zero, but the register
+	 * field for size doesn't have a zero option. Experience says they
+	 * mean allocate from top.
+	 */
+	if (reserved_base == 0)
+		reserved_base = stolen_top - reserved_size;
 
 	dev_priv->dsm_reserved =
 		(struct resource) DEFINE_RES_MEM(reserved_base, reserved_size);