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@xxxxxxxxxxxxxxx>
>>> Fixes: e2c8b8701e2d ("drm/i915: Use atomic helpers for suspend, v2.")
>>> Cc: drm-intel-fixes@xxxxxxxxxxxxxxxxxxxxx
>>> ---
>>> 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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
