[PATCH 2/2] drm/amd/display: Remove wait for hw/flip done in atomic check

[Grodzovsky, Andrey]

On 11/22/2018 02:43 PM, Kazlauskas, Nicholas wrote:
> On 11/22/18 2:39 PM, Grodzovsky, Andrey wrote:
>>
>> On 11/22/2018 12:34 PM, Nicholas Kazlauskas wrote:
>>> [Why]
>>> Atomic check can't truly be non-blocking if amdgpu_dm is waiting for
>>> hw_done and flip_done in atomic check. This introduces waits when
>>> any previous non-blocking commits queued work on a worker thread and
>>> a new atomic commit attempts to do another full update/modeset.
>>>
>>> [How]
>>> Drop the waits and move the global lock acqusition into atomic check.
>>>
>>> This is fine to do since commit tail waits for these dependencies
>>> before calling amdgpu_dm_atomic_commit_tail.
>> Note that drm_atomic_helper_wait_for_dependencies waits only on all
>> preceeding commits that touch the same CRTC
>> while our custom do_aquire_global_lock wait on ALL previous commits in
>> the system - even on other CRTCS. As far as I can
>> remember that was important because we have global dc_state context
>> which must be protected for access/modifications while
>> looks like DRM assumes unrelated CRTCs can be modified concurrently.
>> Try to verify that latest display code will not be broken by this
>> relaxation of synchronization.
>>
>> Andrey
> This is a good point.
>
> What we should really be doing instead here is locking anything that can
> modify dc->current_state from within atomic commit tail. Serializing
> commits into a queue could work too.

This is a possibility by then you have to duplicate 
drm_atomic_helper_commit with only once change which
is to substitute system_unbound_wq with you costume WQ. Alternatively 
propose a change  to update drm_atomic_helper_commit
to allow WQs as parameter.

Andrey

>
> Either should fix non-blocking support.
>
> Nicholas Kazlauskas
>
>>> This is only safe as long as DC never queries anything from within
>>> current_state when doing validation in atomic_check. This is the
>>> case as of writing, but any future uses of dc->current_state from
>>> within atomic_check should be considered incorrect.
>>>
>>> Cc: Harry Wentland <harry.wentland at amd.com>
>>> Cc: Leo Li <sunpeng.li at amd.com>
>>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
>>> ---
>>>     .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 58 ++-----------------
>>>     1 file changed, 6 insertions(+), 52 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 3ae438d9849f..fe21bb86b66a 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -5078,57 +5078,6 @@ void dm_restore_drm_connector_state(struct drm_device *dev,
>>>     		dm_force_atomic_commit(&aconnector->base);
>>>     }
>>>     
>>> -/*
>>> - * Grabs all modesetting locks to serialize against any blocking commits,
>>> - * Waits for completion of all non blocking commits.
>>> - */
>>> -static int do_aquire_global_lock(struct drm_device *dev,
>>> -				 struct drm_atomic_state *state)
>>> -{
>>> -	struct drm_crtc *crtc;
>>> -	struct drm_crtc_commit *commit;
>>> -	long ret;
>>> -
>>> -	/*
>>> -	 * Adding all modeset locks to aquire_ctx will
>>> -	 * ensure that when the framework release it the
>>> -	 * extra locks we are locking here will get released to
>>> -	 */
>>> -	ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
>>> -	if (ret)
>>> -		return ret;
>>> -
>>> -	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>> -		spin_lock(&crtc->commit_lock);
>>> -		commit = list_first_entry_or_null(&crtc->commit_list,
>>> -				struct drm_crtc_commit, commit_entry);
>>> -		if (commit)
>>> -			drm_crtc_commit_get(commit);
>>> -		spin_unlock(&crtc->commit_lock);
>>> -
>>> -		if (!commit)
>>> -			continue;
>>> -
>>> -		/*
>>> -		 * Make sure all pending HW programming completed and
>>> -		 * page flips done
>>> -		 */
>>> -		ret = wait_for_completion_interruptible_timeout(&commit->hw_done, 10*HZ);
>>> -
>>> -		if (ret > 0)
>>> -			ret = wait_for_completion_interruptible_timeout(
>>> -					&commit->flip_done, 10*HZ);
>>> -
>>> -		if (ret == 0)
>>> -			DRM_ERROR("[CRTC:%d:%s] hw_done or flip_done "
>>> -				  "timed out\n", crtc->base.id, crtc->name);
>>> -
>>> -		drm_crtc_commit_put(commit);
>>> -	}
>>> -
>>> -	return ret < 0 ? ret : 0;
>>> -}
>>> -
>>>     void set_freesync_on_stream(struct amdgpu_display_manager *dm,
>>>     			    struct dm_crtc_state *new_crtc_state,
>>>     			    struct dm_connector_state *new_con_state,
>>> @@ -5793,7 +5742,12 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>>>     		if (ret)
>>>     			goto fail;
>>>     
>>> -		ret = do_aquire_global_lock(dev, state);
>>> +		/*
>>> +		 * This should be replaced with finer locking on the
>>> +		 * on the appropriate resources when possible.
>>> +		 * For now it's safer to grab everything.
>>> +		 */
>>> +		ret = drm_modeset_lock_all_ctx(dev, state->acquire_ctx);
>>>     		if (ret)
>>>     			goto fail;
>>>     
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx