All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v2 3/3] drm/i915/stolen: Deduce base of reserved portion as top-size on vlv
Date: Fri, 16 Mar 2018 14:00:45 +0200	[thread overview]
Message-ID: <20180316120045.GF5453@intel.com> (raw)
In-Reply-To: <20180312165206.31772-3-chris@chris-wilson.co.uk>

On Mon, Mar 12, 2018 at 04:52:06PM +0000, Chris Wilson wrote:
> On Valleyview, the HW deduces the base of the reserved portion of stolen
> memory as being (top - size) and the address field within
> GEN6_STOLEN_RESERVED is set to 0. Add yet another GEN6_STOLEN_RESERVED
> reader to cope with the subtly different path required for vlv.
> 
> v2: Avoid using reserved_base = reserved_size = 0 as the invalid
> condition as that typically falls outside of the stolen region,
> provoking a consistency error.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Imre Deak <imre.deak@intel.com>

Didn't spot anything wrong. Series is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem_stolen.c | 103 ++++++++++++++++++---------------
>  1 file changed, 56 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 7cc273e690d0..664afcffc41d 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -185,11 +185,8 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  	DRM_DEBUG_DRIVER("%s_STOLEN_RESERVED = %08x\n",
>  			 IS_GM45(dev_priv) ? "CTG" : "ELK", reg_val);
>  
> -	if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0) {
> -		*base = 0;
> -		*size = 0;
> +	if ((reg_val & G4X_STOLEN_RESERVED_ENABLE) == 0)
>  		return;
> -	}
>  
>  	/*
>  	 * Whether ILK really reuses the ELK register for this is unclear.
> @@ -197,18 +194,13 @@ static void g4x_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  	 */
>  	WARN(IS_GEN5(dev_priv), "ILK stolen reserved found? 0x%08x\n", reg_val);
>  
> -	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
> +	if (!(reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK))
> +		return;
>  
> +	*base = (reg_val & G4X_STOLEN_RESERVED_ADDR2_MASK) << 16;
>  	WARN_ON((reg_val & G4X_STOLEN_RESERVED_ADDR1_MASK) < *base);
>  
> -	/* On these platforms, the register doesn't have a size field, so the
> -	 * size is the distance between the base and the top of the stolen
> -	 * memory. We also have the genuine case where base is zero and there's
> -	 * nothing reserved. */
> -	if (*base == 0)
> -		*size = 0;
> -	else
> -		*size = stolen_top - *base;
> +	*size = stolen_top - *base;
>  }
>  
>  static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
> @@ -219,11 +211,8 @@ static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  
>  	DRM_DEBUG_DRIVER("GEN6_STOLEN_RESERVED = %08x\n", reg_val);
>  
> -	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> -		*base = 0;
> -		*size = 0;
> +	if (!(reg_val & GEN6_STOLEN_RESERVED_ENABLE))
>  		return;
> -	}
>  
>  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
>  
> @@ -246,6 +235,33 @@ static void gen6_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  	}
>  }
>  
> +static void vlv_get_stolen_reserved(struct drm_i915_private *dev_priv,
> +				    resource_size_t *base,
> +				    resource_size_t *size)
> +{
> +	u32 reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> +	resource_size_t stolen_top = dev_priv->dsm.end + 1;
> +
> +	DRM_DEBUG_DRIVER("GEN6_STOLEN_RESERVED = %08x\n", reg_val);
> +
> +	if (!(reg_val & GEN6_STOLEN_RESERVED_ENABLE))
> +		return;
> +
> +	switch (reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK) {
> +	default:
> +		MISSING_CASE(reg_val & GEN7_STOLEN_RESERVED_SIZE_MASK);
> +	case GEN7_STOLEN_RESERVED_1M:
> +		*size = 1024 * 1024;
> +		break;
> +	}
> +
> +	/*
> +	 * On vlv, the ADDR_MASK portion is left as 0 and HW deduces the
> +	 * reserved location as (top - size).
> +	 */
> +	*base = stolen_top - *size;
> +}
> +
>  static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  				     resource_size_t *base,
>  				     resource_size_t *size)
> @@ -254,11 +270,8 @@ static void gen7_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  
>  	DRM_DEBUG_DRIVER("GEN6_STOLEN_RESERVED = %08x\n", reg_val);
>  
> -	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> -		*base = 0;
> -		*size = 0;
> +	if (!(reg_val & GEN6_STOLEN_RESERVED_ENABLE))
>  		return;
> -	}
>  
>  	*base = reg_val & GEN7_STOLEN_RESERVED_ADDR_MASK;
>  
> @@ -283,11 +296,8 @@ static void chv_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  
>  	DRM_DEBUG_DRIVER("GEN6_STOLEN_RESERVED = %08x\n", reg_val);
>  
> -	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> -		*base = 0;
> -		*size = 0;
> +	if (!(reg_val & GEN6_STOLEN_RESERVED_ENABLE))
>  		return;
> -	}
>  
>  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
>  
> @@ -315,28 +325,18 @@ static void bdw_get_stolen_reserved(struct drm_i915_private *dev_priv,
>  				    resource_size_t *size)
>  {
>  	u32 reg_val = I915_READ(GEN6_STOLEN_RESERVED);
> -	resource_size_t stolen_top;
> +	resource_size_t stolen_top = dev_priv->dsm.end + 1;
>  
>  	DRM_DEBUG_DRIVER("GEN6_STOLEN_RESERVED = %08x\n", reg_val);
>  
> -	if ((reg_val & GEN6_STOLEN_RESERVED_ENABLE) == 0) {
> -		*base = 0;
> -		*size = 0;
> +	if (!(reg_val & GEN6_STOLEN_RESERVED_ENABLE))
>  		return;
> -	}
>  
> -	stolen_top = dev_priv->dsm.end + 1;
> +	if (!(reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK))
> +		return;
>  
>  	*base = reg_val & GEN6_STOLEN_RESERVED_ADDR_MASK;
> -
> -	/* On these platforms, the register doesn't have a size field, so the
> -	 * size is the distance between the base and the top of the stolen
> -	 * memory. We also have the genuine case where base is zero and there's
> -	 * nothing reserved. */
> -	if (*base == 0)
> -		*size = 0;
> -	else
> -		*size = stolen_top - *base;
> +	*size = stolen_top - *base;
>  }
>  
>  int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
> @@ -369,7 +369,7 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  	GEM_BUG_ON(dev_priv->dsm.end <= dev_priv->dsm.start);
>  
>  	stolen_top = dev_priv->dsm.end + 1;
> -	reserved_base = 0;
> +	reserved_base = stolen_top;
>  	reserved_size = 0;
>  
>  	switch (INTEL_GEN(dev_priv)) {
> @@ -389,8 +389,12 @@ int i915_gem_init_stolen(struct drm_i915_private *dev_priv)
>  					 &reserved_base, &reserved_size);
>  		break;
>  	case 7:
> -		gen7_get_stolen_reserved(dev_priv,
> -					 &reserved_base, &reserved_size);
> +		if (IS_VALLEYVIEW(dev_priv))
> +			vlv_get_stolen_reserved(dev_priv,
> +						&reserved_base, &reserved_size);
> +		else
> +			gen7_get_stolen_reserved(dev_priv,
> +						 &reserved_base, &reserved_size);
>  		break;
>  	default:
>  		if (IS_LP(dev_priv))
> @@ -402,11 +406,16 @@ 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;
> +	/*
> +	 * Our expectation is that the reserved space is at the top of the
> +	 * stolen region and *never* at the bottom. If we see !reserved_base,
> +	 * it likely means we failed to read the registers correctly.
> +	 */
> +	if (!reserved_base) {
> +		DRM_ERROR("inconsistent reservation %pa + %pa; ignoring\n",
> +			  &reserved_base, &reserved_size);
>  		reserved_base = stolen_top;
> +		reserved_size = 0;
>  	}
>  
>  	dev_priv->dsm_reserved =
> -- 
> 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

  reply	other threads:[~2018-03-16 12:00 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-12 16:52 [PATCH v2 1/3] drm/i915/stolen: Switch from DEBUG_KMS to DEBUG_DRIVER Chris Wilson
2018-03-12 16:52 ` [PATCH v2 2/3] drm/i915/stolen: Checkpatch cleansing Chris Wilson
2018-03-12 16:52 ` [PATCH v2 3/3] drm/i915/stolen: Deduce base of reserved portion as top-size on vlv Chris Wilson
2018-03-16 12:00   ` Ville Syrjälä [this message]
2018-03-16 12:17     ` Chris Wilson
2018-03-12 17:59 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/stolen: Switch from DEBUG_KMS to DEBUG_DRIVER Patchwork
2018-03-12 22:22 ` ✗ Fi.CI.IGT: warning " Patchwork

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180316120045.GF5453@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=chris@chris-wilson.co.uk \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.