Re: [Intel-gfx] [PATCH] drm/i915: Fix mistake in intel_modeset_all_pipes condition

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

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