Re: [PATCH v3 1/2] drm/i915: Fix modeset handling during gpu reset, v3.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 10-05-16 om 09:48 schreef Daniel Vetter:
> On Mon, May 09, 2016 at 03:54:15PM +0300, Ville Syrjälä wrote:
>> On Mon, May 09, 2016 at 01:04:21PM +0200, Maarten Lankhorst wrote:
>>> This function would call drm_modeset_lock_all, while the suspend/resume
>>> functions already have their own locking. Fix this by factoring out
>>> __intel_display_resume, and calling the atomic helpers for duplicating
>>> atomic state and disabling all crtc's during suspend.
>>>
>>> Changes since v1:
>>> - Deal with -EDEADLK right after lock_all and clean up calls
>>>   to hw readout.
>>> - Always take all modeset locks so updates during gpu reset are blocked.
>>> Changes since v2:
>>> - Fix deadlock in intel_update_primary_planes.
>>> - Move WARN_ON(EDEADLK) to __intel_display_resume.
>>> - pctx -> ctx
>>> - only call __intel_display_resume on success in intel_display_resume.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
>>> Cc: drm-intel-fixes@lists.freedesktop.org
>>> ---
>>>  drivers/gpu/drm/i915/intel_display.c | 150 ++++++++++++++++++++++-------------
>>>  1 file changed, 93 insertions(+), 57 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 14e05091c397..6faa529f35df 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -3130,41 +3130,96 @@ static void intel_update_primary_planes(struct drm_device *dev)
>>>  
>>>  	for_each_crtc(dev, crtc) {
>>>  		struct intel_plane *plane = to_intel_plane(crtc->primary);
>>> -		struct intel_plane_state *plane_state;
>>> -
>>> -		drm_modeset_lock_crtc(crtc, &plane->base);
>>> -		plane_state = to_intel_plane_state(plane->base.state);
>>> +		struct intel_plane_state *plane_state =
>>> +			to_intel_plane_state(plane->base.state);
>>>  
>>>  		if (plane_state->visible)
>>>  			plane->update_plane(&plane->base,
>>>  					    to_intel_crtc_state(crtc->state),
>>>  					    plane_state);
>>> +	}
>>> +}
>>> +
>>> +static int
>>> +__intel_display_resume(struct drm_device *dev,
>>> +		       struct drm_atomic_state *state)
>>> +{
>>> +	struct drm_crtc_state *crtc_state;
>>> +	struct drm_crtc *crtc;
>>> +	int i, ret;
>>> +
>>> +	intel_modeset_setup_hw_state(dev);
>>> +	i915_redisable_vga(dev);
>>>  
>>> -		drm_modeset_unlock_crtc(crtc);
>>> +	if (!state)
>>> +		return 0;
>> Thank you diff for making things illegible.
>>
>> I'm still not convinced we should be changing the locking scheme
>> here, especially in what's supposed to be just a fix.
>>
>> In general, I'm not sure we want ->detect() and other irrelevant
>> stuff to block the GPU reset. The nice thing about g4x+ reset so
>> far has been that it's almost unnoticeable. Of course if it's a
>> genuine GPU hang the screen will probably have been frozen for
>> quite a while when we initiate the reset, so perhaps that's not
>> a huge real world issue.
> We don't really need dev->mode_config.mutex for atomic commits, so taking
> that out and use drm_modeset_lock_all_ctx would give you that.
> -Daniel

Except that i915 still complains if we don't hold mode_config.mutex

commit ea49c9acf2db7082f0406bb3a570cc6bad37082b
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Tue Feb 16 15:27:42 2016 +0100

    drm/i915: Lock mode_config.mutex in intel_display_resume.

So keeping it for now would be better.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx