Re: [Freedreno] [v1] drm/msm/dpu: Fix reservation failures in modeset

[kalyan_t@codeaurora.org]

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