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

[Imre Deak <imre.deak@intel.com>]

On ke, 2015-12-16 at 19:31 +0200, Ville Syrjälä wrote:
> 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@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.

Ok, I'll drop this, it was really just for code path simplification
nothing really depends on it.

--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx