Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv function
On Mon, Oct 29, 2018 at 12:08:38PM +0200, Jani Nikula wrote:
> On Fri, 26 Oct 2018, Rodrigo Vivi wrote:
> > 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.
> > v4: Go back and accept simple Jani proposed solution.
>
> Thanks.
>
> Reviewed-by: Jani Nikula
pushed to dinq. Thanks for review and idea!
>
>
>
> >
> > Signed-off-by: Rodrigo Vivi
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 11 ++-
> > 1 file changed, 2 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index bc70f6bb86ae..82c82e233154 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3611,15 +3611,8 @@ static bool skl_needs_memory_bw_wa(struct
> > intel_atomic_state *state)
> > 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;
> > + return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
> > + dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
> > }
> >
> > /*
>
> --
> Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv function
On Fri, 26 Oct 2018, Rodrigo Vivi wrote:
> 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.
> v4: Go back and accept simple Jani proposed solution.
Thanks.
Reviewed-by: Jani Nikula
>
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/intel_pm.c | 11 ++-
> 1 file changed, 2 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bc70f6bb86ae..82c82e233154 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3611,15 +3611,8 @@ static bool skl_needs_memory_bw_wa(struct
> intel_atomic_state *state)
> 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;
> + return (IS_GEN9_BC(dev_priv) || INTEL_GEN(dev_priv) >= 10) &&
> + dev_priv->sagv_status != I915_SAGV_NOT_CONTROLLED;
> }
>
> /*
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
On Wed, Oct 24, 2018 at 01:13:51PM +0300, Jani Nikula wrote: > On Tue, 23 Oct 2018, Rodrigo Vivi wrote: > > 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 :/ > > If that is to be in device info, it doesn't require an extra codename, > it requires an extra device info with the flag. > > This ties to the goal of making dev_priv->info a pointer to the static > const data in i915_pci.c i.e. making ->info truly const. There's three > categories of info: > > 1) immutable device properties > 2) properties set once during probe, immutable afterwards > 3) runtime > > Currently we more or less happily conflate these, along with some module > parameters too. The mkwrite_device_info() use has profilerated much > wider than it was ever intended; we need to nuke that. > > We also have HAS_FOO() and IS_FOO() macros that do checks on pci id or > gen or platform or a combination of them. It's a mess, and it's not > getting better without conscious effort. I agree with all that you said. We probably just disagree on how to make HAS_FOO(dev_priv) IS_FOO(dev_priv) better. In my opinion we should move towards adding all immutable device properties that describe and differentiate the platforms inside device info. With that consolidated there: - code gets uniform - minimize gen and platform checks spread all over the code - it gets easier to add platforms But nevermind, I won't insist on this path ;) > > BR, > Jani. > > > -- > Jani Nikula, Intel Open Source Graphics Center > ___ > 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: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
On Tue, 23 Oct 2018, Rodrigo Vivi wrote: > 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 :/ If that is to be in device info, it doesn't require an extra codename, it requires an extra device info with the flag. This ties to the goal of making dev_priv->info a pointer to the static const data in i915_pci.c i.e. making ->info truly const. There's three categories of info: 1) immutable device properties 2) properties set once during probe, immutable afterwards 3) runtime Currently we more or less happily conflate these, along with some module parameters too. The mkwrite_device_info() use has profilerated much wider than it was ever intended; we need to nuke that. We also have HAS_FOO() and IS_FOO() macros that do checks on pci id or gen or platform or a combination of them. It's a mess, and it's not getting better without conscious effort. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
On Tue, Oct 23, 2018 at 10:23:39AM +0300, Jani Nikula wrote:
> On Mon, 22 Oct 2018, Rodrigo Vivi 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
> > Cc: Lucas De Marchi
> > Signed-off-by: Rodrigo Vivi
> > ---
> > 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.v...@intel.com
>
>
>
> > -
> > /*
> > * 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
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
On Mon, 22 Oct 2018, Rodrigo Vivi 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
> Cc: Lucas De Marchi
> Signed-off-by: Rodrigo Vivi
> ---
> 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;
IMO all iterations since have been worse.
BR,
Jani.
[1]
http://patchwork.freedesktop.org/patch/msgid/20181018233447.5187-9-rodrigo.v...@intel.com
> -
> /*
> * 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 (de
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
Em Seg, 2018-10-22 às 17:06 -0700, Rodrigo Vivi escreveu:
> On Mon, Oct 22, 2018 at 04:48:50PM -0700, Paulo Zanoni wrote:
> > Em Seg, 2018-10-22 às 09:57 -0700, Rodrigo Vivi escreveu:
> > > 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)
> >
> > I know this has probably been discussed in the past and I missed it
> > (since I know this was done a few times for other things), but
> > how/why
> > is this approach better than the current one? Can you please write
> > it
> > in the commit message?
>
> my bad... after stripping this out of my RFC series this got
> indeed out of context.
>
> I'm trying to get rid of many platform codename checks spread
> around the code.
>
> I believe we should do most of the decisions based on display_gen
> or has_feature. And use the platform codename as the last resource
> only when gen or feature cannot describe things better.
In terms of "describing things better", I don't see a lot of difference
between HAS_SOMETHING() and intel_has_something(). The
intel_has_something() version even already gives us a signal that maybe
the decision is made based on runtime information instead of being
simply per-platform, which is this case.
>
> > Quickly checking the implementation of intel_has_sagv() seems much
> > easier than swimming through the nested macros of i915_pci.c. And
> > the
> > special-caseness of the NOT_CONTROLLED case being restricted to
> > gen9 in
> > the current (non-patched) code also at first appears to be better
> > than
> > this proposal. But I'm totally willing to read your arguments and
> > reevaluate my decisions based on them, so please present them.
>
> When you are looking only to a single feature this might be true.
> When you are looking to the platform itself consolidating the
> features
> definitions in a single place is easier to check legacy features and
> add new platforms.
So please make sure this is in the commit message. I may or may not
agree this is the best way to proceed, but at least now I can know why
it is the way it is when I git-blame/git-log.
>
> Specially I'd like to add a new platform gen line gen++ without
> need to use any codename. Or without mixing things up.
> INTEL_GEN >= 11 || IS_CANNONLAKE :/
You can achieve this by redefining intel_has_sagv() instead of killing
it. Currently, our code assumes new platforms won't support sagv, but
that seems unlikely considering the recent history. Just invert it:
has_sagv() {
if (is_gen9_lp || intel_gen < 9)
return false;
if (!is_skl)
return true;
return (status != controlled)
}
Note: I'm not blocking this patch.
>
> So adding gen_n+1 should be as trivial
> as gen_n + 1 legacy...
>
> >
> > >
> > > Cc: Jani Nikula
> > > Cc: Lucas De Marchi
> > > Signed-off-by: Rodrigo Vivi
> > > ---
> > > 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
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
On Mon, Oct 22, 2018 at 04:48:50PM -0700, Paulo Zanoni wrote:
> Em Seg, 2018-10-22 às 09:57 -0700, Rodrigo Vivi escreveu:
> > 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)
>
> I know this has probably been discussed in the past and I missed it
> (since I know this was done a few times for other things), but how/why
> is this approach better than the current one? Can you please write it
> in the commit message?
my bad... after stripping this out of my RFC series this got
indeed out of context.
I'm trying to get rid of many platform codename checks spread
around the code.
I believe we should do most of the decisions based on display_gen
or has_feature. And use the platform codename as the last resource
only when gen or feature cannot describe things better.
> Quickly checking the implementation of intel_has_sagv() seems much
> easier than swimming through the nested macros of i915_pci.c. And the
> special-caseness of the NOT_CONTROLLED case being restricted to gen9 in
> the current (non-patched) code also at first appears to be better than
> this proposal. But I'm totally willing to read your arguments and
> reevaluate my decisions based on them, so please present them.
When you are looking only to a single feature this might be true.
When you are looking to the platform itself consolidating the features
definitions in a single place is easier to check legacy features and
add new platforms.
Specially I'd like to add a new platform gen line gen++ without
need to use any codename. Or without mixing things up.
INTEL_GEN >= 11 || IS_CANNONLAKE :/
So adding gen_n+1 should be as trivial
as gen_n + 1 legacy...
>
> >
> > Cc: Jani Nikula
> > Cc: Lucas De Marchi
> > Signed-off-by: Rodrigo Vivi
> > ---
> > 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;
> > -}
> > -
> > /*
> > * SAGV dynamically adjusts the system agent voltage and clock
> > frequencies
> > * depending on power and per
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
Em Seg, 2018-10-22 às 09:57 -0700, Rodrigo Vivi escreveu:
> 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)
I know this has probably been discussed in the past and I missed it
(since I know this was done a few times for other things), but how/why
is this approach better than the current one? Can you please write it
in the commit message?
Quickly checking the implementation of intel_has_sagv() seems much
easier than swimming through the nested macros of i915_pci.c. And the
special-caseness of the NOT_CONTROLLED case being restricted to gen9 in
the current (non-patched) code also at first appears to be better than
this proposal. But I'm totally willing to read your arguments and
reevaluate my decisions based on them, so please present them.
>
> Cc: Jani Nikula
> Cc: Lucas De Marchi
> Signed-off-by: Rodrigo Vivi
> ---
> 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;
> -}
> -
> /*
> * 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_sag
Re: [Intel-gfx] [PATCH] drm/i915: Simplify has_sagv
On Fri, 19 Oct 2018, Rodrigo Vivi 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)
>
> Cc: Jani Nikula
> Cc: Lucas De Marchi
> Signed-off-by: Rodrigo Vivi
> ---
> drivers/gpu/drm/i915/i915_drv.h | 2 ++
> drivers/gpu/drm/i915/i915_pci.c | 1 +
> drivers/gpu/drm/i915/intel_device_info.h | 1 +
> drivers/gpu/drm/i915/intel_pm.c | 20 +++-
> 4 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3017ef037fed..8eab6bdff8a4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2650,6 +2650,8 @@ 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 && \
> + dev_priv->sagv_status !=
> I915_SAGV_NOT_CONTROLLED)
Grumble. Somehow it feels wrong to tie the HAS_* macros to some runtime
checks.
Anyway, dev_priv should be wrapped in parens.
BR,
Jani.
>
> #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 0a05cc7ace14..21ca9917b86e 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..09c21f6151fd 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;
> -}
> -
> /*
> * SAGV dynamically adjusts the system agent voltage and clock frequencies
> * depending on power and performance requirements. The display engine access
> @@ -3639,7 +3625,7 @@ 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)
> @@ -3676,7 +3662,7 @@ 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)
> @@ -3721,7 +3707,7 @@ 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 (IS_GEN9(dev_priv))
--
Jani Nikula, Intel Open Source Graphics Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
