[PATCH 0/8] SKL/KBL watermark fixes

[PATCH 0/8] SKL/KBL watermark fixes

[Paulo Zanoni]

Hi

This series is the result of me comparing the code against BSpec. It
doesn't solve any particular problem I was seeing, but there's enough
changes that this code could potentially change the watermarks values
for most machines. Maybe this will fix somebody's underrun? Even if it
doesn't, we should still have a code that matches the specification
unless there's a big justification.

I still didn't finish reviewing everything and there's at least one
workaround that we seem to be missing, but let's try to get these
patches merged first.

Thanks,
Paulo

Cc: Lyude <cpaul@redhat.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Mahesh Kumar <mahesh1.kumar@intel.com>

Paulo Zanoni (8):
  drm/i915: SAGV is not SKL-only, so rename a few things
  drm/i915: introduce intel_has_sagv()
  drm/i915/kbl: KBL also needs to run the SAGV code
  drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
  drm/i915/gen9: minimum scanlines for Y tile is not always 4
  drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations
  drm/i915/gen9: fix the watermark res_blocks value
  drm/i915/gen9: implement missing case for SKL watermarks calculation

 drivers/gpu/drm/i915/i915_drv.h      |  10 +-
 drivers/gpu/drm/i915/intel_display.c |   9 +-
 drivers/gpu/drm/i915/intel_drv.h     |   6 +-
 drivers/gpu/drm/i915/intel_pm.c      | 177 ++++++++++++++++++++---------------
 4 files changed, 111 insertions(+), 91 deletions(-)

-- 
2.7.4

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

[PATCH 1/8] drm/i915: SAGV is not SKL-only, so rename a few things

[Paulo Zanoni]

The plan is to introduce intel_has_sagv() and then use it to discover
which platforms actually support it.

I thought about keeping the functions with their current skl names,
but found two problems: (i) skl_has_sagv() would become a very
confusing name, and (ii) intel_atomic_commit_tail() doesn't seem to be
calling any functions whose name start with a platform name, so the
"intel_" naming scheme seems make more sense than the "firstplatorm_"
naming scheme here.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      | 10 +++++-----
 drivers/gpu/drm/i915/intel_display.c |  8 ++++----
 drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
 drivers/gpu/drm/i915/intel_pm.c      | 26 +++++++++++++-------------
 4 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 053a347..503c69d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1972,11 +1972,11 @@ struct drm_i915_private {
 	struct vlv_s0ix_state vlv_s0ix_state;
 
 	enum {
-		I915_SKL_SAGV_UNKNOWN = 0,
-		I915_SKL_SAGV_DISABLED,
-		I915_SKL_SAGV_ENABLED,
-		I915_SKL_SAGV_NOT_CONTROLLED
-	} skl_sagv_status;
+		I915_SAGV_UNKNOWN = 0,
+		I915_SAGV_DISABLED,
+		I915_SAGV_ENABLED,
+		I915_SAGV_NOT_CONTROLLED
+	} sagv_status;
 
 	struct {
 		/*
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6b4d7ac..4dd4961 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14379,8 +14379,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		 * SKL workaround: bspec recommends we disable the SAGV when we
 		 * have more then one pipe enabled
 		 */
-		if (IS_SKYLAKE(dev_priv) && !skl_can_enable_sagv(state))
-			skl_disable_sagv(dev_priv);
+		if (IS_SKYLAKE(dev_priv) && !intel_can_enable_sagv(state))
+			intel_disable_sagv(dev_priv);
 
 		intel_modeset_verify_disabled(dev);
 	}
@@ -14438,8 +14438,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
-	    skl_can_enable_sagv(state))
-		skl_enable_sagv(dev_priv);
+	    intel_can_enable_sagv(state))
+		intel_enable_sagv(dev_priv);
 
 	drm_atomic_helper_commit_hw_done(state);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d084c1b..bb55b61 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1741,9 +1741,9 @@ void ilk_wm_get_hw_state(struct drm_device *dev);
 void skl_wm_get_hw_state(struct drm_device *dev);
 void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 			  struct skl_ddb_allocation *ddb /* out */);
-bool skl_can_enable_sagv(struct drm_atomic_state *state);
-int skl_enable_sagv(struct drm_i915_private *dev_priv);
-int skl_disable_sagv(struct drm_i915_private *dev_priv);
+bool intel_can_enable_sagv(struct drm_atomic_state *state);
+int intel_enable_sagv(struct drm_i915_private *dev_priv);
+int intel_disable_sagv(struct drm_i915_private *dev_priv);
 bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
 			       const struct skl_ddb_allocation *new,
 			       enum pipe pipe);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 4f833a0..32588e3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2896,12 +2896,12 @@ skl_wm_plane_id(const struct intel_plane *plane)
  *  - We're not using an interlaced display configuration
  */
 int
-skl_enable_sagv(struct drm_i915_private *dev_priv)
+intel_enable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
-	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
-	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
+	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
+	    dev_priv->sagv_status == I915_SAGV_ENABLED)
 		return 0;
 
 	DRM_DEBUG_KMS("Enabling the SAGV\n");
@@ -2919,19 +2919,19 @@ skl_enable_sagv(struct drm_i915_private *dev_priv)
 	 */
 	if (ret == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
-		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
 	} else if (ret < 0) {
 		DRM_ERROR("Failed to enable the SAGV\n");
 		return ret;
 	}
 
-	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
+	dev_priv->sagv_status = I915_SAGV_ENABLED;
 	return 0;
 }
 
 static int
-skl_do_sagv_disable(struct drm_i915_private *dev_priv)
+intel_do_sagv_disable(struct drm_i915_private *dev_priv)
 {
 	int ret;
 	uint32_t temp = GEN9_SAGV_DISABLE;
@@ -2945,19 +2945,19 @@ skl_do_sagv_disable(struct drm_i915_private *dev_priv)
 }
 
 int
-skl_disable_sagv(struct drm_i915_private *dev_priv)
+intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret, result;
 
-	if (dev_priv->skl_sagv_status == I915_SKL_SAGV_NOT_CONTROLLED ||
-	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
+	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
+	    dev_priv->sagv_status == I915_SAGV_DISABLED)
 		return 0;
 
 	DRM_DEBUG_KMS("Disabling the SAGV\n");
 	mutex_lock(&dev_priv->rps.hw_lock);
 
 	/* bspec says to keep retrying for at least 1 ms */
-	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
+	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
 	if (ret == -ETIMEDOUT) {
@@ -2971,18 +2971,18 @@ skl_disable_sagv(struct drm_i915_private *dev_priv)
 	 */
 	if (result == -ENXIO) {
 		DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n");
-		dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED;
+		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
 		return 0;
 	} else if (result < 0) {
 		DRM_ERROR("Failed to disable the SAGV\n");
 		return result;
 	}
 
-	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
+	dev_priv->sagv_status = I915_SAGV_DISABLED;
 	return 0;
 }
 
