* [PATCH] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work
@ 2015-10-10 9:44 Chris Wilson
2015-10-13 9:23 ` Ville Syrjälä
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2015-10-10 9:44 UTC (permalink / raw)
To: intel-gfx
We should serialise access to the intel_crtc->unpin_work through the
dev->event_lock spinlock. It should not be possible for it to disappear
without severe error as the mmio_flip worker has not tagged the
unpin_work pending flip-completion. Similarly if the error exists, just
taking the unpin_work whilst holding the spinlock and then using it
unserialised just masks the race. (It is supposed to be valid as the
unpin_work exists until the flip completion interrupt which should not
fire until we flush the mmio writes to update the display base which is
the last time we access the unpin_work from the kthread.)
References: https://bugs.freedesktop.org/show_bug.cgi?id=92335
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++----------------
1 file changed, 32 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cddb0c692334..71d7298648e0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10848,11 +10848,11 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane)
spin_unlock_irqrestore(&dev->event_lock, flags);
}
-static inline void intel_mark_page_flip_active(struct intel_crtc *intel_crtc)
+static inline void intel_mark_page_flip_active(struct intel_unpin_work *work)
{
/* Ensure that the work item is consistent when activating it ... */
smp_wmb();
- atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
+ atomic_set(&work->pending, INTEL_FLIP_PENDING);
/* and that it is marked active as soon as the irq could fire. */
smp_wmb();
}
@@ -10888,7 +10888,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
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);
+ intel_mark_page_flip_active(intel_crtc->unpin_work);
return 0;
}
@@ -10920,7 +10920,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
intel_ring_emit(ring, MI_NOOP);
- intel_mark_page_flip_active(intel_crtc);
+ intel_mark_page_flip_active(intel_crtc->unpin_work);
return 0;
}
@@ -10959,7 +10959,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
intel_ring_emit(ring, pf | pipesrc);
- intel_mark_page_flip_active(intel_crtc);
+ intel_mark_page_flip_active(intel_crtc->unpin_work);
return 0;
}
@@ -10995,7 +10995,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
intel_ring_emit(ring, pf | pipesrc);
- intel_mark_page_flip_active(intel_crtc);
+ intel_mark_page_flip_active(intel_crtc->unpin_work);
return 0;
}
@@ -11090,7 +11090,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
intel_ring_emit(ring, (MI_NOOP));
- intel_mark_page_flip_active(intel_crtc);
+ intel_mark_page_flip_active(intel_crtc->unpin_work);
return 0;
}
@@ -11121,7 +11121,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
return ring != i915_gem_request_get_ring(obj->last_write_req);
}
-static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
+static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
+ struct intel_unpin_work *work)
{
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -11162,11 +11163,12 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
I915_WRITE(PLANE_CTL(pipe, 0), ctl);
I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
- I915_WRITE(PLANE_SURF(pipe, 0), intel_crtc->unpin_work->gtt_offset);
+ I915_WRITE(PLANE_SURF(pipe, 0), work->gtt_offset);
POSTING_READ(PLANE_SURF(pipe, 0));
}
-static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
+static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc,
+ struct intel_unpin_work *work)
{
struct drm_device *dev = intel_crtc->base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -11186,31 +11188,36 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
I915_WRITE(reg, dspcntr);
- I915_WRITE(DSPSURF(intel_crtc->plane),
- intel_crtc->unpin_work->gtt_offset);
+ I915_WRITE(DSPSURF(intel_crtc->plane), work->gtt_offset);
POSTING_READ(DSPSURF(intel_crtc->plane));
-
}
/*
* XXX: This is the temporary way to update the plane registers until we get
* around to using the usual plane update functions for MMIO flips
*/
-static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
+static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
{
- struct drm_device *dev = intel_crtc->base.dev;
+ struct intel_crtc *crtc = mmio_flip->crtc;
+ struct intel_unpin_work *work;
- intel_mark_page_flip_active(intel_crtc);
+ spin_lock_irq(&crtc->base.dev->event_lock);
+ work = crtc->unpin_work;
+ spin_unlock_irq(&crtc->base.dev->event_lock);
+ if (work == NULL)
+ return;
- intel_pipe_update_start(intel_crtc);
+ intel_mark_page_flip_active(work);
- if (INTEL_INFO(dev)->gen >= 9)
- skl_do_mmio_flip(intel_crtc);
+ intel_pipe_update_start(crtc);
+
+ if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
+ skl_do_mmio_flip(crtc, work);
else
/* use_mmio_flip() retricts MMIO flips to ilk+ */
- ilk_do_mmio_flip(intel_crtc);
+ ilk_do_mmio_flip(crtc, work);
- intel_pipe_update_end(intel_crtc);
+ intel_pipe_update_end(crtc);
}
static void intel_mmio_flip_work_func(struct work_struct *work)
@@ -11218,15 +11225,15 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
struct intel_mmio_flip *mmio_flip =
container_of(work, struct intel_mmio_flip, work);
- if (mmio_flip->req)
+ if (mmio_flip->req) {
WARN_ON(__i915_wait_request(mmio_flip->req,
mmio_flip->crtc->reset_counter,
false, NULL,
&mmio_flip->i915->rps.mmioflips));
+ i915_gem_request_unreference__unlocked(mmio_flip->req);
+ }
- intel_do_mmio_flip(mmio_flip->crtc);
-
- i915_gem_request_unreference__unlocked(mmio_flip->req);
+ intel_do_mmio_flip(mmio_flip);
kfree(mmio_flip);
}
--
2.6.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work
2015-10-10 9:44 [PATCH] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work Chris Wilson
@ 2015-10-13 9:23 ` Ville Syrjälä
2015-10-13 11:09 ` Daniel Vetter
2015-10-14 16:02 ` Chris Wilson
0 siblings, 2 replies; 4+ messages in thread
From: Ville Syrjälä @ 2015-10-13 9:23 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sat, Oct 10, 2015 at 10:44:32AM +0100, Chris Wilson wrote:
> We should serialise access to the intel_crtc->unpin_work through the
> dev->event_lock spinlock. It should not be possible for it to disappear
> without severe error as the mmio_flip worker has not tagged the
> unpin_work pending flip-completion. Similarly if the error exists, just
> taking the unpin_work whilst holding the spinlock and then using it
> unserialised just masks the race. (It is supposed to be valid as the
> unpin_work exists until the flip completion interrupt which should not
> fire until we flush the mmio writes to update the display base which is
> the last time we access the unpin_work from the kthread.)
>
> References: https://bugs.freedesktop.org/show_bug.cgi?id=92335
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
So not sure what's going on yet?
Patch looks OK anyway so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++----------------
> 1 file changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cddb0c692334..71d7298648e0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10848,11 +10848,11 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane)
> spin_unlock_irqrestore(&dev->event_lock, flags);
> }
>
> -static inline void intel_mark_page_flip_active(struct intel_crtc *intel_crtc)
> +static inline void intel_mark_page_flip_active(struct intel_unpin_work *work)
> {
> /* Ensure that the work item is consistent when activating it ... */
> smp_wmb();
> - atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> + atomic_set(&work->pending, INTEL_FLIP_PENDING);
> /* and that it is marked active as soon as the irq could fire. */
> smp_wmb();
> }
> @@ -10888,7 +10888,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> 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);
> + intel_mark_page_flip_active(intel_crtc->unpin_work);
> return 0;
> }
>
> @@ -10920,7 +10920,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
> intel_ring_emit(ring, MI_NOOP);
>
> - intel_mark_page_flip_active(intel_crtc);
> + intel_mark_page_flip_active(intel_crtc->unpin_work);
> return 0;
> }
>
> @@ -10959,7 +10959,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
> intel_ring_emit(ring, pf | pipesrc);
>
> - intel_mark_page_flip_active(intel_crtc);
> + intel_mark_page_flip_active(intel_crtc->unpin_work);
> return 0;
> }
>
> @@ -10995,7 +10995,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
> intel_ring_emit(ring, pf | pipesrc);
>
> - intel_mark_page_flip_active(intel_crtc);
> + intel_mark_page_flip_active(intel_crtc->unpin_work);
> return 0;
> }
>
> @@ -11090,7 +11090,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
> intel_ring_emit(ring, (MI_NOOP));
>
> - intel_mark_page_flip_active(intel_crtc);
> + intel_mark_page_flip_active(intel_crtc->unpin_work);
> return 0;
> }
>
> @@ -11121,7 +11121,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
> return ring != i915_gem_request_get_ring(obj->last_write_req);
> }
>
> -static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> +static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> + struct intel_unpin_work *work)
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -11162,11 +11163,12 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> I915_WRITE(PLANE_CTL(pipe, 0), ctl);
> I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
>
> - I915_WRITE(PLANE_SURF(pipe, 0), intel_crtc->unpin_work->gtt_offset);
> + I915_WRITE(PLANE_SURF(pipe, 0), work->gtt_offset);
> POSTING_READ(PLANE_SURF(pipe, 0));
> }
>
> -static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
> +static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc,
> + struct intel_unpin_work *work)
> {
> struct drm_device *dev = intel_crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -11186,31 +11188,36 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
>
> I915_WRITE(reg, dspcntr);
>
> - I915_WRITE(DSPSURF(intel_crtc->plane),
> - intel_crtc->unpin_work->gtt_offset);
> + I915_WRITE(DSPSURF(intel_crtc->plane), work->gtt_offset);
> POSTING_READ(DSPSURF(intel_crtc->plane));
> -
> }
>
> /*
> * XXX: This is the temporary way to update the plane registers until we get
> * around to using the usual plane update functions for MMIO flips
> */
> -static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
> +static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
> {
> - struct drm_device *dev = intel_crtc->base.dev;
> + struct intel_crtc *crtc = mmio_flip->crtc;
> + struct intel_unpin_work *work;
>
> - intel_mark_page_flip_active(intel_crtc);
> + spin_lock_irq(&crtc->base.dev->event_lock);
> + work = crtc->unpin_work;
> + spin_unlock_irq(&crtc->base.dev->event_lock);
> + if (work == NULL)
> + return;
>
> - intel_pipe_update_start(intel_crtc);
> + intel_mark_page_flip_active(work);
>
> - if (INTEL_INFO(dev)->gen >= 9)
> - skl_do_mmio_flip(intel_crtc);
> + intel_pipe_update_start(crtc);
> +
> + if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
> + skl_do_mmio_flip(crtc, work);
> else
> /* use_mmio_flip() retricts MMIO flips to ilk+ */
> - ilk_do_mmio_flip(intel_crtc);
> + ilk_do_mmio_flip(crtc, work);
>
> - intel_pipe_update_end(intel_crtc);
> + intel_pipe_update_end(crtc);
> }
>
> static void intel_mmio_flip_work_func(struct work_struct *work)
> @@ -11218,15 +11225,15 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
> struct intel_mmio_flip *mmio_flip =
> container_of(work, struct intel_mmio_flip, work);
>
> - if (mmio_flip->req)
> + if (mmio_flip->req) {
> WARN_ON(__i915_wait_request(mmio_flip->req,
> mmio_flip->crtc->reset_counter,
> false, NULL,
> &mmio_flip->i915->rps.mmioflips));
> + i915_gem_request_unreference__unlocked(mmio_flip->req);
> + }
>
> - intel_do_mmio_flip(mmio_flip->crtc);
> -
> - i915_gem_request_unreference__unlocked(mmio_flip->req);
> + intel_do_mmio_flip(mmio_flip);
> kfree(mmio_flip);
> }
>
> --
> 2.6.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work
2015-10-13 9:23 ` Ville Syrjälä
@ 2015-10-13 11:09 ` Daniel Vetter
2015-10-14 16:02 ` Chris Wilson
1 sibling, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2015-10-13 11:09 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Tue, Oct 13, 2015 at 12:23:38PM +0300, Ville Syrjälä wrote:
> On Sat, Oct 10, 2015 at 10:44:32AM +0100, Chris Wilson wrote:
> > We should serialise access to the intel_crtc->unpin_work through the
> > dev->event_lock spinlock. It should not be possible for it to disappear
> > without severe error as the mmio_flip worker has not tagged the
> > unpin_work pending flip-completion. Similarly if the error exists, just
> > taking the unpin_work whilst holding the spinlock and then using it
> > unserialised just masks the race. (It is supposed to be valid as the
> > unpin_work exists until the flip completion interrupt which should not
> > fire until we flush the mmio writes to update the display base which is
> > the last time we access the unpin_work from the kthread.)
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=92335
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> So not sure what's going on yet?
>
> Patch looks OK anyway so
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Merged to dinq since it's unclear still.
Thanks, Daniel
>
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 57 ++++++++++++++++++++----------------
> > 1 file changed, 32 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index cddb0c692334..71d7298648e0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -10848,11 +10848,11 @@ void intel_prepare_page_flip(struct drm_device *dev, int plane)
> > spin_unlock_irqrestore(&dev->event_lock, flags);
> > }
> >
> > -static inline void intel_mark_page_flip_active(struct intel_crtc *intel_crtc)
> > +static inline void intel_mark_page_flip_active(struct intel_unpin_work *work)
> > {
> > /* Ensure that the work item is consistent when activating it ... */
> > smp_wmb();
> > - atomic_set(&intel_crtc->unpin_work->pending, INTEL_FLIP_PENDING);
> > + atomic_set(&work->pending, INTEL_FLIP_PENDING);
> > /* and that it is marked active as soon as the irq could fire. */
> > smp_wmb();
> > }
> > @@ -10888,7 +10888,7 @@ static int intel_gen2_queue_flip(struct drm_device *dev,
> > 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);
> > + intel_mark_page_flip_active(intel_crtc->unpin_work);
> > return 0;
> > }
> >
> > @@ -10920,7 +10920,7 @@ static int intel_gen3_queue_flip(struct drm_device *dev,
> > intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
> > intel_ring_emit(ring, MI_NOOP);
> >
> > - intel_mark_page_flip_active(intel_crtc);
> > + intel_mark_page_flip_active(intel_crtc->unpin_work);
> > return 0;
> > }
> >
> > @@ -10959,7 +10959,7 @@ static int intel_gen4_queue_flip(struct drm_device *dev,
> > pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
> > intel_ring_emit(ring, pf | pipesrc);
> >
> > - intel_mark_page_flip_active(intel_crtc);
> > + intel_mark_page_flip_active(intel_crtc->unpin_work);
> > return 0;
> > }
> >
> > @@ -10995,7 +10995,7 @@ static int intel_gen6_queue_flip(struct drm_device *dev,
> > pipesrc = I915_READ(PIPESRC(intel_crtc->pipe)) & 0x0fff0fff;
> > intel_ring_emit(ring, pf | pipesrc);
> >
> > - intel_mark_page_flip_active(intel_crtc);
> > + intel_mark_page_flip_active(intel_crtc->unpin_work);
> > return 0;
> > }
> >
> > @@ -11090,7 +11090,7 @@ static int intel_gen7_queue_flip(struct drm_device *dev,
> > intel_ring_emit(ring, intel_crtc->unpin_work->gtt_offset);
> > intel_ring_emit(ring, (MI_NOOP));
> >
> > - intel_mark_page_flip_active(intel_crtc);
> > + intel_mark_page_flip_active(intel_crtc->unpin_work);
> > return 0;
> > }
> >
> > @@ -11121,7 +11121,8 @@ static bool use_mmio_flip(struct intel_engine_cs *ring,
> > return ring != i915_gem_request_get_ring(obj->last_write_req);
> > }
> >
> > -static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> > +static void skl_do_mmio_flip(struct intel_crtc *intel_crtc,
> > + struct intel_unpin_work *work)
> > {
> > struct drm_device *dev = intel_crtc->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -11162,11 +11163,12 @@ static void skl_do_mmio_flip(struct intel_crtc *intel_crtc)
> > I915_WRITE(PLANE_CTL(pipe, 0), ctl);
> > I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> >
> > - I915_WRITE(PLANE_SURF(pipe, 0), intel_crtc->unpin_work->gtt_offset);
> > + I915_WRITE(PLANE_SURF(pipe, 0), work->gtt_offset);
> > POSTING_READ(PLANE_SURF(pipe, 0));
> > }
> >
> > -static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
> > +static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc,
> > + struct intel_unpin_work *work)
> > {
> > struct drm_device *dev = intel_crtc->base.dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -11186,31 +11188,36 @@ static void ilk_do_mmio_flip(struct intel_crtc *intel_crtc)
> >
> > I915_WRITE(reg, dspcntr);
> >
> > - I915_WRITE(DSPSURF(intel_crtc->plane),
> > - intel_crtc->unpin_work->gtt_offset);
> > + I915_WRITE(DSPSURF(intel_crtc->plane), work->gtt_offset);
> > POSTING_READ(DSPSURF(intel_crtc->plane));
> > -
> > }
> >
> > /*
> > * XXX: This is the temporary way to update the plane registers until we get
> > * around to using the usual plane update functions for MMIO flips
> > */
> > -static void intel_do_mmio_flip(struct intel_crtc *intel_crtc)
> > +static void intel_do_mmio_flip(struct intel_mmio_flip *mmio_flip)
> > {
> > - struct drm_device *dev = intel_crtc->base.dev;
> > + struct intel_crtc *crtc = mmio_flip->crtc;
> > + struct intel_unpin_work *work;
> >
> > - intel_mark_page_flip_active(intel_crtc);
> > + spin_lock_irq(&crtc->base.dev->event_lock);
> > + work = crtc->unpin_work;
> > + spin_unlock_irq(&crtc->base.dev->event_lock);
> > + if (work == NULL)
> > + return;
> >
> > - intel_pipe_update_start(intel_crtc);
> > + intel_mark_page_flip_active(work);
> >
> > - if (INTEL_INFO(dev)->gen >= 9)
> > - skl_do_mmio_flip(intel_crtc);
> > + intel_pipe_update_start(crtc);
> > +
> > + if (INTEL_INFO(mmio_flip->i915)->gen >= 9)
> > + skl_do_mmio_flip(crtc, work);
> > else
> > /* use_mmio_flip() retricts MMIO flips to ilk+ */
> > - ilk_do_mmio_flip(intel_crtc);
> > + ilk_do_mmio_flip(crtc, work);
> >
> > - intel_pipe_update_end(intel_crtc);
> > + intel_pipe_update_end(crtc);
> > }
> >
> > static void intel_mmio_flip_work_func(struct work_struct *work)
> > @@ -11218,15 +11225,15 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
> > struct intel_mmio_flip *mmio_flip =
> > container_of(work, struct intel_mmio_flip, work);
> >
> > - if (mmio_flip->req)
> > + if (mmio_flip->req) {
> > WARN_ON(__i915_wait_request(mmio_flip->req,
> > mmio_flip->crtc->reset_counter,
> > false, NULL,
> > &mmio_flip->i915->rps.mmioflips));
> > + i915_gem_request_unreference__unlocked(mmio_flip->req);
> > + }
> >
> > - intel_do_mmio_flip(mmio_flip->crtc);
> > -
> > - i915_gem_request_unreference__unlocked(mmio_flip->req);
> > + intel_do_mmio_flip(mmio_flip);
> > kfree(mmio_flip);
> > }
> >
> > --
> > 2.6.1
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work
2015-10-13 9:23 ` Ville Syrjälä
2015-10-13 11:09 ` Daniel Vetter
@ 2015-10-14 16:02 ` Chris Wilson
1 sibling, 0 replies; 4+ messages in thread
From: Chris Wilson @ 2015-10-14 16:02 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
On Tue, Oct 13, 2015 at 12:23:38PM +0300, Ville Syrjälä wrote:
> On Sat, Oct 10, 2015 at 10:44:32AM +0100, Chris Wilson wrote:
> > We should serialise access to the intel_crtc->unpin_work through the
> > dev->event_lock spinlock. It should not be possible for it to disappear
> > without severe error as the mmio_flip worker has not tagged the
> > unpin_work pending flip-completion. Similarly if the error exists, just
> > taking the unpin_work whilst holding the spinlock and then using it
> > unserialised just masks the race. (It is supposed to be valid as the
> > unpin_work exists until the flip completion interrupt which should not
> > fire until we flush the mmio writes to update the display base which is
> > the last time we access the unpin_work from the kthread.)
> >
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=92335
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> So not sure what's going on yet?
After a couple of nights sleeping on it, not a clue. Either I've missed
something that allows unpin_work to silenty disappear before we mark
work->pending as pending (let alone complete) or the oops is from
another racy pointer dereference. I'm going to guess the latter and see
if there are any candidates (here, I can believe that in the middle of
programming the flip we get an interrupt that causes the unpin work to
disappear).
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-10-14 16:02 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-10 9:44 [PATCH] drm/i915: Hold dev->event_lock whilst inspecting intel_crtc->unpin_work Chris Wilson
2015-10-13 9:23 ` Ville Syrjälä
2015-10-13 11:09 ` Daniel Vetter
2015-10-14 16:02 ` Chris Wilson
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.