Re: [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'

[John Harrison <John.C.Harrison@xxxxxxxxx>]

On 19/10/2014 15:04, Daniel Vetter wrote:
> On Fri, Oct 10, 2014 at 12:39:49PM +0100, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > For: VIZ-4377
> > Signed-off-by: John.C.Harrison@xxxxxxxxx
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxxx
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx