On Fri, Nov 21, 2014 at 09:54:29PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> On gen4 and earlier the GPU reset also resets the display, so we should
> protect against concurrent modeset operations. Grab all the modeset locks
> around the entire GPU reset dance, remebering first ti dislogde any
> pending page flip to make sure we don't deadlock. Any pageflip coming
> in between these two steps should fail anyway due to reset_in_progress,
> so this should be safe.
>
> This fixes a lot of failed asserts in the modeset code when there's a
> modeset racing with the reset. Naturally the asserts aren't happy when
> the expected state has disappeared.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Two comments on this one, otherwise looks good (well didn't bother to
check the new reset register frobbing).
-Daniel
> ---
> drivers/gpu/drm/i915/i915_drv.c | 19 -------
> drivers/gpu/drm/i915/i915_irq.c | 5 +-
> drivers/gpu/drm/i915/intel_display.c | 96 ++++++++++++++++++++++++++++++------
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> 4 files changed, 86 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 5066fd1..1e9c136 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -880,25 +880,6 @@ int i915_reset(struct drm_device *dev)
> */
> if (INTEL_INFO(dev)->gen > 5)
> intel_reset_gt_powersave(dev);
> -
> -
> - if (IS_GEN3(dev) || (IS_GEN4(dev) && !IS_G4X(dev))) {
> - intel_runtime_pm_disable_interrupts(dev_priv);
> - intel_runtime_pm_enable_interrupts(dev_priv);
> -
> - intel_modeset_init_hw(dev);
> -
> - spin_lock_irq(&dev_priv->irq_lock);
> - if (dev_priv->display.hpd_irq_setup)
> - dev_priv->display.hpd_irq_setup(dev);
> - spin_unlock_irq(&dev_priv->irq_lock);
> -
> - drm_modeset_lock_all(dev);
> - intel_modeset_setup_hw_state(dev, true);
> - drm_modeset_unlock_all(dev);
> -
> - intel_hpd_init(dev_priv);
> - }
> } else {
> mutex_unlock(&dev->struct_mutex);
> }
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5908580d..8887674 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2428,6 +2428,9 @@ static void i915_error_work_func(struct work_struct *work)
> * simulated reset via debugs, so get an RPM reference.
> */
> intel_runtime_pm_get(dev_priv);
> +
> + intel_prepare_reset(dev);
> +
> /*
> * All state reset _must_ be completed before we update the
> * reset counter, for otherwise waiters might miss the reset
> @@ -2436,7 +2439,7 @@ static void i915_error_work_func(struct work_struct *work)
> */
> ret = i915_reset(dev);
>
> - intel_display_handle_reset(dev);
> + intel_finish_reset(dev);
>
> intel_runtime_pm_put(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3218455..8329f7c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2765,25 +2765,10 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> return 0;
> }
>
> -void intel_display_handle_reset(struct drm_device *dev)
> +static void intel_complete_page_flips(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> struct drm_crtc *crtc;
>
> - /*
> - * Flips in the rings have been nuked by the reset,
> - * so complete all pending flips so that user space
> - * will get its events and not get stuck.
> - *
> - * Also update the base address of all primary
> - * planes to the the last fb to make sure we're
> - * showing the correct fb after a reset.
> - *
> - * Need to make two loops over the crtcs so that we
> - * don't try to grab a crtc mutex before the
> - * pending_flip_queue really got woken up.
> - */
> -
> for_each_crtc(dev, crtc) {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> enum plane plane = intel_crtc->plane;
> @@ -2791,6 +2776,12 @@ void intel_display_handle_reset(struct drm_device *dev)
> intel_prepare_page_flip(dev, plane);
> intel_finish_page_flip_plane(dev, plane);
> }
> +}
> +
> +static void intel_update_primary_planes(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc;
>
> for_each_crtc(dev, crtc) {
> struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -2810,6 +2801,79 @@ void intel_display_handle_reset(struct drm_device *dev)
> }
> }
>
> +void intel_prepare_reset(struct drm_device *dev)
> +{
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
ums just officially died please remove. Same for the one below.
> +
> + /*
> + * Flips in the rings will be nuked by the reset,
> + * so complete all pending flips so that user space
> + * will get its events and not get stuck.
> + *
> + * Old platforms will also reset the display, so we
> + * need to grab the modeset locks around the reset.
> + * But in order to do that we must let any pending
> + * page flip wait complete since the waiters may be
> + * holding some modeset locks.
> + */
> + intel_complete_page_flips(dev);
Is this really required? We complete them afterwards, and all the pageflip
waiters I've found do check for gpu hangs and abort the pageflip wait.
That's already required since the mmio flip might go missing, and thus far
we've only completed the flip _after_ having reset the gpu and gem state
(and grabbed dev->struct_mutex).
> +
> + /* no reset support for gen2 */
> + if (IS_GEN2(dev))
> + return;
> +
> + /* reset doesn't touch the display */
> + if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
> + return;
> +
> + drm_modeset_lock_all(dev);
> +}
> +
> +void intel_finish_reset(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = to_i915(dev);
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return;
> +
> + /* no reset support for gen2 */
> + if (IS_GEN2(dev))
> + return;
> +
> + /* reset doesn't touch the display */
> + if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev)) {
> + /*
> + * Flips in the rings have been nuked by the reset,
> + * so update the base address of all primary
> + * planes to the the last fb to make sure we're
> + * showing the correct fb after a reset.
> + */
> + intel_update_primary_planes(dev);
> + return;
> + }
> +
> + /*
> + * The display has been reset as well,
> + * so need a full re-initialization.
> + */
> + intel_runtime_pm_disable_interrupts(dev_priv);
> + intel_runtime_pm_enable_interrupts(dev_priv);
> +
> + intel_modeset_init_hw(dev);
> +
> + spin_lock_irq(&dev_priv->irq_lock);
> + if (dev_priv->display.hpd_irq_setup)
> + dev_priv->display.hpd_irq_setup(dev);
> + spin_unlock_irq(&dev_priv->irq_lock);
> +
> + intel_modeset_setup_hw_state(dev, true);
> +
> + intel_hpd_init(dev_priv);
> +
> + drm_modeset_unlock_all(dev);
> +}
> +
> static int
> intel_finish_fb(struct drm_framebuffer *old_fb)
> {
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f0a46ec..25fdbb1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -958,7 +958,8 @@ unsigned long intel_gen4_compute_page_offset(int *x, int *y,
> unsigned int tiling_mode,
> unsigned int bpp,
> unsigned int pitch);
> -void intel_display_handle_reset(struct drm_device *dev);
> +void intel_prepare_reset(struct drm_device *dev);
> +void intel_finish_reset(struct drm_device *dev);
> void hsw_enable_pc8(struct drm_i915_private *dev_priv);
> void hsw_disable_pc8(struct drm_i915_private *dev_priv);
> void intel_dp_get_m_n(struct intel_crtc *crtc,
> --
> 2.0.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
