On Tue, Oct 23, 2018 at 10:23:39AM +0300, Jani Nikula wrote:
> On Mon, 22 Oct 2018, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx> wrote:
> > Let's add a platform has_sagv instead of having a full
> > function that handle platform by platform.
> >
> > The specially case for SKL for not controlled sagv
> > is already taken care inside intel_enable_sagv, so there's
> > no need to duplicate the check here.
> >
> > v2: Go one step further and remove skl special case. (Jani)
> >
> > v3: Separate runtime status handle from has_sagv flag. (Jani)
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Lucas De Marchi <lucas.demarchi@xxxxxxxxx>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_pci.c | 1 +
> > drivers/gpu/drm/i915/intel_device_info.h | 1 +
> > drivers/gpu/drm/i915/intel_pm.c | 29 ++++++++----------------
> > 4 files changed, 13 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3017ef037fed..9b98ceb2d029 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2650,6 +2650,7 @@ intel_info(const struct drm_i915_private *dev_priv)
> > #define HAS_DDI(dev_priv) ((dev_priv)->info.has_ddi)
> > #define HAS_FPGA_DBG_UNCLAIMED(dev_priv) ((dev_priv)->info.has_fpga_dbg)
> > #define HAS_PSR(dev_priv) ((dev_priv)->info.has_psr)
> > +#define HAS_SAGV(dev_priv) ((dev_priv)->info.has_sagv)
> >
> > #define HAS_RC6(dev_priv) ((dev_priv)->info.has_rc6)
> > #define HAS_RC6p(dev_priv) ((dev_priv)->info.has_rc6p)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c b/drivers/gpu/drm/i915/i915_pci.c
> > index 44e745921ac1..0b09155eab62 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -465,6 +465,7 @@ static const struct intel_device_info intel_cherryview_info = {
> > .has_csr = 1, \
> > .has_guc = 1, \
> > .has_ipc = 1, \
> > + .has_sagv = 1, \
> > .ddb_size = 896
> >
> > #define SKL_PLATFORM \
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.h b/drivers/gpu/drm/i915/intel_device_info.h
> > index af7002640cdf..e77c8b62783f 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.h
> > +++ b/drivers/gpu/drm/i915/intel_device_info.h
> > @@ -117,6 +117,7 @@ enum intel_ppgtt {
> > func(hws_needs_physical); \
> > func(overlay_needs_physical); \
> > func(supports_tv); \
> > + func(has_sagv); \
> > func(has_ipc);
> >
> > #define GEN_MAX_SLICES (6) /* CNL upper bound */
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 67a4d0735291..7e38ed8421c7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3609,20 +3609,6 @@ static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
> > return false;
> > }
> >
> > -static bool
> > -intel_has_sagv(struct drm_i915_private *dev_priv)
> > -{
> > - if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv) ||
> > - IS_CANNONLAKE(dev_priv) || IS_ICELAKE(dev_priv))
> > - return true;
> > -
> > - if (IS_SKYLAKE(dev_priv) &&
> > - dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED)
> > - return true;
> > -
> > - return false;
> > -}
>
> Argh. All I ever wanted was a version of [1] that removed the Skylake
> special casing for I915_SAGV_NOT_CONTROLLED. I even wrote what the
> function could be in its entirety:
>
> return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
> dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
Well, that works for me as well.
>
> IMO all iterations since have been worse.
But I thought while doing this I could consolidade it along with all
the other has_feature cases.
I believe we should either have everything as info.has_feature or everything
as has_feature().
for instance if we end up ever having 2 platforms of same gen where
one has_sagv and the other doesn't we would have that in the platform
definition while making us to define another codename and add it here
or even worse if we don't have a codename available like CNL_WITH_PORT_F :/
But yeap, for now let's move with your suggestion.
Thanks,
Rodrigo.
>
> BR,
> Jani.
>
>
> [1] http://patchwork.freedesktop.org/patch/msgid/20181018233447.5187-9-rodrigo.vivi@xxxxxxxxx
>
>
>
> > -
> > /*
> > * SAGV dynamically adjusts the system agent voltage and clock frequencies
> > * depending on power and performance requirements. The display engine access
> > @@ -3639,10 +3625,11 @@ intel_enable_sagv(struct drm_i915_private *dev_priv)
> > {
> > int ret;
> >
> > - if (!intel_has_sagv(dev_priv))
> > + if (!HAS_SAGV(dev_priv))
> > return 0;
> >
> > - if (dev_priv->sagv_status == I915_SAGV_ENABLED)
> > + if (dev_priv->sagv_status == I915_SAGV_ENABLED ||
> > + dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> > return 0;
> >
> > DRM_DEBUG_KMS("Enabling the SAGV\n");
> > @@ -3676,10 +3663,11 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> > {
> > int ret;
> >
> > - if (!intel_has_sagv(dev_priv))
> > + if (!HAS_SAGV(dev_priv))
> > return 0;
> >
> > - if (dev_priv->sagv_status == I915_SAGV_DISABLED)
> > + if (dev_priv->sagv_status == I915_SAGV_DISABLED ||
> > + dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> > return 0;
> >
> > DRM_DEBUG_KMS("Disabling the SAGV\n");
> > @@ -3721,7 +3709,10 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > int level, latency;
> > int sagv_block_time_us;
> >
> > - if (!intel_has_sagv(dev_priv))
> > + if (!HAS_SAGV(dev_priv))
> > + return false;
> > +
> > + if (dev_priv->sagv_status == I915_SAGV_NOT_CONTROLLED)
> > return false;
> >
> > if (IS_GEN9(dev_priv))
>
> --
> Jani Nikula, Intel Open Source Graphics Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
