Messages in this thread | | | Date | Thu, 06 Aug 2020 20:15:51 +0530 | From | kalyan_t@codeauro ... | Subject | Re: [Freedreno] [v1] drm/msm/dpu: Fix reservation failures in modeset |
| |
On 2020-08-05 21:18, Rob Clark wrote: > On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota <kalyan_t@codeaurora.org> > wrote: >> >> In TEST_ONLY commit, rm global_state will duplicate the >> object and request for new reservations, once they pass >> then the new state will be swapped with the old and will >> be available for the Atomic Commit. >> >> This patch fixes some of missing links in the resource >> reservation sequence mentioned above. >> >> 1) Creation of a duplicate state in test_only commit (Rob) >> 2) Allow resource release only during crtc_active false. >> >> For #2 >> In a modeset operation, swap state happens well before disable. >> Hence clearing reservations in disable will cause failures >> in modeset enable. >> >> Sequence: >> Swap state --> old, new >> modeset disables --> virt disable >> modeset enable --> virt modeset >> >> Allow reservations to be cleared only when crtc active is false >> as in that case there wont be any modeset enable after disable. >> >> Signed-off-by: Kalyan Thota <kalyan_t@codeaurora.org> >> --- >> drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> index 63976dc..b85a576 100644 >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c >> @@ -582,7 +582,7 @@ static int dpu_encoder_virt_atomic_check( >> dpu_kms = to_dpu_kms(priv->kms); >> mode = &crtc_state->mode; >> adj_mode = &crtc_state->adjusted_mode; >> - global_state = dpu_kms_get_existing_global_state(dpu_kms); >> + global_state = dpu_kms_get_global_state(crtc_state->state); >> trace_dpu_enc_atomic_check(DRMID(drm_enc)); >> >> /* >> @@ -1172,6 +1172,7 @@ static void dpu_encoder_virt_disable(struct >> drm_encoder *drm_enc) >> struct msm_drm_private *priv; >> struct dpu_kms *dpu_kms; >> struct dpu_global_state *global_state; >> + struct drm_crtc_state *crtc_state; >> int i = 0; >> >> if (!drm_enc) { >> @@ -1191,6 +1192,7 @@ static void dpu_encoder_virt_disable(struct >> drm_encoder *drm_enc) >> priv = drm_enc->dev->dev_private; >> dpu_kms = to_dpu_kms(priv->kms); >> global_state = dpu_kms_get_existing_global_state(dpu_kms); >> + crtc_state = drm_enc->crtc->state; >> >> trace_dpu_enc_disable(DRMID(drm_enc)); >> >> @@ -1220,7 +1222,8 @@ static void dpu_encoder_virt_disable(struct >> drm_encoder *drm_enc) >> >> DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n"); >> >> - dpu_rm_release(global_state, drm_enc); >> + if (crtc_state->active_changed && !crtc_state->active) >> + dpu_rm_release(global_state, drm_enc); > > I still think releasing the state in the atomic_commit() path is the > wrong thing to do. In the commit path, the various state objects > should be immutable.. ie. in the atomic_test() path you derive the new > hw state (including assignment/release of resources), and > atomic_commit() is simply pushing the state down to the hw. > > Otherwise, this looks better than v1. > > BR, > -R > okay. Should we avoid reservation all together if active=0 on that crtc and trigger rm_release on the enc during atomic_check ? how do you see the approach ?
-Kalyan >> >> mutex_unlock(&dpu_enc->enc_lock); >> } >> -- >> 1.9.1 >> > _______________________________________________ > Freedreno mailing list > Freedreno@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/freedreno
| |