On Mon, Mar 25, 2024 at 11:44:59AM -0300, Gustavo Sousa wrote:
> Quoting Ville Syrjälä (2024-03-22 14:45:29-03:00)
> >On Fri, Mar 22, 2024 at 01:40:44PM +0200, Stanislav Lisovskiy wrote:
> >> In order to make sure we are not breaking the proper sequence
> >> lets to updates step by step and don't change MBUS join value
> >> during MDCLK/CDCLK programming stage.
> >> MBUS join programming would be taken care by pre/post ddb hooks.
> >>
> >> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> >> ---
> >> drivers/gpu/drm/i915/display/intel_cdclk.c | 12 +++++++++++-
> >> 1 file changed, 11 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> index 31aaa9780dfcf..43a9616c78260 100644
> >> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> >> @@ -2611,9 +2611,19 @@ intel_set_cdclk_pre_plane_update(struct intel_atomic_state *state)
> >>
> >> if (pipe == INVALID_PIPE ||
> >> old_cdclk_state->actual.cdclk <= new_cdclk_state->actual.cdclk) {
> >> + struct intel_cdclk_config cdclk_config;
> >> +
> >> drm_WARN_ON(&i915->drm, !new_cdclk_state->base.changed);
> >>
> >> - intel_set_cdclk(i915, &new_cdclk_state->actual, pipe);
> >> + /*
> >> + * By this hack we want to prevent mbus_join to be programmed
> >> + * beforehand - we will take care of this later in pre ddb
> >> + * programming hook.
> >> + */
> >
> >We're not doing anything to prevent mbus joining to be
> >programmed here. It will simply not be programmed here,
> >which is why we need to use the old mbus_join based ratio.
>
> Hey, guys.
>
> Just so I understand this better. What I understood from the
> recent discussion was:
>
> CDCLK programming should only care about the current MBus joining
> state and not consider the new one in the current hardware commit,
> which must actually be handled later in the sequence by the proper
> "entity".
>
> Is my understanding correct? If so, sorry for the confusion introduced
> by my patches.
>
> My previous understanding was that that the CDCLK change sequence would
> need to consider the new MBus joining state in case we were in a modeset
> where mbus joining changed, so I added that odd-looking condition in
> update_mbus_pre_enable() (not moved into
> intel_dbuf_mdclk_min_tracker_update()), thinking that the update should
> be handled by the cdclk sequence.
I don't think we can handle it from the cdclk code as
that can't handle the proper ordering between plane
ddb updates vs. mbus_join changes.
It's rather infuriating that we need to update the ratio
from both places. I'm not sure how careful we actually
have to be between programming the ratio vs. actually
changing mbus_join+mdclk/cdclk. I guess we should ask
the hardware folks for more details and if the sequence
doesn't have to super accurate then maybe think about
a simpler way to do things...
--
Ville Syrjälä
Intel
