Messages in this thread | | | Date | Fri, 07 Aug 2020 18:43:10 +0530 | From | kalyan_t@codeauro ... | Subject | Re: [Freedreno] [v1] drm/msm/dpu: Fix reservation failures in modeset |
| |
On 2020-08-06 22:15, Rob Clark wrote: > On Thu, Aug 6, 2020 at 7:46 AM <kalyan_t@codeaurora.org> wrote: >> >> 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 ? > > Yeah, I suppose something like: > > if (drm_atomic_crtc_needs_modeset()) { > reserve() > } else if (active_changed && !active) { > release() > } > > I think it could happen (at least with atomic api) that you get a > modeset without active_changed, so we might need to release() and then > reserve() in that case? (This is probably where starting to run more > IGT tests would be useful) > > BR, > -R > Thanks Rob, please review the v2 version. >> -Kalyan >> >> >> >> mutex_unlock(&dpu_enc->enc_lock); >> >> } >> >> -- >> >> 1.9.1 >> >> >> > _______________________________________________ >> > Freedreno mailing list >> > Freedreno@lists.freedesktop.org >> > https://lists.freedesktop.org/mailman/listinfo/freedreno
| |