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

[Lyude <cpaul@redhat.com>]

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