Re: [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno

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

On 19/10/2014 13:55, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> To thin commit message.
>
> Also I wonder whethere we should track the olr state more explicitly in
> the request structure instead of jumping through all these hoops. And
> explicit olr state for a request might also help to clarify control flow
> and checks in a bunch of places.

What would be nicer would be to get rid of the OLR completely. If code 
wants to write to the ring then it should request a request structure at 
the start, pass that request in to all the ring writing code and submit 
the same request structure at the end. The whole idea of some random 
request floating around in the ether scooping up any ring accesses that 
happen to occur with no idea about ownership, sequence, etc. just seems 
highly dodgy. Indeed it definitely causes issues with the scheduler with 
things like page flips being mixed up in completely unrelated batch 
buffer submissions! I believe that is one of the things Chris did in his 
re-write the universe patch.


> > For: VIZ-4377
> > Signed-off-by: John.C.Harrison@xxxxxxxxx
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h      |   17 ++++++++++++++++-
> >   drivers/gpu/drm/i915/i915_gem.c      |   25 ++++++++++---------------
> >   drivers/gpu/drm/i915/intel_display.c |    2 +-
> >   3 files changed, 27 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 1401266..9504206 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2483,7 +2483,7 @@ bool i915_gem_retire_requests(struct drm_device *dev);
> >   void i915_gem_retire_requests_ring(struct intel_engine_cs *ring);
> >   int __must_check i915_gem_check_wedge(struct i915_gpu_error *error,
> >   				      bool interruptible);
> > -int __must_check i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno);
> > +int __must_check i915_gem_check_olr(struct drm_i915_gem_request *req);
> >   
> >   static inline bool i915_reset_in_progress(struct i915_gpu_error *error)
> >   {
> > @@ -3020,4 +3020,19 @@ wait_remaining_ms_from_jiffies(unsigned long timestamp_jiffies, int to_wait_ms)
> >   	}
> >   }
> >   
> > +/************************* Deprecated *************************/
> There's a gcc flag for functions if you want to mark this as depracated,
> otherwise I'd just go with a "XXX: This temporary function will disappear
> at the end of the patch series" somewhere.
>
> > +static inline int __must_check i915_gem_check_ols(struct intel_engine_cs *ring, u32 seqno)
> > +{
> > +	int ret;
> > +
> > +	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> WARN_ON instead of BUG_ON for locking checks.
>
> > +
> > +	ret = 0;
> > +	if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
> > +		ret = i915_add_request(ring, NULL);
> > +
> > +	return ret;
> > +}
> > +/************************* Deprecated *************************/
> > +
> >   #endif
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 0d0eb26..2c7deca 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -1097,19 +1097,18 @@ i915_gem_check_wedge(struct i915_gpu_error *error,
> >   }
> >   
> >   /*
> > - * Compare seqno against outstanding lazy request. Emit a request if they are
> > - * equal.
> > + * Compare arbitrary request against outstanding lazy request. Emit on match.
> >    */
> >   int
> > -i915_gem_check_olr(struct intel_engine_cs *ring, u32 seqno)
> > +i915_gem_check_olr(struct drm_i915_gem_request *req)
> >   {
> >   	int ret;
> >   
> > -	BUG_ON(!mutex_is_locked(&ring->dev->struct_mutex));
> > +	BUG_ON(!mutex_is_locked(&req->ring->dev->struct_mutex));
> Same here. Plus please a stern warning in the commit message and cc: to
> whomever who managed to sneak this in (and failed to catch it in review).
> >   
> >   	ret = 0;
> > -	if (seqno == i915_gem_request_get_seqno(ring->outstanding_lazy_request))
> > -		ret = i915_add_request(ring, NULL);
> > +	if (req == req->ring->outstanding_lazy_request)
> > +		ret = i915_add_request(req->ring, NULL);
> >   
> >   	return ret;
> >   }
> > @@ -1271,7 +1270,7 @@ i915_wait_seqno(struct intel_engine_cs *ring, uint32_t seqno)
> >   	if (ret)
> >   		return ret;
> >   
> > -	ret = i915_gem_check_olr(ring, seqno);
> > +	ret = i915_gem_check_ols(ring, seqno);
> >   	if (ret)
> >   		return ret;
> >   
> > @@ -1338,7 +1337,6 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
> >   	struct drm_i915_private *dev_priv = dev->dev_private;
> >   	struct intel_engine_cs *ring = obj->ring;
> >   	unsigned reset_counter;
> > -	u32 seqno;
> >   	int ret;
> >   
> >   	BUG_ON(!mutex_is_locked(&dev->struct_mutex));
> > @@ -1348,21 +1346,18 @@ i915_gem_object_wait_rendering__nonblocking(struct drm_i915_gem_object *obj,
> >   	if (!req)
> >   		return 0;
> >   
> > -	seqno = i915_gem_request_get_seqno(req);
> > -	BUG_ON(seqno == 0);
> > -
> >   	ret = i915_gem_check_wedge(&dev_priv->gpu_error, true);
> >   	if (ret)
> >   		return ret;
> >   
> > -	ret = i915_gem_check_olr(ring, seqno);
> > +	ret = i915_gem_check_olr(req);
> >   	if (ret)
> >   		return ret;
> >   
> >   	reset_counter = atomic_read(&dev_priv->gpu_error.reset_counter);
> >   	i915_gem_request_reference(req);
> >   	mutex_unlock(&dev->struct_mutex);
> > -	ret = __wait_seqno(ring, seqno, reset_counter, true, NULL, file_priv);
> > +	ret = __wait_seqno(ring, i915_gem_request_get_seqno(req), reset_counter, true, NULL, file_priv);
> >   	mutex_lock(&dev->struct_mutex);
> >   	i915_gem_request_unreference(req);
> >   	if (ret)
> > @@ -2786,7 +2781,7 @@ i915_gem_object_flush_active(struct drm_i915_gem_object *obj)
> >   	int ret;
> >   
> >   	if (obj->active) {
> > -		ret = i915_gem_check_olr(obj->ring, i915_gem_request_get_seqno(obj->last_read_req));
> > +		ret = i915_gem_check_olr(obj->last_read_req);
> >   		if (ret)
> >   			return ret;
> >   
> > @@ -2913,7 +2908,7 @@ i915_gem_object_sync(struct drm_i915_gem_object *obj,
> >   	if (seqno <= from->semaphore.sync_seqno[idx]) /* <--- broken?! needs to use i915_seqno_passed()??? */
> >   		return 0;
> >   
> > -	ret = i915_gem_check_olr(obj->ring, seqno);
> > +	ret = i915_gem_check_olr(obj->last_read_req);
> >   	if (ret)
> >   		return ret;
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 6da18c5..2af421e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9775,7 +9775,7 @@ static int intel_postpone_flip(struct drm_i915_gem_object *obj)
> >   			      i915_gem_request_get_seqno(obj->last_write_req)))
> >   		return 0;
> >   
> > -	ret = i915_gem_check_olr(ring, i915_gem_request_get_seqno(obj->last_write_req));
> > +	ret = i915_gem_check_olr(obj->last_write_req);
> >   	if (ret)
> >   		return ret;
> >   
> > --
> > 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