On 2018-12-05 3:26 p.m., Grodzovsky, Andrey wrote: > > > On 12/05/2018 02:59 PM, Nicholas Kazlauskas wrote: >> [Why] >> Legacy cursor plane updates from drm helpers go through the full >> atomic codepath. A high volume of cursor updates through this slow >> code path can cause subsequent page-flips to skip vblank intervals >> since each individual update is slow. >> >> This problem is particularly noticeable for the compton compositor. >> >> [How] >> A fast path for cursor plane updates is added by using DRM asynchronous >> commit support provided by async_check and async_update. These don't do >> a full state/flip_done dependency stall and they don't block other >> commit work. >> >> However, DC still expects itself to be single-threaded for anything >> that can issue register writes. Screen corruption or hangs can occur >> if write sequences overlap. Every call that potentially perform >> register writes needs to be guarded for asynchronous updates to work. >> The dc_lock mutex was added for this. > > As far as page flip related register writes concerned this I think this > was never an issue - we always > supported fully concurrent page flips on different CRTCs meaning writing > surface address concurrently > on different pipes. So Are you sure you can't do cursor update > concurrently against at least page flips ? > I am not sure about full updates. > > Andrey I think this was true before we started locking the pipes around cursor updates (and probably only for dce). The problem that occur here is writing to the lock register from multiple threads at the same time. In general I think the "contract" between dm/dc now is that anything from dc that does register write sequences can't be called from multiple threads. Nicholas Kazlauskas > >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106175 >> >> Cc: Leo Li <sunpeng.li@xxxxxxx> >> Cc: Harry Wentland <harry.wentland@xxxxxxx> >> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@xxxxxxx> >> --- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 67 ++++++++++++++++++- >> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 8 +++ >> 2 files changed, 73 insertions(+), 2 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 23d61570df17..4df14a50a8ac 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >> @@ -57,6 +57,7 @@ >> >> #include <drm/drmP.h> >> #include <drm/drm_atomic.h> >> +#include <drm/drm_atomic_uapi.h> >> #include <drm/drm_atomic_helper.h> >> #include <drm/drm_dp_mst_helper.h> >> #include <drm/drm_fb_helper.h> >> @@ -133,6 +134,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state); >> static int amdgpu_dm_atomic_check(struct drm_device *dev, >> struct drm_atomic_state *state); >> >> +static void handle_cursor_update(struct drm_plane *plane, >> + struct drm_plane_state *old_plane_state); >> >> >> >> @@ -402,6 +405,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev) >> /* Zero all the fields */ >> memset(&init_data, 0, sizeof(init_data)); >> >> + mutex_init(&adev->dm.dc_lock); >> + >> if(amdgpu_dm_irq_init(adev)) { >> DRM_ERROR("amdgpu: failed to initialize DM IRQ support.\n"); >> goto error; >> @@ -516,6 +521,9 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev) >> /* DC Destroy TODO: Replace destroy DAL */ >> if (adev->dm.dc) >> dc_destroy(&adev->dm.dc); >> + >> + mutex_destroy(&adev->dm.dc_lock); >> + >> return; >> } >> >> @@ -3615,10 +3623,43 @@ static int dm_plane_atomic_check(struct drm_plane *plane, >> return -EINVAL; >> } >> >> +static int dm_plane_atomic_async_check(struct drm_plane *plane, >> + struct drm_plane_state *new_plane_state) >> +{ >> + /* Only support async updates on cursor planes. */ >> + if (plane->type != DRM_PLANE_TYPE_CURSOR) >> + return -EINVAL; >> + >> + return 0; >> +} >> + >> +static void dm_plane_atomic_async_update(struct drm_plane *plane, >> + struct drm_plane_state *new_state) >> +{ >> + struct drm_plane_state *old_state = >> + drm_atomic_get_old_plane_state(new_state->state, plane); >> + >> + if (plane->state->fb != new_state->fb) >> + drm_atomic_set_fb_for_plane(plane->state, new_state->fb); >> + >> + plane->state->src_x = new_state->src_x; >> + plane->state->src_y = new_state->src_y; >> + plane->state->src_w = new_state->src_w; >> + plane->state->src_h = new_state->src_h; >> + plane->state->crtc_x = new_state->crtc_x; >> + plane->state->crtc_y = new_state->crtc_y; >> + plane->state->crtc_w = new_state->crtc_w; >> + plane->state->crtc_h = new_state->crtc_h; >> + >> + handle_cursor_update(plane, old_state); >> +} >> + >> static const struct drm_plane_helper_funcs dm_plane_helper_funcs = { >> .prepare_fb = dm_plane_helper_prepare_fb, >> .cleanup_fb = dm_plane_helper_cleanup_fb, >> .atomic_check = dm_plane_atomic_check, >> + .atomic_async_check = dm_plane_atomic_async_check, >> + .atomic_async_update = dm_plane_atomic_async_update >> }; >> >> /* >> @@ -4307,6 +4348,7 @@ static int get_cursor_position(struct drm_plane *plane, struct drm_crtc *crtc, >> static void handle_cursor_update(struct drm_plane *plane, >> struct drm_plane_state *old_plane_state) >> { >> + struct amdgpu_device *adev = plane->dev->dev_private; >> struct amdgpu_framebuffer *afb = to_amdgpu_framebuffer(plane->state->fb); >> struct drm_crtc *crtc = afb ? plane->state->crtc : old_plane_state->crtc; >> struct dm_crtc_state *crtc_state = crtc ? to_dm_crtc_state(crtc->state) : NULL; >> @@ -4331,9 +4373,12 @@ static void handle_cursor_update(struct drm_plane *plane, >> >> if (!position.enable) { >> /* turn off cursor */ >> - if (crtc_state && crtc_state->stream) >> + if (crtc_state && crtc_state->stream) { >> + mutex_lock(&adev->dm.dc_lock); >> dc_stream_set_cursor_position(crtc_state->stream, >> &position); >> + mutex_unlock(&adev->dm.dc_lock); >> + } >> return; >> } >> >> @@ -4351,6 +4396,7 @@ static void handle_cursor_update(struct drm_plane *plane, >> attributes.pitch = attributes.width; >> >> if (crtc_state->stream) { >> + mutex_lock(&adev->dm.dc_lock); >> if (!dc_stream_set_cursor_attributes(crtc_state->stream, >> &attributes)) >> DRM_ERROR("DC failed to set cursor attributes\n"); >> @@ -4358,6 +4404,7 @@ static void handle_cursor_update(struct drm_plane *plane, >> if (!dc_stream_set_cursor_position(crtc_state->stream, >> &position)) >> DRM_ERROR("DC failed to set cursor position\n"); >> + mutex_unlock(&adev->dm.dc_lock); >> } >> } >> >> @@ -4573,6 +4620,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc, >> &acrtc_state->stream->vrr_infopacket; >> } >> >> + mutex_lock(&adev->dm.dc_lock); >> dc_commit_updates_for_stream(adev->dm.dc, >> surface_updates, >> 1, >> @@ -4580,6 +4628,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc, >> &stream_update, >> &surface_updates->surface, >> state); >> + mutex_unlock(&adev->dm.dc_lock); >> >> DRM_DEBUG_DRIVER("%s Flipping to hi: 0x%x, low: 0x%x \n", >> __func__, >> @@ -4594,6 +4643,7 @@ static void amdgpu_dm_do_flip(struct drm_crtc *crtc, >> * with a dc_plane_state and follow the atomic model a bit more closely here. >> */ >> static bool commit_planes_to_stream( >> + struct amdgpu_display_manager *dm, >> struct dc *dc, >> struct dc_plane_state **plane_states, >> uint8_t new_plane_count, >> @@ -4670,11 +4720,13 @@ static bool commit_planes_to_stream( >> updates[i].scaling_info = &scaling_info[i]; >> } >> >> + mutex_lock(&dm->dc_lock); >> dc_commit_updates_for_stream( >> dc, >> updates, >> new_plane_count, >> dc_stream, stream_update, plane_states, state); >> + mutex_unlock(&dm->dc_lock); >> >> kfree(flip_addr); >> kfree(plane_info); >> @@ -4780,7 +4832,8 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, >> >> dc_stream_attach->abm_level = acrtc_state->abm_level; >> >> - if (false == commit_planes_to_stream(dm->dc, >> + if (false == commit_planes_to_stream(dm, >> + dm->dc, >> plane_states_constructed, >> planes_count, >> acrtc_state, >> @@ -4950,7 +5003,9 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) >> >> if (dc_state) { >> dm_enable_per_frame_crtc_master_sync(dc_state); >> + mutex_lock(&dm->dc_lock); >> WARN_ON(!dc_commit_state(dm->dc, dc_state)); >> + mutex_unlock(&dm->dc_lock); >> } >> >> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) { >> @@ -5012,6 +5067,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) >> >> /*TODO How it works with MPO ?*/ >> if (!commit_planes_to_stream( >> + dm, >> dm->dc, >> status->plane_states, >> status->plane_count, >> @@ -5904,6 +5960,13 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev, >> ret = -EINVAL; >> goto fail; >> } >> + } else if (state->legacy_cursor_update) { >> + /* >> + * This is a fast cursor update coming from the plane update >> + * helper, check if it can be done asynchronously for better >> + * performance. >> + */ >> + state->async_update = !drm_atomic_helper_async_check(dev, state); >> } >> >> /* Must be success */ >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> index 4326dc256491..25bb91ee80ba 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h >> @@ -134,6 +134,14 @@ struct amdgpu_display_manager { >> >> struct drm_modeset_lock atomic_obj_lock; >> >> + /** >> + * @dc_lock: >> + * >> + * Guards access to DC functions that can issue register write >> + * sequences. >> + */ >> + struct mutex dc_lock; >> + >> /** >> * @irq_handler_list_low_tab: >> * > _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
