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@xxxxxxxxxx>
>
> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
