| Message ID | 20230809082918.18631-1-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: Fix mistake in intel_modeset_all_pipes condition | expand |
On Wed, 09 Aug 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > It is supposed to be "!intel_crtc_needs_modeset" - otherwise, > we are active exactly vice versa for active pipes: skipping if modeset > is needed and not skipping if not needed. If the crtc *already* needs a full modeset, there's no need to force a modeset on it. BR, Jani. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > index 763ab569d8f32..4b1d7034078ca 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -5439,7 +5439,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state, > return PTR_ERR(crtc_state); > > if (!crtc_state->hw.active || > - intel_crtc_needs_modeset(crtc_state)) > + !intel_crtc_needs_modeset(crtc_state)) > continue; > > drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n",
On Wed, Aug 09, 2023 at 11:38:08AM +0300, Jani Nikula wrote: > On Wed, 09 Aug 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > > It is supposed to be "!intel_crtc_needs_modeset" - otherwise, > > we are active exactly vice versa for active pipes: skipping if modeset > > is needed and not skipping if not needed. > > If the crtc *already* needs a full modeset, there's no need to force a > modeset on it. > > BR, > Jani. We have curently some issue with that. There are multiple places from where intel_modeset_all_pipes is called. One is that when we do detect that mbus join state had changed. All the previous checks indicated that fastset is enough, however once we detect mbus join state had changed in skl_watermarks.c we call this function there as well and I think it might act in a wrong way then. So basically this condition checks whether we need to force a modeset, but not if we need it, so no crtc's are supposed to escape this? Could be then that we just calling that whole function there wrongly. Stan > > > > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > > index 763ab569d8f32..4b1d7034078ca 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -5439,7 +5439,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state, > > return PTR_ERR(crtc_state); > > > > if (!crtc_state->hw.active || > > - intel_crtc_needs_modeset(crtc_state)) > > + !intel_crtc_needs_modeset(crtc_state)) > > continue; > > > > drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n", > > -- > Jani Nikula, Intel Open Source Graphics Center
On Wed, 09 Aug 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > On Wed, Aug 09, 2023 at 11:38:08AM +0300, Jani Nikula wrote: >> On Wed, 09 Aug 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: >> > It is supposed to be "!intel_crtc_needs_modeset" - otherwise, >> > we are active exactly vice versa for active pipes: skipping if modeset >> > is needed and not skipping if not needed. >> >> If the crtc *already* needs a full modeset, there's no need to force a >> modeset on it. >> >> BR, >> Jani. > > We have curently some issue with that. There are multiple places from where > intel_modeset_all_pipes is called. One is that when we do detect that mbus join > state had changed. All the previous checks indicated that fastset is enough, > however once we detect mbus join state had changed in skl_watermarks.c we call > this function there as well and I think it might act in a wrong way then. > > So basically this condition checks whether we need to force a modeset, but not > if we need it, so no crtc's are supposed to escape this? > Could be then that we just calling that whole function there wrongly. Simplified, it's an optimization of: if (crtc_state->uapi.mode_changed) continue; crtc_state->uapi.mode_changed = true; With your change, it becomes: if (!crtc_state->uapi.mode_changed) continue; crtc_state->uapi.mode_changed = true; and intel_modeset_all_pipes() roughly becomes a no-op. BR, Jani. > > Stan > >> >> > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> > --- >> > drivers/gpu/drm/i915/display/intel_display.c | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> > index 763ab569d8f32..4b1d7034078ca 100644 >> > --- a/drivers/gpu/drm/i915/display/intel_display.c >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c >> > @@ -5439,7 +5439,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state, >> > return PTR_ERR(crtc_state); >> > >> > if (!crtc_state->hw.active || >> > - intel_crtc_needs_modeset(crtc_state)) >> > + !intel_crtc_needs_modeset(crtc_state)) >> > continue; >> > >> > drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n", >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
On Wed, Aug 09, 2023 at 12:01:25PM +0300, Jani Nikula wrote: > On Wed, 09 Aug 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > > On Wed, Aug 09, 2023 at 11:38:08AM +0300, Jani Nikula wrote: > >> On Wed, 09 Aug 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: > >> > It is supposed to be "!intel_crtc_needs_modeset" - otherwise, > >> > we are active exactly vice versa for active pipes: skipping if modeset > >> > is needed and not skipping if not needed. > >> > >> If the crtc *already* needs a full modeset, there's no need to force a > >> modeset on it. > >> > >> BR, > >> Jani. > > > > We have curently some issue with that. There are multiple places from where > > intel_modeset_all_pipes is called. One is that when we do detect that mbus join > > state had changed. All the previous checks indicated that fastset is enough, > > however once we detect mbus join state had changed in skl_watermarks.c we call > > this function there as well and I think it might act in a wrong way then. > > > > So basically this condition checks whether we need to force a modeset, but not > > if we need it, so no crtc's are supposed to escape this? > > Could be then that we just calling that whole function there wrongly. > > Simplified, it's an optimization of: > > if (crtc_state->uapi.mode_changed) > continue; > > crtc_state->uapi.mode_changed = true; > > With your change, it becomes: > > if (!crtc_state->uapi.mode_changed) > continue; > > crtc_state->uapi.mode_changed = true; > > and intel_modeset_all_pipes() roughly becomes a no-op. Okay, basically I was wrong in interpretation of intel_modeset_all_pipes - mine was that it is supposed to modeset only pipes, which actually _need_ a full modeset, while the real one is supposed to force a modeset on those which even don't need that. Regarding mbus join, I think it could be just wrong to call it there rightaway. Most likely we can live with fastset there, unless ddb allocations haven't changed(we could then just update the mbus join state) Stan > > > BR, > Jani. > > > > > Stan > > > >> > >> > > >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > >> > --- > >> > drivers/gpu/drm/i915/display/intel_display.c | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c > >> > index 763ab569d8f32..4b1d7034078ca 100644 > >> > --- a/drivers/gpu/drm/i915/display/intel_display.c > >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c > >> > @@ -5439,7 +5439,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state, > >> > return PTR_ERR(crtc_state); > >> > > >> > if (!crtc_state->hw.active || > >> > - intel_crtc_needs_modeset(crtc_state)) > >> > + !intel_crtc_needs_modeset(crtc_state)) > >> > continue; > >> > > >> > drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n", > >> > >> -- > >> Jani Nikula, Intel Open Source Graphics Center > > -- > Jani Nikula, Intel Open Source Graphics Center
On Wed, 09 Aug 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: > On Wed, Aug 09, 2023 at 12:01:25PM +0300, Jani Nikula wrote: >> On Wed, 09 Aug 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com> wrote: >> > On Wed, Aug 09, 2023 at 11:38:08AM +0300, Jani Nikula wrote: >> >> On Wed, 09 Aug 2023, Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> wrote: >> >> > It is supposed to be "!intel_crtc_needs_modeset" - otherwise, >> >> > we are active exactly vice versa for active pipes: skipping if modeset >> >> > is needed and not skipping if not needed. >> >> >> >> If the crtc *already* needs a full modeset, there's no need to force a >> >> modeset on it. >> >> >> >> BR, >> >> Jani. >> > >> > We have curently some issue with that. There are multiple places from where >> > intel_modeset_all_pipes is called. One is that when we do detect that mbus join >> > state had changed. All the previous checks indicated that fastset is enough, >> > however once we detect mbus join state had changed in skl_watermarks.c we call >> > this function there as well and I think it might act in a wrong way then. >> > >> > So basically this condition checks whether we need to force a modeset, but not >> > if we need it, so no crtc's are supposed to escape this? >> > Could be then that we just calling that whole function there wrongly. >> >> Simplified, it's an optimization of: >> >> if (crtc_state->uapi.mode_changed) >> continue; >> >> crtc_state->uapi.mode_changed = true; >> >> With your change, it becomes: >> >> if (!crtc_state->uapi.mode_changed) >> continue; >> >> crtc_state->uapi.mode_changed = true; >> >> and intel_modeset_all_pipes() roughly becomes a no-op. > > Okay, basically I was wrong in interpretation of intel_modeset_all_pipes - mine was that it is > supposed to modeset only pipes, which actually _need_ a full modeset, while the real one is supposed > to force a modeset on those which even don't need that. > Regarding mbus join, I think it could be just wrong to call it there rightaway. > Most likely we can live with fastset there, unless ddb allocations haven't changed(we could then just > update the mbus join state) Well it's right there in skl_watermark.c lines 2618 and 3488! ;D /* TODO: Implement vblank synchronized MBUS joining changes */ /* * TODO: Implement vblank synchronized MBUS joining changes. * Must be properly coordinated with dbuf reprogramming. */ added way back when mbus programming was added in commit f4dc00863226 ("drm/i915/adl_p: MBUS programming"). It's not "wrong" per se to do a full modeset, it's just that there's a gap in handling the fastset with mbus joining changes. BR, Jani. > > Stan > >> >> >> BR, >> Jani. >> >> > >> > Stan >> > >> >> >> >> > >> >> > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> >> > --- >> >> > drivers/gpu/drm/i915/display/intel_display.c | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c >> >> > index 763ab569d8f32..4b1d7034078ca 100644 >> >> > --- a/drivers/gpu/drm/i915/display/intel_display.c >> >> > +++ b/drivers/gpu/drm/i915/display/intel_display.c >> >> > @@ -5439,7 +5439,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state, >> >> > return PTR_ERR(crtc_state); >> >> > >> >> > if (!crtc_state->hw.active || >> >> > - intel_crtc_needs_modeset(crtc_state)) >> >> > + !intel_crtc_needs_modeset(crtc_state)) >> >> > continue; >> >> > >> >> > drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n", >> >> >> >> -- >> >> Jani Nikula, Intel Open Source Graphics Center >> >> -- >> Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index 763ab569d8f32..4b1d7034078ca 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -5439,7 +5439,7 @@ int intel_modeset_all_pipes(struct intel_atomic_state *state, return PTR_ERR(crtc_state); if (!crtc_state->hw.active || - intel_crtc_needs_modeset(crtc_state)) + !intel_crtc_needs_modeset(crtc_state)) continue; drm_dbg_kms(&dev_priv->drm, "[CRTC:%d:%s] Full modeset due to %s\n",
It is supposed to be "!intel_crtc_needs_modeset" - otherwise, we are active exactly vice versa for active pipes: skipping if modeset is needed and not skipping if not needed. Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> --- drivers/gpu/drm/i915/display/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)