[PATCH 2/3] drm/amd/display: Refactor to prevent crtc state access in DM IRQ handler
Aurabindo Pillai
aurabindo.pillai at amd.com
Fri Sep 11 16:28:34 UTC 2020
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 at 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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200911/9c4fd455/attachment-0001.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: This is a digitally signed message part
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20200911/9c4fd455/attachment-0001.sig>
More information about the amd-gfx
mailing list