On Thu, Aug 6, 2020 at 7:46 AM <[email protected]> wrote:
>
> On 2020-08-05 21:18, Rob Clark wrote:
> > On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota <[email protected]>
> > 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 <[email protected]>
> >> ---
> >> 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
> -Kalyan
> >>
> >> mutex_unlock(&dpu_enc->enc_lock);
> >> }
> >> --
> >> 1.9.1
> >>
> > _______________________________________________
> > Freedreno mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
On 2020-08-06 22:15, Rob Clark wrote:
> On Thu, Aug 6, 2020 at 7:46 AM <[email protected]> wrote:
>>
>> On 2020-08-05 21:18, Rob Clark wrote:
>> > On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota <[email protected]>
>> > 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 <[email protected]>
>> >> ---
>> >> 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
>> > [email protected]
>> > https://lists.freedesktop.org/mailman/listinfo/freedreno