On Mon, Feb 12, 2018 at 10:21:11PM +0000, Chris Wilson wrote:
> Cc: Ville Syrjälä <ville.syrj...@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 65 
> ++++++++++++++++++++++---------------
>  1 file changed, 38 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index e098e4b2c85c..ad707c97a9d9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -74,6 +74,14 @@ int intel_usecs_to_scanlines(const struct drm_display_mode 
> *adjusted_mode,
>  #define VBLANK_EVASION_TIME_US 100
>  #endif
>  
> +static inline bool scanline_in_vblank(struct intel_crtc *crtc,
> +                                   int min, int max,
> +                                   int *scanline)

The function name is wrong. Not sure what to call it really.
scanline_ok(), or reverse the condition and call it
scanline_bad() or scanline_in_danger_zone() (cue Kenny Loggins)?

> +{
> +     *scanline = intel_get_crtc_scanline(crtc);
> +     return *scanline < min || *scanline > max;
> +}
> +
>  /**
>   * intel_pipe_update_start() - start update of a set of display registers
>   * @new_crtc_state: the new crtc state
> @@ -91,12 +99,9 @@ void intel_pipe_update_start(const struct intel_crtc_state 
> *new_crtc_state)
>       struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc);
>       struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>       const struct drm_display_mode *adjusted_mode = 
> &new_crtc_state->base.adjusted_mode;
> -     long timeout = msecs_to_jiffies_timeout(1);
>       int scanline, min, max, vblank_start;
> -     wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
>       bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || 
> IS_CHERRYVIEW(dev_priv)) &&
>               intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI);
> -     DEFINE_WAIT(wait);
>  
>       vblank_start = adjusted_mode->crtc_vblank_start;
>       if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> @@ -112,41 +117,46 @@ void intel_pipe_update_start(const struct 
> intel_crtc_state *new_crtc_state)
>       if (min <= 0 || max <= 0)
>               return;
>  
> -     if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> -             return;
> -
>       crtc->debug.min_vbl = min;
>       crtc->debug.max_vbl = max;
>       trace_i915_pipe_update_start(crtc);
>  
> -     for (;;) {
> -             /*
> -              * prepare_to_wait() has a memory barrier, which guarantees
> -              * other CPUs can see the task state update by the time we
> -              * read the scanline.
> -              */
> -             prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> +     if (!scanline_in_vblank(crtc, min, max, &scanline)) {

'goto skip_evade' to avoid messing with the loop indentation?

> +             wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> +             long timeout = msecs_to_jiffies_timeout(1);
> +             DEFINE_WAIT(wait);
>  
> -             scanline = intel_get_crtc_scanline(crtc);
> -             if (scanline < min || scanline > max)
> -                     break;
> +             if (WARN_ON(drm_crtc_vblank_get(&crtc->base)))
> +                     goto skip_evade;
>  
> -             if (timeout <= 0) {
> -                     DRM_ERROR("Potential atomic update failure on pipe 
> %c\n",
> -                               pipe_name(crtc->pipe));
> -                     break;
> -             }
> +             for (;;) {
> +                     /*
> +                      * prepare_to_wait() has a memory barrier, which
> +                      * guarantees other CPUs can see the task state update
> +                      * by the time we read the scanline.
> +                      */
> +                     prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>  
> -             local_irq_enable();
> +                     if (scanline_in_vblank(crtc, min, max, &scanline))
> +                             break;
>  
> -             timeout = schedule_timeout(timeout);
> +                     if (timeout <= 0) {
> +                             DRM_ERROR("Potential atomic update failure on 
> pipe %c\n",
> +                                       pipe_name(crtc->pipe));
> +                             break;
> +                     }
>  
> -             local_irq_disable();
> -     }
> +                     local_irq_enable();
>  
> -     finish_wait(wq, &wait);
> +                     timeout = schedule_timeout(timeout);
>  
> -     drm_crtc_vblank_put(&crtc->base);
> +                     local_irq_disable();
> +             }
> +
> +             finish_wait(wq, &wait);
> +
> +             drm_crtc_vblank_put(&crtc->base);
> +     }
>  
>       /*
>        * On VLV/CHV DSI the scanline counter would appear to
> @@ -166,6 +176,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);

I think we'll need to do the dsi w/a regardless, so the label should be
before it.

>  
> +skip_evade:
>       crtc->debug.scanline_start = scanline;
>       crtc->debug.start_vbl_time = ktime_get();
>       crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc);
> -- 
> 2.16.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to