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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 2020-08-06 22:15, Rob Clark wrote:
On Thu, Aug 6, 2020 at 7:46 AM <kalyan_t@xxxxxxxxxxxxxx> wrote:

On 2020-08-05 21:18, Rob Clark wrote:
> On Wed, Aug 5, 2020 at 6:34 AM Kalyan Thota <kalyan_t@xxxxxxxxxxxxxx>
> 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@xxxxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/freedreno



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux