[Intel-gfx] [PATCH v3 06/11] drm/i915: Handle locking better in i915_sink_crc.

[Maarten Lankhorst]

Op 10-11-17 om 14:24 schreef Daniel Vetter:
> On Fri, Nov 10, 2017 at 02:13:39PM +0100, Daniel Vetter wrote:
>> On Fri, Nov 10, 2017 at 12:34:58PM +0100, Maarten Lankhorst wrote:
>>> Lock the bare minimum, instead of the entire world, and
>>> use interruptible locking because we can.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c | 40 +++++++++++++++++++++++++++++--------
>>>  1 file changed, 32 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index 39883cd915db..7e8f40eb970d 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2736,39 +2736,63 @@ static int i915_sink_crc(struct seq_file *m, void *data)
>>>  	struct intel_connector *connector;
>>>  	struct drm_connector_list_iter conn_iter;
>>>  	struct intel_dp *intel_dp = NULL;
>>> +	struct drm_modeset_acquire_ctx ctx;
>>>  	int ret;
>>>  	u8 crc[6];
>>>  
>>> -	drm_modeset_lock_all(dev);
>>> +	drm_modeset_acquire_init(&ctx, DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>>> +
>>>  	drm_connector_list_iter_begin(dev, &conn_iter);
>> I kinda expect this funny locking nesting to upset lockdep, I think we
>> have a bunch of places where we nest list_iter and modeset locks
>> differently.
>>
>> I think the correct nesting is list_iter entirely within the ww_mutex
>> stuff, which means you'd need to terminate the loop (and remember the
>> connector), then after list_iter_end do the ww_mutex dance. Of course
>> that's all assuming CI shows I'm right (hopefully it does since we no
>> longer reboot, in the past finding these took a few runs until you had the
>> right test combination to trigger this stuff).
>>
>> Even if we don't yet have such a case I'd really prefer that list_iter
>> isn't nested between the acquire_ctx and modeset ww_mutex lockdep
>> contexts.
> Correction, this exact locking pattern already exists in
> drm_atomic_add_affected_connectors, changing to what I suggested would
> actually upset lockdep. Hence
>
> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Thanks, pushed up to here, would be nice if 7/11 would get a r-b too. No IPS related changes there yet, just the preparations. :)