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

[Imre Deak]

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.

--Imre