-bool skl_can_enable_sagv(struct drm_atomic_state *state)
+bool intel_can_enable_sagv(struct drm_atomic_state *state)
 {
 	struct drm_device *dev = state->dev;
 	struct drm_i915_private *dev_priv = to_i915(dev);
-- 
2.7.4

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

Re: [PATCH 1/8] drm/i915: SAGV is not SKL-only, so rename a few things

[Lyude]

My only thought is that it seems like we prefix functions skl_, kbl_,
etc. just to indicate which generation introduced the feature. Skl uses
quite a few sandybridge and haswell functions. If this is a little
closer to what most intel devs would expect the naming to be though
then:

Reviewed-by: Lyude <cpaul@redhat.com>

going through the other patches now as well

On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> The plan is to introduce intel_has_sagv() and then use it to discover
> which platforms actually support it.
> 
> I thought about keeping the functions with their current skl names,
> but found two problems: (i) skl_has_sagv() would become a very
> confusing name, and (ii) intel_atomic_commit_tail() doesn't seem to
> be
> calling any functions whose name start with a platform name, so the
> "intel_" naming scheme seems make more sense than the "firstplatorm_"
> naming scheme here.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      | 10 +++++-----
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
>  drivers/gpu/drm/i915/intel_pm.c      | 26 +++++++++++++-------------
>  4 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 053a347..503c69d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1972,11 +1972,11 @@ struct drm_i915_private {
>  	struct vlv_s0ix_state vlv_s0ix_state;
>  
>  	enum {
> -		I915_SKL_SAGV_UNKNOWN = 0,
> -		I915_SKL_SAGV_DISABLED,
> -		I915_SKL_SAGV_ENABLED,
> -		I915_SKL_SAGV_NOT_CONTROLLED
> -	} skl_sagv_status;
> +		I915_SAGV_UNKNOWN = 0,
> +		I915_SAGV_DISABLED,
> +		I915_SAGV_ENABLED,
> +		I915_SAGV_NOT_CONTROLLED
> +	} sagv_status;
>  
>  	struct {
>  		/*
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 6b4d7ac..4dd4961 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14379,8 +14379,8 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  		 * SKL workaround: bspec recommends we disable the
> SAGV when we
>  		 * have more then one pipe enabled
>  		 */
> -		if (IS_SKYLAKE(dev_priv) &&
> !skl_can_enable_sagv(state))
> -			skl_disable_sagv(dev_priv);
> +		if (IS_SKYLAKE(dev_priv) &&
> !intel_can_enable_sagv(state))
> +			intel_disable_sagv(dev_priv);
>  
>  		intel_modeset_verify_disabled(dev);
>  	}
> @@ -14438,8 +14438,8 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  	}
>  
>  	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> -	    skl_can_enable_sagv(state))
> -		skl_enable_sagv(dev_priv);
> +	    intel_can_enable_sagv(state))
> +		intel_enable_sagv(dev_priv);
>  
>  	drm_atomic_helper_commit_hw_done(state);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d084c1b..bb55b61 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1741,9 +1741,9 @@ void ilk_wm_get_hw_state(struct drm_device
> *dev);
>  void skl_wm_get_hw_state(struct drm_device *dev);
>  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  			  struct skl_ddb_allocation *ddb /* out */);
> -bool skl_can_enable_sagv(struct drm_atomic_state *state);
> -int skl_enable_sagv(struct drm_i915_private *dev_priv);
> -int skl_disable_sagv(struct drm_i915_private *dev_priv);
> +bool intel_can_enable_sagv(struct drm_atomic_state *state);
> +int intel_enable_sagv(struct drm_i915_private *dev_priv);
> +int intel_disable_sagv(struct drm_i915_private *dev_priv);
>  bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old,
>  			       const struct skl_ddb_allocation *new,
>  			       enum pipe pipe);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 4f833a0..32588e3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2896,12 +2896,12 @@ skl_wm_plane_id(const struct intel_plane
> *plane)
>   *  - We're not using an interlaced display configuration
>   */
>  int
> -skl_enable_sagv(struct drm_i915_private *dev_priv)
> +intel_enable_sagv(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  
> -	if (dev_priv->skl_sagv_status ==
> I915_SKL_SAGV_NOT_CONTROLLED ||
> -	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
> +	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
> +	    dev_priv->sagv_status == I915_SAGV_ENABLED)
>  		return 0;
>  
>  	DRM_DEBUG_KMS("Enabling the SAGV\n");
> @@ -2919,19 +2919,19 @@ skl_enable_sagv(struct drm_i915_private
> *dev_priv)
>  	 */
>  	if (ret == -ENXIO) {
>  		DRM_DEBUG_DRIVER("No SAGV found on system,
> ignoring\n");
> -		dev_priv->skl_sagv_status =
> I915_SKL_SAGV_NOT_CONTROLLED;
> +		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
>  		return 0;
>  	} else if (ret < 0) {
>  		DRM_ERROR("Failed to enable the SAGV\n");
>  		return ret;
>  	}
>  
> -	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
> +	dev_priv->sagv_status = I915_SAGV_ENABLED;
>  	return 0;
>  }
>  
>  static int
> -skl_do_sagv_disable(struct drm_i915_private *dev_priv)
> +intel_do_sagv_disable(struct drm_i915_private *dev_priv)
>  {
>  	int ret;
>  	uint32_t temp = GEN9_SAGV_DISABLE;
> @@ -2945,19 +2945,19 @@ skl_do_sagv_disable(struct drm_i915_private
> *dev_priv)
>  }
>  
>  int
> -skl_disable_sagv(struct drm_i915_private *dev_priv)
> +intel_disable_sagv(struct drm_i915_private *dev_priv)
>  {
>  	int ret, result;
>  
> -	if (dev_priv->skl_sagv_status ==
> I915_SKL_SAGV_NOT_CONTROLLED ||
> -	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
> +	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
> +	    dev_priv->sagv_status == I915_SAGV_DISABLED)
>  		return 0;
>  
>  	DRM_DEBUG_KMS("Disabling the SAGV\n");
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  
>  	/* bspec says to keep retrying for at least 1 ms */
> -	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> +	ret = wait_for(result = intel_do_sagv_disable(dev_priv), 1);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
>  	if (ret == -ETIMEDOUT) {
> @@ -2971,18 +2971,18 @@ skl_disable_sagv(struct drm_i915_private
> *dev_priv)
>  	 */
>  	if (result == -ENXIO) {
>  		DRM_DEBUG_DRIVER("No SAGV found on system,
> ignoring\n");
> -		dev_priv->skl_sagv_status =
> I915_SKL_SAGV_NOT_CONTROLLED;
> +		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
>  		return 0;
>  	} else if (result < 0) {
>  		DRM_ERROR("Failed to disable the SAGV\n");
>  		return result;
>  	}
>  
> -	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
> +	dev_priv->sagv_status = I915_SAGV_DISABLED;
>  	return 0;
>  }
>  
> -bool skl_can_enable_sagv(struct drm_atomic_state *state)
> +bool intel_can_enable_sagv(struct drm_atomic_state *state)
>  {
>  	struct drm_device *dev = state->dev;
>  	struct drm_i915_private *dev_priv = to_i915(dev);
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/8] drm/i915: SAGV is not SKL-only, so rename a few things

[Zanoni, Paulo R]

Em Qua, 2016-09-07 às 12:05 -0400, Lyude escreveu:
> My only thought is that it seems like we prefix functions skl_, kbl_,
> etc. just to indicate which generation introduced the feature. Skl
> uses
> quite a few sandybridge and haswell functions. If this is a little
> closer to what most intel devs would expect the naming to be though
> then:

We actually have both schemes, but my understanding is that
firstplatform_something is more used when we actually need 2+ functions
(like i9xx_crtc_enable and then ilk_crtc_enable), and we usually have
something like intel_something calling firstplatform_something, or just
dev_priv->something() calling the firstplatform_something vfunc. But my
understanding may be wrong.

Anyway, I just did the rename because skl_has_sagv() would be super
confusing (returns true on KBL). I'm happy to accept suggestions here
that would avoid the renaming I did.

> 
> Reviewed-by: Lyude <cpaul@redhat.com>
> 
> going through the other patches now as well
> 
> On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> > 
> > The plan is to introduce intel_has_sagv() and then use it to
> > discover
> > which platforms actually support it.
> > 
> > I thought about keeping the functions with their current skl names,
> > but found two problems: (i) skl_has_sagv() would become a very
> > confusing name, and (ii) intel_atomic_commit_tail() doesn't seem to
> > be
> > calling any functions whose name start with a platform name, so the
> > "intel_" naming scheme seems make more sense than the
> > "firstplatorm_"
> > naming scheme here.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      | 10 +++++-----
> >  drivers/gpu/drm/i915/intel_display.c |  8 ++++----
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 +++---
> >  drivers/gpu/drm/i915/intel_pm.c      | 26 +++++++++++++-----------
> > --
> >  4 files changed, 25 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 053a347..503c69d 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1972,11 +1972,11 @@ struct drm_i915_private {
> >  	struct vlv_s0ix_state vlv_s0ix_state;
> >  
> >  	enum {
> > -		I915_SKL_SAGV_UNKNOWN = 0,
> > -		I915_SKL_SAGV_DISABLED,
> > -		I915_SKL_SAGV_ENABLED,
> > -		I915_SKL_SAGV_NOT_CONTROLLED
> > -	} skl_sagv_status;
> > +		I915_SAGV_UNKNOWN = 0,
> > +		I915_SAGV_DISABLED,
> > +		I915_SAGV_ENABLED,
> > +		I915_SAGV_NOT_CONTROLLED
> > +	} sagv_status;
> >  
> >  	struct {
> >  		/*
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 6b4d7ac..4dd4961 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14379,8 +14379,8 @@ static void intel_atomic_commit_tail(struct
> > drm_atomic_state *state)
> >  		 * SKL workaround: bspec recommends we disable the
> > SAGV when we
> >  		 * have more then one pipe enabled
> >  		 */
> > -		if (IS_SKYLAKE(dev_priv) &&
> > !skl_can_enable_sagv(state))
> > -			skl_disable_sagv(dev_priv);
> > +		if (IS_SKYLAKE(dev_priv) &&
> > !intel_can_enable_sagv(state))
> > +			intel_disable_sagv(dev_priv);
> >  
> >  		intel_modeset_verify_disabled(dev);
> >  	}
> > @@ -14438,8 +14438,8 @@ static void intel_atomic_commit_tail(struct
> > drm_atomic_state *state)
> >  	}
> >  
> >  	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> > -	    skl_can_enable_sagv(state))
> > -		skl_enable_sagv(dev_priv);
> > +	    intel_can_enable_sagv(state))
> > +		intel_enable_sagv(dev_priv);
> >  
> >  	drm_atomic_helper_commit_hw_done(state);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d084c1b..bb55b61 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1741,9 +1741,9 @@ void ilk_wm_get_hw_state(struct drm_device
> > *dev);
> >  void skl_wm_get_hw_state(struct drm_device *dev);
> >  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
> >  			  struct skl_ddb_allocation *ddb /* out
> > */);
> > -bool skl_can_enable_sagv(struct drm_atomic_state *state);
> > -int skl_enable_sagv(struct drm_i915_private *dev_priv);
> > -int skl_disable_sagv(struct drm_i915_private *dev_priv);
> > +bool intel_can_enable_sagv(struct drm_atomic_state *state);
> > +int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > +int intel_disable_sagv(struct drm_i915_private *dev_priv);
> >  bool skl_ddb_allocation_equals(const struct skl_ddb_allocation
> > *old,
> >  			       const struct skl_ddb_allocation
> > *new,
> >  			       enum pipe pipe);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 4f833a0..32588e3 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2896,12 +2896,12 @@ skl_wm_plane_id(const struct intel_plane
> > *plane)
> >   *  - We're not using an interlaced display configuration
> >   */
> >  int
> > -skl_enable_sagv(struct drm_i915_private *dev_priv)
> > +intel_enable_sagv(struct drm_i915_private *dev_priv)
> >  {
> >  	int ret;
> >  
> > -	if (dev_priv->skl_sagv_status ==
> > I915_SKL_SAGV_NOT_CONTROLLED ||
> > -	    dev_priv->skl_sagv_status == I915_SKL_SAGV_ENABLED)
> > +	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
> > +	    dev_priv->sagv_status == I915_SAGV_ENABLED)
> >  		return 0;
> >  
> >  	DRM_DEBUG_KMS("Enabling the SAGV\n");
> > @@ -2919,19 +2919,19 @@ skl_enable_sagv(struct drm_i915_private
> > *dev_priv)
> >  	 */
> >  	if (ret == -ENXIO) {
> >  		DRM_DEBUG_DRIVER("No SAGV found on system,
> > ignoring\n");
> > -		dev_priv->skl_sagv_status =
> > I915_SKL_SAGV_NOT_CONTROLLED;
> > +		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
> >  		return 0;
> >  	} else if (ret < 0) {
> >  		DRM_ERROR("Failed to enable the SAGV\n");
> >  		return ret;
> >  	}
> >  
> > -	dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED;
> > +	dev_priv->sagv_status = I915_SAGV_ENABLED;
> >  	return 0;
> >  }
> >  
> >  static int
> > -skl_do_sagv_disable(struct drm_i915_private *dev_priv)
> > +intel_do_sagv_disable(struct drm_i915_private *dev_priv)
> >  {
> >  	int ret;
> >  	uint32_t temp = GEN9_SAGV_DISABLE;
> > @@ -2945,19 +2945,19 @@ skl_do_sagv_disable(struct drm_i915_private
> > *dev_priv)
> >  }
> >  
> >  int
> > -skl_disable_sagv(struct drm_i915_private *dev_priv)
> > +intel_disable_sagv(struct drm_i915_private *dev_priv)
> >  {
> >  	int ret, result;
> >  
> > -	if (dev_priv->skl_sagv_status ==
> > I915_SKL_SAGV_NOT_CONTROLLED ||
> > -	    dev_priv->skl_sagv_status == I915_SKL_SAGV_DISABLED)
> > +	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
> > +	    dev_priv->sagv_status == I915_SAGV_DISABLED)
> >  		return 0;
> >  
> >  	DRM_DEBUG_KMS("Disabling the SAGV\n");
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  
> >  	/* bspec says to keep retrying for at least 1 ms */
> > -	ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1);
> > +	ret = wait_for(result = intel_do_sagv_disable(dev_priv),
> > 1);
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> >  
> >  	if (ret == -ETIMEDOUT) {
> > @@ -2971,18 +2971,18 @@ skl_disable_sagv(struct drm_i915_private
> > *dev_priv)
> >  	 */
> >  	if (result == -ENXIO) {
> >  		DRM_DEBUG_DRIVER("No SAGV found on system,
> > ignoring\n");
> > -		dev_priv->skl_sagv_status =
> > I915_SKL_SAGV_NOT_CONTROLLED;
> > +		dev_priv->sagv_status = I915_SAGV_NOT_CONTROLLED;
> >  		return 0;
> >  	} else if (result < 0) {
> >  		DRM_ERROR("Failed to disable the SAGV\n");
> >  		return result;
> >  	}
> >  
> > -	dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED;
> > +	dev_priv->sagv_status = I915_SAGV_DISABLED;
> >  	return 0;
> >  }
> >  
> > -bool skl_can_enable_sagv(struct drm_atomic_state *state)
> > +bool intel_can_enable_sagv(struct drm_atomic_state *state)
> >  {
> >  	struct drm_device *dev = state->dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Paulo Zanoni]

And use it to move knowledge about the SAGV-supporting platforms from
the callers to the SAGV code.

We'll add more platforms to intel_has_sagv(), so IMHO it makes more
sense to move all this to a single function instead of patching all
the callers every time we add SAGV support to a new platform.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  5 ++---
 drivers/gpu/drm/i915/intel_pm.c      | 15 +++++++++++++++
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4dd4961..2442ab2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14379,7 +14379,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		 * SKL workaround: bspec recommends we disable the SAGV when we
 		 * have more then one pipe enabled
 		 */
-		if (IS_SKYLAKE(dev_priv) && !intel_can_enable_sagv(state))
+		if (!intel_can_enable_sagv(state))
 			intel_disable_sagv(dev_priv);
 
 		intel_modeset_verify_disabled(dev);
@@ -14437,8 +14437,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
 		intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state);
 	}
 
