[Intel-gfx] [PATCH] drm/i915: Grab RPM wakeref around enabling vblank interrupts

[Ville Syrjälä]

On Fri, Oct 07, 2016 at 08:00:35PM +0100, Chris Wilson wrote:
> On Fri, Oct 07, 2016 at 07:57:21PM +0100, Chris Wilson wrote:
> > On Fri, Oct 07, 2016 at 09:44:53PM +0300, Ville Syrjälä wrote:
> > > On Fri, Oct 07, 2016 at 07:33:00PM +0100, Chris Wilson wrote:
> > > > On Fri, Oct 07, 2016 at 09:06:07PM +0300, Ville Syrjälä wrote:
> > > > > On Fri, Oct 07, 2016 at 02:50:32PM +0100, Chris Wilson wrote:
> > > > > > Whilst the vblank is configured to send an interrupt everytime, we need
> > > > > > to keep the device awake to process those interrupts.
> > > > > 
> > > > > If we can enable vblanks the pipe will be active, and thus we can't
> > > > > runtime suspend anyway. Also might_sleep() would be a problem.
> > > > 
> > > > Where is the pipe made active? Is it just having any mode is sufficient
> > > > to disable rpm? Will that always be the case?
> > > 
> > > Active pipe holds a power domain reference, which implies an rpm
> > > reference as well.
> > 
> > What's the best way to write
> > 
> > 	/* vblank IRQ requires the powerwell to stay awake */
> > 	WARN_ON(!to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe])->active);

Probably just that. We have some kind of function to hide the details
of the array a bit though, I just never rememer what it's called.
And I think I have patches somewhere to convert all of that over to
storing intel_crtc * instead of drm_crtc * since that's what everyone
wants.

> 
> Also, any reason why we shouldn't merge valleyview_enable_vblank() with
> i915_enable_vblank()?

No reason AFAICS. Same for disable. Though maybe we should add the gen
check to disable to make it symmetric, just to avoid some head scratching.

Hmm. Or actually, maybe just split to i915_*_vblank() and i965_*_vblank()
instead. Not much point in having the if in there when you can just
assign the right function pointer to begin with.

-- 
Ville Syrjälä
Intel OTC