| Message ID | 1416854990-1920-13-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Nov 24, 2014 at 06:49:34PM +0000, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > Converted the mmio_flip 'seqno' value to be a request structure as part of the > on going seqno to request changes. This includes reference counting the request > being saved away to ensure it can not be retired and freed while the flip code > is still waiting on it. > > v2: Used the IRQ friendly request dereference call in the notify handler as that > code is called asynchronously without holding any useful mutex locks. > > For: VIZ-4377 > Signed-off-by: John Harrison <John.C.Harrison@Intel.com> > Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- > drivers/gpu/drm/i915/intel_drv.h | 3 +-- > 2 files changed, 11 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 097e8a1..cbf3cb7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9550,18 +9550,19 @@ static void intel_mmio_flip_work_func(struct work_struct *work) > { > struct intel_crtc *intel_crtc = > container_of(work, struct intel_crtc, mmio_flip.work); > - struct intel_engine_cs *ring; > - uint32_t seqno; > - > - seqno = intel_crtc->mmio_flip.seqno; > - ring = intel_crtc->mmio_flip.ring; > + struct intel_mmio_flip *mmio_flip; > > - if (seqno) > - WARN_ON(__i915_wait_seqno(ring, seqno, > + mmio_flip = &intel_crtc->mmio_flip; > + if (mmio_flip->req) > + WARN_ON(__i915_wait_seqno(i915_gem_request_get_ring(mmio_flip->req), > + i915_gem_request_get_seqno(mmio_flip->req), > intel_crtc->reset_counter, > false, NULL, NULL) != 0); > > intel_do_mmio_flip(intel_crtc); > + if (mmio_flip->req) > + i915_gem_request_unreference_irq(mmio_flip->req); Can't we just grab dev->struct_mutex around here and reuse the normal request_unref? That would allow us to ditch all the unref_irq complexity. -Daniel > + mmio_flip->req = NULL; > } > > static int intel_queue_mmio_flip(struct drm_device *dev, > @@ -9573,9 +9574,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > - intel_crtc->mmio_flip.seqno = > - i915_gem_request_get_seqno(obj->last_write_req); > - intel_crtc->mmio_flip.ring = obj->ring; > + i915_gem_request_assign(&intel_crtc->mmio_flip.req, > + obj->last_write_req); > > schedule_work(&intel_crtc->mmio_flip.work); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 44b153c5..2755532 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -406,8 +406,7 @@ struct intel_pipe_wm { > }; > > struct intel_mmio_flip { > - u32 seqno; > - struct intel_engine_cs *ring; > + struct drm_i915_gem_request *req; > struct work_struct work; > }; > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 26/11/2014 09:23, Daniel Vetter wrote: > On Mon, Nov 24, 2014 at 06:49:34PM +0000, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> Converted the mmio_flip 'seqno' value to be a request structure as part of the >> on going seqno to request changes. This includes reference counting the request >> being saved away to ensure it can not be retired and freed while the flip code >> is still waiting on it. >> >> v2: Used the IRQ friendly request dereference call in the notify handler as that >> code is called asynchronously without holding any useful mutex locks. >> >> For: VIZ-4377 >> Signed-off-by: John Harrison <John.C.Harrison@Intel.com> >> Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++---------- >> drivers/gpu/drm/i915/intel_drv.h | 3 +-- >> 2 files changed, 11 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 097e8a1..cbf3cb7 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9550,18 +9550,19 @@ static void intel_mmio_flip_work_func(struct work_struct *work) >> { >> struct intel_crtc *intel_crtc = >> container_of(work, struct intel_crtc, mmio_flip.work); >> - struct intel_engine_cs *ring; >> - uint32_t seqno; >> - >> - seqno = intel_crtc->mmio_flip.seqno; >> - ring = intel_crtc->mmio_flip.ring; >> + struct intel_mmio_flip *mmio_flip; >> >> - if (seqno) >> - WARN_ON(__i915_wait_seqno(ring, seqno, >> + mmio_flip = &intel_crtc->mmio_flip; >> + if (mmio_flip->req) >> + WARN_ON(__i915_wait_seqno(i915_gem_request_get_ring(mmio_flip->req), >> + i915_gem_request_get_seqno(mmio_flip->req), >> intel_crtc->reset_counter, >> false, NULL, NULL) != 0); >> >> intel_do_mmio_flip(intel_crtc); >> + if (mmio_flip->req) >> + i915_gem_request_unreference_irq(mmio_flip->req); > Can't we just grab dev->struct_mutex around here and reuse the normal > request_unref? That would allow us to ditch all the unref_irq complexity. > -Daniel The unref_irq is needed for the flip_queued_req as that is still dereferenced from interrupt time. Possibly this one could now be downgraded to a mutex lock but before the recent rebase, the mmio_flip request was also being dereferenced at interrupt time so definitely needed the irq friendly version. Is there a worry that waiting for the mutex lock could stall the flip handler so long that it misses the next flip? >> + mmio_flip->req = NULL; >> } >> >> static int intel_queue_mmio_flip(struct drm_device *dev, >> @@ -9573,9 +9574,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev, >> { >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> >> - intel_crtc->mmio_flip.seqno = >> - i915_gem_request_get_seqno(obj->last_write_req); >> - intel_crtc->mmio_flip.ring = obj->ring; >> + i915_gem_request_assign(&intel_crtc->mmio_flip.req, >> + obj->last_write_req); >> >> schedule_work(&intel_crtc->mmio_flip.work); >> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 44b153c5..2755532 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -406,8 +406,7 @@ struct intel_pipe_wm { >> }; >> >> struct intel_mmio_flip { >> - u32 seqno; >> - struct intel_engine_cs *ring; >> + struct drm_i915_gem_request *req; >> struct work_struct work; >> }; >> >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Wed, Nov 26, 2014 at 1:12 PM, John Harrison <John.C.Harrison@intel.com> wrote: > The unref_irq is needed for the flip_queued_req as that is still > dereferenced from interrupt time. Possibly this one could now be downgraded > to a mutex lock but before the recent rebase, the mmio_flip request was also > being dereferenced at interrupt time so definitely needed the irq friendly > version. Is there a worry that waiting for the mutex lock could stall the > flip handler so long that it misses the next flip? Yup this is only possible very recently. I've simply dropped the _irq and wrapped with struct_mutex locking, but a lockless unref (which only grabs the lock on the final kput in the free function) like Chris suggested would indeed be tidier. -Daniel
On 26/11/2014 12:49, Daniel Vetter wrote: > On Wed, Nov 26, 2014 at 1:12 PM, John Harrison > <John.C.Harrison@intel.com> wrote: >> The unref_irq is needed for the flip_queued_req as that is still >> dereferenced from interrupt time. Possibly this one could now be downgraded >> to a mutex lock but before the recent rebase, the mmio_flip request was also >> being dereferenced at interrupt time so definitely needed the irq friendly >> version. Is there a worry that waiting for the mutex lock could stall the >> flip handler so long that it misses the next flip? > Yup this is only possible very recently. I've simply dropped the _irq > and wrapped with struct_mutex locking, but a lockless unref (which > only grabs the lock on the final kput in the free function) like Chris > suggested would indeed be tidier. > -Daniel A lockless unref only works if you can guarantee never to call it when you are holding the last reference. And that is something that is extremely difficult to prove in advance. Just because the flip code usually releases its reference before the request has been retired does not mean that it always will. I have definitely seen it occur the other way around in my testing. Having a 'lockless' unref that can acquire the mutex lock internally is not much use when you are needing to call it from code that cannot lock (e.g. an interrupt handler). In that context, the free must be deferred until a later time.
On Wed, Nov 26, 2014 at 03:21:30PM +0000, John Harrison wrote: > On 26/11/2014 12:49, Daniel Vetter wrote: > >On Wed, Nov 26, 2014 at 1:12 PM, John Harrison > ><John.C.Harrison@intel.com> wrote: > >>The unref_irq is needed for the flip_queued_req as that is still > >>dereferenced from interrupt time. Possibly this one could now be downgraded > >>to a mutex lock but before the recent rebase, the mmio_flip request was also > >>being dereferenced at interrupt time so definitely needed the irq friendly > >>version. Is there a worry that waiting for the mutex lock could stall the > >>flip handler so long that it misses the next flip? > >Yup this is only possible very recently. I've simply dropped the _irq > >and wrapped with struct_mutex locking, but a lockless unref (which > >only grabs the lock on the final kput in the free function) like Chris > >suggested would indeed be tidier. > >-Daniel > > A lockless unref only works if you can guarantee never to call it when you > are holding the last reference. And that is something that is extremely > difficult to prove in advance. Just because the flip code usually releases > its reference before the request has been retired does not mean that it > always will. I have definitely seen it occur the other way around in my > testing. > > Having a 'lockless' unref that can acquire the mutex lock internally is not > much use when you are needing to call it from code that cannot lock (e.g. an > interrupt handler). In that context, the free must be deferred until a later > time. The idea is to implement something like drm_gem_object_unreference_unlocked. It would be mostly for code tideness and self-documentation really. That it avoids a needless mutex grabbing cycle is just a free bonus. And yeah still forbidden to call this from irq contexts or anything else nasty. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 097e8a1..cbf3cb7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9550,18 +9550,19 @@ static void intel_mmio_flip_work_func(struct work_struct *work) { struct intel_crtc *intel_crtc = container_of(work, struct intel_crtc, mmio_flip.work); - struct intel_engine_cs *ring; - uint32_t seqno; - - seqno = intel_crtc->mmio_flip.seqno; - ring = intel_crtc->mmio_flip.ring; + struct intel_mmio_flip *mmio_flip; - if (seqno) - WARN_ON(__i915_wait_seqno(ring, seqno, + mmio_flip = &intel_crtc->mmio_flip; + if (mmio_flip->req) + WARN_ON(__i915_wait_seqno(i915_gem_request_get_ring(mmio_flip->req), + i915_gem_request_get_seqno(mmio_flip->req), intel_crtc->reset_counter, false, NULL, NULL) != 0); intel_do_mmio_flip(intel_crtc); + if (mmio_flip->req) + i915_gem_request_unreference_irq(mmio_flip->req); + mmio_flip->req = NULL; } static int intel_queue_mmio_flip(struct drm_device *dev, @@ -9573,9 +9574,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev, { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - intel_crtc->mmio_flip.seqno = - i915_gem_request_get_seqno(obj->last_write_req); - intel_crtc->mmio_flip.ring = obj->ring; + i915_gem_request_assign(&intel_crtc->mmio_flip.req, + obj->last_write_req); schedule_work(&intel_crtc->mmio_flip.work); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 44b153c5..2755532 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -406,8 +406,7 @@ struct intel_pipe_wm { }; struct intel_mmio_flip { - u32 seqno; - struct intel_engine_cs *ring; + struct drm_i915_gem_request *req; struct work_struct work; };