| Message ID | 20180208085538.74764-1-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Maarten Lankhorst (2018-02-08 08:55:38) > References: https://bugs.freedesktop.org/show_bug.cgi?id=104975 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 707406d1bf57..81087722bc49 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14476,6 +14476,7 @@ int intel_modeset_init(struct drm_device *dev) > dev_priv->modeset_wq = alloc_ordered_workqueue("i915_modeset", 0); Noticed in passing, no error handling for a failed kmalloc here. -Chris
On Thu, Feb 08, 2018 at 09:55:38AM +0100, Maarten Lankhorst wrote: > References: https://bugs.freedesktop.org/show_bug.cgi?id=104975 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/i915_reg.h | 3 +++ > drivers/gpu/drm/i915/intel_display.c | 1 + > drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++++++ > 4 files changed, 22 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index d5e695da2fc4..11251e0107f4 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1967,6 +1967,8 @@ struct drm_i915_private { > /* Display functions */ > struct drm_i915_display_funcs display; > > + spinlock_t display_evasion_lock; > + > /* PCH chipset type */ > enum intel_pch pch_type; > unsigned short pch_id; > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 93f6ec2ea8f2..20af56644844 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -6594,6 +6594,9 @@ enum { > #define DIGITAL_PORTA_HOTPLUG_SHORT_DETECT (1 << 0) > #define DIGITAL_PORTA_HOTPLUG_LONG_DETECT (2 << 0) > > +#define DOUBLE_BUFFER_CTL _MMIO(0x44500) > +#define DOUBLE_BUFFER_CTL_GLOBAL_DISABLE (1 << 0) > + > /* refresh rate hardware control */ > #define RR_HW_CTL _MMIO(0x45300) > #define RR_HW_LOW_POWER_FRAMES_MASK 0xff > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 707406d1bf57..81087722bc49 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14476,6 +14476,7 @@ int intel_modeset_init(struct drm_device *dev) > dev_priv->modeset_wq = alloc_ordered_workqueue("i915_modeset", 0); > > drm_mode_config_init(dev); > + spin_lock_init(&dev_priv->display_evasion_lock); > > dev->mode_config.min_width = 0; > dev->mode_config.min_height = 0; > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index ac0a4f9c1954..4d1e9b166d57 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -98,6 +98,13 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) > intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI); > DEFINE_WAIT(wait); > > + if (INTEL_GEN(dev_priv) >= 9) { > + spin_lock_irq(&dev_priv->display_evasion_lock); > + I915_WRITE(DOUBLE_BUFFER_CTL, DOUBLE_BUFFER_CTL_GLOBAL_DISABLE); > + scanline = intel_get_crtc_scanline(crtc); > + goto done; > + } I think we still want to do the vblank evasion. It gives us more accurate infromation on which frame the operation will complete. If we don't do it we may end up signalling the completion one frame late. Although I suppose it doesn't matter too much from the user POV since in each case we'd end up dropping a frame. Maybe for av sync etc. it might matter in some cases. It would become even more important if we allowed flips to be overridden because then we'd really need to know whether the previous flip happened at all or not (so that we could signal fences and whatnot correctly to keep userspace informed on which framebuffer is actually being scanned out). And we might end up holding the lock across every vblank start, thus preventing the display from updating at all. So I think we should use DOUBLE_BUFFER_CTL just make missing the deadline bit less dangerous in the presence of fragile hardware. I do think we should still try to optimize plane/pipe updates more to reduce the chances of that happening in general. Also IIRC there are some "(dis)allow double buffer disable" bits in various hardware blocks which have to set correctly for this to work. Did you check whether we're setting them appropriately? > + > vblank_start = adjusted_mode->crtc_vblank_start; > if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) > vblank_start = DIV_ROUND_UP(vblank_start, 2); > @@ -166,6 +173,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) > while (need_vlv_dsi_wa && scanline == vblank_start) > scanline = intel_get_crtc_scanline(crtc); > > +done: > crtc->debug.scanline_start = scanline; > crtc->debug.start_vbl_time = ktime_get(); > crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc); > @@ -192,6 +200,9 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > > trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end); > > + if (INTEL_GEN(dev_priv) >= 9) > + I915_WRITE(DOUBLE_BUFFER_CTL, 0); > + > /* We're still in the vblank-evade critical section, this can't race. > * Would be slightly nice to just grab the vblank count and arm the > * event outside of the critical section - the spinlock might spin for a > @@ -206,6 +217,11 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) > new_crtc_state->base.event = NULL; > } > > + if (INTEL_GEN(dev_priv) >= 9) { > + spin_unlock_irq(&dev_priv->display_evasion_lock); > + return; > + } > + > local_irq_enable(); > > if (intel_vgpu_active(dev_priv)) > -- > 2.16.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 08-02-18 om 19:27 schreef Ville Syrjälä: > On Thu, Feb 08, 2018 at 09:55:38AM +0100, Maarten Lankhorst wrote: >> References: https://bugs.freedesktop.org/show_bug.cgi?id=104975 >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 2 ++ >> drivers/gpu/drm/i915/i915_reg.h | 3 +++ >> drivers/gpu/drm/i915/intel_display.c | 1 + >> drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++++++ >> 4 files changed, 22 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index d5e695da2fc4..11251e0107f4 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1967,6 +1967,8 @@ struct drm_i915_private { >> /* Display functions */ >> struct drm_i915_display_funcs display; >> >> + spinlock_t display_evasion_lock; >> + >> /* PCH chipset type */ >> enum intel_pch pch_type; >> unsigned short pch_id; >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> index 93f6ec2ea8f2..20af56644844 100644 >> --- a/drivers/gpu/drm/i915/i915_reg.h >> +++ b/drivers/gpu/drm/i915/i915_reg.h >> @@ -6594,6 +6594,9 @@ enum { >> #define DIGITAL_PORTA_HOTPLUG_SHORT_DETECT (1 << 0) >> #define DIGITAL_PORTA_HOTPLUG_LONG_DETECT (2 << 0) >> >> +#define DOUBLE_BUFFER_CTL _MMIO(0x44500) >> +#define DOUBLE_BUFFER_CTL_GLOBAL_DISABLE (1 << 0) >> + >> /* refresh rate hardware control */ >> #define RR_HW_CTL _MMIO(0x45300) >> #define RR_HW_LOW_POWER_FRAMES_MASK 0xff >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 707406d1bf57..81087722bc49 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -14476,6 +14476,7 @@ int intel_modeset_init(struct drm_device *dev) >> dev_priv->modeset_wq = alloc_ordered_workqueue("i915_modeset", 0); >> >> drm_mode_config_init(dev); >> + spin_lock_init(&dev_priv->display_evasion_lock); >> >> dev->mode_config.min_width = 0; >> dev->mode_config.min_height = 0; >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index ac0a4f9c1954..4d1e9b166d57 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -98,6 +98,13 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) >> intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI); >> DEFINE_WAIT(wait); >> >> + if (INTEL_GEN(dev_priv) >= 9) { >> + spin_lock_irq(&dev_priv->display_evasion_lock); >> + I915_WRITE(DOUBLE_BUFFER_CTL, DOUBLE_BUFFER_CTL_GLOBAL_DISABLE); >> + scanline = intel_get_crtc_scanline(crtc); >> + goto done; >> + } > I think we still want to do the vblank evasion. It gives us more accurate > infromation on which frame the operation will complete. If we don't do > it we may end up signalling the completion one frame late. Although I > suppose it doesn't matter too much from the user POV since in each case > we'd end up dropping a frame. Maybe for av sync etc. it might matter > in some cases. > > It would become even more important if we allowed flips to be overridden > because then we'd really need to know whether the previous flip happened > at all or not (so that we could signal fences and whatnot correctly to > keep userspace informed on which framebuffer is actually being scanned > out). And we might end up holding the lock across every vblank start, > thus preventing the display from updating at all. > > So I think we should use DOUBLE_BUFFER_CTL just make missing the > deadline bit less dangerous in the presence of fragile hardware. I do > think we should still try to optimize plane/pipe updates more to reduce > the chances of that happening in general. > > Also IIRC there are some "(dis)allow double buffer disable" bits in various > hardware blocks which have to set correctly for this to work. Did you check > whether we're setting them appropriately? The global switch doesn't mention this at least. It only mentions that async MMIO and command flips are not affected. ~Maarten
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d5e695da2fc4..11251e0107f4 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1967,6 +1967,8 @@ struct drm_i915_private { /* Display functions */ struct drm_i915_display_funcs display; + spinlock_t display_evasion_lock; + /* PCH chipset type */ enum intel_pch pch_type; unsigned short pch_id; diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 93f6ec2ea8f2..20af56644844 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6594,6 +6594,9 @@ enum { #define DIGITAL_PORTA_HOTPLUG_SHORT_DETECT (1 << 0) #define DIGITAL_PORTA_HOTPLUG_LONG_DETECT (2 << 0) +#define DOUBLE_BUFFER_CTL _MMIO(0x44500) +#define DOUBLE_BUFFER_CTL_GLOBAL_DISABLE (1 << 0) + /* refresh rate hardware control */ #define RR_HW_CTL _MMIO(0x45300) #define RR_HW_LOW_POWER_FRAMES_MASK 0xff diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 707406d1bf57..81087722bc49 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14476,6 +14476,7 @@ int intel_modeset_init(struct drm_device *dev) dev_priv->modeset_wq = alloc_ordered_workqueue("i915_modeset", 0); drm_mode_config_init(dev); + spin_lock_init(&dev_priv->display_evasion_lock); dev->mode_config.min_width = 0; dev->mode_config.min_height = 0; diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index ac0a4f9c1954..4d1e9b166d57 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -98,6 +98,13 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI); DEFINE_WAIT(wait); + if (INTEL_GEN(dev_priv) >= 9) { + spin_lock_irq(&dev_priv->display_evasion_lock); + I915_WRITE(DOUBLE_BUFFER_CTL, DOUBLE_BUFFER_CTL_GLOBAL_DISABLE); + scanline = intel_get_crtc_scanline(crtc); + goto done; + } + vblank_start = adjusted_mode->crtc_vblank_start; if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) vblank_start = DIV_ROUND_UP(vblank_start, 2); @@ -166,6 +173,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) while (need_vlv_dsi_wa && scanline == vblank_start) scanline = intel_get_crtc_scanline(crtc); +done: crtc->debug.scanline_start = scanline; crtc->debug.start_vbl_time = ktime_get(); crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc); @@ -192,6 +200,9 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) trace_i915_pipe_update_end(crtc, end_vbl_count, scanline_end); + if (INTEL_GEN(dev_priv) >= 9) + I915_WRITE(DOUBLE_BUFFER_CTL, 0); + /* We're still in the vblank-evade critical section, this can't race. * Would be slightly nice to just grab the vblank count and arm the * event outside of the critical section - the spinlock might spin for a @@ -206,6 +217,11 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state) new_crtc_state->base.event = NULL; } + if (INTEL_GEN(dev_priv) >= 9) { + spin_unlock_irq(&dev_priv->display_evasion_lock); + return; + } + local_irq_enable(); if (intel_vgpu_active(dev_priv))
References: https://bugs.freedesktop.org/show_bug.cgi?id=104975 Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/i915_reg.h | 3 +++ drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_sprite.c | 16 ++++++++++++++++ 4 files changed, 22 insertions(+)