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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
