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

[Jani Nikula <jani.nikula@intel.com>]

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

-- 
Jani Nikula, Intel Open Source Graphics Center