[Intel-gfx] [RFC 09/21] drm/i915: Make 'i915_gem_check_olr' actually check by request not seqno
John Harrison
John.C.Harrison at Intel.com
Tue Oct 28 15:01:52 CET 2014
On 19/10/2014 13:55, Daniel Vetter wrote:
> On Mon, Oct 06, 2014 at 03:15:13PM +0100, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
> 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 at Intel.com
>> ---
>> 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list