On Wed, 09 Aug 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@xxxxxxxxx> wrote:
> On Wed, Aug 09, 2023 at 12:01:25PM +0300, Jani Nikula wrote:
>> On Wed, 09 Aug 2023, "Lisovskiy, Stanislav" <stanislav.lisovskiy@xxxxxxxxx> wrote:
>> > On Wed, Aug 09, 2023 at 11:38:08AM +0300, Jani Nikula wrote:
>> >> On Wed, 09 Aug 2023, Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx> 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@xxxxxxxxx>
>> >> > ---
>> >> > 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
