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

From: John Harrison <John.C.Harrison@Intel.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Intel-GFX@Lists.FreeDesktop.Org
Subject: Re: [RFC 16/25] drm/i915: Convert most 'i915_seqno_passed' calls into 'i915_gem_request_completed'
Date: Tue, 28 Oct 2014 14:02:16 +0000	[thread overview]
Message-ID: <544FA1E8.7040901@Intel.com> (raw)
In-Reply-To: <20141019140419.GB26941@phenom.ffwll.local>

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

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