| Message ID | 1403281762-1927-4-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > Before we've installed the handler, we can set this and avoid confusing > init code that then thinks IRQs are enabled and spews complaints > everywhere. But then at some point the DRM layer will call our IRQ init callbacks, which will initalize the interrupts but leave irqs_disabled as true, which will also confuse some code somewhere at some point. And it will only be set to false after we {runtime,}-suspend/resume. This is why I had kept the runtime PM code only used by the runtime PM stuff. Recently we tried to reuse the runtime PM interrupt code at other contexts, got regressions and now we're fixing the regressions using duct tape... Maybe the best approach would be to revert some patches... > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> > --- > drivers/gpu/drm/i915/i915_irq.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index bc953cc..86638b9 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -4373,6 +4373,9 @@ void intel_irq_init(struct drm_device *dev) > > pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE); > > + /* Haven't installed the IRQ handler yet */ > + dev_priv->pm._irqs_disabled = true; > + > if (IS_GEN2(dev)) { > dev->max_vblank_count = 0; > dev->driver->get_vblank_counter = i8xx_get_vblank_counter; > -- > 1.8.3.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 14, 2014 at 12:23:11PM -0300, Paulo Zanoni wrote: > 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > > Before we've installed the handler, we can set this and avoid confusing > > init code that then thinks IRQs are enabled and spews complaints > > everywhere. > > But then at some point the DRM layer will call our IRQ init callbacks, > which will initalize the interrupts but leave irqs_disabled as true, > which will also confuse some code somewhere at some point. And it will > only be set to false after we {runtime,}-suspend/resume. The drm irq stuff is _strictly_ a helper library, at least for modesetting drivers. Which means it will never call our callbacks on its own. > This is why I had kept the runtime PM code only used by the runtime PM > stuff. Recently we tried to reuse the runtime PM interrupt code at > other contexts, got regressions and now we're fixing the regressions > using duct tape... Maybe the best approach would be to revert some > patches... Those patches where for soix, so feature work. -Daniel
2014-07-14 14:26 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Jul 14, 2014 at 12:23:11PM -0300, Paulo Zanoni wrote: >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: >> > Before we've installed the handler, we can set this and avoid confusing >> > init code that then thinks IRQs are enabled and spews complaints >> > everywhere. >> >> But then at some point the DRM layer will call our IRQ init callbacks, >> which will initalize the interrupts but leave irqs_disabled as true, >> which will also confuse some code somewhere at some point. And it will >> only be set to false after we {runtime,}-suspend/resume. > > The drm irq stuff is _strictly_ a helper library, at least for modesetting > drivers. Which means it will never call our callbacks on its own. I was talking about drm_irq_install(), which is called by i915_driver_load(). > >> This is why I had kept the runtime PM code only used by the runtime PM >> stuff. Recently we tried to reuse the runtime PM interrupt code at >> other contexts, got regressions and now we're fixing the regressions >> using duct tape... Maybe the best approach would be to revert some >> patches... > > Those patches where for soix, so feature work. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, 14 Jul 2014 14:47:07 -0300 Paulo Zanoni <przanoni@gmail.com> wrote: > 2014-07-14 14:26 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>: > > On Mon, Jul 14, 2014 at 12:23:11PM -0300, Paulo Zanoni wrote: > >> 2014-06-20 13:29 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > >> > Before we've installed the handler, we can set this and avoid confusing > >> > init code that then thinks IRQs are enabled and spews complaints > >> > everywhere. > >> > >> But then at some point the DRM layer will call our IRQ init callbacks, > >> which will initalize the interrupts but leave irqs_disabled as true, > >> which will also confuse some code somewhere at some point. And it will > >> only be set to false after we {runtime,}-suspend/resume. > > > > The drm irq stuff is _strictly_ a helper library, at least for modesetting > > drivers. Which means it will never call our callbacks on its own. > > I was talking about drm_irq_install(), which is called by i915_driver_load(). Since this set fixes the failures I've found, I'd rather see it merged than to regress everyone's power consumption. I think it'll probably take us awhile to figure out how we want to do this stuff. Paulo, maybe you can create a task with some of the things you'd like to see done? With these patches, we go away from ever using the drm irq bits except for at load and unload. We use the pm irq fields for tracking our own stuff, which means some special init handling and then calls at suspend/resume, so I don't think it's too complex as-is, but if you want to do something better, that's ok too.
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index bc953cc..86638b9 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -4373,6 +4373,9 @@ void intel_irq_init(struct drm_device *dev) pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE); + /* Haven't installed the IRQ handler yet */ + dev_priv->pm._irqs_disabled = true; + if (IS_GEN2(dev)) { dev->max_vblank_count = 0; dev->driver->get_vblank_counter = i8xx_get_vblank_counter;
Before we've installed the handler, we can set this and avoid confusing init code that then thinks IRQs are enabled and spews complaints everywhere. Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> --- drivers/gpu/drm/i915/i915_irq.c | 3 +++ 1 file changed, 3 insertions(+)