-	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
-	    intel_can_enable_sagv(state))
+	if (intel_state->modeset && intel_can_enable_sagv(state))
 		intel_enable_sagv(dev_priv);
 
 	drm_atomic_helper_commit_hw_done(state);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 32588e3..af75011 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2884,6 +2884,12 @@ skl_wm_plane_id(const struct intel_plane *plane)
 	}
 }
 
+static bool
+intel_has_sagv(struct drm_i915_private *dev_priv)
+{
+	return IS_SKYLAKE(dev_priv);
+}
+
 /*
  * SAGV dynamically adjusts the system agent voltage and clock frequencies
  * depending on power and performance requirements. The display engine access
@@ -2900,6 +2906,9 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret;
 
+	if (!intel_has_sagv(dev_priv))
+		return 0;
+
 	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
 	    dev_priv->sagv_status == I915_SAGV_ENABLED)
 		return 0;
@@ -2949,6 +2958,9 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
 {
 	int ret, result;
 
+	if (!intel_has_sagv(dev_priv))
+		return 0;
+
 	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
 	    dev_priv->sagv_status == I915_SAGV_DISABLED)
 		return 0;
@@ -2991,6 +3003,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
 	enum pipe pipe;
 	int level, plane;
 
+	if (!intel_has_sagv(dev_priv))
+		return false;
+
 	/*
 	 * SKL workaround: bspec recommends we disable the SAGV when we have
 	 * more then one pipe enabled
-- 
2.7.4

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

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Lyude]

On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> And use it to move knowledge about the SAGV-supporting platforms from
> the callers to the SAGV code.
> 
> We'll add more platforms to intel_has_sagv(), so IMHO it makes more
> sense to move all this to a single function instead of patching all
> the callers every time we add SAGV support to a new platform.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  5 ++---
>  drivers/gpu/drm/i915/intel_pm.c      | 15 +++++++++++++++
>  2 files changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4dd4961..2442ab2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14379,7 +14379,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  		 * SKL workaround: bspec recommends we disable the
> SAGV when we
>  		 * have more then one pipe enabled
>  		 */
> -		if (IS_SKYLAKE(dev_priv) &&
> !intel_can_enable_sagv(state))
> +		if (!intel_can_enable_sagv(state))
>  			intel_disable_sagv(dev_priv);
>  
>  		intel_modeset_verify_disabled(dev);
> @@ -14437,8 +14437,7 @@ static void intel_atomic_commit_tail(struct
> drm_atomic_state *state)
>  		intel_modeset_verify_crtc(crtc, old_crtc_state,
> crtc->state);
>  	}
>  
> -	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> -	    intel_can_enable_sagv(state))
> +	if (intel_state->modeset && intel_can_enable_sagv(state))
>  		intel_enable_sagv(dev_priv);
>  
>  	drm_atomic_helper_commit_hw_done(state);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 32588e3..af75011 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2884,6 +2884,12 @@ skl_wm_plane_id(const struct intel_plane
> *plane)
>  	}
>  }
>  
> +static bool
> +intel_has_sagv(struct drm_i915_private *dev_priv)
> +{
> +	return IS_SKYLAKE(dev_priv);
> +}
> +

Not sure I agree on this one. Even if a system is skylake or kabylake,
there's a couple of very early skylake machines that don't actually
have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value we set
if we get mailbox errors.

So if we're going to split SAGV detection into a different function, I
would also move the dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED
check into there:

if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
	return false;
if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
	return false;

return true;

