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

["Zanoni, Paulo R" <paulo.r.zanoni@intel.com>]

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