| Message ID | 20180808221614.15486-1-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/i915: kill intel_display_power_well_is_enabled() | expand |
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 <imre.deak@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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",
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 <imre.deak@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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",
Em Qua, 2018-08-08 às 22:22 +0000, Patchwork escreveu: > == Series Details == > > Series: series starting with [1/4] drm/i915: kill > intel_display_power_well_is_enabled() > URL : https://patchwork.freedesktop.org/series/47908/ > State : warning > > == Summary == > > $ dim checkpatch origin/drm-tip > 94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled() > dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well() > -:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using > WARN_ON & recovery code rather than BUG() or BUG_ON() > #31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124: > + BUG(); See the commit message of patch 2, Mr. Robot. I don't think it's worth adding checking code in the callers, and replacing the current null pointer dereference with a BUG() is an improvement IMHO. If anybody disagrees with this, please say so. > > total: 0 errors, 1 warnings, 0 checks, 9 lines checked > e5298551c379 drm/i915: use for_each_power_well in lookup_power_well() > 942f90859025 drm/i915: move lookup_power_well() up > -:40: WARNING:AVOID_BUG: Avoid crashing the kernel - try using > WARN_ON & recovery code rather than BUG() or BUG_ON() > #40: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:683: > + BUG(); > > total: 0 errors, 1 warnings, 0 checks, 50 lines checked >
On Thu, 09 Aug 2018 00:58:53 +0200, Paulo Zanoni <paulo.r.zanoni@intel.com> wrote: > Em Qua, 2018-08-08 às 22:22 +0000, Patchwork escreveu: >> == Series Details == >> >> Series: series starting with [1/4] drm/i915: kill >> intel_display_power_well_is_enabled() >> URL : https://patchwork.freedesktop.org/series/47908/ >> State : warning >> >> == Summary == >> >> $ dim checkpatch origin/drm-tip >> 94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled() >> dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well() >> -:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using >> WARN_ON & recovery code rather than BUG() or BUG_ON() >> #31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124: >> + BUG(); > > See the commit message of patch 2, Mr. Robot. I don't think it's worth > adding checking code in the callers, and replacing the current null > pointer dereference with a BUG() is an improvement IMHO. > > If anybody disagrees with this, please say so. other option could be to use WARN_ON to make Mr. Robot happy and then return first/last/default power well to make callers happy /michal
Em Qui, 2018-08-09 às 08:29 +0200, Michal Wajdeczko escreveu: > On Thu, 09 Aug 2018 00:58:53 +0200, Paulo Zanoni > <paulo.r.zanoni@intel.com> wrote: > > > Em Qua, 2018-08-08 às 22:22 +0000, Patchwork escreveu: > > > == Series Details == > > > > > > Series: series starting with [1/4] drm/i915: kill > > > intel_display_power_well_is_enabled() > > > URL : https://patchwork.freedesktop.org/series/47908/ > > > State : warning > > > > > > == Summary == > > > > > > $ dim checkpatch origin/drm-tip > > > 94cddb6f9752 drm/i915: kill intel_display_power_well_is_enabled() > > > dfb09a8944b0 drm/i915: BUG() if we can't lookup_power_well() > > > -:31: WARNING:AVOID_BUG: Avoid crashing the kernel - try using > > > WARN_ON & recovery code rather than BUG() or BUG_ON() > > > #31: FILE: drivers/gpu/drm/i915/intel_runtime_pm.c:1124: > > > + BUG(); > > > > See the commit message of patch 2, Mr. Robot. I don't think it's > > worth > > adding checking code in the callers, and replacing the current null > > pointer dereference with a BUG() is an improvement IMHO. > > > > If anybody disagrees with this, please say so. > > other option could be to use WARN_ON to make Mr. Robot happy and > then return first/last/default power well to make callers happy Yeah, that's probably an improvement. I'll submit a new version. Thanks! > > /michal
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 <imre.deak@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > 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 >
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 <imre.deak@intel.com> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > 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 > >
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 <imre.deak@intel.com> > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > 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
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 <imre.deak@intel.com> > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > 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
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 <imre.deak@intel.com> > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > --- > > > > > 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 > 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. > > > > > > > > > > > > > > > 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
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
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; -} - static const struct i915_power_well_desc skl_power_wells[] = { { .name = "always-on",
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 <imre.deak@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)