>  /*
>   * SAGV dynamically adjusts the system agent voltage and clock
> frequencies
>   * depending on power and performance requirements. The display
> engine access
> @@ -2900,6 +2906,9 @@ intel_enable_sagv(struct drm_i915_private
> *dev_priv)
>  {
>  	int ret;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return 0;
> +
>  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
>  	    dev_priv->sagv_status == I915_SAGV_ENABLED)
>  		return 0;
> @@ -2949,6 +2958,9 @@ intel_disable_sagv(struct drm_i915_private
> *dev_priv)
>  {
>  	int ret, result;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return 0;
> +
>  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
>  	    dev_priv->sagv_status == I915_SAGV_DISABLED)
>  		return 0;
> @@ -2991,6 +3003,9 @@ bool intel_can_enable_sagv(struct
> drm_atomic_state *state)
>  	enum pipe pipe;
>  	int level, plane;
>  
> +	if (!intel_has_sagv(dev_priv))
> +		return false;
> +
>  	/*
>  	 * SKL workaround: bspec recommends we disable the SAGV when
> we have
>  	 * more then one pipe enabled
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Jani Nikula]

On Wed, 07 Sep 2016, Lyude <cpaul@redhat.com> wrote:
> On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
>> And use it to move knowledge about the SAGV-supporting platforms from
>> the callers to the SAGV code.
>> 
>> We'll add more platforms to intel_has_sagv(), so IMHO it makes more
>> sense to move all this to a single function instead of patching all
>> the callers every time we add SAGV support to a new platform.
>> 
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |  5 ++---
>>  drivers/gpu/drm/i915/intel_pm.c      | 15 +++++++++++++++
>>  2 files changed, 17 insertions(+), 3 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 4dd4961..2442ab2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14379,7 +14379,7 @@ static void intel_atomic_commit_tail(struct
>> drm_atomic_state *state)
>>  		 * SKL workaround: bspec recommends we disable the
>> SAGV when we
>>  		 * have more then one pipe enabled
>>  		 */
>> -		if (IS_SKYLAKE(dev_priv) &&
>> !intel_can_enable_sagv(state))
>> +		if (!intel_can_enable_sagv(state))
>>  			intel_disable_sagv(dev_priv);
>>  
>>  		intel_modeset_verify_disabled(dev);
>> @@ -14437,8 +14437,7 @@ static void intel_atomic_commit_tail(struct
>> drm_atomic_state *state)
>>  		intel_modeset_verify_crtc(crtc, old_crtc_state,
>> crtc->state);
>>  	}
>>  
>> -	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
>> -	    intel_can_enable_sagv(state))
>> +	if (intel_state->modeset && intel_can_enable_sagv(state))
>>  		intel_enable_sagv(dev_priv);
>>  
>>  	drm_atomic_helper_commit_hw_done(state);
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c
>> b/drivers/gpu/drm/i915/intel_pm.c
>> index 32588e3..af75011 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2884,6 +2884,12 @@ skl_wm_plane_id(const struct intel_plane
>> *plane)
>>  	}
>>  }
>>  
>> +static bool
>> +intel_has_sagv(struct drm_i915_private *dev_priv)
>> +{
>> +	return IS_SKYLAKE(dev_priv);
>> +}
>> +
>
> Not sure I agree on this one. Even if a system is skylake or kabylake,
> there's a couple of very early skylake machines that don't actually
> have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value we set
> if we get mailbox errors.

If by "very early" you mean pre-production, we don't care.

BR,
Jani.


>
> So if we're going to split SAGV detection into a different function, I
> would also move the dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED
> check into there:
>
> if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
> 	return false;
> if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> 	return false;
>
> return true;
>
>>  /*
>>   * SAGV dynamically adjusts the system agent voltage and clock
>> frequencies
>>   * depending on power and performance requirements. The display
>> engine access
>> @@ -2900,6 +2906,9 @@ intel_enable_sagv(struct drm_i915_private
>> *dev_priv)
>>  {
>>  	int ret;
>>  
>> +	if (!intel_has_sagv(dev_priv))
>> +		return 0;
>> +
>>  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
>>  	    dev_priv->sagv_status == I915_SAGV_ENABLED)
>>  		return 0;
>> @@ -2949,6 +2958,9 @@ intel_disable_sagv(struct drm_i915_private
>> *dev_priv)
>>  {
>>  	int ret, result;
>>  
>> +	if (!intel_has_sagv(dev_priv))
>> +		return 0;
>> +
>>  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
>>  	    dev_priv->sagv_status == I915_SAGV_DISABLED)
>>  		return 0;
>> @@ -2991,6 +3003,9 @@ bool intel_can_enable_sagv(struct
>> drm_atomic_state *state)
>>  	enum pipe pipe;
>>  	int level, plane;
>>  
>> +	if (!intel_has_sagv(dev_priv))
>> +		return false;
>> +
>>  	/*
>>  	 * SKL workaround: bspec recommends we disable the SAGV when
>> we have
>>  	 * more then one pipe enabled

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Lyude Paul]

On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
> On Wed, 07 Sep 2016, Lyude <cpaul@redhat.com> wrote:
> > 
> > On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> > > 
> > > And use it to move knowledge about the SAGV-supporting platforms from
> > > the callers to the SAGV code.
> > > 
> > > We'll add more platforms to intel_has_sagv(), so IMHO it makes more
> > > sense to move all this to a single function instead of patching all
> > > the callers every time we add SAGV support to a new platform.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |  5 ++---
> > >  drivers/gpu/drm/i915/intel_pm.c      | 15 +++++++++++++++
> > >  2 files changed, 17 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 4dd4961..2442ab2 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14379,7 +14379,7 @@ static void intel_atomic_commit_tail(struct
> > > drm_atomic_state *state)
> > >  		 * SKL workaround: bspec recommends we disable the
> > > SAGV when we
> > >  		 * have more then one pipe enabled
> > >  		 */
> > > -		if (IS_SKYLAKE(dev_priv) &&
> > > !intel_can_enable_sagv(state))
> > > +		if (!intel_can_enable_sagv(state))
> > >  			intel_disable_sagv(dev_priv);
> > >  
> > >  		intel_modeset_verify_disabled(dev);
> > > @@ -14437,8 +14437,7 @@ static void intel_atomic_commit_tail(struct
> > > drm_atomic_state *state)
> > >  		intel_modeset_verify_crtc(crtc, old_crtc_state,
> > > crtc->state);
> > >  	}
> > >  
> > > -	if (IS_SKYLAKE(dev_priv) && intel_state->modeset &&
> > > -	    intel_can_enable_sagv(state))
> > > +	if (intel_state->modeset && intel_can_enable_sagv(state))
> > >  		intel_enable_sagv(dev_priv);
> > >  
> > >  	drm_atomic_helper_commit_hw_done(state);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 32588e3..af75011 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2884,6 +2884,12 @@ skl_wm_plane_id(const struct intel_plane
> > > *plane)
> > >  	}
> > >  }
> > >  
> > > +static bool
> > > +intel_has_sagv(struct drm_i915_private *dev_priv)
> > > +{
> > > +	return IS_SKYLAKE(dev_priv);
> > > +}
> > > +
> > 
> > Not sure I agree on this one. Even if a system is skylake or kabylake,
> > there's a couple of very early skylake machines that don't actually
> > have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value we set
> > if we get mailbox errors.
> 
> If by "very early" you mean pre-production, we don't care.

