* [PATCH 0/3] Refactor DM IRQ handling
@ 2020-09-09 14:28 Aurabindo Pillai
2020-09-09 14:28 ` [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Aurabindo Pillai @ 2020-09-09 14:28 UTC (permalink / raw)
To: amd-gfx; +Cc: nicholas.kazlauskas
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 | 157 +++++++++---------
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 -
.../display/amdgpu_dm/amdgpu_dm_irq_params.h | 37 +++++
4 files changed, 116 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] 8+ messages in thread* [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct 2020-09-09 14:28 [PATCH 0/3] Refactor DM IRQ handling Aurabindo Pillai @ 2020-09-09 14:28 ` Aurabindo Pillai 2020-09-09 14:58 ` Kazlauskas, Nicholas 2020-09-09 14:28 ` [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai 2020-09-09 14:28 ` [PATCH 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai 2 siblings, 1 reply; 8+ messages in thread From: Aurabindo Pillai @ 2020-09-09 14:28 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] 8+ messages in thread
* Re: [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct 2020-09-09 14:28 ` [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai @ 2020-09-09 14:58 ` Kazlauskas, Nicholas 0 siblings, 0 replies; 8+ messages in thread From: Kazlauskas, Nicholas @ 2020-09-09 14:58 UTC (permalink / raw) To: Aurabindo Pillai, amd-gfx On 2020-09-09 10:28 a.m., Aurabindo Pillai wrote: > [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> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com> Regards, Nicholas Kazlauskas > --- > 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__ */ > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler 2020-09-09 14:28 [PATCH 0/3] Refactor DM IRQ handling Aurabindo Pillai 2020-09-09 14:28 ` [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai @ 2020-09-09 14:28 ` Aurabindo Pillai 2020-09-09 15:00 ` Kazlauskas, Nicholas 2020-09-09 14:28 ` [PATCH 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai 2 siblings, 1 reply; 8+ messages in thread From: Aurabindo Pillai @ 2020-09-09 14:28 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 | 115 ++++++++++-------- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 - .../display/amdgpu_dm/amdgpu_dm_irq_params.h | 4 + 3 files changed, 68 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..0603436a3313 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); @@ -7651,9 +7658,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) const struct dc_stream_status *status = dc_stream_get_status(dm_new_crtc_state->stream); - if (!status) + if (!status) { status = dc_stream_get_status_from_state(dc_state, dm_new_crtc_state->stream); + dc_stream_retain(dm_new_crtc_state->stream); + acrtc->dm_irq_params.stream = 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); @@ -7780,8 +7790,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 +7807,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 +8059,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] 8+ messages in thread
* Re: [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler 2020-09-09 14:28 ` [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai @ 2020-09-09 15:00 ` Kazlauskas, Nicholas 2020-09-11 16:28 ` Aurabindo Pillai 0 siblings, 1 reply; 8+ messages in thread From: Kazlauskas, Nicholas @ 2020-09-09 15:00 UTC (permalink / raw) To: Aurabindo Pillai, amd-gfx On 2020-09-09 10:28 a.m., Aurabindo Pillai wrote: > [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 | 115 ++++++++++-------- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 1 - > .../display/amdgpu_dm/amdgpu_dm_irq_params.h | 4 + > 3 files changed, 68 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..0603436a3313 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); > > @@ -7651,9 +7658,12 @@ static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state) > const struct dc_stream_status *status = > dc_stream_get_status(dm_new_crtc_state->stream); > > - if (!status) > + if (!status) { > status = dc_stream_get_status_from_state(dc_state, > dm_new_crtc_state->stream); > + dc_stream_retain(dm_new_crtc_state->stream); You're missing a release on this reference (dc_stream_release) so this will cause a memory leak. > + acrtc->dm_irq_params.stream = 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); > @@ -7780,8 +7790,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 +7807,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); This retain is also missing a dc_stream_release. Regards, Nicholas Kazlauskas > + 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 +8059,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__ */ > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler 2020-09-09 15:00 ` Kazlauskas, Nicholas @ 2020-09-11 16:28 ` Aurabindo Pillai 0 siblings, 0 replies; 8+ messages in thread From: Aurabindo Pillai @ 2020-09-11 16:28 UTC (permalink / raw) To: Kazlauskas, Nicholas, amd-gfx [-- Attachment #1.1.1: Type: text/plain, Size: 15189 bytes --] On Wed, 2020-09-09 at 11:00 -0400, Kazlauskas, Nicholas wrote: > On 2020-09-09 10:28 a.m., Aurabindo Pillai wrote: > > [Why&How]Currently commit_tail holds global locks and wait for > > dependencies which isagainst the DRM API contracts. Inorder to fix > > this, IRQ handler should be ableto run without having to access > > crtc state. Required parameters are copied overso 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 | 115 > > ++++++++++-------- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h > > | 1 - .../display/amdgpu_dm/amdgpu_dm_irq_params.h | 4 + 3 > > files changed, 68 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.cindex > > 40814cdd8c92..0603436a3313 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 { str > > uct 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; els > > e { 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_stre > > am_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_STAT > > E_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_irqsa > > ve(&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; - acr > > tc_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_lo > > ck_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_A > > CTIVE_VARIABLE) { mod_freesync_handle_v_update(adev- > > >dm.freesync_module,- ac > > rtc_state->stream,- &acrtc_sta > > te->vrr_params);+ acrtc- > > >dm_irq_params.stream,+ &a > > crtc->dm_irq_params.vrr_params); - dc_stream_adjust_vmin_v > > max(adev->dm.dc, acrtc_state->stream,- > > &acrtc_state->vrr_params.adjust);+ dc_stream_a > > djust_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_ev > > ent(&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_cr > > tc_state->vrr_params.adjust,+ (memcmp(&acrtc- > > >dm_irq_params.vrr_params.adjust, &vrr_params.adj > > ust, sizeof(vrr_params.adjust)) != 0); @@ > > -6967,10 +6969,10 @@ static void update_freesync_state_on_stream( > > &vrr_infopacket, sizeof(vrr_infopack > > et)) != 0); - new_crtc_state->vrr_params = vrr_params;+ acr > > tc->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; spi > > n_lock_irqsave(&adev_to_drm(adev)->event_lock, flags);- vrr_par > > ams = 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_st > > ate->freesync_timing_changed |=- (memcmp(&new_crtc_state > > ->vrr_params.adjust,- &vrr_params.adjust,- > > sizeof(vrr_params.adjust)) != 0);+ (memcmp(&ac > > rtc->dm_irq_params.vrr_params.adjust,+ &vrr_pa > > rams.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 */+ acr > > tc->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_irq > > restore(&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(&pcrt > > c->dev->event_lock, flags); dc_stream_adjus > > t_vmin_vmax( dm->dc, acrtc_state- > > >stream,- &acrtc_state- > > >vrr_params.adjust);+ &acrtc_attach- > > >dm_irq_params.vrr_params.adjust); spin_un > > lock_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_help > > er_update_legacy_modeset_state(dev, state); @@ -7651,9 +7658,12 @@ > > static void amdgpu_dm_atomic_commit_tail(struct drm_atomic_state > > *state) const struct dc_stream_status *status > > = dc_stream_get_status(dm_new_crt > > c_state->stream); - if (!status)+ > > if (!status) { status = > > dc_stream_get_status_from_state(dc_state, > > dm_new_crtc_state->stream);+ > > dc_stream_retain(dm_new_crtc_state->stream); > > You're missing a release on this reference (dc_stream_release) so > this will cause a memory leak. > > + acrtc->dm_irq_params.stream = > > 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);@@ > > -7780,8 +7790,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_st > > ream(dm, dm_new_crtc_state);+ /* For freesync config > > update on crtc state and params for irq */+ update_stream_i > > rq_parameters(dm, dm_new_crtc_state); /* Handle vrr > > on->off / off->on transitions */ amdgpu_dm_handle_vrr_tr > > ansition(dm_old_crtc_state,@@ -7797,10 +7807,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_st > > ate->stream); > > This retain is also missing a dc_stream_release. > Regards,Nicholas Kazlauskas > Hi Nick, Thanks for the review, I've posted V2. [-- Attachment #1.1.2: Type: text/html, Size: 19181 bytes --] [-- Attachment #1.2: This is a digitally signed message part --] [-- Type: application/pgp-signature, Size: 659 bytes --] [-- Attachment #2: Type: text/plain, Size: 154 bytes --] _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] drm/amd/display: Move disable interrupt into commit tail 2020-09-09 14:28 [PATCH 0/3] Refactor DM IRQ handling Aurabindo Pillai 2020-09-09 14:28 ` [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai 2020-09-09 14:28 ` [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai @ 2020-09-09 14:28 ` Aurabindo Pillai 2020-09-09 15:01 ` Kazlauskas, Nicholas 2 siblings, 1 reply; 8+ messages in thread From: Aurabindo Pillai @ 2020-09-09 14:28 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 | 38 +++++-------------- 1 file changed, 10 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 0603436a3313..a40de242e04a 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,16 @@ 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); + + if (old_crtc_state->active && + (!new_crtc_state->active || + drm_atomic_crtc_needs_modeset(new_crtc_state))) + manage_dm_interrupts(adev, acrtc, false); + } + /* 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] 8+ messages in thread
* Re: [PATCH 3/3] drm/amd/display: Move disable interrupt into commit tail 2020-09-09 14:28 ` [PATCH 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai @ 2020-09-09 15:01 ` Kazlauskas, Nicholas 0 siblings, 0 replies; 8+ messages in thread From: Kazlauskas, Nicholas @ 2020-09-09 15:01 UTC (permalink / raw) To: Aurabindo Pillai, amd-gfx On 2020-09-09 10:28 a.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> This patch is fine in idea, but it'll need the reference counting fixed in patch #2 first and be adjusted after. Regards, Nicholas Kazlauskas > --- > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 38 +++++-------------- > 1 file changed, 10 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 0603436a3313..a40de242e04a 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,16 @@ 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); > + > + if (old_crtc_state->active && > + (!new_crtc_state->active || > + drm_atomic_crtc_needs_modeset(new_crtc_state))) > + manage_dm_interrupts(adev, acrtc, false); > + } > + > /* 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] 8+ messages in thread
end of thread, other threads:[~2020-09-11 16:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-09-09 14:28 [PATCH 0/3] Refactor DM IRQ handling Aurabindo Pillai 2020-09-09 14:28 ` [PATCH 1/3] drm/amdgpu: Move existing pflip fields into separate struct Aurabindo Pillai 2020-09-09 14:58 ` Kazlauskas, Nicholas 2020-09-09 14:28 ` [PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler Aurabindo Pillai 2020-09-09 15:00 ` Kazlauskas, Nicholas 2020-09-11 16:28 ` Aurabindo Pillai 2020-09-09 14:28 ` [PATCH 3/3] drm/amd/display: Move disable interrupt into commit tail Aurabindo Pillai 2020-09-09 15:01 ` Kazlauskas, Nicholas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox