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.
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@xxxxxxx>
>> Cc: Leo Li <sunpeng.li@xxxxxxx>
>> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