The problem is if we don't handle that case though then a couple of the machines
in CI start failing tests since all of the SAGV mailbox commands don't end up
working :(

> 
> BR,
> Jani.
> 
> 
> > 
> > 
> > So if we're going to split SAGV detection into a different function, I
> > would also move the dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED
> > check into there:
> > 
> > if (!IS_SKYLAKE(dev_priv) && !IS_KABYLAKE(dev_priv))
> > 	return false;
> > if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> > 	return false;
> > 
> > return true;
> > 
> > > 
> > >  /*
> > >   * SAGV dynamically adjusts the system agent voltage and clock
> > > frequencies
> > >   * depending on power and performance requirements. The display
> > > engine access
> > > @@ -2900,6 +2906,9 @@ intel_enable_sagv(struct drm_i915_private
> > > *dev_priv)
> > >  {
> > >  	int ret;
> > >  
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return 0;
> > > +
> > >  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
> > >  	    dev_priv->sagv_status == I915_SAGV_ENABLED)
> > >  		return 0;
> > > @@ -2949,6 +2958,9 @@ intel_disable_sagv(struct drm_i915_private
> > > *dev_priv)
> > >  {
> > >  	int ret, result;
> > >  
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return 0;
> > > +
> > >  	if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED ||
> > >  	    dev_priv->sagv_status == I915_SAGV_DISABLED)
> > >  		return 0;
> > > @@ -2991,6 +3003,9 @@ bool intel_can_enable_sagv(struct
> > > drm_atomic_state *state)
> > >  	enum pipe pipe;
> > >  	int level, plane;
> > >  
> > > +	if (!intel_has_sagv(dev_priv))
> > > +		return false;
> > > +
> > >  	/*
> > >  	 * SKL workaround: bspec recommends we disable the SAGV when
> > > we have
> > >  	 * more then one pipe enabled
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Jani Nikula]

On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote:
> On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
>> On Wed, 07 Sep 2016, Lyude <cpaul@redhat.com> wrote:
>> > 
>> > On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
>> > > +static bool
>> > > +intel_has_sagv(struct drm_i915_private *dev_priv)
>> > > +{
>> > > +	return IS_SKYLAKE(dev_priv);
>> > > +}
>> > > +
>> > 
>> > Not sure I agree on this one. Even if a system is skylake or kabylake,
>> > there's a couple of very early skylake machines that don't actually
>> > have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value we set
>> > if we get mailbox errors.
>> 
>> If by "very early" you mean pre-production, we don't care.
>
> The problem is if we don't handle that case though then a couple of
> the machines in CI start failing tests since all of the SAGV mailbox
> commands don't end up working :(

Regardless of whose CI you refer to, no pre-production machines should
be used for CI. Which machines are these?

Can we be sure all production machines have SAGV?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Zanoni, Paulo R]

Em Sex, 2016-09-09 às 11:06 +0300, Jani Nikula escreveu:
> On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote:
> > 
> > On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
> > > 
> > > On Wed, 07 Sep 2016, Lyude <cpaul@redhat.com> wrote:
> > > > 
> > > > 
> > > > On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> > > > > 
> > > > > +static bool
> > > > > +intel_has_sagv(struct drm_i915_private *dev_priv)
> > > > > +{
> > > > > +	return IS_SKYLAKE(dev_priv);
> > > > > +}
> > > > > +
> > > > 
> > > > Not sure I agree on this one. Even if a system is skylake or
> > > > kabylake,
> > > > there's a couple of very early skylake machines that don't
> > > > actually
> > > > have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value
> > > > we set
> > > > if we get mailbox errors.
> > > 
> > > If by "very early" you mean pre-production, we don't care.

Ok, so I'd like some clarification regarding this from the maintainers.
I always thought we didn't really care, but do this:

$ git grep _REVID_

If we don't care, why do we have this? Newer platforms also have this.
And many of these REVID checks are only pre-prod.

> > 
> > The problem is if we don't handle that case though then a couple of
> > the machines in CI start failing tests since all of the SAGV
> > mailbox
> > commands don't end up working :(
> 
> Regardless of whose CI you refer to, no pre-production machines
> should
> be used for CI. Which machines are these?

I suppose he's talking about our CI.

> 
> Can we be sure all production machines have SAGV?

Our specs don't mention anything regarding this. I'll have to ask for
clarification, but I don't think it will be a good idea to remove the
code if CI starts complaining.

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

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Ville Syrjälä]

On Fri, Sep 09, 2016 at 07:51:15PM +0000, Zanoni, Paulo R wrote:
> Em Sex, 2016-09-09 às 11:06 +0300, Jani Nikula escreveu:
> > On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote:
> > > 
> > > On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
> > > > 
> > > > On Wed, 07 Sep 2016, Lyude <cpaul@redhat.com> wrote:
> > > > > 
> > > > > 
> > > > > On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> > > > > > 
> > > > > > +static bool
> > > > > > +intel_has_sagv(struct drm_i915_private *dev_priv)
> > > > > > +{
> > > > > > +	return IS_SKYLAKE(dev_priv);
> > > > > > +}
> > > > > > +
> > > > > 
> > > > > Not sure I agree on this one. Even if a system is skylake or
> > > > > kabylake,
> > > > > there's a couple of very early skylake machines that don't
> > > > > actually
> > > > > have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value
> > > > > we set
> > > > > if we get mailbox errors.
> > > > 
> > > > If by "very early" you mean pre-production, we don't care.
> 
> Ok, so I'd like some clarification regarding this from the maintainers.
> I always thought we didn't really care, but do this:
> 
> $ git grep _REVID_
> 
> If we don't care, why do we have this? Newer platforms also have this.
> And many of these REVID checks are only pre-prod.

For some reason no one has stepped up to remove them.

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

Re: [PATCH 2/8] drm/i915: introduce intel_has_sagv()

[Jani Nikula]

On Sat, 10 Sep 2016, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Fri, Sep 09, 2016 at 07:51:15PM +0000, Zanoni, Paulo R wrote:
>> Em Sex, 2016-09-09 às 11:06 +0300, Jani Nikula escreveu:
>> > On Thu, 08 Sep 2016, Lyude Paul <cpaul@redhat.com> wrote:
>> > > 
>> > > On Thu, 2016-09-08 at 11:59 +0300, Jani Nikula wrote:
>> > > > 
>> > > > On Wed, 07 Sep 2016, Lyude <cpaul@redhat.com> wrote:
>> > > > > 
>> > > > > 
>> > > > > On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
>> > > > > > 
>> > > > > > +static bool
>> > > > > > +intel_has_sagv(struct drm_i915_private *dev_priv)
>> > > > > > +{
>> > > > > > +	return IS_SKYLAKE(dev_priv);
>> > > > > > +}
>> > > > > > +
>> > > > > 
>> > > > > Not sure I agree on this one. Even if a system is skylake or
>> > > > > kabylake,
>> > > > > there's a couple of very early skylake machines that don't
>> > > > > actually
>> > > > > have an SAGV on them. Hence the I915_SAGV_NOT_CONTROLLED value
>> > > > > we set
>> > > > > if we get mailbox errors.
>> > > > 
>> > > > If by "very early" you mean pre-production, we don't care.
>> 
>> Ok, so I'd like some clarification regarding this from the maintainers.
>> I always thought we didn't really care, but do this:
>> 
>> $ git grep _REVID_
>> 
>> If we don't care, why do we have this? Newer platforms also have this.
>> And many of these REVID checks are only pre-prod.
>
> For some reason no one has stepped up to remove them.

All the pre-production revid checks for each platform should be thrown
out when production hardware is readily available and in CI. We've had
this discussion before [1], but nobody has still stepped up. Perhaps
because this would involve checking and double checking what the first
shipped revision really was...

We're just hurting ourselves catering for a tiny portion of people who
still have pre-production hardware laying around. We should just warn
and taint the kernel for them.

BR,
Jani.


[1] http://mid.mail-archive.com/87inykiz6f.fsf@intel.com



-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Paulo Zanoni]

According to BSpec, it's the "core CPUs" that need the code, which
means SKL and KBL, but not BXT.

I don't have a KBL to test this patch on it.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index af75011..baacd95 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2887,7 +2887,7 @@ skl_wm_plane_id(const struct intel_plane *plane)
 static bool
 intel_has_sagv(struct drm_i915_private *dev_priv)
 {
-	return IS_SKYLAKE(dev_priv);
+	return IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv);
 }
 
 /*
-- 
2.7.4

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

Re: [PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Ville Syrjälä]

On Tue, Sep 06, 2016 at 09:52:14PM -0300, Paulo Zanoni wrote:
> According to BSpec, it's the "core CPUs" that need the code, which
> means SKL and KBL, but not BXT.
> 
> I don't have a KBL to test this patch on it.

IIRC bspec doesn't specify the sagv latency for anything but
SKL, and the relevant w/a was only listed for SKL as well. So not sure
this is correct.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index af75011..baacd95 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2887,7 +2887,7 @@ skl_wm_plane_id(const struct intel_plane *plane)
>  static bool
>  intel_has_sagv(struct drm_i915_private *dev_priv)
>  {
> -	return IS_SKYLAKE(dev_priv);
> +	return IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv);
>  }
>  
>  /*
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Lyude]

I'm not sure that kbl has this either. The kbl machine I've been
working with thus-far has passed a few modesetting stress tests with
the chameleon, and I don't have anything trying to control sagv stuff
on it.

This being said though the sagv for skylake did happen to get added
right before release and wasn't in any SDPs, so even so we should keep
our eyes out when kbl starts shipping…

On Wed, 2016-09-07 at 19:11 +0300, Ville Syrjälä wrote:
> On Tue, Sep 06, 2016 at 09:52:14PM -0300, Paulo Zanoni wrote:
> > 
> > According to BSpec, it's the "core CPUs" that need the code, which
> > means SKL and KBL, but not BXT.
> > 
> > I don't have a KBL to test this patch on it.
> 
> IIRC bspec doesn't specify the sagv latency for anything but
> SKL, and the relevant w/a was only listed for SKL as well. So not
> sure
> this is correct.
> 
> > 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index af75011..baacd95 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2887,7 +2887,7 @@ skl_wm_plane_id(const struct intel_plane
> > *plane)
> >  static bool
> >  intel_has_sagv(struct drm_i915_private *dev_priv)
> >  {
> > -	return IS_SKYLAKE(dev_priv);
> > +	return IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv);
> >  }
> >  
> >  /*
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Zanoni, Paulo R]

Em Qua, 2016-09-07 às 12:17 -0400, Lyude escreveu:
> I'm not sure that kbl has this either. The kbl machine I've been
> working with thus-far has passed a few modesetting stress tests with
> the chameleon, and I don't have anything trying to control sagv stuff
> on it.
> 
> This being said though the sagv for skylake did happen to get added
> right before release and wasn't in any SDPs, so even so we should
> keep
> our eyes out when kbl starts shipping…

I got confirmation from the Hardware guys that KBL does need to run the
SAGV code, and it has the same latency as SKL. Also, all SKL production
steppings need to run the SAGV code.

> 
> On Wed, 2016-09-07 at 19:11 +0300, Ville Syrjälä wrote:
> > 
> > On Tue, Sep 06, 2016 at 09:52:14PM -0300, Paulo Zanoni wrote:
> > > 
> > > 
> > > According to BSpec, it's the "core CPUs" that need the code,
> > > which
> > > means SKL and KBL, but not BXT.
> > > 
> > > I don't have a KBL to test this patch on it.
> > 
> > IIRC bspec doesn't specify the sagv latency for anything but
> > SKL, and the relevant w/a was only listed for SKL as well. So not
> > sure
> > this is correct.
> > 
> > > 
> > > 
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index af75011..baacd95 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2887,7 +2887,7 @@ skl_wm_plane_id(const struct intel_plane
> > > *plane)
> > >  static bool
> > >  intel_has_sagv(struct drm_i915_private *dev_priv)
> > >  {
> > > -	return IS_SKYLAKE(dev_priv);
> > > +	return IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv);
> > >  }
> > >  
> > >  /*
> > > -- 
> > > 2.7.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Jani Nikula]

On Tue, 13 Sep 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote:
> I got confirmation from the Hardware guys that KBL does need to run the
> SAGV code, and it has the same latency as SKL. Also, all SKL production
> steppings need to run the SAGV code.

Can you get confirmation what's the first shipped production stepping?

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Zanoni, Paulo R]

Em Qua, 2016-09-14 às 12:59 +0300, Jani Nikula escreveu:
> On Tue, 13 Sep 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
> wrote:
> > 
> > I got confirmation from the Hardware guys that KBL does need to run
> > the
> > SAGV code, and it has the same latency as SKL. Also, all SKL
> > production
> > steppings need to run the SAGV code.
> 
> Can you get confirmation what's the first shipped production
> stepping?

https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl
-vol04-configurations.pdf#page=15

But I have to admit that I still have pre-prod machines and it would be
very convenient to me if they keep working :)

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

Re: [PATCH 3/8] drm/i915/kbl: KBL also needs to run the SAGV code

[Jani Nikula]

On Wed, 14 Sep 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com> wrote:
> Em Qua, 2016-09-14 às 12:59 +0300, Jani Nikula escreveu:
>> On Tue, 13 Sep 2016, "Zanoni, Paulo R" <paulo.r.zanoni@intel.com>
>> wrote:
>> > 
>> > I got confirmation from the Hardware guys that KBL does need to run
>> > the
>> > SAGV code, and it has the same latency as SKL. Also, all SKL
>> > production
>> > steppings need to run the SAGV code.
>> 
>> Can you get confirmation what's the first shipped production
>> stepping?
>
> https://01.org/sites/default/files/documentation/intel-gfx-prm-osrc-skl
> -vol04-configurations.pdf#page=15
>
> But I have to admit that I still have pre-prod machines and it would be
> very convenient to me if they keep working :)

I think that's a false convenience. If you're developing or testing
stuff on early hardware, you run the risk of having issues only related
to that hardware, already fixed in production. Regression reports from
people running early hardware risk stalling otherwise valid patches for
the wrong reasons.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/8] drm/i915/gen9: fix the WaWmMemoryReadLatency implementation

[Paulo Zanoni]

Bspec says:
  "The mailbox response data may not account for memory read latency.
   If the mailbox response data for level 0 is 0us, add 2 microseconds
   to the result for each valid level."

This means we should only do the +2 in case wm[0] == 0, not always.

So split the sanitizing implementation from the WA implementation and
fix the WA implementation.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 42 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index baacd95..f8ac928 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2127,32 +2127,34 @@ static void intel_read_wm_latency(struct drm_device *dev, uint16_t wm[8])
 				GEN9_MEM_LATENCY_LEVEL_MASK;
 
 		/*
+		 * If a level n (n > 1) has a 0us latency, all levels m (m >= n)
+		 * need to be disabled. We make sure to sanitize the values out
+		 * of the punit to satisfy this requirement.
+		 */
+		for (level = 1; level <= max_level; level++) {
+			if (wm[level] == 0) {
+				for (i = level + 1; i <= max_level; i++)
+					wm[i] = 0;
+				break;
+			}
+		}
+
+		/*
 		 * WaWmMemoryReadLatency:skl
 		 *
 		 * punit doesn't take into account the read latency so we need
-		 * to add 2us to the various latency levels we retrieve from
-		 * the punit.
-		 *   - W0 is a bit special in that it's the only level that
-		 *   can't be disabled if we want to have display working, so
-		 *   we always add 2us there.
-		 *   - For levels >=1, punit returns 0us latency when they are
-		 *   disabled, so we respect that and don't add 2us then
-		 *
-		 * Additionally, if a level n (n > 1) has a 0us latency, all
-		 * levels m (m >= n) need to be disabled. We make sure to
-		 * sanitize the values out of the punit to satisfy this
-		 * requirement.
+		 * to add 2us to the various latency levels we retrieve from the
+		 * punit when level 0 response data us 0us.
 		 */
-		wm[0] += 2;
-		for (level = 1; level <= max_level; level++)
-			if (wm[level] != 0)
+		if (wm[0] == 0) {
+			wm[0] += 2;
+			for (level = 1; level <= max_level; level++) {
+				if (wm[level] == 0)
+					break;
 				wm[level] += 2;
-			else {
-				for (i = level + 1; i <= max_level; i++)
-					wm[i] = 0;
-
-				break;
 			}
+		}
+
 	} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
 		uint64_t sskpd = I915_READ64(MCH_SSKPD);
 
-- 
2.7.4

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

Re: [PATCH 4/8] drm/i915/gen9: fix the WaWmMemoryReadLatency implementation

[Maarten Lankhorst]

Op 07-09-16 om 02:52 schreef Paulo Zanoni:
> Bspec says:
>   "The mailbox response data may not account for memory read latency.
>    If the mailbox response data for level 0 is 0us, add 2 microseconds
>    to the result for each valid level."
>
> This means we should only do the +2 in case wm[0] == 0, not always.
>
> So split the sanitizing implementation from the WA implementation and
> fix the WA implementation.
>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
This one probably needs a dim fixes $commit, just like patch 6 and 7.

For the rest the changes look sane, so with the review comments in the other patches addressed:
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/8] drm/i915/gen9: minimum scanlines for Y tile is not always 4

[Paulo Zanoni]

During watermarks calculations, this value is used in 3 different
places. Only one of them was not using a hardcoded 4. Move the code up
so everybody can benefit from the actual value.

This should only help on situations with Y tiling + 90/270 rotation +
1 or 2 bpp or NV12.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 56 +++++++++++++++++++++++------------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index f8ac928..5a23a91 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3499,7 +3499,8 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
 
 static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 			       uint32_t horiz_pixels, uint8_t cpp,
-			       uint64_t tiling, uint32_t latency)
+			       uint64_t tiling, uint32_t latency,
+			       uint32_t y_min_scanlines)
 {
 	uint32_t ret;
 	uint32_t plane_bytes_per_line, plane_blocks_per_line;
@@ -3512,9 +3513,9 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
 
 	if (tiling == I915_FORMAT_MOD_Y_TILED ||
 	    tiling == I915_FORMAT_MOD_Yf_TILED) {
-		plane_bytes_per_line *= 4;
+		plane_bytes_per_line *= y_min_scanlines;
 		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
-		plane_blocks_per_line /= 4;
+		plane_blocks_per_line /= y_min_scanlines;
 	} else if (tiling == DRM_FORMAT_MOD_NONE) {
 		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
 	} else {
@@ -3571,6 +3572,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint8_t cpp;
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
+	uint32_t y_min_scanlines;
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
@@ -3586,38 +3588,44 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
 	plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate, intel_pstate);
 
+	if (intel_rotation_90_or_270(pstate->rotation)) {
+		int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
+			drm_format_plane_cpp(fb->pixel_format, 1) :
+			drm_format_plane_cpp(fb->pixel_format, 0);
+
+		switch (cpp) {
+		case 1:
+			y_min_scanlines = 16;
+			break;
+		case 2:
+			y_min_scanlines = 8;
+			break;
+		default:
+			WARN(1, "Unsupported pixel depth for rotation");
+		case 4:
+			y_min_scanlines = 4;
+			break;
+		}
+	} else {
+		y_min_scanlines = 4;
+	}
+
 	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
 	method2 = skl_wm_method2(plane_pixel_rate,
 				 cstate->base.adjusted_mode.crtc_htotal,
 				 width,
 				 cpp,
 				 fb->modifier[0],
-				 latency);
+				 latency,
+				 y_min_scanlines);
 
 	plane_bytes_per_line = width * cpp;
 	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 
 	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t min_scanlines = 4;
-		uint32_t y_tile_minimum;
-		if (intel_rotation_90_or_270(pstate->rotation)) {
-			int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
-				drm_format_plane_cpp(fb->pixel_format, 1) :
-				drm_format_plane_cpp(fb->pixel_format, 0);
-
-			switch (cpp) {
-			case 1:
-				min_scanlines = 16;
-				break;
-			case 2:
-				min_scanlines = 8;
-				break;
-			case 8:
-				WARN(1, "Unsupported pixel depth for rotation");
-			}
-		}
-		y_tile_minimum = plane_blocks_per_line * min_scanlines;
+		uint32_t y_tile_minimum = plane_blocks_per_line *
+					  y_min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3632,7 +3640,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	if (level >= 1 && level <= 7) {
 		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
-			res_lines += 4;
+			res_lines += y_min_scanlines;
 		else
 			res_blocks++;
 	}
-- 
2.7.4

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

Re: [PATCH 5/8] drm/i915/gen9: minimum scanlines for Y tile is not always 4

[Lyude]

