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 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

-- 
Jani Nikula, Intel Open Source Graphics Center