From d6ef9b4175e870b52468fed4fd77b0d3d54e20fc Mon Sep 17 00:00:00 2001 From: Nicholas Kazlauskas Date: Fri, 5 Apr 2019 09:35:14 -0400 Subject: [PATCH] drm/amd/display: Refactor CRTC interrupt toggling logic [Why] The vblank and pageflip interrupts should only be enabled for a CRTC that's enabled and has active planes. The current logic takes care of this, but isn't setup to handle the case where the active plane count goes to zero but the stream remains enabled. We currently block this case since we don't allow commits that enable a CRTC with no active planes, but shouldn't be any reason we can't support this from a hardware perspective and many userspace applications expect to be able to do it (like IGT). [How] The count_crtc_active_planes function fills in the number of "active_planes" on the dm_crtc_state. This should be the same as DC's plane_count on the stream_status but easier to access since we don't need to lock the private atomic state with the DC context. Add the "interrupts_enabled" flag to the dm_crtc_state and set it based on whether the stream exists and if there are active planes on the stream. Update the disable and enable logic to make use of this new flag. There shouldn't be any functional change (yet) with this patch. Signed-off-by: Nicholas Kazlauskas Reviewed-by: David Francis Acked-by: Leo Li Signed-off-by: Alex Deucher --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 86 ++++++++++++++----- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 3 + 2 files changed, 68 insertions(+), 21 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 a3dc7ab523de..c82048cb901d 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3438,6 +3438,8 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc) dc_stream_retain(state->stream); } + state->active_planes = cur->active_planes; + state->interrupts_enabled = cur->interrupts_enabled; state->vrr_params = cur->vrr_params; state->vrr_infopacket = cur->vrr_infopacket; state->abm_level = cur->abm_level; @@ -3862,7 +3864,7 @@ static void dm_crtc_helper_disable(struct drm_crtc *crtc) { } -static bool does_crtc_have_active_plane(struct drm_crtc_state *new_crtc_state) +static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state) { struct drm_atomic_state *state = new_crtc_state->state; struct drm_plane *plane; @@ -3891,7 +3893,32 @@ static bool does_crtc_have_active_plane(struct drm_crtc_state *new_crtc_state) num_active += (new_plane_state->fb != NULL); } - return num_active > 0; + return num_active; +} + +/* + * Sets whether interrupts should be enabled on a specific CRTC. + * We require that the stream be enabled and that there exist active + * DC planes on the stream. + */ +static void +dm_update_crtc_interrupt_state(struct drm_crtc *crtc, + struct drm_crtc_state *new_crtc_state) +{ + struct dm_crtc_state *dm_new_crtc_state = + to_dm_crtc_state(new_crtc_state); + + dm_new_crtc_state->active_planes = 0; + dm_new_crtc_state->interrupts_enabled = false; + + if (!dm_new_crtc_state->stream) + return; + + dm_new_crtc_state->active_planes = + count_crtc_active_planes(new_crtc_state); + + dm_new_crtc_state->interrupts_enabled = + dm_new_crtc_state->active_planes > 0; } static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, @@ -3902,6 +3929,14 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(state); int ret = -EINVAL; + /* + * Update interrupt state for the CRTC. This needs to happen whenever + * the CRTC has changed or whenever any of its planes have changed. + * Atomic check satisfies both of these requirements since the CRTC + * is added to the state by DRM during drm_atomic_helper_check_planes. + */ + dm_update_crtc_interrupt_state(crtc, state); + if (unlikely(!dm_crtc_state->stream && modeset_required(state, NULL, dm_crtc_state->stream))) { WARN_ON(1); @@ -3914,7 +3949,7 @@ static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc, /* We want at least one hardware plane enabled to use the stream. */ if (state->enable && state->active && - !does_crtc_have_active_plane(state)) + dm_crtc_state->active_planes == 0) return -EINVAL; if (dc_validate_stream(dc, dm_crtc_state->stream) == DC_OK) @@ -5390,24 +5425,33 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev, int i; /* - * We evade vblanks and pflips on crtc that - * should be changed. We do it here to flush & disable - * interrupts before drm_swap_state is called in drm_atomic_helper_commit - * it will update crtc->dm_crtc_state->stream pointer which is used in - * the ISRs. + * We evade vblank and pflip interrupts on CRTCs that are undergoing + * a modeset, being disabled, or have no active planes. + * + * It's done in atomic commit rather than commit tail for now since + * some of these interrupt handlers access the current CRTC state and + * potentially the stream pointer itself. + * + * Since the atomic state is swapped within atomic commit and not within + * commit tail this would leave to new state (that hasn't been committed yet) + * being accesssed from within the handlers. + * + * TODO: Fix this so we can do this in commit tail and not have to block + * in atomic check. */ for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { struct dm_crtc_state *dm_old_crtc_state = to_dm_crtc_state(old_crtc_state); struct dm_crtc_state *dm_new_crtc_state = to_dm_crtc_state(new_crtc_state); struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc); - if (drm_atomic_crtc_needs_modeset(new_crtc_state) - && dm_old_crtc_state->stream) { + if (dm_old_crtc_state->interrupts_enabled && + (!dm_new_crtc_state->interrupts_enabled || + drm_atomic_crtc_needs_modeset(new_crtc_state))) { /* * If the stream is removed and CRC capture was * enabled on the CRTC the extra vblank reference - * needs to be dropped since CRC capture will be - * disabled. + * needs to be dropped since CRC capture will not + * be re-enabled. */ if (!dm_new_crtc_state->stream && dm_new_crtc_state->crc_enabled) { @@ -5635,13 +5679,14 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) pre_update_freesync_state_on_stream(dm, dm_new_crtc_state); } + /* + * Enable interrupts on CRTCs that are newly active, undergone + * a modeset, or have active planes again. + */ for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) { - /* - * loop to enable interrupts on newly arrived crtc - */ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc); - bool modeset_needed; + bool enable; if (old_crtc_state->active && !new_crtc_state->active) crtc_disable_count++; @@ -5653,12 +5698,11 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) amdgpu_dm_handle_vrr_transition(dm_old_crtc_state, dm_new_crtc_state); - modeset_needed = modeset_required( - new_crtc_state, - dm_new_crtc_state->stream, - dm_old_crtc_state->stream); + enable = dm_new_crtc_state->interrupts_enabled && + (!dm_old_crtc_state->interrupts_enabled || + drm_atomic_crtc_needs_modeset(new_crtc_state)); - if (dm_new_crtc_state->stream == NULL || !modeset_needed) + if (!enable) continue; manage_dm_interrupts(adev, acrtc, true); 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 3a0b6164c755..978ff14a7d45 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h @@ -271,6 +271,9 @@ struct dm_crtc_state { struct drm_crtc_state base; struct dc_stream_state *stream; + int active_planes; + bool interrupts_enabled; + int crc_skip_count; bool crc_enabled; -- 2.30.2