* [PATCH v2 0/3] Refactor DM IRQ handling
@ 2020-09-11 16:27 Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: Aurabindo Pillai @ 2020-09-11 16:27 UTC (permalink / raw)
To: amd-gfx; +Cc: nicholas.kazlauskas
Changes in V2:
* fix memory leak.
Interrupts are disabled too early in DM's atomic_commit() which can
cause issues in certain situations with non blocking commits timing
out on flip_done interrupt. The early disabling of interrupts were
necessary due to interrupts accessing crtc state directly. This
refactor removes direct access of crtc state from the irq handler
so that disabling of interrupts can be done later in commit_tail()
--
Aurabindo Pillai (3):
drm/amdgpu: Move existing pflip fields into separate struct
drm/amd/display: Refactor to prevent crtc state access in DM IRQ
handler
drm/amd/display: Move disable interrupt into commit tail
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 4 +-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 156 +++++++++---------
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 37 +++++
4 files changed, 115 insertions(+), 83 deletions(-)
create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/3] drm/amdgpu: Move existing pflip fields into separate struct
2020-09-11 16:27 [PATCH v2 0/3] Refactor DM IRQ handling Aurabindo Pillai
@ 2020-09-11 16:27 ` Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai
2 siblings, 0 replies; 5+ messages in thread
From: Aurabindo Pillai @ 2020-09-11 16:27 UTC (permalink / raw)
To: amd-gfx; +Cc: nicholas.kazlauskas
[Why&How]
To refactor DM IRQ management, all fields used by IRQ is best moved
to a separate struct so that main amdgpu_crtc struct need not be changed
Location of the new struct shall be in DM
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 4 ++-
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 4 +--
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 33 +++++++++++++++++++
3 files changed, 38 insertions(+), 3 deletions(-)
create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index aa3e22be4f2d..345cb0464370 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -46,6 +46,7 @@
#include <drm/drm_dp_mst_helper.h>
#include "modules/inc/mod_freesync.h"
+#include "amdgpu_dm_irq_params.h"
struct amdgpu_bo;
struct amdgpu_device;
@@ -410,7 +411,8 @@ struct amdgpu_crtc {
struct amdgpu_flip_work *pflip_works;
enum amdgpu_flip_status pflip_status;
int deferred_flip_completion;
- u32 last_flip_vblank;
+ /* parameters access from DM IRQ handler */
+ struct dm_irq_params dm_irq_params;
/* pll sharing */
struct amdgpu_atom_ss ss;
bool ss_enabled;
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 cb624ee70545..40814cdd8c92 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -389,7 +389,7 @@ static void dm_pflip_high_irq(void *interrupt_params)
* of pageflip completion, so last_flip_vblank is the forbidden count
* for queueing new pageflips if vsync + VRR is enabled.
*/
- amdgpu_crtc->last_flip_vblank =
+ amdgpu_crtc->dm_irq_params.last_flip_vblank =
amdgpu_get_vblank_counter_kms(&amdgpu_crtc->base);
amdgpu_crtc->pflip_status = AMDGPU_FLIP_NONE;
@@ -7248,7 +7248,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
* on late submission of flips.
*/
spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
- last_flip_vblank = acrtc_attach->last_flip_vblank;
+ last_flip_vblank = acrtc_attach->dm_irq_params.last_flip_vblank;
spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
new file mode 100644
index 000000000000..55ef237eed8b
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
@@ -0,0 +1,33 @@
+/*
+ * Copyright 2020 Advanced Micro Devices, Inc.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be included in
+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+ * OTHER DEALINGS IN THE SOFTWARE.
+ *
+ * Authors: AMD
+ *
+ */
+
+#ifndef __AMDGPU_DM_IRQ_PARAMS_H__
+#define __AMDGPU_DM_IRQ_PARAMS_H__
+
+struct dm_irq_params {
+ u32 last_flip_vblank;
+};
+
+#endif /* __AMDGPU_DM_IRQ_PARAMS_H__ */
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler
2020-09-11 16:27 [PATCH v2 0/3] Refactor DM IRQ handling Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai
@ 2020-09-11 16:27 ` Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai
2 siblings, 0 replies; 5+ messages in thread
From: Aurabindo Pillai @ 2020-09-11 16:27 UTC (permalink / raw)
To: amd-gfx; +Cc: nicholas.kazlauskas
[Why&How]
Currently commit_tail holds global locks and wait for dependencies which is
against the DRM API contracts. Inorder to fix this, IRQ handler should be able
to run without having to access crtc state. Required parameters are copied over
so that they can be directly accessed from the interrupt handler
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 111 ++++++++++--------
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 4 +
3 files changed, 64 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 40814cdd8c92..1455acf5beca 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -192,17 +192,14 @@ static u32 dm_vblank_get_counter(struct amdgpu_device *adev, int crtc)
return 0;
else {
struct amdgpu_crtc *acrtc = adev->mode_info.crtcs[crtc];
- struct dm_crtc_state *acrtc_state = to_dm_crtc_state(
- acrtc->base.state);
-
- if (acrtc_state->stream == NULL) {
+ if (acrtc->dm_irq_params.stream == NULL) {
DRM_ERROR("dc_stream_state is NULL for crtc '%d'!\n",
crtc);
return 0;
}
- return dc_stream_get_vblank_counter(acrtc_state->stream);
+ return dc_stream_get_vblank_counter(acrtc->dm_irq_params.stream);
}
}
@@ -215,10 +212,8 @@ static int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc,
return -EINVAL;
else {
struct amdgpu_crtc *acrtc = adev->mode_info.crtcs[crtc];
- struct dm_crtc_state *acrtc_state = to_dm_crtc_state(
- acrtc->base.state);
- if (acrtc_state->stream == NULL) {
+ if (acrtc->dm_irq_params.stream == NULL) {
DRM_ERROR("dc_stream_state is NULL for crtc '%d'!\n",
crtc);
return 0;
@@ -228,7 +223,7 @@ static int dm_crtc_get_scanoutpos(struct amdgpu_device *adev, int crtc,
* TODO rework base driver to use values directly.
* for now parse it back into reg-format
*/
- dc_stream_get_scanoutpos(acrtc_state->stream,
+ dc_stream_get_scanoutpos(acrtc->dm_irq_params.stream,
&v_blank_start,
&v_blank_end,
&h_position,
@@ -287,6 +282,14 @@ get_crtc_by_otg_inst(struct amdgpu_device *adev,
return NULL;
}
+static inline bool amdgpu_dm_vrr_active_irq(struct amdgpu_crtc *acrtc)
+{
+ return acrtc->dm_irq_params.freesync_config.state ==
+ VRR_STATE_ACTIVE_VARIABLE ||
+ acrtc->dm_irq_params.freesync_config.state ==
+ VRR_STATE_ACTIVE_FIXED;
+}
+
static inline bool amdgpu_dm_vrr_active(struct dm_crtc_state *dm_state)
{
return dm_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE ||
@@ -307,7 +310,6 @@ static void dm_pflip_high_irq(void *interrupt_params)
struct amdgpu_device *adev = irq_params->adev;
unsigned long flags;
struct drm_pending_vblank_event *e;
- struct dm_crtc_state *acrtc_state;
uint32_t vpos, hpos, v_blank_start, v_blank_end;
bool vrr_active;
@@ -339,12 +341,11 @@ static void dm_pflip_high_irq(void *interrupt_params)
if (!e)
WARN_ON(1);
- acrtc_state = to_dm_crtc_state(amdgpu_crtc->base.state);
- vrr_active = amdgpu_dm_vrr_active(acrtc_state);
+ vrr_active = amdgpu_dm_vrr_active_irq(amdgpu_crtc);
/* Fixed refresh rate, or VRR scanout position outside front-porch? */
if (!vrr_active ||
- !dc_stream_get_scanoutpos(acrtc_state->stream, &v_blank_start,
+ !dc_stream_get_scanoutpos(amdgpu_crtc->dm_irq_params.stream, &v_blank_start,
&v_blank_end, &hpos, &vpos) ||
(vpos < v_blank_start)) {
/* Update to correct count and vblank timestamp if racing with
@@ -405,17 +406,17 @@ static void dm_vupdate_high_irq(void *interrupt_params)
struct common_irq_params *irq_params = interrupt_params;
struct amdgpu_device *adev = irq_params->adev;
struct amdgpu_crtc *acrtc;
- struct dm_crtc_state *acrtc_state;
unsigned long flags;
+ int vrr_active;
acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VUPDATE);
if (acrtc) {
- acrtc_state = to_dm_crtc_state(acrtc->base.state);
+ vrr_active = amdgpu_dm_vrr_active_irq(acrtc);
DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d\n",
acrtc->crtc_id,
- amdgpu_dm_vrr_active(acrtc_state));
+ vrr_active);
/* Core vblank handling is done here after end of front-porch in
* vrr mode, as vblank timestamping will give valid results
@@ -423,22 +424,22 @@ static void dm_vupdate_high_irq(void *interrupt_params)
* page-flip completion events that have been queued to us
* if a pageflip happened inside front-porch.
*/
- if (amdgpu_dm_vrr_active(acrtc_state)) {
+ if (vrr_active) {
drm_crtc_handle_vblank(&acrtc->base);
/* BTR processing for pre-DCE12 ASICs */
- if (acrtc_state->stream &&
+ if (acrtc->dm_irq_params.stream &&
adev->family < AMDGPU_FAMILY_AI) {
spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
mod_freesync_handle_v_update(
adev->dm.freesync_module,
- acrtc_state->stream,
- &acrtc_state->vrr_params);
+ acrtc->dm_irq_params.stream,
+ &acrtc->dm_irq_params.vrr_params);
dc_stream_adjust_vmin_vmax(
adev->dm.dc,
- acrtc_state->stream,
- &acrtc_state->vrr_params.adjust);
+ acrtc->dm_irq_params.stream,
+ &acrtc->dm_irq_params.vrr_params.adjust);
spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
}
}
@@ -457,18 +458,17 @@ static void dm_crtc_high_irq(void *interrupt_params)
struct common_irq_params *irq_params = interrupt_params;
struct amdgpu_device *adev = irq_params->adev;
struct amdgpu_crtc *acrtc;
- struct dm_crtc_state *acrtc_state;
unsigned long flags;
+ int vrr_active;
acrtc = get_crtc_by_otg_inst(adev, irq_params->irq_src - IRQ_TYPE_VBLANK);
if (!acrtc)
return;
- acrtc_state = to_dm_crtc_state(acrtc->base.state);
+ vrr_active = amdgpu_dm_vrr_active_irq(acrtc);
DRM_DEBUG_VBL("crtc:%d, vupdate-vrr:%d, planes:%d\n", acrtc->crtc_id,
- amdgpu_dm_vrr_active(acrtc_state),
- acrtc_state->active_planes);
+ vrr_active, acrtc->dm_irq_params.active_planes);
/**
* Core vblank handling at start of front-porch is only possible
@@ -476,7 +476,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
* valid results while done in front-porch. Otherwise defer it
* to dm_vupdate_high_irq after end of front-porch.
*/
- if (!amdgpu_dm_vrr_active(acrtc_state))
+ if (!vrr_active)
drm_crtc_handle_vblank(&acrtc->base);
/**
@@ -491,14 +491,16 @@ static void dm_crtc_high_irq(void *interrupt_params)
spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
- if (acrtc_state->stream && acrtc_state->vrr_params.supported &&
- acrtc_state->freesync_config.state == VRR_STATE_ACTIVE_VARIABLE) {
+ if (acrtc->dm_irq_params.stream &&
+ acrtc->dm_irq_params.vrr_params.supported &&
+ acrtc->dm_irq_params.freesync_config.state ==
+ VRR_STATE_ACTIVE_VARIABLE) {
mod_freesync_handle_v_update(adev->dm.freesync_module,
- acrtc_state->stream,
- &acrtc_state->vrr_params);
+ acrtc->dm_irq_params.stream,
+ &acrtc->dm_irq_params.vrr_params);
- dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc_state->stream,
- &acrtc_state->vrr_params.adjust);
+ dc_stream_adjust_vmin_vmax(adev->dm.dc, acrtc->dm_irq_params.stream,
+ &acrtc->dm_irq_params.vrr_params.adjust);
}
/*
@@ -513,7 +515,7 @@ static void dm_crtc_high_irq(void *interrupt_params)
*/
if (adev->family >= AMDGPU_FAMILY_RV &&
acrtc->pflip_status == AMDGPU_FLIP_SUBMITTED &&
- acrtc_state->active_planes == 0) {
+ acrtc->dm_irq_params.active_planes == 0) {
if (acrtc->event) {
drm_crtc_send_vblank_event(&acrtc->base, acrtc->event);
acrtc->event = NULL;
@@ -4845,7 +4847,6 @@ dm_crtc_duplicate_state(struct drm_crtc *crtc)
}
state->active_planes = cur->active_planes;
- state->vrr_params = cur->vrr_params;
state->vrr_infopacket = cur->vrr_infopacket;
state->abm_level = cur->abm_level;
state->vrr_supported = cur->vrr_supported;
@@ -6913,6 +6914,7 @@ static void update_freesync_state_on_stream(
struct mod_vrr_params vrr_params;
struct dc_info_packet vrr_infopacket = {0};
struct amdgpu_device *adev = dm->adev;
+ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(new_crtc_state->base.crtc);
unsigned long flags;
if (!new_stream)
@@ -6927,7 +6929,7 @@ static void update_freesync_state_on_stream(
return;
spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
- vrr_params = new_crtc_state->vrr_params;
+ vrr_params = acrtc->dm_irq_params.vrr_params;
if (surface) {
mod_freesync_handle_preflip(
@@ -6958,7 +6960,7 @@ static void update_freesync_state_on_stream(
&vrr_infopacket);
new_crtc_state->freesync_timing_changed |=
- (memcmp(&new_crtc_state->vrr_params.adjust,
+ (memcmp(&acrtc->dm_irq_params.vrr_params.adjust,
&vrr_params.adjust,
sizeof(vrr_params.adjust)) != 0);
@@ -6967,10 +6969,10 @@ static void update_freesync_state_on_stream(
&vrr_infopacket,
sizeof(vrr_infopacket)) != 0);
- new_crtc_state->vrr_params = vrr_params;
+ acrtc->dm_irq_params.vrr_params = vrr_params;
new_crtc_state->vrr_infopacket = vrr_infopacket;
- new_stream->adjust = new_crtc_state->vrr_params.adjust;
+ new_stream->adjust = acrtc->dm_irq_params.vrr_params.adjust;
new_stream->vrr_infopacket = vrr_infopacket;
if (new_crtc_state->freesync_vrr_info_changed)
@@ -6982,7 +6984,7 @@ static void update_freesync_state_on_stream(
spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
}
-static void pre_update_freesync_state_on_stream(
+static void update_stream_irq_parameters(
struct amdgpu_display_manager *dm,
struct dm_crtc_state *new_crtc_state)
{
@@ -6990,6 +6992,7 @@ static void pre_update_freesync_state_on_stream(
struct mod_vrr_params vrr_params;
struct mod_freesync_config config = new_crtc_state->freesync_config;
struct amdgpu_device *adev = dm->adev;
+ struct amdgpu_crtc *acrtc = to_amdgpu_crtc(new_crtc_state->base.crtc);
unsigned long flags;
if (!new_stream)
@@ -7003,7 +7006,7 @@ static void pre_update_freesync_state_on_stream(
return;
spin_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);
- vrr_params = new_crtc_state->vrr_params;
+ vrr_params = acrtc->dm_irq_params.vrr_params;
if (new_crtc_state->vrr_supported &&
config.min_refresh_in_uhz &&
@@ -7020,11 +7023,14 @@ static void pre_update_freesync_state_on_stream(
&config, &vrr_params);
new_crtc_state->freesync_timing_changed |=
- (memcmp(&new_crtc_state->vrr_params.adjust,
- &vrr_params.adjust,
- sizeof(vrr_params.adjust)) != 0);
+ (memcmp(&acrtc->dm_irq_params.vrr_params.adjust,
+ &vrr_params.adjust, sizeof(vrr_params.adjust)) != 0);
- new_crtc_state->vrr_params = vrr_params;
+ new_crtc_state->freesync_config = config;
+ /* Copy state for access from DM IRQ handler */
+ acrtc->dm_irq_params.freesync_config = config;
+ acrtc->dm_irq_params.active_planes = new_crtc_state->active_planes;
+ acrtc->dm_irq_params.vrr_params = vrr_params;
spin_unlock_irqrestore(&adev_to_drm(adev)->event_lock, flags);
}
@@ -7332,7 +7338,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
spin_lock_irqsave(&pcrtc->dev->event_lock, flags);
dc_stream_adjust_vmin_vmax(
dm->dc, acrtc_state->stream,
- &acrtc_state->vrr_params.adjust);
+ &acrtc_attach->dm_irq_params.vrr_params.adjust);
spin_unlock_irqrestore(&pcrtc->dev->event_lock, flags);
}
mutex_lock(&dm->dc_lock);
@@ -7545,6 +7551,7 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
struct dm_crtc_state *dm_old_crtc_state, *dm_new_crtc_state;
int crtc_disable_count = 0;
bool mode_set_reset_required = false;
+ struct amdgpu_crtc *acrtc;
drm_atomic_helper_update_legacy_modeset_state(dev, state);
@@ -7654,7 +7661,6 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
if (!status)
status = dc_stream_get_status_from_state(dc_state,
dm_new_crtc_state->stream);
-
if (!status)
DC_ERR("got no status for stream %p on acrtc%p\n", dm_new_crtc_state->stream, acrtc);
else
@@ -7780,8 +7786,8 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
- /* Update freesync active state. */
- pre_update_freesync_state_on_stream(dm, dm_new_crtc_state);
+ /* For freesync config update on crtc state and params for irq */
+ update_stream_irq_parameters(dm, dm_new_crtc_state);
/* Handle vrr on->off / off->on transitions */
amdgpu_dm_handle_vrr_transition(dm_old_crtc_state,
@@ -7797,10 +7803,15 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
+ dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
+
if (new_crtc_state->active &&
(!old_crtc_state->active ||
drm_atomic_crtc_needs_modeset(new_crtc_state))) {
+ dc_stream_retain(dm_new_crtc_state->stream);
+ acrtc->dm_irq_params.stream = dm_new_crtc_state->stream;
manage_dm_interrupts(adev, acrtc, true);
+
#ifdef CONFIG_DEBUG_FS
/**
* Frontend may have changed so reapply the CRC capture
@@ -8044,8 +8055,6 @@ static void reset_freesync_config_for_crtc(
{
new_crtc_state->vrr_supported = false;
- memset(&new_crtc_state->vrr_params, 0,
- sizeof(new_crtc_state->vrr_params));
memset(&new_crtc_state->vrr_infopacket, 0,
sizeof(new_crtc_state->vrr_infopacket));
}
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 4da7cd065ba0..6d4a751a389f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -434,7 +434,6 @@ struct dm_crtc_state {
bool vrr_supported;
struct mod_freesync_config freesync_config;
- struct mod_vrr_params vrr_params;
struct dc_info_packet vrr_infopacket;
int abm_level;
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
index 55ef237eed8b..45825a34f8eb 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq_params.h
@@ -28,6 +28,10 @@
struct dm_irq_params {
u32 last_flip_vblank;
+ struct mod_vrr_params vrr_params;
+ struct dc_stream_state *stream;
+ int active_planes;
+ struct mod_freesync_config freesync_config;
};
#endif /* __AMDGPU_DM_IRQ_PARAMS_H__ */
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 3/3] drm/amd/display: Move disable interrupt into commit tail
2020-09-11 16:27 [PATCH v2 0/3] Refactor DM IRQ handling Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai
@ 2020-09-11 16:27 ` Aurabindo Pillai
2020-09-11 17:08 ` Kazlauskas, Nicholas
2 siblings, 1 reply; 5+ messages in thread
From: Aurabindo Pillai @ 2020-09-11 16:27 UTC (permalink / raw)
To: amd-gfx; +Cc: nicholas.kazlauskas
[Why&How]
Since there is no need for accessing crtc state in the interrupt
handler, interrupts need not be disabled well in advance, and
can be moved to commit_tail where it should be.
Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
---
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 ++++++-------------
1 file changed, 13 insertions(+), 28 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 1455acf5beca..b5af1f692499 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7488,34 +7488,6 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
struct drm_atomic_state *state,
bool nonblock)
{
- struct drm_crtc *crtc;
- struct drm_crtc_state *old_crtc_state, *new_crtc_state;
- struct amdgpu_device *adev = drm_to_adev(dev);
- int i;
-
- /*
- * 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 amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
-
- if (old_crtc_state->active &&
- (!new_crtc_state->active ||
- drm_atomic_crtc_needs_modeset(new_crtc_state)))
- manage_dm_interrupts(adev, acrtc, false);
- }
/*
* Add check here for SoC's that support hardware cursor plane, to
* unset legacy_cursor_update
@@ -7566,6 +7538,19 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
dc_resource_state_copy_construct_current(dm->dc, dc_state);
}
+ for_each_oldnew_crtc_in_state (state, crtc, old_crtc_state,
+ new_crtc_state, i) {
+ acrtc = to_amdgpu_crtc(crtc);
+ dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
+
+ if (old_crtc_state->active &&
+ (!new_crtc_state->active ||
+ drm_atomic_crtc_needs_modeset(new_crtc_state))) {
+ manage_dm_interrupts(adev, acrtc, false);
+ dc_stream_release(dm_old_crtc_state->stream);
+ }
+ }
+
/* update changed items */
for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
--
2.25.1
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 3/3] drm/amd/display: Move disable interrupt into commit tail
2020-09-11 16:27 ` [PATCH v2 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai
@ 2020-09-11 17:08 ` Kazlauskas, Nicholas
0 siblings, 0 replies; 5+ messages in thread
From: Kazlauskas, Nicholas @ 2020-09-11 17:08 UTC (permalink / raw)
To: Aurabindo Pillai, amd-gfx
On 2020-09-11 12:27 p.m., Aurabindo Pillai wrote:
> [Why&How]
> Since there is no need for accessing crtc state in the interrupt
> handler, interrupts need not be disabled well in advance, and
> can be moved to commit_tail where it should be.
>
> Signed-off-by: Aurabindo Pillai <aurabindo.pillai@amd.com>
> ---
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 ++++++-------------
> 1 file changed, 13 insertions(+), 28 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 1455acf5beca..b5af1f692499 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7488,34 +7488,6 @@ static int amdgpu_dm_atomic_commit(struct drm_device *dev,
> struct drm_atomic_state *state,
> bool nonblock)
> {
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> - struct amdgpu_device *adev = drm_to_adev(dev);
> - int i;
> -
> - /*
> - * 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 amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
> -
> - if (old_crtc_state->active &&
> - (!new_crtc_state->active ||
> - drm_atomic_crtc_needs_modeset(new_crtc_state)))
> - manage_dm_interrupts(adev, acrtc, false);
> - }
> /*
> * Add check here for SoC's that support hardware cursor plane, to
> * unset legacy_cursor_update
> @@ -7566,6 +7538,19 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
> dc_resource_state_copy_construct_current(dm->dc, dc_state);
> }
>
> + for_each_oldnew_crtc_in_state (state, crtc, old_crtc_state,
> + new_crtc_state, i) {
> + acrtc = to_amdgpu_crtc(crtc);
> + dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
> +
> + if (old_crtc_state->active &&
> + (!new_crtc_state->active ||
> + drm_atomic_crtc_needs_modeset(new_crtc_state))) {
> + manage_dm_interrupts(adev, acrtc, false);
> + dc_stream_release(dm_old_crtc_state->stream);
Please include this dc_stream_release() in patch #2 as well to prevent
memory leaks during bisections.
With that change, this series is Reviewed-by: Nicholas Kazlauskas
<nicholas.kazlauskas@amd.com>
Regards,
Nicholas Kazlauskas
> + }
> + }
> +
> /* update changed items */
> for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-11 17:08 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-11 16:27 [PATCH v2 0/3] Refactor DM IRQ handling Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai
2020-09-11 16:27 ` [PATCH v2 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai
2020-09-11 17:08 ` Kazlauskas, Nicholas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.