On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> During watermarks calculations, this value is used in 3 different
> places. Only one of them was not using a hardcoded 4. Move the code
> up
> so everybody can benefit from the actual value.
> 
> This should only help on situations with Y tiling + 90/270 rotation +
> 1 or 2 bpp or NV12.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 56 +++++++++++++++++++++++------
> ------------
>  1 file changed, 32 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index f8ac928..5a23a91 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3499,7 +3499,8 @@ static uint32_t skl_wm_method1(uint32_t
> pixel_rate, uint8_t cpp, uint32_t latenc
>  
>  static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t
> pipe_htotal,
>  			       uint32_t horiz_pixels, uint8_t cpp,
> -			       uint64_t tiling, uint32_t latency)
> +			       uint64_t tiling, uint32_t latency,
> +			       uint32_t y_min_scanlines)
>  {
>  	uint32_t ret;
>  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> @@ -3512,9 +3513,9 @@ static uint32_t skl_wm_method2(uint32_t
> pixel_rate, uint32_t pipe_htotal,
>  
>  	if (tiling == I915_FORMAT_MOD_Y_TILED ||
>  	    tiling == I915_FORMAT_MOD_Yf_TILED) {
> -		plane_bytes_per_line *= 4;
> +		plane_bytes_per_line *= y_min_scanlines;
>  		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512);
> -		plane_blocks_per_line /= 4;
> +		plane_blocks_per_line /= y_min_scanlines;
>  	} else if (tiling == DRM_FORMAT_MOD_NONE) {
>  		plane_blocks_per_line =
> DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
>  	} else {
> @@ -3571,6 +3572,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	uint8_t cpp;
>  	uint32_t width = 0, height = 0;
>  	uint32_t plane_pixel_rate;
> +	uint32_t y_min_scanlines;
>  
>  	if (latency == 0 || !cstate->base.active || !intel_pstate-
> >base.visible) {
>  		*enabled = false;
> @@ -3586,38 +3588,44 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
>  	plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate,
> intel_pstate);
>  
> +	if (intel_rotation_90_or_270(pstate->rotation)) {
> +		int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
> +			drm_format_plane_cpp(fb->pixel_format, 1) :
> +			drm_format_plane_cpp(fb->pixel_format, 0);
> +
> +		switch (cpp) {
> +		case 1:
> +			y_min_scanlines = 16;
> +			break;
> +		case 2:
> +			y_min_scanlines = 8;
> +			break;
> +		default:
> +			WARN(1, "Unsupported pixel depth for
> rotation");

This looks like it's leftover from the code that you moved around, but
we should be erroring out here and returning -EINVAL here so the
modeset actually fails.

> +		case 4:
> +			y_min_scanlines = 4;
> +			break;
> +		}
> +	} else {
> +		y_min_scanlines = 4;
> +	}
> +
>  	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
>  	method2 = skl_wm_method2(plane_pixel_rate,
>  				 cstate-
> >base.adjusted_mode.crtc_htotal,
>  				 width,
>  				 cpp,
>  				 fb->modifier[0],
> -				 latency);
> +				 latency,
> +				 y_min_scanlines);
>  
>  	plane_bytes_per_line = width * cpp;
>  	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line,
> 512);
>  
>  	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>  	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> -		uint32_t min_scanlines = 4;
> -		uint32_t y_tile_minimum;
> -		if (intel_rotation_90_or_270(pstate->rotation)) {
> -			int cpp = (fb->pixel_format ==
> DRM_FORMAT_NV12) ?
> -				drm_format_plane_cpp(fb-
> >pixel_format, 1) :
> -				drm_format_plane_cpp(fb-
> >pixel_format, 0);
> -
> -			switch (cpp) {
> -			case 1:
> -				min_scanlines = 16;
> -				break;
> -			case 2:
> -				min_scanlines = 8;
> -				break;
> -			case 8:
> -				WARN(1, "Unsupported pixel depth for
> rotation");
> -			}
> -		}
> -		y_tile_minimum = plane_blocks_per_line *
> min_scanlines;
> +		uint32_t y_tile_minimum = plane_blocks_per_line *
> +					  y_min_scanlines;
>  		selected_result = max(method2, y_tile_minimum);
>  	} else {
>  		if ((ddb_allocation / plane_blocks_per_line) >= 1)
> @@ -3632,7 +3640,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>  	if (level >= 1 && level <= 7) {
>  		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
>  		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
> -			res_lines += 4;
> +			res_lines += y_min_scanlines;
>  		else
>  			res_blocks++;
>  	}
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/8] drm/i915/gen9: minimum scanlines for Y tile is not always 4

[Zanoni, Paulo R]

Em Qua, 2016-09-07 às 19:03 -0400, Lyude escreveu:
> On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> > 
> > During watermarks calculations, this value is used in 3 different
> > places. Only one of them was not using a hardcoded 4. Move the code
> > up
> > so everybody can benefit from the actual value.
> > 
> > This should only help on situations with Y tiling + 90/270 rotation
> > +
> > 1 or 2 bpp or NV12.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 56 +++++++++++++++++++++++------
> > ------------
> >  1 file changed, 32 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index f8ac928..5a23a91 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3499,7 +3499,8 @@ static uint32_t skl_wm_method1(uint32_t
> > pixel_rate, uint8_t cpp, uint32_t latenc
> >  
> >  static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t
> > pipe_htotal,
> >  			       uint32_t horiz_pixels, uint8_t cpp,
> > -			       uint64_t tiling, uint32_t latency)
> > +			       uint64_t tiling, uint32_t latency,
> > +			       uint32_t y_min_scanlines)
> >  {
> >  	uint32_t ret;
> >  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
> > @@ -3512,9 +3513,9 @@ static uint32_t skl_wm_method2(uint32_t
> > pixel_rate, uint32_t pipe_htotal,
> >  
> >  	if (tiling == I915_FORMAT_MOD_Y_TILED ||
> >  	    tiling == I915_FORMAT_MOD_Yf_TILED) {
> > -		plane_bytes_per_line *= 4;
> > +		plane_bytes_per_line *= y_min_scanlines;
> >  		plane_blocks_per_line =
> > DIV_ROUND_UP(plane_bytes_per_line, 512);
> > -		plane_blocks_per_line /= 4;
> > +		plane_blocks_per_line /= y_min_scanlines;
> >  	} else if (tiling == DRM_FORMAT_MOD_NONE) {
> >  		plane_blocks_per_line =
> > DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
> >  	} else {
> > @@ -3571,6 +3572,7 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  	uint8_t cpp;
> >  	uint32_t width = 0, height = 0;
> >  	uint32_t plane_pixel_rate;
> > +	uint32_t y_min_scanlines;
> >  
> >  	if (latency == 0 || !cstate->base.active || !intel_pstate-
> > > 
> > > base.visible) {
> >  		*enabled = false;
> > @@ -3586,38 +3588,44 @@ static int skl_compute_plane_wm(const
> > struct
> > drm_i915_private *dev_priv,
> >  	cpp = drm_format_plane_cpp(fb->pixel_format, 0);
> >  	plane_pixel_rate = skl_adjusted_plane_pixel_rate(cstate,
> > intel_pstate);
> >  
> > +	if (intel_rotation_90_or_270(pstate->rotation)) {
> > +		int cpp = (fb->pixel_format == DRM_FORMAT_NV12) ?
> > +			drm_format_plane_cpp(fb->pixel_format, 1)
> > :
> > +			drm_format_plane_cpp(fb->pixel_format, 0);
> > +
> > +		switch (cpp) {
> > +		case 1:
> > +			y_min_scanlines = 16;
> > +			break;
> > +		case 2:
> > +			y_min_scanlines = 8;
> > +			break;
> > +		default:
> > +			WARN(1, "Unsupported pixel depth for
> > rotation");
> 
> This looks like it's leftover from the code that you moved around,
> but
> we should be erroring out here and returning -EINVAL here so the
> modeset actually fails.

Good catch. I'll do this as a separate patch since it's a separate fix.

> 
> > 
> > +		case 4:
> > +			y_min_scanlines = 4;
> > +			break;
> > +		}
> > +	} else {
> > +		y_min_scanlines = 4;
> > +	}
> > +
> >  	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
> >  	method2 = skl_wm_method2(plane_pixel_rate,
> >  				 cstate-
> > > 
> > > base.adjusted_mode.crtc_htotal,
> >  				 width,
> >  				 cpp,
> >  				 fb->modifier[0],
> > -				 latency);
> > +				 latency,
> > +				 y_min_scanlines);
> >  
> >  	plane_bytes_per_line = width * cpp;
> >  	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line,
> > 512);
> >  
> >  	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >  	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
> > -		uint32_t min_scanlines = 4;
> > -		uint32_t y_tile_minimum;
> > -		if (intel_rotation_90_or_270(pstate->rotation)) {
> > -			int cpp = (fb->pixel_format ==
> > DRM_FORMAT_NV12) ?
> > -				drm_format_plane_cpp(fb-
> > > 
> > > pixel_format, 1) :
> > -				drm_format_plane_cpp(fb-
> > > 
> > > pixel_format, 0);
> > -
> > -			switch (cpp) {
> > -			case 1:
> > -				min_scanlines = 16;
> > -				break;
> > -			case 2:
> > -				min_scanlines = 8;
> > -				break;
> > -			case 8:
> > -				WARN(1, "Unsupported pixel depth
> > for
> > rotation");
> > -			}
> > -		}
> > -		y_tile_minimum = plane_blocks_per_line *
> > min_scanlines;
> > +		uint32_t y_tile_minimum = plane_blocks_per_line *
> > +					  y_min_scanlines;
> >  		selected_result = max(method2, y_tile_minimum);
> >  	} else {
> >  		if ((ddb_allocation / plane_blocks_per_line) >= 1)
> > @@ -3632,7 +3640,7 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> >  	if (level >= 1 && level <= 7) {
> >  		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
> >  		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
> > -			res_lines += 4;
> > +			res_lines += y_min_scanlines;
> >  		else
> >  			res_blocks++;
> >  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/8] drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations

[Paulo Zanoni]

The confusing thing is that plane_blocks_per_line is listed as part of
the method 2 calculation but is also used for other things. We
calculated it in two different places and different ways: one inside
skl_wm_method2() and the other inside skl_compute_plane_wm(). The
skl_wm_method2() implementation is the one that matches the
specification.

With this patch we fix the skl_compute_plane_wm() calculation and just
pass it as a parameter to skl_wm_method2(). We also take care to not
modify the value of plane_bytes_per_line since we're going to rely on
it having a correct value in later patches.

This should affect the watermarks for Linear and Y-tiled.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 39 +++++++++++++++------------------------
 1 file changed, 15 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5a23a91..70dc0c4 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3498,30 +3498,14 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t cpp, uint32_t latenc
 }
 
 static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
-			       uint32_t horiz_pixels, uint8_t cpp,
-			       uint64_t tiling, uint32_t latency,
-			       uint32_t y_min_scanlines)
+			       uint32_t latency, uint32_t plane_blocks_per_line)
 {
 	uint32_t ret;
-	uint32_t plane_bytes_per_line, plane_blocks_per_line;
 	uint32_t wm_intermediate_val;
 
 	if (latency == 0)
 		return UINT_MAX;
 
-	plane_bytes_per_line = horiz_pixels * cpp;
-
-	if (tiling == I915_FORMAT_MOD_Y_TILED ||
-	    tiling == I915_FORMAT_MOD_Yf_TILED) {
-		plane_bytes_per_line *= y_min_scanlines;
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
-		plane_blocks_per_line /= y_min_scanlines;
-	} else if (tiling == DRM_FORMAT_MOD_NONE) {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512) + 1;
-	} else {
-		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
-	}
-
 	wm_intermediate_val = latency * pixel_rate;
 	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
 				plane_blocks_per_line;
@@ -3610,17 +3594,24 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 		y_min_scanlines = 4;
 	}
 
+	plane_bytes_per_line = width * cpp;
+	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
+	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
+		plane_blocks_per_line =
+		      DIV_ROUND_UP(plane_bytes_per_line * y_min_scanlines, 512);
+		plane_blocks_per_line /= y_min_scanlines;
+	} else if (fb->modifier[0] == DRM_FORMAT_MOD_NONE) {
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512)
+					+ 1;
+	} else {
+		plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+	}
+
 	method1 = skl_wm_method1(plane_pixel_rate, cpp, latency);
 	method2 = skl_wm_method2(plane_pixel_rate,
 				 cstate->base.adjusted_mode.crtc_htotal,
-				 width,
-				 cpp,
-				 fb->modifier[0],
 				 latency,
-				 y_min_scanlines);
-
-	plane_bytes_per_line = width * cpp;
-	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
+				 plane_blocks_per_line);
 
 	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
