Quoting Maarten Lankhorst (2018-02-09 09:54:01)
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> drivers/gpu/drm/i915/intel_sprite.c | 10 ++++++----
> 3 files changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b886bd459acc..eda9543a0199 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -845,7 +845,7 @@ static u32 __intel_get_crtc_scanline_from_timestamp(struct intel_crtc *crtc)
> }
>
> /* I915_READ_FW, only for fast reads of display block, no need for forcewake etc. */
> -static int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> +int __intel_get_crtc_scanline(struct intel_crtc *crtc)
> {
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 468ec1e90e16..fbdbbe741b2f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1340,6 +1340,7 @@ static inline bool intel_irqs_enabled(struct drm_i915_private *dev_priv)
> }
>
> int intel_get_crtc_scanline(struct intel_crtc *crtc);
> +int __intel_get_crtc_scanline(struct intel_crtc *crtc);
> void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv,
> u8 pipe_mask);
> void gen8_irq_power_well_pre_disable(struct drm_i915_private *dev_priv,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 971a1ea0db45..3a34be4fd956 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -119,6 +119,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> crtc->debug.max_vbl = max;
> trace_i915_pipe_update_start(crtc);
>
> + spin_lock(&dev_priv->uncore.lock);
> for (;;) {
> /*
> * prepare_to_wait() has a memory barrier, which guarantees
> @@ -127,7 +128,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> */
> prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
>
> - scanline = intel_get_crtc_scanline(crtc);
> + scanline = __intel_get_crtc_scanline(crtc);
> if (scanline < min || scanline > max)
> break;
>
> @@ -137,11 +138,11 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state)
> break;
> }
>
> - local_irq_enable();
> + spin_unlock_irq(&dev_priv->uncore.lock);
>
> timeout = schedule_timeout(timeout);
>
> - local_irq_disable();
> + spin_lock_irq(&dev_priv->uncore.lock);
> }
>
> finish_wait(wq, &wait);
There's no danger that drm_crtc_vblank_put() does something crazy here.
(Feels like a layering violation to call into DRM with the low level
uncore.lock held at least.) It looks like the driver can be tricked into
called ->disable_vblank()?
Overall though, I think it is just this need_vlv_dsi_wa chunk that has
any benefit here (although trading lock_irq for lock_irqsave is enough
to justify a change if frequently hit).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
