[Intel-gfx] [PATCH 02/10] drm/i915: disable power well support on platforms without runtime PM support

[Ville Syrjälä]

On Wed, Dec 16, 2015 at 03:21:00PM +0200, Imre Deak wrote:
> On ke, 2015-12-16 at 12:12 +0100, Daniel Vetter wrote:
> > On Tue, Dec 15, 2015 at 10:57:31PM +0200, Ville Syrjälä wrote:
> > > On Tue, Dec 15, 2015 at 08:10:30PM +0200, Imre Deak wrote:
> > > > All platforms with power well support have runtime PM support, so
> > > > simplify things by explicitly disabling power well support on
> > > > platforms
> > > > without runtime PM support. This results in holding the init
> > > > power domain
> > > > reference whenever the driver is loaded in addition to an RPM
> > > > reference,
> > > > which reflects the reality better and makes it possible to
> > > > simplify
> > > > things by removing the HAS_RUNTIME_PM special casing from more
> > > > places in
> > > > a follow-up patch.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_runtime_pm.c | 5 +++++
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > index 9945040..f4ff5f5 100644
> > > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > > > @@ -1908,6 +1908,11 @@ static int
> > > >  sanitize_disable_power_well_option(const struct drm_i915_private
> > > > *dev_priv,
> > > >  				   int disable_power_well)
> > > >  {
> > > > +	if (!HAS_RUNTIME_PM(dev_priv)) {
> > > > +		DRM_DEBUG_KMS("No runtime PM support, disabling
> > > > display power well support\n");
> > > > +		return 0;
> > > > +	}
> > > 
> > > Feels a bit too magic to me. Needs a comment at least, otherwise
> > > someone
> > > is going to change disable_power_well back into something you can
> > > disable
> > > at runtime and then all the old stuff might break.
> > > 
> > > Grabbing an extra rpm reference explicitly for this purpose might
> > > be
> > > less confusing.
> > 
> > Changing module options that are marked as debug taints your kernel.
> > Keeping them read-only (so that at least nothing can change at
> > runtime) is
> > imo much preferred, and it's what I started to require on the gem
> > side.
> > Really not worth to have all these checks and complexity around for
> > the
> > off chance some developers want to change the option without
> > reloading the
> > driver.
> 
> Well, disabling power well support if the platform doesn't support RPM
> is for simplicity, so we don't need to think about different code
> paths. What Ville suggested is just for documentation purposes and for
> ensuring that RPM won't get re-enabled if somebody decides in the
> future to change how the disable_power_well option behaves. I think it
> makes sense.

Not sure about this still. I'm thinking that it might reduce coverage
if we ever add some power domain refcount asserts to some places.

-- 
Ville Syrjälä
Intel OTC