On Mon, Feb 12, 2018 at 10:21:11PM +0000, Chris Wilson wrote:
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
