Re: [PATCH 17/32] drm/i915: Remove the lazy_coherency parameter from request-completed?

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

Hi,

On 11/12/15 11:33, Chris Wilson wrote:
> Now that we have split out the seqno-barrier from the
> engine->get_seqno() callback itself, we can move the users of the
> seqno-barrier to the required callsites simplifying the common code and
> making the required workaround handling much more explicit.

What bothers me about this patch, and the one preceding it, is that I 
don't see a tangible improvement for the programmer who still has to 
know when to read the seqno and when to "read it harder, read for real".

Barrier in this sense has a relation to the state of things but somehow 
feels too low level to me when used from the code. But to be fair I am 
not sure how to better define it.

Would ring->get_seqno paired with ring->read_seqno perhaps make sense? 
Implementation for ring->read_seqno would just be a flush followed by 
ring->get_seqno then. Or maybe keep the barrier and add ring->read_seqno 
which would be ring->seqno_barrier + ring_get_seqno?

Regards,

Tvrtko

> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>   drivers/gpu/drm/i915/i915_debugfs.c  |  4 ++--
>   drivers/gpu/drm/i915/i915_drv.h      | 10 ++--------
>   drivers/gpu/drm/i915/i915_gem.c      | 24 +++++++++++++++---------
>   drivers/gpu/drm/i915/intel_display.c |  2 +-
>   drivers/gpu/drm/i915/intel_pm.c      |  4 ++--
>   5 files changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 6344fe69ab82..8860dec36aae 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -601,7 +601,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,
>   					   ring->get_seqno(ring),
> -					   i915_gem_request_completed(work->flip_queued_req, true));
> +					   i915_gem_request_completed(work->flip_queued_req));
>   			} 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",
> @@ -1353,8 +1353,8 @@ static int i915_hangcheck_info(struct seq_file *m, void *unused)
>   	intel_runtime_pm_get(dev_priv);
>
>   	for_each_ring(ring, dev_priv, i) {
> -		seqno[i] = ring->get_seqno(ring);
>   		acthd[i] = intel_ring_get_active_head(ring);
> +		seqno[i] = ring->get_seqno(ring);
>   	}
>
>   	intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ff83f148658f..d099e960f9b8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2978,20 +2978,14 @@ i915_seqno_passed(uint32_t seq1, uint32_t seq2)
>   	return (int32_t)(seq1 - seq2) >= 0;
>   }
>
> -static inline bool i915_gem_request_started(struct drm_i915_gem_request *req,
> -					   bool lazy_coherency)
> +static inline bool i915_gem_request_started(struct drm_i915_gem_request *req)
>   {
> -	if (!lazy_coherency && req->ring->seqno_barrier)
> -		req->ring->seqno_barrier(req->ring);
>   	return i915_seqno_passed(req->ring->get_seqno(req->ring),
>   				 req->previous_seqno);
>   }
>
> -static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req,
> -					      bool lazy_coherency)
> +static inline bool i915_gem_request_completed(struct drm_i915_gem_request *req)
>   {
> -	if (!lazy_coherency && req->ring->seqno_barrier)
> -		req->ring->seqno_barrier(req->ring);
>   	return i915_seqno_passed(req->ring->get_seqno(req->ring),
>   				 req->seqno);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fa0cf6c9f4d0..f3c1e268f614 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1173,12 +1173,12 @@ static bool __i915_spin_request(struct drm_i915_gem_request *req,
>   	 */
>
>   	/* Only spin if we know the GPU is processing this request */
> -	if (!i915_gem_request_started(req, true))
> +	if (!i915_gem_request_started(req))
>   		return false;
>
>   	timeout = local_clock_us(&cpu) + 5;
>   	do {
> -		if (i915_gem_request_completed(req, true))
> +		if (i915_gem_request_completed(req))
>   			return true;
>
>   		if (signal_pending_state(state, wait->task))
> @@ -1230,7 +1230,7 @@ int __i915_wait_request(struct drm_i915_gem_request *req,
>   	if (list_empty(&req->list))
>   		return 0;
>
> -	if (i915_gem_request_completed(req, true))
> +	if (i915_gem_request_completed(req))
>   		return 0;
>
>   	timeout_remain = MAX_SCHEDULE_TIMEOUT;
> @@ -1299,7 +1299,10 @@ wakeup:		set_task_state(wait.task, state);
>   		 * but it is easier and safer to do it every time the waiter
>   		 * is woken.
>   		 */
> -		if (i915_gem_request_completed(req, false))
> +		if (req->ring->seqno_barrier)
> +			req->ring->seqno_barrier(req->ring);
> +
> +		if (i915_gem_request_completed(req))
>   			break;
>
>   		/* We need to check whether any gpu reset happened in between
> @@ -2731,8 +2734,11 @@ i915_gem_find_active_request(struct intel_engine_cs *ring)
>   {
>   	struct drm_i915_gem_request *request;
>
> +	if (ring->seqno_barrier)
> +		ring->seqno_barrier(ring);
> +
>   	list_for_each_entry(request, &ring->request_list, list) {
> -		if (i915_gem_request_completed(request, false))
> +		if (i915_gem_request_completed(request))
>   			continue;
>
>   		return request;
> @@ -2873,7 +2879,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>   					   struct drm_i915_gem_request,
>   					   list);
>
> -		if (!i915_gem_request_completed(request, true))
> +		if (!i915_gem_request_completed(request))
>   			break;
>
>   		i915_gem_request_retire(request);
> @@ -2897,7 +2903,7 @@ i915_gem_retire_requests_ring(struct intel_engine_cs *ring)
>   	}
>
>   	if (unlikely(ring->trace_irq_req &&
> -		     i915_gem_request_completed(ring->trace_irq_req, true))) {
> +		     i915_gem_request_completed(ring->trace_irq_req))) {
>   		ring->irq_put(ring);
>   		i915_gem_request_assign(&ring->trace_irq_req, NULL);
>   	}
> @@ -3007,7 +3013,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
>   		if (list_empty(&req->list))
>   			goto retire;
>
> -		if (i915_gem_request_completed(req, true)) {
> +		if (i915_gem_request_completed(req)) {
>   			__i915_gem_request_retire__upto(req);
>   retire:
>   			i915_gem_object_retire__read(obj, i);
> @@ -3116,7 +3122,7 @@ __i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   	if (to == from)
>   		return 0;
>
> -	if (i915_gem_request_completed(from_req, true))
> +	if (i915_gem_request_completed(from_req))
>   		return 0;
>
>   	if (!i915_semaphore_is_enabled(obj->base.dev)) {
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 875bdf814d73..ffcdc2c631e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11459,7 +11459,7 @@ static bool __intel_pageflip_stall_check(struct drm_device *dev,
>
>   	if (work->flip_ready_vblank == 0) {
>   		if (work->flip_queued_req &&
> -		    !i915_gem_request_completed(work->flip_queued_req, true))
> +		    !i915_gem_request_completed(work->flip_queued_req))
>   			return false;
>
>   		work->flip_ready_vblank = drm_crtc_vblank_count(crtc);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 99f2642fd5df..570628628a90 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7188,7 +7188,7 @@ static void __intel_rps_boost_work(struct work_struct *work)
>   	struct request_boost *boost = container_of(work, struct request_boost, work);
>   	struct drm_i915_gem_request *req = boost->req;
>
> -	if (!i915_gem_request_completed(req, true))
> +	if (!i915_gem_request_completed(req))
>   		gen6_rps_boost(to_i915(req->ring->dev), NULL,
>   			       req->emitted_jiffies);
>
> @@ -7204,7 +7204,7 @@ void intel_queue_rps_boost_for_request(struct drm_device *dev,
>   	if (req == NULL || INTEL_INFO(dev)->gen < 6)
>   		return;
>
> -	if (i915_gem_request_completed(req, true))
> +	if (i915_gem_request_completed(req))
>   		return;
>
>   	boost = kmalloc(sizeof(*boost), GFP_ATOMIC);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx