[Intel-gfx] [PATCH v2 1/5] drm/i915: Fix mmio vs. CS flip race on ILK+
sourab gupta
sourabgupta at gmail.com
Thu May 22 10:43:23 CEST 2014
Reviewed the code. Everything looks good, so
Reviewed-by: Sourab Gupta <sourabgupta at gmail.com>
On Wed, May 21, 2014 at 6:09 AM, Rodrigo Vivi <rodrigo.vivi at gmail.com>wrote:
> needs a small rebase but everything looks good for me so
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>
>
> On Tue, Apr 15, 2014 at 11:41 AM, <ville.syrjala at linux.intel.com> wrote:
>
>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>
>> Starting from ILK, mmio flips also cause a flip done interrupt to be
>> signalled. This means if we first do a set_base and follow it
>> immediately with the CS flip, we might mistake the flip done interrupt
>> caused by the set_base as the flip done interrupt caused by the CS
>> flip.
>>
>> The hardware has a flip counter which increments every time a mmio or
>> CS flip is issued. It basically counts the number of DSPSURF register
>> writes. This means we can sample the counter before we put the CS
>> flip into the ring, and then when we get a flip done interrupt we can
>> check whether the CS flip has actually performed the surface address
>> update, or if the interrupt was caused by a previous but yet
>> unfinished mmio flip.
>>
>> Even with the flip counter we still have a race condition of the CS flip
>> base address update happens after the mmio flip done interrupt was
>> raised but not yet processed by the driver. When the interrupt is
>> eventually processed, the flip counter will already indicate that the
>> CS flip has been executed, but it would not actually complete until the
>> next start of vblank. We can use the DSPSURFLIVE register to check
>> whether the hardware is actually scanning out of the buffer we expect,
>> or if we managed hit this race window.
>>
>> This covers all the cases where the CS flip actually changes the base
>> address. If the base address remains unchanged, we might still complete
>> the CS flip before it has actually completed. But since the address
>> didn't change anyway, the premature flip completion can't result in
>> userspace overwriting data that's still being scanned out.
>>
>> CTG already has the flip counter and DSPSURFLIVE registers, and
>> although the flip done interrupt is still limited to CS flips alone,
>> the code now also checks the flip counter on CTG as well.
>>
>> v2: s/dspsurf/gtt_offset/ (Chris)
>>
>> Testcase: igt/kms_mmio_vs_cs_flip/setcrtc_vs_cs_flip
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73027
>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>> ---
>> drivers/gpu/drm/i915/i915_reg.h | 1 +
>> drivers/gpu/drm/i915/intel_display.c | 73
>> ++++++++++++++++++++++++++++++++----
>> drivers/gpu/drm/i915/intel_drv.h | 2 +
>> 3 files changed, 69 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>> b/drivers/gpu/drm/i915/i915_reg.h
>> index 8f84555..c28169c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3638,6 +3638,7 @@ enum punit_power_well {
>> #define _PIPEA_FRMCOUNT_GM45 0x70040
>> #define _PIPEA_FLIPCOUNT_GM45 0x70044
>> #define PIPE_FRMCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FRMCOUNT_GM45)
>> +#define PIPE_FLIPCOUNT_GM45(pipe) _PIPE2(pipe, _PIPEA_FLIPCOUNT_GM45)
>>
>> /* Cursor A & B regs */
>> #define _CURACNTR (dev_priv->info.display_mmio_offset +
>> 0x70080)
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 1390ab5..32c6c16 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -8577,6 +8577,48 @@ void intel_finish_page_flip_plane(struct
>> drm_device *dev, int plane)
>> do_intel_finish_page_flip(dev, crtc);
>> }
>>
>> +/* Is 'a' after or equal to 'b'? */
>> +static bool flip_count_after_eq(u32 a, u32 b)
>> +{
>> + return !((a - b) & 0x80000000);
>> +}
>> +
>> +static bool page_flip_finished(struct intel_crtc *crtc)
>> +{
>> + struct drm_device *dev = crtc->base.dev;
>> + struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> + /*
>> + * The relevant registers doen't exist on pre-ctg.
>> + * As the flip done interrupt doesn't trigger for mmio
>> + * flips on gmch platforms, a flip count check isn't
>> + * really needed there. But since ctg has the registers,
>> + * include it in the check anyway.
>> + */
>> + if (INTEL_INFO(dev)->gen < 5 && !IS_G4X(dev))
>> + return true;
>> +
>> + /*
>> + * A DSPSURFLIVE check isn't enough in case the mmio and CS flips
>> + * used the same base address. In that case the mmio flip might
>> + * have completed, but the CS hasn't even executed the flip yet.
>> + *
>> + * A flip count check isn't enough as the CS might have updated
>> + * the base address just after start of vblank, but before we
>> + * managed to process the interrupt. This means we'd complete the
>> + * CS flip too soon.
>> + *
>> + * Combining both checks should get us a good enough result. It
>> may
>> + * still happen that the CS flip has been executed, but has not
>> + * yet actually completed. But in case the base address is the
>> same
>> + * anyway, we don't really care.
>> + */
>> + return (I915_READ(DSPSURFLIVE(crtc->plane)) & ~0xfff) ==
>> + crtc->unpin_work->gtt_offset &&
>> +
>> flip_count_after_eq(I915_READ(PIPE_FLIPCOUNT_GM45(crtc->pipe)),
>> + crtc->unpin_work->flip_count);
>> +}
>> +
>> void intel_prepare_page_flip(struct drm_device *dev, int plane)
>> {
>> struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -8589,7 +8631,7 @@ void intel_prepare_page_flip(struct drm_device
>> *dev, int plane)
>> * is also accompanied by a spurious intel_prepare_page_flip().
>> */
>> spin_lock_irqsave(&dev->event_lock, flags);
>> - if (intel_crtc->unpin_work)
>> + if (intel_crtc->unpin_work && page_flip_finished(intel_crtc))
>> atomic_inc_not_zero(&intel_crtc->unpin_work->pending);
>> spin_unlock_irqrestore(&dev->event_lock, flags);
>> }
>> @@ -8619,6 +8661,9 @@ static int intel_gen2_queue_flip(struct drm_device
>> *dev,
>> if (ret)
>> goto err;
>>
>> + intel_crtc->unpin_work->gtt_offset =
>> + i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>> ret = intel_ring_begin(ring, 6);
>> if (ret)
>> goto err_unpin;
>> @@ -8635,7 +8680,7 @@ static int intel_gen2_queue_flip(struct drm_device
>> *dev,
>> intel_ring_emit(ring, MI_DISPLAY_FLIP |
>> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>> intel_ring_emit(ring, fb->pitches[0]);
>> - intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> + intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>> intel_ring_emit(ring, 0); /* aux display base address, unused */
>>
>> intel_mark_page_flip_active(intel_crtc);
>> @@ -8664,6 +8709,9 @@ static int intel_gen3_queue_flip(struct drm_device
>> *dev,
>> if (ret)
>> goto err;
>>
>> + intel_crtc->unpin_work->gtt_offset =
>> + i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>> ret = intel_ring_begin(ring, 6);
>> if (ret)
>> goto err_unpin;
>> @@ -8677,7 +8725,7 @@ static int intel_gen3_queue_flip(struct drm_device
>> *dev,
>> intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 |
>> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>> intel_ring_emit(ring, fb->pitches[0]);
>> - intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> + intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>> intel_ring_emit(ring, MI_NOOP);
>>
>> intel_mark_page_flip_active(intel_crtc);
>> @@ -8706,6 +8754,9 @@ static int intel_gen4_queue_flip(struct drm_device
>> *dev,
>> if (ret)
>> goto err;
>>
>> + intel_crtc->unpin_work->gtt_offset =
>> + i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>> ret = intel_ring_begin(ring, 4);
>> if (ret)
>> goto err_unpin;
>> @@ -8717,8 +8768,7 @@ static int intel_gen4_queue_flip(struct drm_device
>> *dev,
>> intel_ring_emit(ring, MI_DISPLAY_FLIP |
>> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>> intel_ring_emit(ring, fb->pitches[0]);
>> - intel_ring_emit(ring,
>> - (i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset) |
>> + intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset |
>> obj->tiling_mode);
>>
>> /* XXX Enabling the panel-fitter across page-flip is so far
>> @@ -8755,6 +8805,9 @@ static int intel_gen6_queue_flip(struct drm_device
>> *dev,
>> if (ret)
>> goto err;
>>
>> + intel_crtc->unpin_work->gtt_offset =
>> + i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>> ret = intel_ring_begin(ring, 4);
>> if (ret)
>> goto err_unpin;
>> @@ -8762,7 +8815,7 @@ static int intel_gen6_queue_flip(struct drm_device
>> *dev,
>> intel_ring_emit(ring, MI_DISPLAY_FLIP |
>> MI_DISPLAY_FLIP_PLANE(intel_crtc->plane));
>> intel_ring_emit(ring, fb->pitches[0] | obj->tiling_mode);
>> - intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> + intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>>
>> /* Contrary to the suggestions in the documentation,
>> * "Enable Panel Fitter" does not seem to be required when page
>> @@ -8804,6 +8857,9 @@ static int intel_gen7_queue_flip(struct drm_device
>> *dev,
>> if (ret)
>> goto err;
>>
>> + intel_crtc->unpin_work->gtt_offset =
>> + i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset;
>> +
>> switch(intel_crtc->plane) {
>> case PLANE_A:
>> plane_bit = MI_DISPLAY_FLIP_IVB_PLANE_A;
>> @@ -8881,7 +8937,7 @@ static int intel_gen7_queue_flip(struct drm_device
>> *dev,
>>
>> intel_ring_emit(ring, MI_DISPLAY_FLIP_I915 | plane_bit);
>> intel_ring_emit(ring, (fb->pitches[0] | obj->tiling_mode));
>> - intel_ring_emit(ring, i915_gem_obj_ggtt_offset(obj) +
>> intel_crtc->dspaddr_offset);
>> + intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
>> intel_ring_emit(ring, (MI_NOOP));
>>
>> intel_mark_page_flip_active(intel_crtc);
>> @@ -8979,6 +9035,9 @@ static int intel_crtc_page_flip(struct drm_crtc
>> *crtc,
>> atomic_inc(&intel_crtc->unpin_work_count);
>> intel_crtc->reset_counter =
>> atomic_read(&dev_priv->gpu_error.reset_counter);
>>
>> + if (INTEL_INFO(dev)->gen >= 5 || IS_G4X(dev))
>> + work->flip_count =
>> I915_READ(PIPE_FLIPCOUNT_GM45(intel_crtc->pipe)) + 1;
>> +
>> ret = dev_priv->display.queue_flip(dev, crtc, fb, obj,
>> page_flip_flags);
>> if (ret)
>> goto cleanup_pending;
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index c551472..edef34e 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -590,6 +590,8 @@ struct intel_unpin_work {
>> #define INTEL_FLIP_INACTIVE 0
>> #define INTEL_FLIP_PENDING 1
>> #define INTEL_FLIP_COMPLETE 2
>> + u32 flip_count;
>> + u32 gtt_offset;
>> bool enable_stall_check;
>> };
>>
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>
>
>
> --
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/intel-gfx/attachments/20140522/0a687e5b/attachment.html>
More information about the Intel-gfx
mailing list