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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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

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