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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> writes:

> 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.
>
> Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  4 ++--
>  drivers/gpu/drm/i915/i915_drv.h      | 17 ++++++++---------
>  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, 29 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 1499e2337e5d..d09e48455dcb 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",
> @@ -1354,8 +1354,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);

Oh! Perhaps I am overly optimistic but did you just show how to solve
the 'hangcheck elapsed <random> ring idle..' coherency issue
in hangcheck?

I would like to have a separate patch that does this ordering
in here and also in i915_hangcheck_elapsed to be in the safe
side, regardless.

Or at minimum, copy the acthd read, seqno read ordering
into the i915_hangcheck_elapsed also.

-Mika


>  	}
>  
>  	i915_get_extra_instdone(dev, instdone);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9762aa76bb0a..44d46018ee13 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2969,20 +2969,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->irq_seqno_barrier)
> -		req->ring->irq_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->irq_seqno_barrier)
> -		req->ring->irq_seqno_barrier(req->ring);
>  	return i915_seqno_passed(req->ring->get_seqno(req->ring),
>  				 req->seqno);
>  }
> @@ -3636,6 +3630,8 @@ static inline void i915_trace_irq_get(struct intel_engine_cs *ring,
>  
>  static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
>  {
> +	struct intel_engine_cs *engine = req->ring;
> +
>  	/* Ensure our read of the seqno is coherent so that we
>  	 * do not "miss an interrupt" (i.e. if this is the last
>  	 * request and the seqno write from the GPU is not visible
> @@ -3647,7 +3643,10 @@ static inline bool __i915_request_irq_complete(struct drm_i915_gem_request *req)
>  	 * but it is easier and safer to do it every time the waiter
>  	 * is woken.
>  	 */
> -	if (i915_gem_request_completed(req, false))
> +	if (engine->irq_seqno_barrier)
> +		engine->irq_seqno_barrier(engine);
> +
> +	if (i915_gem_request_completed(req))
>  		return true;
>  
>  	/* We need to check whether any gpu reset happened in between
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4b26529f1f44..d125820c6309 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1171,12 +1171,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))
> @@ -1228,7 +1228,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;
> @@ -2724,8 +2724,16 @@ i915_gem_find_active_request(struct intel_engine_cs *ring)
>  {
>  	struct drm_i915_gem_request *request;
>  
> +	/* We are called by the error capture and reset at a random
> +	 * point in time. In particular, note that neither is crucially
> +	 * ordered with an interrupt. After a hang, the GPU is dead and we
> +	 * assume that no more writes can happen (we waited long enough for
> +	 * all writes that were in transaction to be flushed) - adding an
> +	 * extra delay for a recent interrupt is pointless. Hence, we do
> +	 * not need an engine->irq_seqno_barrier() before the seqno reads.
> +	 */
>  	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;
> @@ -2859,7 +2867,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);
> @@ -2883,7 +2891,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);
>  	}
> @@ -2995,7 +3003,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);
> @@ -3104,7 +3112,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 7e36f85d3109..de4d4a0d923a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11523,7 +11523,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 9df9e9a22f3c..401c3770057d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7286,7 +7286,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);
>  
> @@ -7302,7 +7302,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);
> -- 
> 2.7.0.rc3
>
> _______________________________________________
> 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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux