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 <[email protected]> >>> Cc: Leo Li <[email protected]> >>> Signed-off-by: Nicholas Kazlauskas <[email protected]> >>> --- >>> .../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 >> [email protected] >> https://lists.freedesktop.org/mailman/listinfo/amd-gfx >> > _______________________________________________ > amd-gfx mailing list > [email protected] > https://lists.freedesktop.org/mailman/listinfo/amd-gfx _______________________________________________ amd-gfx mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/amd-gfx
