Re: [PATCH 3/5] drm/i915: Use old mbus_join value when increasing CDCLK

[Gustavo Sousa <gustavo.sousa@intel.com>]

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@intel.com>
>> ---
>>  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.

--
Gustavo Sousa

>
>I would also include the actual function name here instead
>of "pre ddb programming hook" since that's rather vague.
>
>So this could use a bit of rewording. Otherwise lgtm
>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
>> +                cdclk_config = new_cdclk_state->actual;
>> +                cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
>> +
>> +                intel_set_cdclk(i915, &cdclk_config, pipe);
>>          }
>>  }
>>  
>> -- 
>> 2.37.3
>
>-- 
>Ville Syrjälä
>Intel