Re: [PATCH 09/23] drm/i915: make PC8 be part of runtime PM suspend/resume

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Feb 28, 2014 at 09:42:02AM +0000, Chris Wilson wrote:
> On Thu, Feb 27, 2014 at 07:26:36PM -0300, Paulo Zanoni wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index ce582eb..788b165 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6614,7 +6614,7 @@ static void hsw_restore_lcpll(struct drm_i915_private *dev_priv)
> >  
> >  	/* Make sure we're not on PC8 state before disabling PC8, otherwise
> >  	 * we'll hang the machine! */
> > -	gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
> > +	gen6_gt_force_wake_get_no_rpm(dev_priv, FORCEWAKE_ALL);
> 
> I want more indications that this is legal - i.e. that the function is
> only called from a special context handling rpm. As it is, it is not
> immediately clear that the rearrangements in disable_package_c8 is
> complete. If you moved this forcwake dance higher and comment why the
> intel_runtime_pm_get() handling is so special, you might not scare so
> many causal readers.

I agree with Chris, this is tricky and should be obvious and stand out.

What about open-coding the _no_rpm function here (like we have an
open-coded fw get/put around register acccess) and putting a big comment
above this explaining what exactly is going on, why we need it and why
this is safe?

For this case here being explicit and verbose is imo more important than
the little common code extraction the _no_rpm function allows us to do.
Especially since we have open-coded forcewake dances at other places
already, too.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch