Re: [Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()
On Tue, Aug 21, 2018 at 12:54:12PM -0700, Paulo Zanoni wrote:
> Em Ter, 2018-08-21 às 14:12 +0300, Imre Deak escreveu:
> > On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> > > Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > > > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > > > [...]
> > > > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > > > *dev_priv,
> > > > > > -enum i915_power_well_id
> > > > > > power_well_id)
> > > > > > -{
> > > > > > - struct i915_power_well *power_well;
> > > > > > - bool ret;
> > > > > > -
> > > > > > - power_well = lookup_power_well(dev_priv, power_well_id);
> > > > > > - ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
> > > > > > -
> > > > > > - return ret;
> > > > > > -}
> > > > > > -
> > > > >
> > > > > Or rather export a locked version of it and use that in intel_hdcp.c
> > > > > to better hide the internals?
> > > >
> > > > That should probably be combined with José's idea of using
> > > > ->enabled so we trust the hardware sync.
> > > >
> > > > Thanks for the suggestions.
> > >
> > > After further analysis, I wonder if intel_hdcp.c should really be
> > > checking for enabled power wells or if it should be doing
> > > something else, such as actually grabbing power domain references
> > > to make sure we're able to enable/disable HDCP whenever we need.
> > > Most of our code should not be checking for power wells/domains
> > > being enabled/disabled (except for HW readout), it should actually
> > > be requesting those resources to make sure we have them when we
> > > need them.
> > >
> > > CCing Ramaligam for that.
> >
> > There is no separate power resource for HDCP, it just uses the power
> > wells the encoder already uses. Those are guaranteed to be on, since
> > intel_hdcp_enable/disable are called from the encoder enable/disable
> > hooks. As such hdcp_key_loadable() is just an assert.
>
> But then in this case, an assertion wouldn't make sense at all, since
> if the condition for the assert was not valid, everything before that
> call would have been broken too. If this were the only problem I would
> just vote to remove the assertion.
It is an assertion, hdcp_key_loadable() always returns true, unless
there is a bug somewhere. I think it also makes sense to have an
assertion past the point where the condition should be true, close to a
place where you logically depend on this condition. For instance for
documentation or to make sure some bug in between didn't cause the
condition to become false. In this case it looks like it was added to
match the conditions listed by BSpec.
> The problem here is that we have a work function that also runs the
> assert, and since it's a work function we really have no guarantees
> about the power wells being held when it runs.
>
> intel_hdcp_check_work() -> intel_hdcp_check_link() ->
> _intel_hdcp_enable() -> hdcp_key_loadable().
>
> I'm not sure what's the best thing to do here, but perhaps preventing
> the hardware from going away when this work is scheduled would help.
The work is cancelled in intel_hdcp_disable(), so it's guarateed to run
only while the encoder is active (and hence that the required power
wells are enabled).
> > Defining a new power domain for this would be a bit overkill imo
> > and as PW#1 is handled automatically by HW (and so not the usual
> > driver get/put ops via power domain handles) we would have to
> > special case it.
>
> It only gets automatically handled by the HW once we drop a well
> defined set of power domains. If we grab those power domains we
> prevent automatic hardware handling.
Sure, but the assert is specifically about PW#1, not about some other
indirect dependency that should cause PW#1 to be enabled. (That is
should unless there is a bug somewhere.)
--Imre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()
Em Ter, 2018-08-21 às 14:12 +0300, Imre Deak escreveu:
> On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> > Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > > Use the same coding pattern as we use in the other functions
> > > > > of
> > > > > the
> > > > > same file: just call lookup_power_well() directly in the only
> > > > > caller.
> > > > >
> > > > > Cc: Imre Deak
> > > > > Signed-off-by: Paulo Zanoni
> > > > > ---
> > > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++--
> > > > >
> > > > > ---
> > > > > 1 file changed, 3 insertions(+), 17 deletions(-)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > index e209edbc561d..e0947f662361 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > > @@ -49,9 +49,6 @@
> > > > > * present for a given platform.
> > > > > */
> > > > >
> > > > > -bool intel_display_power_well_is_enabled(struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > > - enum
> > > > > i915_power_well_id
> > > > > power_well_id);
> > > > > -
> > > > > static struct i915_power_well *
> > > > > lookup_power_well(struct drm_i915_private *dev_priv,
> > > > > enum i915_power_well_id power_well_id);
> > > > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > > > drm_i915_private *dev_priv)
> > > > >
> > > > > static void assert_can_enable_dc5(struct drm_i915_private
> > > > > *dev_priv)
> > > > > {
> > > > > - bool pg2_enabled =
> > > > > intel_display_power_well_is_enabled(dev_priv,
> > > > > - SKL_DISP_PW_2);
> > > > > + struct i915_power_well *pg2 =
> > > > > lookup_power_well(dev_priv,
> > > > > + SKL_
> > > > > DISP
> > > > > _P
> > > > > W_2);
> > > > > + bool pg2_enabled = pg2->desc->ops-
> > > > > >is_enabled(dev_priv,
> > > > > pg2);
> > > > >
> > > > > WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > > > DC5.\n");
> > > > >
> > > > > @@ -2302,18 +2300,6 @@ static const struct
> > > > > i915_power_well_desc
> > > > > chv_power_wells[] = {
> > > > > },
> > > > > };
> > > > >
> > > > > -bool intel_display_power_well_is_enabled(struct
> > > > > drm_i915_private
> > > > > *dev_priv,
> > > > > - enum
> > > > > i915_power_well_id
> > > > > power_well_id)
> > > > > -{
> > > > > - struct i915_power_well *power_well;
> > > > > - bool ret;
> > > > > -
> > > > > - power_well = lookup_power_well(dev_priv,
> > > > > power_well_id);
> > > > > - ret = power_well->desc->ops->is_enabled(dev_priv,
> > > > > power_well);
> > > > > -
> > > > > - return ret;
> > > > > -}
> > > > > -
> > > >
> > > > Or rather export a locked version of it and use that in
> > > > intel_hdcp.c
> > > > to
> > > > better hide the internals?
> > >
> > > That should probably be combined with José's idea of using
> > > ->enabled
> > > so
> > > we trust the hardware sync.
> > >
> > > Thanks for the suggestions.
> >
> > After further analysis, I wonder if intel_hdcp.c should really be
> > checking for enabled power wells or if it should be doing something
> > else, such as actually grabbing power domain references to make
> > sure
> > we're able to enable/disable HDCP whenever we need. Most of our
> > code
> > should not be checking for power wells/domains being
> > enabled/disabled
> > (except for HW readout), it should actually be requesting those
> > resources to make sure we have them when we need them.
> >
> > CCing Ramaligam for that.
>
> There is no separate power resource for HDCP, it just uses the power
> wells the encoder already uses. Those are guaranteed to be on, since
> intel_hdcp_enable/disable are called from the encoder enable/disable
> hooks. As such hdcp_key_loadable() is just an assert.
But then in this case, an assertion wouldn't make sense at all, since
if the condition for the assert was not valid, everything before that
call would have been broken too. If this were the only problem I would
just vote to remove the assertion.
The problem here is that we have a work function that also runs the
assert, and since it's a work function we really have no guarantees
about the power wells being held when it runs.
intel_hdcp_check_work() -> intel_hdcp_check_link() ->
_intel_hdcp_enable() -> hdcp_key_loadable().
I'm not sure what's the best thing to do here, but perhaps preventing
the hardware from going away when this work is scheduled would help.
> Defining a new
> power domain for this would be a bit overkill imo and as PW#1 is
> handled
> automatically by HW (and so not the usual driver get/put ops via
>
Re: [Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()
On Mon, Aug 20, 2018 at 04:11:27PM -0700, Paulo Zanoni wrote:
> Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> > Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > > Use the same coding pattern as we use in the other functions of
> > > > the
> > > > same file: just call lookup_power_well() directly in the only
> > > > caller.
> > > >
> > > > Cc: Imre Deak
> > > > Signed-off-by: Paulo Zanoni
> > > > ---
> > > > drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++--
> > > > ---
> > > > 1 file changed, 3 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index e209edbc561d..e0947f662361 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -49,9 +49,6 @@
> > > > * present for a given platform.
> > > > */
> > > >
> > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > -enum i915_power_well_id
> > > > power_well_id);
> > > > -
> > > > static struct i915_power_well *
> > > > lookup_power_well(struct drm_i915_private *dev_priv,
> > > > enum i915_power_well_id power_well_id);
> > > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > > drm_i915_private *dev_priv)
> > > >
> > > > static void assert_can_enable_dc5(struct drm_i915_private
> > > > *dev_priv)
> > > > {
> > > > - bool pg2_enabled =
> > > > intel_display_power_well_is_enabled(dev_priv,
> > > > - SKL_DISP_PW_2);
> > > > + struct i915_power_well *pg2 =
> > > > lookup_power_well(dev_priv,
> > > > + SKL_DISP
> > > > _P
> > > > W_2);
> > > > + bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > > > pg2);
> > > >
> > > > WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > > DC5.\n");
> > > >
> > > > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > > > chv_power_wells[] = {
> > > > },
> > > > };
> > > >
> > > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > > *dev_priv,
> > > > -enum i915_power_well_id
> > > > power_well_id)
> > > > -{
> > > > - struct i915_power_well *power_well;
> > > > - bool ret;
> > > > -
> > > > - power_well = lookup_power_well(dev_priv, power_well_id);
> > > > - ret = power_well->desc->ops->is_enabled(dev_priv,
> > > > power_well);
> > > > -
> > > > - return ret;
> > > > -}
> > > > -
> > >
> > > Or rather export a locked version of it and use that in
> > > intel_hdcp.c
> > > to
> > > better hide the internals?
> >
> > That should probably be combined with José's idea of using ->enabled
> > so
> > we trust the hardware sync.
> >
> > Thanks for the suggestions.
>
> After further analysis, I wonder if intel_hdcp.c should really be
> checking for enabled power wells or if it should be doing something
> else, such as actually grabbing power domain references to make sure
> we're able to enable/disable HDCP whenever we need. Most of our code
> should not be checking for power wells/domains being enabled/disabled
> (except for HW readout), it should actually be requesting those
> resources to make sure we have them when we need them.
>
> CCing Ramaligam for that.
There is no separate power resource for HDCP, it just uses the power
wells the encoder already uses. Those are guaranteed to be on, since
intel_hdcp_enable/disable are called from the encoder enable/disable
hooks. As such hdcp_key_loadable() is just an assert. Defining a new
power domain for this would be a bit overkill imo and as PW#1 is handled
automatically by HW (and so not the usual driver get/put ops via power
domain handles) we would have to special case it.
>
> >
> > >
> > > > static const struct i915_power_well_desc skl_power_wells[] = {
> > > > {
> > > > .name = "always-on",
> > > > --
> > > > 2.14.4
> > > >
> >
> > ___
> > 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 1/4] drm/i915: kill intel_display_power_well_is_enabled()
Em Sex, 2018-08-17 às 16:41 -0700, Paulo Zanoni escreveu:
> Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> > On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > > Use the same coding pattern as we use in the other functions of
> > > the
> > > same file: just call lookup_power_well() directly in the only
> > > caller.
> > >
> > > Cc: Imre Deak
> > > Signed-off-by: Paulo Zanoni
> > > ---
> > > drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++--
> > > ---
> > > 1 file changed, 3 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > index e209edbc561d..e0947f662361 100644
> > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > @@ -49,9 +49,6 @@
> > > * present for a given platform.
> > > */
> > >
> > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > > - enum i915_power_well_id
> > > power_well_id);
> > > -
> > > static struct i915_power_well *
> > > lookup_power_well(struct drm_i915_private *dev_priv,
> > > enum i915_power_well_id power_well_id);
> > > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > > drm_i915_private *dev_priv)
> > >
> > > static void assert_can_enable_dc5(struct drm_i915_private
> > > *dev_priv)
> > > {
> > > - bool pg2_enabled =
> > > intel_display_power_well_is_enabled(dev_priv,
> > > - SKL_DISP_PW_2);
> > > + struct i915_power_well *pg2 =
> > > lookup_power_well(dev_priv,
> > > + SKL_DISP
> > > _P
> > > W_2);
> > > + bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > > pg2);
> > >
> > > WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > > DC5.\n");
> > >
> > > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > > chv_power_wells[] = {
> > > },
> > > };
> > >
> > > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > > *dev_priv,
> > > - enum i915_power_well_id
> > > power_well_id)
> > > -{
> > > - struct i915_power_well *power_well;
> > > - bool ret;
> > > -
> > > - power_well = lookup_power_well(dev_priv, power_well_id);
> > > - ret = power_well->desc->ops->is_enabled(dev_priv,
> > > power_well);
> > > -
> > > - return ret;
> > > -}
> > > -
> >
> > Or rather export a locked version of it and use that in
> > intel_hdcp.c
> > to
> > better hide the internals?
>
> That should probably be combined with José's idea of using ->enabled
> so
> we trust the hardware sync.
>
> Thanks for the suggestions.
After further analysis, I wonder if intel_hdcp.c should really be
checking for enabled power wells or if it should be doing something
else, such as actually grabbing power domain references to make sure
we're able to enable/disable HDCP whenever we need. Most of our code
should not be checking for power wells/domains being enabled/disabled
(except for HW readout), it should actually be requesting those
resources to make sure we have them when we need them.
CCing Ramaligam for that.
>
> >
> > > static const struct i915_power_well_desc skl_power_wells[] = {
> > > {
> > > .name = "always-on",
> > > --
> > > 2.14.4
> > >
>
> ___
> 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 1/4] drm/i915: kill intel_display_power_well_is_enabled()
Em Qua, 2018-08-15 às 23:27 +0300, Imre Deak escreveu:
> On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> > Use the same coding pattern as we use in the other functions of the
> > same file: just call lookup_power_well() directly in the only
> > caller.
> >
> > Cc: Imre Deak
> > Signed-off-by: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-
> > 1 file changed, 3 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index e209edbc561d..e0947f662361 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,9 +49,6 @@
> > * present for a given platform.
> > */
> >
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -enum i915_power_well_id
> > power_well_id);
> > -
> > static struct i915_power_well *
> > lookup_power_well(struct drm_i915_private *dev_priv,
> > enum i915_power_well_id power_well_id);
> > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > drm_i915_private *dev_priv)
> >
> > static void assert_can_enable_dc5(struct drm_i915_private
> > *dev_priv)
> > {
> > - bool pg2_enabled =
> > intel_display_power_well_is_enabled(dev_priv,
> > - SKL_DISP_PW_2);
> > + struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> > + SKL_DISP_P
> > W_2);
> > + bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > pg2);
> >
> > WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > DC5.\n");
> >
> > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > chv_power_wells[] = {
> > },
> > };
> >
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -enum i915_power_well_id
> > power_well_id)
> > -{
> > - struct i915_power_well *power_well;
> > - bool ret;
> > -
> > - power_well = lookup_power_well(dev_priv, power_well_id);
> > - ret = power_well->desc->ops->is_enabled(dev_priv,
> > power_well);
> > -
> > - return ret;
> > -}
> > -
>
> Or rather export a locked version of it and use that in intel_hdcp.c
> to
> better hide the internals?
That should probably be combined with José's idea of using ->enabled so
we trust the hardware sync.
Thanks for the suggestions.
>
> > static const struct i915_power_well_desc skl_power_wells[] = {
> > {
> > .name = "always-on",
> > --
> > 2.14.4
> >
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()
On Wed, Aug 08, 2018 at 03:16:11PM -0700, Paulo Zanoni wrote:
> Use the same coding pattern as we use in the other functions of the
> same file: just call lookup_power_well() directly in the only caller.
>
> Cc: Imre Deak
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index e209edbc561d..e0947f662361 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,9 +49,6 @@
> * present for a given platform.
> */
>
> -bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> - enum i915_power_well_id power_well_id);
> -
> static struct i915_power_well *
> lookup_power_well(struct drm_i915_private *dev_priv,
> enum i915_power_well_id power_well_id);
> @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct drm_i915_private
> *dev_priv)
>
> static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
> {
> - bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
> - SKL_DISP_PW_2);
> + struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> + SKL_DISP_PW_2);
> + bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
>
> WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
>
> @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> chv_power_wells[] = {
> },
> };
>
> -bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> - enum i915_power_well_id power_well_id)
> -{
> - struct i915_power_well *power_well;
> - bool ret;
> -
> - power_well = lookup_power_well(dev_priv, power_well_id);
> - ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
> -
> - return ret;
> -}
> -
Or rather export a locked version of it and use that in intel_hdcp.c to
better hide the internals?
> static const struct i915_power_well_desc skl_power_wells[] = {
> {
> .name = "always-on",
> --
> 2.14.4
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()
Em Qua, 2018-08-08 às 15:22 -0700, Souza, Jose escreveu:
> On Wed, 2018-08-08 at 15:16 -0700, Paulo Zanoni wrote:
> > Use the same coding pattern as we use in the other functions of the
> > same file: just call lookup_power_well() directly in the only
> > caller.
> >
> > Cc: Imre Deak
> > Signed-off-by: Paulo Zanoni
> > ---
> > drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-
> > 1 file changed, 3 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index e209edbc561d..e0947f662361 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -49,9 +49,6 @@
> > * present for a given platform.
> > */
> >
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -enum i915_power_well_id
> > power_well_id);
> > -
> > static struct i915_power_well *
> > lookup_power_well(struct drm_i915_private *dev_priv,
> > enum i915_power_well_id power_well_id);
> > @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> > drm_i915_private *dev_priv)
> >
> > static void assert_can_enable_dc5(struct drm_i915_private
> > *dev_priv)
> > {
> > - bool pg2_enabled =
> > intel_display_power_well_is_enabled(dev_priv,
> > - SKL_DISP_PW_2);
> > + struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> > + SKL_DISP_P
> > W_2);
> > + bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv,
> > pg2);
>
> Why not trust our sync with hardware and use pg2->hw_enabled?
While that's a very reasonable thing and it sounds like something we
should do (as far as I have investigated in 5 minutes), it's a
functional change with a potential downside and should be in a separate
patch IMHO.
>
> >
> > WARN_ONCE(pg2_enabled, "PG2 not disabled to enable
> > DC5.\n");
> >
> > @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> > chv_power_wells[] = {
> > },
> > };
> >
> > -bool intel_display_power_well_is_enabled(struct drm_i915_private
> > *dev_priv,
> > -enum i915_power_well_id
> > power_well_id)
> > -{
> > - struct i915_power_well *power_well;
> > - bool ret;
> > -
> > - power_well = lookup_power_well(dev_priv, power_well_id);
> > - ret = power_well->desc->ops->is_enabled(dev_priv,
> > power_well);
> > -
> > - return ret;
> > -}
> > -
> > static const struct i915_power_well_desc skl_power_wells[] = {
> > {
> > .name = "always-on",
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/4] drm/i915: kill intel_display_power_well_is_enabled()
On Wed, 2018-08-08 at 15:16 -0700, Paulo Zanoni wrote:
> Use the same coding pattern as we use in the other functions of the
> same file: just call lookup_power_well() directly in the only caller.
>
> Cc: Imre Deak
> Signed-off-by: Paulo Zanoni
> ---
> drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++-
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index e209edbc561d..e0947f662361 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -49,9 +49,6 @@
> * present for a given platform.
> */
>
> -bool intel_display_power_well_is_enabled(struct drm_i915_private
> *dev_priv,
> - enum i915_power_well_id
> power_well_id);
> -
> static struct i915_power_well *
> lookup_power_well(struct drm_i915_private *dev_priv,
> enum i915_power_well_id power_well_id);
> @@ -678,8 +675,9 @@ static void assert_csr_loaded(struct
> drm_i915_private *dev_priv)
>
> static void assert_can_enable_dc5(struct drm_i915_private *dev_priv)
> {
> - bool pg2_enabled =
> intel_display_power_well_is_enabled(dev_priv,
> - SKL_DISP_PW_2);
> + struct i915_power_well *pg2 = lookup_power_well(dev_priv,
> + SKL_DISP_PW_2);
> + bool pg2_enabled = pg2->desc->ops->is_enabled(dev_priv, pg2);
Why not trust our sync with hardware and use pg2->hw_enabled?
>
> WARN_ONCE(pg2_enabled, "PG2 not disabled to enable DC5.\n");
>
> @@ -2302,18 +2300,6 @@ static const struct i915_power_well_desc
> chv_power_wells[] = {
> },
> };
>
> -bool intel_display_power_well_is_enabled(struct drm_i915_private
> *dev_priv,
> - enum i915_power_well_id
> power_well_id)
> -{
> - struct i915_power_well *power_well;
> - bool ret;
> -
> - power_well = lookup_power_well(dev_priv, power_well_id);
> - ret = power_well->desc->ops->is_enabled(dev_priv, power_well);
> -
> - return ret;
> -}
> -
> static const struct i915_power_well_desc skl_power_wells[] = {
> {
> .name = "always-on",
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
