[PATCH 2/2] drm/amd/display: Allow for vblank enabled with no active planes
Lakha, Bhawanpreet
Bhawanpreet.Lakha at amd.com
Mon Jul 13 16:30:49 UTC 2020
[AMD Official Use Only - Internal Distribution Only]
Reviewed-by: Bhawanpreet Lakha <Bhawanpreet.Lakha at amd.com>
________________________________
From: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
Sent: July 13, 2020 11:39 AM
To: amd-gfx at lists.freedesktop.org <amd-gfx at lists.freedesktop.org>
Cc: Kazlauskas, Nicholas <Nicholas.Kazlauskas at amd.com>; Lakha, Bhawanpreet <Bhawanpreet.Lakha at amd.com>
Subject: [PATCH 2/2] drm/amd/display: Allow for vblank enabled with no active planes
[Why]
CRC capture doesn't work when the active plane count is 0 since we
currently tie both vblank and pageflip interrupts to active_plane_count
greater than 0.
[How]
The frontend is what generates the vblank interrupts while the backend
is what generates pageflip interrupts. Both have a requirement for
the CRTC to be active, so control the overall interrupt state based
on that instead.
Pageflip interrupts need to be enabled based on active plane count, but
we actually rely on power gating to take care of disabling the interrupt
for us on pipes that can be power gated.
For pipes that can't be power gated it's still fine to leave it enabled
since the interrupt only triggers after the address has been written
to that particular pipe - which we won't be doing without an active
plane.
The issue we had before with this setup was that we couldn't force
the state back on. We were essentially manipulating the refcount
to enable or disable as needed in a two pass approach.
However, there is a function that solves this problem more elegantly:
amdgpu_irq_update() will unconditionally call the set based on what it
thinks the current enablement state is.
This leaves two future TODO items for our IRQ handling:
- Disabling IRQs in commit tail instead of atomic commit
- Mapping the pageflip interrupt to VUPDATE or something that's tied to
the frontend instead of the backend since the mapping to CRTC is not
correct
Cc: Bhawanpreet Lakha <Bhawanpreet.Lakha at amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas at amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 168 ++++++++----------
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
2 files changed, 78 insertions(+), 91 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 d9f8e9c26390..05160c6bbc03 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -4719,7 +4719,6 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
}
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;
@@ -5393,29 +5392,19 @@ static int count_crtc_active_planes(struct drm_crtc_state *new_crtc_state)
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)
+static void dm_update_crtc_active_planes(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,
@@ -5426,13 +5415,7 @@ 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);
+ dm_update_crtc_active_planes(crtc, state);
if (unlikely(!dm_crtc_state->stream &&
modeset_required(state, NULL, dm_crtc_state->stream))) {
@@ -6547,8 +6530,10 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
bool enable)
{
/*
- * this is not correct translation but will work as soon as VBLANK
- * constant is the same as PFLIP
+ * We have no guarantee that the frontend index maps to the same
+ * backend index - some even map to more than one.
+ *
+ * TODO: Use a different interrupt or check DC itself for the mapping.
*/
int irq_type =
amdgpu_display_crtc_idx_to_irq_type(
@@ -6571,6 +6556,19 @@ static void manage_dm_interrupts(struct amdgpu_device *adev,
}
}
+static void dm_update_pflip_irq_state(struct amdgpu_device *adev,
+ struct amdgpu_crtc *acrtc)
+{
+ int irq_type =
+ amdgpu_display_crtc_idx_to_irq_type(adev, acrtc->crtc_id);
+
+ /**
+ * This reads the current state for the IRQ and force reapplies
+ * the setting to hardware.
+ */
+ amdgpu_irq_update(adev, &adev->pageflip_irq, irq_type);
+}
+
static bool
is_scaling_state_different(const struct dm_connector_state *dm_state,
const struct dm_connector_state *old_dm_state)
@@ -7154,7 +7152,16 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
usleep_range(1000, 1100);
}
- if (acrtc_attach->base.state->event) {
+ /**
+ * Prepare the flip event for the pageflip interrupt to handle.
+ *
+ * This only works in the case where we've already turned on the
+ * appropriate hardware blocks (eg. HUBP) so in the transition case
+ * from 0 -> n planes we have to skip a hardware generated event
+ * and rely on sending it from software.
+ */
+ if (acrtc_attach->base.state->event &&
+ acrtc_state->active_planes > 0) {
drm_crtc_vblank_get(pcrtc);
spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
@@ -7223,6 +7230,24 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
&bundle->stream_update,
dc_state);
+ /**
+ * Enable or disable the interrupts on the backend.
+ *
+ * Most pipes are put into power gating when unused.
+ *
+ * When power gating is enabled on a pipe we lose the
+ * interrupt enablement state when power gating is disabled.
+ *
+ * So we need to update the IRQ control state in hardware
+ * whenever the pipe turns on (since it could be previously
+ * power gated) or off (since some pipes can't be power gated
+ * on some ASICs).
+ */
+ if (dm_old_crtc_state->active_planes != acrtc_state->active_planes)
+ dm_update_pflip_irq_state(
+ (struct amdgpu_device *)dev->dev_private,
+ acrtc_attach);
+
if ((acrtc_state->update_type > UPDATE_TYPE_FAST) &&
acrtc_state->stream->link->psr_settings.psr_version != DC_PSR_VERSION_UNSUPPORTED &&
!acrtc_state->stream->link->psr_settings.psr_feature_enabled)
@@ -7323,64 +7348,6 @@ static void amdgpu_dm_commit_audio(struct drm_device *dev,
}
}
-/*
- * Enable interrupts on CRTCs that are newly active, undergone
- * a modeset, or have active planes again.
- *
- * Done in two passes, based on the for_modeset flag:
- * Pass 1: For CRTCs going through modeset
- * Pass 2: For CRTCs going from 0 to n active planes
- *
- * Interrupts can only be enabled after the planes are programmed,
- * so this requires a two-pass approach since we don't want to
- * just defer the interrupts until after commit planes every time.
- */
-static void amdgpu_dm_enable_crtc_interrupts(struct drm_device *dev,
- struct drm_atomic_state *state,
- bool for_modeset)
-{
- struct amdgpu_device *adev = dev->dev_private;
- struct drm_crtc *crtc;
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- int i;
-#ifdef CONFIG_DEBUG_FS
- enum amdgpu_dm_pipe_crc_source source;
-#endif
-
- for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
- new_crtc_state, i) {
- struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
- struct dm_crtc_state *dm_new_crtc_state =
- to_dm_crtc_state(new_crtc_state);
- struct dm_crtc_state *dm_old_crtc_state =
- to_dm_crtc_state(old_crtc_state);
- bool modeset = drm_atomic_crtc_needs_modeset(new_crtc_state);
- bool run_pass;
-
- run_pass = (for_modeset && modeset) ||
- (!for_modeset && !modeset &&
- !dm_old_crtc_state->interrupts_enabled);
-
- if (!run_pass)
- continue;
-
- if (!dm_new_crtc_state->interrupts_enabled)
- continue;
-
- manage_dm_interrupts(adev, acrtc, true);
-
-#ifdef CONFIG_DEBUG_FS
- /* The stream has changed so CRC capture needs to re-enabled. */
- source = dm_new_crtc_state->crc_src;
- if (amdgpu_dm_is_valid_crc_source(source)) {
- amdgpu_dm_crtc_configure_crc_source(
- crtc, dm_new_crtc_state,
- dm_new_crtc_state->crc_src);
- }
-#endif
- }
-}
-
/*
* amdgpu_dm_crtc_copy_transient_flags - copy mirrored flags from DRM to DC
* @crtc_state: the DRM CRTC state
@@ -7420,12 +7387,10 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
* 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 (dm_old_crtc_state->interrupts_enabled &&
- (!dm_new_crtc_state->interrupts_enabled ||
+ if (old_crtc_state->active &&
+ (!new_crtc_state->active ||
drm_atomic_crtc_needs_modeset(new_crtc_state)))
manage_dm_interrupts(adev, acrtc, false);
}
@@ -7704,8 +7669,34 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
dm_new_crtc_state);
}
- /* Enable interrupts for CRTCs going through a modeset. */
- amdgpu_dm_enable_crtc_interrupts(dev, state, true);
+ /**
+ * Enable interrupts for CRTCs that are newly enabled or went through
+ * a modeset. It was intentionally deferred until after the front end
+ * state was modified to wait until the OTG was on and so the IRQ
+ * handlers didn't access stale or invalid state.
+ */
+ for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
+ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+
+ if (new_crtc_state->active &&
+ (!old_crtc_state->active ||
+ drm_atomic_crtc_needs_modeset(new_crtc_state))) {
+ manage_dm_interrupts(adev, acrtc, true);
+#ifdef CONFIG_DEBUG_FS
+ /**
+ * Frontend may have changed so reapply the CRC capture
+ * settings for the stream.
+ */
+ dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+
+ if (amdgpu_dm_is_valid_crc_source(dm_new_crtc_state->crc_src)) {
+ amdgpu_dm_crtc_configure_crc_source(
+ crtc, dm_new_crtc_state,
+ dm_new_crtc_state->crc_src);
+ }
+#endif
+ }
+ }
for_each_new_crtc_in_state(state, crtc, new_crtc_state, j)
if (new_crtc_state->async_flip)
@@ -7720,9 +7711,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
dm, crtc, wait_for_vblank);
}
- /* Enable interrupts for CRTCs going from 0 to n active planes. */
- amdgpu_dm_enable_crtc_interrupts(dev, state, false);
-
/* Update audio instances for each connector. */
amdgpu_dm_commit_audio(dev, state);
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 94d3ff727c25..82cdf07b4bd0 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -414,7 +414,6 @@ struct dm_crtc_state {
int update_type;
int active_planes;
- bool interrupts_enabled;
int crc_skip_count;
enum amdgpu_dm_pipe_crc_source crc_src;
--
2.25.1
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200713/5536b708/attachment-0001.htm>
More information about the amd-gfx
mailing list