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
