| Message ID | 20200703122221.591656-19-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Use ww locking in execbuf submission. | expand |
On 03/07/2020 13:22, Maarten Lankhorst wrote: > Some i915 selftests still use i915_vma_lock() as inner lock, and > intel_context_create_request() intel_timeline->mutex as outer lock. > Fortunately for selftests this is not an issue, they should be fixed > but we can move ahead and cleanify lockdep now. Mentions and existence of "dirty hacks" will hopefully be removed from the series before it can be considered merge ready? Regards, Tvrtko > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c > index 64948386630f..fe9fff5a63b1 100644 > --- a/drivers/gpu/drm/i915/gt/intel_context.c > +++ b/drivers/gpu/drm/i915/gt/intel_context.c > @@ -459,6 +459,18 @@ struct i915_request *intel_context_create_request(struct intel_context *ce) > rq = i915_request_create(ce); > intel_context_unpin(ce); > > + if (IS_ERR(rq)) > + return rq; > + > + /* > + * timeline->mutex should be the inner lock, but is used as outer lock. > + * Hack around this to shut up lockdep in selftests.. > + */ > + lockdep_unpin_lock(&ce->timeline->mutex, rq->cookie); > + mutex_release(&ce->timeline->mutex.dep_map, _RET_IP_); > + mutex_acquire(&ce->timeline->mutex.dep_map, SINGLE_DEPTH_NESTING, 0, _RET_IP_); > + rq->cookie = lockdep_pin_lock(&ce->timeline->mutex); > + > return rq; > } > >
Op 03-07-2020 om 15:48 schreef Tvrtko Ursulin: > > On 03/07/2020 13:22, Maarten Lankhorst wrote: >> Some i915 selftests still use i915_vma_lock() as inner lock, and >> intel_context_create_request() intel_timeline->mutex as outer lock. >> Fortunately for selftests this is not an issue, they should be fixed >> but we can move ahead and cleanify lockdep now. > > Mentions and existence of "dirty hacks" will hopefully be removed from the series before it can be considered merge ready? > > Regards, > > Tvrtko > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c >> index 64948386630f..fe9fff5a63b1 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_context.c >> +++ b/drivers/gpu/drm/i915/gt/intel_context.c >> @@ -459,6 +459,18 @@ struct i915_request *intel_context_create_request(struct intel_context *ce) >> rq = i915_request_create(ce); >> intel_context_unpin(ce); >> + if (IS_ERR(rq)) >> + return rq; >> + >> + /* >> + * timeline->mutex should be the inner lock, but is used as outer lock. >> + * Hack around this to shut up lockdep in selftests.. >> + */ >> + lockdep_unpin_lock(&ce->timeline->mutex, rq->cookie); >> + mutex_release(&ce->timeline->mutex.dep_map, _RET_IP_); >> + mutex_acquire(&ce->timeline->mutex.dep_map, SINGLE_DEPTH_NESTING, 0, _RET_IP_); >> + rq->cookie = lockdep_pin_lock(&ce->timeline->mutex); >> + >> return rq; >> } >> Hey, We're trying to invert the locking order with vma lock vs request lock, while this is a hack, it will not affect normal running code, it's only meant to shut up lockdep in the selftests. This is mainly so we can fix the selftests one by one, without breaking the world. Ideally when mm.obj lands, we already corrected the lock ordering. We may keep this macro for selftests, but until lock reordering in selftests is complete we will need this temporarily. ~Maarten
On 07/07/2020 11:19, Maarten Lankhorst wrote: > Op 03-07-2020 om 15:48 schreef Tvrtko Ursulin: >> >> On 03/07/2020 13:22, Maarten Lankhorst wrote: >>> Some i915 selftests still use i915_vma_lock() as inner lock, and >>> intel_context_create_request() intel_timeline->mutex as outer lock. >>> Fortunately for selftests this is not an issue, they should be fixed >>> but we can move ahead and cleanify lockdep now. >> >> Mentions and existence of "dirty hacks" will hopefully be removed from the series before it can be considered merge ready? >> >> Regards, >> >> Tvrtko >> >>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c >>> index 64948386630f..fe9fff5a63b1 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_context.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c >>> @@ -459,6 +459,18 @@ struct i915_request *intel_context_create_request(struct intel_context *ce) >>> rq = i915_request_create(ce); >>> intel_context_unpin(ce); >>> + if (IS_ERR(rq)) >>> + return rq; >>> + >>> + /* >>> + * timeline->mutex should be the inner lock, but is used as outer lock. >>> + * Hack around this to shut up lockdep in selftests.. >>> + */ >>> + lockdep_unpin_lock(&ce->timeline->mutex, rq->cookie); >>> + mutex_release(&ce->timeline->mutex.dep_map, _RET_IP_); >>> + mutex_acquire(&ce->timeline->mutex.dep_map, SINGLE_DEPTH_NESTING, 0, _RET_IP_); >>> + rq->cookie = lockdep_pin_lock(&ce->timeline->mutex); >>> + >>> return rq; >>> } >>> > > Hey, > > We're trying to invert the locking order with vma lock vs request lock, while this is a hack, > it will not affect normal running code, it's only meant to shut up lockdep in the selftests. > > This is mainly so we can fix the selftests one by one, without breaking the world. Ideally > when mm.obj lands, we already corrected the lock ordering. We may keep this macro for selftests, > but until lock reordering in selftests is complete we will need this temporarily. Is there a relationship between obj->mm.lock and this particular lock inversion? I don't see it. It will become critical to have selftests adjusted to proper locking order for every which will/can trigger eviction. But okay,if you want to stage the pieces perhaps it is acceptable. As previous patch in the series removes intel_context_create_request usages outside selftests I suggest you mention this in this commit message, as part of justification why it is safe. Also it would be good to wrap intel_context_create_request in a selftest #ifdef so that accidental usage is prevented in the inter-rim stages of refactoring. Regards, Tvrtko
Op 07-07-2020 om 12:56 schreef Tvrtko Ursulin: > > On 07/07/2020 11:19, Maarten Lankhorst wrote: >> Op 03-07-2020 om 15:48 schreef Tvrtko Ursulin: >>> >>> On 03/07/2020 13:22, Maarten Lankhorst wrote: >>>> Some i915 selftests still use i915_vma_lock() as inner lock, and >>>> intel_context_create_request() intel_timeline->mutex as outer lock. >>>> Fortunately for selftests this is not an issue, they should be fixed >>>> but we can move ahead and cleanify lockdep now. >>> >>> Mentions and existence of "dirty hacks" will hopefully be removed from the series before it can be considered merge ready? >>> >>> Regards, >>> >>> Tvrtko >>> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c >>>> index 64948386630f..fe9fff5a63b1 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_context.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_context.c >>>> @@ -459,6 +459,18 @@ struct i915_request *intel_context_create_request(struct intel_context *ce) >>>> rq = i915_request_create(ce); >>>> intel_context_unpin(ce); >>>> + if (IS_ERR(rq)) >>>> + return rq; >>>> + >>>> + /* >>>> + * timeline->mutex should be the inner lock, but is used as outer lock. >>>> + * Hack around this to shut up lockdep in selftests.. >>>> + */ >>>> + lockdep_unpin_lock(&ce->timeline->mutex, rq->cookie); >>>> + mutex_release(&ce->timeline->mutex.dep_map, _RET_IP_); >>>> + mutex_acquire(&ce->timeline->mutex.dep_map, SINGLE_DEPTH_NESTING, 0, _RET_IP_); >>>> + rq->cookie = lockdep_pin_lock(&ce->timeline->mutex); >>>> + >>>> return rq; >>>> } >>>> >> >> Hey, >> >> We're trying to invert the locking order with vma lock vs request lock, while this is a hack, >> it will not affect normal running code, it's only meant to shut up lockdep in the selftests. >> >> This is mainly so we can fix the selftests one by one, without breaking the world. Ideally >> when mm.obj lands, we already corrected the lock ordering. We may keep this macro for selftests, >> but until lock reordering in selftests is complete we will need this temporarily. > > Is there a relationship between obj->mm.lock and this particular lock inversion? I don't see it. It will become critical to have selftests adjusted to proper locking order for every which will/can trigger eviction. But okay,if you want to stage the pieces perhaps it is acceptable. It's the request timeline lock being inverted, not the mm.lock. :) It needs fixing, because we want to wire ww everywhere, but it should be ok in the meantime for just selftests. For mm.lock removal, we will need to fix this completely, as this function will use its own ww context, and all pins should be fixed to use the scope of the ww lock only. > As previous patch in the series removes intel_context_create_request usages outside selftests I suggest you mention this in this commit message, as part of justification why it is safe. True. > Also it would be good to wrap intel_context_create_request in a selftest #ifdef so that accidental usage is prevented in the inter-rim stages of refactoring. Sounds good. :)
diff --git a/drivers/gpu/drm/i915/gt/intel_context.c b/drivers/gpu/drm/i915/gt/intel_context.c index 64948386630f..fe9fff5a63b1 100644 --- a/drivers/gpu/drm/i915/gt/intel_context.c +++ b/drivers/gpu/drm/i915/gt/intel_context.c @@ -459,6 +459,18 @@ struct i915_request *intel_context_create_request(struct intel_context *ce) rq = i915_request_create(ce); intel_context_unpin(ce); + if (IS_ERR(rq)) + return rq; + + /* + * timeline->mutex should be the inner lock, but is used as outer lock. + * Hack around this to shut up lockdep in selftests.. + */ + lockdep_unpin_lock(&ce->timeline->mutex, rq->cookie); + mutex_release(&ce->timeline->mutex.dep_map, _RET_IP_); + mutex_acquire(&ce->timeline->mutex.dep_map, SINGLE_DEPTH_NESTING, 0, _RET_IP_); + rq->cookie = lockdep_pin_lock(&ce->timeline->mutex); + return rq; }
Some i915 selftests still use i915_vma_lock() as inner lock, and intel_context_create_request() intel_timeline->mutex as outer lock. Fortunately for selftests this is not an issue, they should be fixed but we can move ahead and cleanify lockdep now. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/gt/intel_context.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)