diff mbox series

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

Message ID 20240322114046.24930-4-stanislav.lisovskiy@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable fastset for mbus_join state change | expand

Commit Message

Lisovskiy, Stanislav March 22, 2024, 11:40 a.m. UTC 
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(-)

Comments

Ville Syrjälä March 22, 2024, 5:45 p.m. UTC | #1 
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.

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
Gustavo Sousa March 25, 2024, 2:44 p.m. UTC | #2 
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
Ville Syrjälä March 25, 2024, 2:55 p.m. UTC | #3 
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@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.

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...
diff mbox series

Patch

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.
+		 */
+		cdclk_config = new_cdclk_state->actual;
+		cdclk_config.joined_mbus = old_cdclk_state->actual.joined_mbus;
+
+		intel_set_cdclk(i915, &cdclk_config, pipe);
 	}
 }