-- 
2.7.4

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

[PATCH 7/8] drm/i915/gen9: fix the watermark res_blocks value

[Paulo Zanoni]

We forgot the "res_blocks += y_tile_minimum" that's described on step
V of our documentation.

Again, this should only affect the Y tiling cases.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 70dc0c4..381c482 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3556,7 +3556,7 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint8_t cpp;
 	uint32_t width = 0, height = 0;
 	uint32_t plane_pixel_rate;
-	uint32_t y_min_scanlines;
+	uint32_t y_tile_minimum, y_min_scanlines;
 
 	if (latency == 0 || !cstate->base.active || !intel_pstate->base.visible) {
 		*enabled = false;
@@ -3613,10 +3613,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 				 latency,
 				 plane_blocks_per_line);
 
+	y_tile_minimum = plane_blocks_per_line * y_min_scanlines;
+
 	if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
-		uint32_t y_tile_minimum = plane_blocks_per_line *
-					  y_min_scanlines;
 		selected_result = max(method2, y_tile_minimum);
 	} else {
 		if ((ddb_allocation / plane_blocks_per_line) >= 1)
@@ -3630,10 +3630,12 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 
 	if (level >= 1 && level <= 7) {
 		if (fb->modifier[0] == I915_FORMAT_MOD_Y_TILED ||
-		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED)
+		    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
+			res_blocks += y_tile_minimum;
 			res_lines += y_min_scanlines;
-		else
+		} else {
 			res_blocks++;
+		}
 	}
 
 	if (res_blocks >= ddb_allocation || res_lines > 31) {
-- 
2.7.4

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

[PATCH 8/8] drm/i915/gen9: implement missing case for SKL watermarks calculation

[Paulo Zanoni]

This should affect linear and X tiled planes on really small htotal
cases. It doesn't seem to be a very feasible case, but let's implement
it since it's on the specification and it's better to have it and
never need than not have it and realize we needed it.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 381c482..86c6d66 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3619,7 +3619,10 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	    fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED) {
 		selected_result = max(method2, y_tile_minimum);
 	} else {
-		if ((ddb_allocation / plane_blocks_per_line) >= 1)
+		if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) &&
+		    (plane_bytes_per_line / 512 < 1))
+			selected_result = method2;
+		else if ((ddb_allocation / plane_blocks_per_line) >= 1)
 			selected_result = min(method1, method2);
 		else
 			selected_result = method1;
-- 
2.7.4

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

✗ Fi.CI.BAT: warning for SKL/KBL watermark fixes

[Patchwork]

== Series Details ==

Series: SKL/KBL watermark fixes
URL   : https://patchwork.freedesktop.org/series/12082/
State : warning

== Summary ==

Series 12082v1 SKL/KBL watermark fixes
http://patchwork.freedesktop.org/api/1.0/series/12082/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b-frame-sequence:
                pass       -> SKIP       (fi-hsw-4770r)

fi-bdw-5557u     total:252  pass:233  dwarn:2   dfail:1   fail:1   skip:15 
fi-bsw-n3050     total:252  pass:203  dwarn:1   dfail:1   fail:1   skip:46 
fi-byt-n2820     total:252  pass:206  dwarn:2   dfail:1   fail:2   skip:41 
fi-hsw-4770k     total:252  pass:226  dwarn:2   dfail:1   fail:1   skip:22 
fi-hsw-4770r     total:252  pass:221  dwarn:2   dfail:1   fail:1   skip:27 
fi-ivb-3520m     total:252  pass:217  dwarn:2   dfail:1   fail:1   skip:31 
fi-skl-6260u     total:252  pass:234  dwarn:2   dfail:1   fail:1   skip:14 
fi-skl-6700k     total:252  pass:219  dwarn:3   dfail:1   fail:1   skip:28 
fi-snb-2520m     total:252  pass:204  dwarn:2   dfail:1   fail:2   skip:43 
fi-snb-2600      total:252  pass:205  dwarn:2   dfail:1   fail:1   skip:43 

Results at /archive/results/CI_IGT_test/Patchwork_2478/

980cf7a5d9c420afbaf52a339a2005339f9f8319 drm-intel-nightly: 2016y-09m-06d-18h-01m-33s UTC integration manifest
64dbb86 drm/i915/gen9: implement missing case for SKL watermarks calculation
5527234 drm/i915/gen9: fix the watermark res_blocks value
b2815b7 drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations
9f1d799 drm/i915/gen9: minimum scanlines for Y tile is not always 4
40ada27 drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
4bd3277 drm/i915/kbl: KBL also needs to run the SAGV code
e3e0712 drm/i915: introduce intel_has_sagv()
7cace1e drm/i915: SAGV is not SKL-only, so rename a few things

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

Re: [PATCH 0/8] SKL/KBL watermark fixes

[Lyude]

For patches 6-8:

Reviewed-by: Lyude <cpaul@redhat.com>

On Tue, 2016-09-06 at 21:52 -0300, Paulo Zanoni wrote:
> Hi
> 
> This series is the result of me comparing the code against BSpec. It
> doesn't solve any particular problem I was seeing, but there's enough
> changes that this code could potentially change the watermarks values
> for most machines. Maybe this will fix somebody's underrun? Even if
> it
> doesn't, we should still have a code that matches the specification
> unless there's a big justification.
> 
> I still didn't finish reviewing everything and there's at least one
> workaround that we seem to be missing, but let's try to get these
> patches merged first.
> 
> Thanks,
> Paulo
> 
> Cc: Lyude <cpaul@redhat.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Mahesh Kumar <mahesh1.kumar@intel.com>
> 
> Paulo Zanoni (8):
>   drm/i915: SAGV is not SKL-only, so rename a few things
>   drm/i915: introduce intel_has_sagv()
>   drm/i915/kbl: KBL also needs to run the SAGV code
>   drm/i915/gen9: fix the WaWmMemoryReadLatency implementation
>   drm/i915/gen9: minimum scanlines for Y tile is not always 4
>   drm/i915/gen9: fix plane_blocks_per_line on watermarks calculations
>   drm/i915/gen9: fix the watermark res_blocks value
>   drm/i915/gen9: implement missing case for SKL watermarks
> calculation
> 
>  drivers/gpu/drm/i915/i915_drv.h      |  10 +-
>  drivers/gpu/drm/i915/intel_display.c |   9 +-
>  drivers/gpu/drm/i915/intel_drv.h     |   6 +-
>  drivers/gpu/drm/i915/intel_pm.c      | 177 ++++++++++++++++++++-----
> ----------
>  4 files changed, 111 insertions(+), 91 deletions(-)
> 
-- 
Cheers,
	Lyude
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx