[v1] drm/msm/dpu: Fix reservation failures in modeset
diff mbox series

Message ID 1596634446-1413-1-git-send-email-kalyan_t@codeaurora.org
State New
Headers show
Series
  • [v1] drm/msm/dpu: Fix reservation failures in modeset
Related show

Commit Message

Kalyan Thota Aug. 5, 2020, 1:34 p.m. UTC
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(-)

Comments

Kalyan Thota Aug. 6, 2020, 2:45 p.m. UTC | #1
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
Rob Clark Aug. 6, 2020, 4:45 p.m. UTC | #2
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

> -Kalyan
> >>
> >>         mutex_unlock(&dpu_enc->enc_lock);
> >>  }
> >> --
> >> 1.9.1
> >>
> > _______________________________________________
> > Freedreno mailing list
> > Freedreno@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/freedreno
Daniel Vetter Aug. 7, 2020, 8:57 a.m. UTC | #3
On Wed, Aug 05, 2020 at 08:48:34AM -0700, 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.

Yeah this races and is total no-go. If you touch your state in commit
(except some very specific exceptions), then a next TEST_ONLY atomic_check
might duplicate the atomic state object in an incosistent state, and fail.

Worse, this looks like if you race like that then you might duplicate an
object with old reservations still in place, and then we've essentially
leaked those resources and need to reboot. Yes most compositors use
blocking modesets, but some actually do full nonblocking modesets.

This stuff needs to be moved into the atomic_check code, and your commit
code needs to be careful to use the right state (old or new) when pushing
it into hw.

Cheers, Daniel

> 
> BR,
> -R
> 
> >
> >         mutex_unlock(&dpu_enc->enc_lock);
> >  }
> > --
> > 1.9.1
> >
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Kalyan Thota Aug. 7, 2020, 1:13 p.m. UTC | #4
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

Patch
diff mbox series

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);
 
 	mutex_unlock(&dpu_enc->enc_lock);
 }