| Message ID | 1412941189-5254-1-git-send-email-John.C.Harrison@Intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Oct 10, 2014 at 12:39:49PM +0100, John.C.Harrison@Intel.com wrote: > From: John Harrison <John.C.Harrison@Intel.com> > > For: VIZ-4377 > Signed-off-by: John.C.Harrison@Intel.com > --- > drivers/gpu/drm/i915/i915_debugfs.c | 3 +-- > drivers/gpu/drm/i915/i915_drv.h | 18 ++++++++++++++++++ > drivers/gpu/drm/i915/i915_gem.c | 12 ++++-------- > drivers/gpu/drm/i915/intel_display.c | 11 +++-------- > 4 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index 52ddd19..e764af9 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -550,8 +550,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) > i915_gem_request_get_seqno(work->flip_queued_req), > dev_priv->next_seqno, > work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), > - i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), > - i915_gem_request_get_seqno(work->flip_queued_req))); > + i915_gem_request_completed(work->flip_queued_req, true)); > } else > seq_printf(m, "Flip not associated with any ring\n"); > seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 6fe3be0..650f712 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1970,6 +1970,12 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) > kref_put(&req->ref, i915_gem_request_free); > } > > +/* > + * NB: i915_gem_request_completed should be here but currently needs the > + * definition of i915_seqno_passed() which is below. It will be moved in > + * a later patch when the call to i915_seqno_passed() is obsoleted... > + */ > + > struct drm_i915_file_private { > struct drm_i915_private *dev_priv; > struct drm_file *file; > @@ -3019,4 +3025,16 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) > } > } > > +static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req, > + bool lazy_coherency) > +{ > + u32 seqno; > + > + BUG_ON(req == NULL); > + > + seqno = req->ring->get_seqno(req->ring, lazy_coherency); > + > + return i915_seqno_passed(seqno, req->seqno); > +} Tbh this thing looks a bit big for a static inline. Have you double-checked that inlining this doesn't increase binary size? If it does then we need performance data justifying the cost. Same probably applies to some of the other static inline functions you've added. -Daniel > + > #endif > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index e1a58f1..b602e8c 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -2253,8 +2253,7 @@ i915_gem_object_retire(struct drm_i915_gem_object *obj) > if (ring == NULL) > return; > > - if (i915_seqno_passed(ring->get_seqno(ring, true), > - i915_gem_request_get_seqno(obj->last_read_req))) > + if (i915_gem_request_completed(obj->last_read_req, true)) > i915_gem_object_move_to_inactive(obj); > } > > @@ -2521,12 +2520,9 @@ struct drm_i915_gem_request * > i915_gem_find_active_request(struct intel_engine_cs *ring) > { > struct drm_i915_gem_request *request; > - u32 completed_seqno; > - > - completed_seqno = ring->get_seqno(ring, false); > > list_for_each_entry(request, &ring->request_list, list) { > - if (i915_seqno_passed(completed_seqno, request->seqno)) > + if (i915_gem_request_completed(request, false)) > continue; > > return request; > @@ -2670,7 +2666,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) > struct drm_i915_gem_object, > ring_list); > > - if (!i915_seqno_passed(seqno, i915_gem_request_get_seqno(obj->last_read_req))) > + if (!i915_gem_request_completed(obj->last_read_req, true)) > break; > > i915_gem_object_move_to_inactive(obj); > @@ -2685,7 +2681,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) > struct drm_i915_gem_request, > list); > > - if (!i915_seqno_passed(seqno, request->seqno)) > + if (!i915_gem_request_completed(request, true)) > break; > > trace_i915_gem_request_retire(ring, request->seqno); > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 26fdd96..9ca8f94 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj) > > ring = obj->ring; > > - if (i915_seqno_passed(ring->get_seqno(ring, true), > - i915_gem_request_get_seqno(obj->last_write_req))) > + if (i915_gem_request_completed(obj->last_write_req, true)) > return 0; > > ret = i915_gem_check_olr(obj->last_write_req); > @@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) > struct drm_i915_private *dev_priv = to_i915(ring->dev); > struct intel_crtc *intel_crtc; > unsigned long irq_flags; > - u32 seqno; > - > - seqno = ring->get_seqno(ring, false); > > spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags); > for_each_intel_crtc(ring->dev, intel_crtc) { > @@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) > if (ring->id != mmio_flip->ring_id) > continue; > > - if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) { > + if (i915_gem_request_completed(mmio_flip->req, false)) { > intel_do_mmio_flip(intel_crtc); > i915_gem_request_unreference(mmio_flip->req); > mmio_flip->req = NULL; > @@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev, > > if (work->flip_ready_vblank == 0) { > if (work->flip_queued_ring && > - !i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), > - i915_gem_request_get_seqno(work->flip_queued_req))) > + !i915_gem_request_completed(work->flip_queued_req, true)) > return false; > > work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe); > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On 19/10/2014 15:04, Daniel Vetter wrote: > On Fri, Oct 10, 2014 at 12:39:49PM +0100, John.C.Harrison@Intel.com wrote: >> From: John Harrison <John.C.Harrison@Intel.com> >> >> For: VIZ-4377 >> Signed-off-by: John.C.Harrison@Intel.com >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 3 +-- >> drivers/gpu/drm/i915/i915_drv.h | 18 ++++++++++++++++++ >> drivers/gpu/drm/i915/i915_gem.c | 12 ++++-------- >> drivers/gpu/drm/i915/intel_display.c | 11 +++-------- >> 4 files changed, 26 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index 52ddd19..e764af9 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -550,8 +550,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) >> i915_gem_request_get_seqno(work->flip_queued_req), >> dev_priv->next_seqno, >> work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), >> - i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), >> - i915_gem_request_get_seqno(work->flip_queued_req))); >> + i915_gem_request_completed(work->flip_queued_req, true)); >> } else >> seq_printf(m, "Flip not associated with any ring\n"); >> seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index 6fe3be0..650f712 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1970,6 +1970,12 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) >> kref_put(&req->ref, i915_gem_request_free); >> } >> >> +/* >> + * NB: i915_gem_request_completed should be here but currently needs the >> + * definition of i915_seqno_passed() which is below. It will be moved in >> + * a later patch when the call to i915_seqno_passed() is obsoleted... >> + */ >> + >> struct drm_i915_file_private { >> struct drm_i915_private *dev_priv; >> struct drm_file *file; >> @@ -3019,4 +3025,16 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) >> } >> } >> >> +static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req, >> + bool lazy_coherency) >> +{ >> + u32 seqno; >> + >> + BUG_ON(req == NULL); >> + >> + seqno = req->ring->get_seqno(req->ring, lazy_coherency); >> + >> + return i915_seqno_passed(seqno, req->seqno); >> +} > Tbh this thing looks a bit big for a static inline. Have you > double-checked that inlining this doesn't increase binary size? If it > does then we need performance data justifying the cost. Same probably > applies to some of the other static inline functions you've added. > -Daniel It is a replacement for i915_seqno_passed() which is a static inline so it seemed sensible that this should be too. Note that the ultimate aim is for this to be a simple 'return req->completed'. At which point it should certainly be worthy of inlining. But no I haven't done any actual analysis of the benefit/cost yet. As has been mentioned, this RFC was a quick knock up to get something to discuss as soon as possible. >> + >> #endif >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c >> index e1a58f1..b602e8c 100644 >> --- a/drivers/gpu/drm/i915/i915_gem.c >> +++ b/drivers/gpu/drm/i915/i915_gem.c >> @@ -2253,8 +2253,7 @@ i915_gem_object_retire(struct drm_i915_gem_object *obj) >> if (ring == NULL) >> return; >> >> - if (i915_seqno_passed(ring->get_seqno(ring, true), >> - i915_gem_request_get_seqno(obj->last_read_req))) >> + if (i915_gem_request_completed(obj->last_read_req, true)) >> i915_gem_object_move_to_inactive(obj); >> } >> >> @@ -2521,12 +2520,9 @@ struct drm_i915_gem_request * >> i915_gem_find_active_request(struct intel_engine_cs *ring) >> { >> struct drm_i915_gem_request *request; >> - u32 completed_seqno; >> - >> - completed_seqno = ring->get_seqno(ring, false); >> >> list_for_each_entry(request, &ring->request_list, list) { >> - if (i915_seqno_passed(completed_seqno, request->seqno)) >> + if (i915_gem_request_completed(request, false)) >> continue; >> >> return request; >> @@ -2670,7 +2666,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) >> struct drm_i915_gem_object, >> ring_list); >> >> - if (!i915_seqno_passed(seqno, i915_gem_request_get_seqno(obj->last_read_req))) >> + if (!i915_gem_request_completed(obj->last_read_req, true)) >> break; >> >> i915_gem_object_move_to_inactive(obj); >> @@ -2685,7 +2681,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) >> struct drm_i915_gem_request, >> list); >> >> - if (!i915_seqno_passed(seqno, request->seqno)) >> + if (!i915_gem_request_completed(request, true)) >> break; >> >> trace_i915_gem_request_retire(ring, request->seqno); >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 26fdd96..9ca8f94 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj) >> >> ring = obj->ring; >> >> - if (i915_seqno_passed(ring->get_seqno(ring, true), >> - i915_gem_request_get_seqno(obj->last_write_req))) >> + if (i915_gem_request_completed(obj->last_write_req, true)) >> return 0; >> >> ret = i915_gem_check_olr(obj->last_write_req); >> @@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) >> struct drm_i915_private *dev_priv = to_i915(ring->dev); >> struct intel_crtc *intel_crtc; >> unsigned long irq_flags; >> - u32 seqno; >> - >> - seqno = ring->get_seqno(ring, false); >> >> spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags); >> for_each_intel_crtc(ring->dev, intel_crtc) { >> @@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) >> if (ring->id != mmio_flip->ring_id) >> continue; >> >> - if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) { >> + if (i915_gem_request_completed(mmio_flip->req, false)) { >> intel_do_mmio_flip(intel_crtc); >> i915_gem_request_unreference(mmio_flip->req); >> mmio_flip->req = NULL; >> @@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev, >> >> if (work->flip_ready_vblank == 0) { >> if (work->flip_queued_ring && >> - !i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), >> - i915_gem_request_get_seqno(work->flip_queued_req))) >> + !i915_gem_request_completed(work->flip_queued_req, true)) >> return false; >> >> work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe); >> -- >> 1.7.9.5 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 52ddd19..e764af9 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -550,8 +550,7 @@ static int i915_gem_pageflip_info(struct seq_file *m, void *data) i915_gem_request_get_seqno(work->flip_queued_req), dev_priv->next_seqno, work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), - i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), - i915_gem_request_get_seqno(work->flip_queued_req))); + i915_gem_request_completed(work->flip_queued_req, true)); } else seq_printf(m, "Flip not associated with any ring\n"); seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 6fe3be0..650f712 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1970,6 +1970,12 @@ i915_gem_request_unreference(struct drm_i915_gem_request *req) kref_put(&req->ref, i915_gem_request_free); } +/* + * NB: i915_gem_request_completed should be here but currently needs the + * definition of i915_seqno_passed() which is below. It will be moved in + * a later patch when the call to i915_seqno_passed() is obsoleted... + */ + struct drm_i915_file_private { struct drm_i915_private *dev_priv; struct drm_file *file; @@ -3019,4 +3025,16 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms) } } +static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req, + bool lazy_coherency) +{ + u32 seqno; + + BUG_ON(req == NULL); + + seqno = req->ring->get_seqno(req->ring, lazy_coherency); + + return i915_seqno_passed(seqno, req->seqno); +} + #endif diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e1a58f1..b602e8c 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2253,8 +2253,7 @@ i915_gem_object_retire(struct drm_i915_gem_object *obj) if (ring == NULL) return; - if (i915_seqno_passed(ring->get_seqno(ring, true), - i915_gem_request_get_seqno(obj->last_read_req))) + if (i915_gem_request_completed(obj->last_read_req, true)) i915_gem_object_move_to_inactive(obj); } @@ -2521,12 +2520,9 @@ struct drm_i915_gem_request * i915_gem_find_active_request(struct intel_engine_cs *ring) { struct drm_i915_gem_request *request; - u32 completed_seqno; - - completed_seqno = ring->get_seqno(ring, false); list_for_each_entry(request, &ring->request_list, list) { - if (i915_seqno_passed(completed_seqno, request->seqno)) + if (i915_gem_request_completed(request, false)) continue; return request; @@ -2670,7 +2666,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) struct drm_i915_gem_object, ring_list); - if (!i915_seqno_passed(seqno, i915_gem_request_get_seqno(obj->last_read_req))) + if (!i915_gem_request_completed(obj->last_read_req, true)) break; i915_gem_object_move_to_inactive(obj); @@ -2685,7 +2681,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring) struct drm_i915_gem_request, list); - if (!i915_seqno_passed(seqno, request->seqno)) + if (!i915_gem_request_completed(request, true)) break; trace_i915_gem_request_retire(ring, request->seqno); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 26fdd96..9ca8f94 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9771,8 +9771,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj) ring = obj->ring; - if (i915_seqno_passed(ring->get_seqno(ring, true), - i915_gem_request_get_seqno(obj->last_write_req))) + if (i915_gem_request_completed(obj->last_write_req, true)) return 0; ret = i915_gem_check_olr(obj->last_write_req); @@ -9790,9 +9789,6 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) struct drm_i915_private *dev_priv = to_i915(ring->dev); struct intel_crtc *intel_crtc; unsigned long irq_flags; - u32 seqno; - - seqno = ring->get_seqno(ring, false); spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags); for_each_intel_crtc(ring->dev, intel_crtc) { @@ -9805,7 +9801,7 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) if (ring->id != mmio_flip->ring_id) continue; - if (i915_seqno_passed(seqno, i915_gem_request_get_seqno(mmio_flip->req))) { + if (i915_gem_request_completed(mmio_flip->req, false)) { intel_do_mmio_flip(intel_crtc); i915_gem_request_unreference(mmio_flip->req); mmio_flip->req = NULL; @@ -9878,8 +9874,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev, if (work->flip_ready_vblank == 0) { if (work->flip_queued_ring && - !i915_seqno_passed(work->flip_queued_ring->get_seqno(work->flip_queued_ring, true), - i915_gem_request_get_seqno(work->flip_queued_req))) + !i915_gem_request_completed(work->flip_queued_req, true)) return false; work->flip_ready_vblank = drm_vblank_count(dev, intel_crtc->pipe);