diff mbox series

[v2,05/14] drm/i915: Avoid triggering unwanted cdclk changes due to dbuf bandwidth changes

Message ID 20250326162544.3642-6-ville.syrjala@linux.intel.com (mailing list archive)
State New
Headers show
Series drm/i915: sagv/bw cleanup | expand

Commit Message

Ville Syrjälä March 26, 2025, 4:25 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Currently intel_bw_calc_min_cdclk() always adds the bw_state
to the atomic state. Not only does it result in potentially
redundant work later, it's also currently causing unwanted cdclk
changes during driver load.

Check if the dbuf bw is actually changing before we decide to
pull in the bw state.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_bw.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

Comments

Jani Nikula March 27, 2025, 8 a.m. UTC | #1 
On Wed, 26 Mar 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently intel_bw_calc_min_cdclk() always adds the bw_state
> to the atomic state. Not only does it result in potentially
> redundant work later, it's also currently causing unwanted cdclk
> changes during driver load.

Can you elaborate how we currently end up changing cdclk even if the
dbuf bw isn't changing? Different rules wrt to GOP?

Anyway, the change make sense no matter what,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


>
> Check if the dbuf bw is actually changing before we decide to
> pull in the bw state.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_bw.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 67d088da1f38..19b516084fac 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -1294,7 +1294,8 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
>  	struct intel_bw_state *new_bw_state = NULL;
>  	const struct intel_bw_state *old_bw_state = NULL;
>  	const struct intel_cdclk_state *cdclk_state;
> -	const struct intel_crtc_state *crtc_state;
> +	const struct intel_crtc_state *old_crtc_state;
> +	const struct intel_crtc_state *new_crtc_state;
>  	int old_min_cdclk, new_min_cdclk;
>  	struct intel_crtc *crtc;
>  	int i;
> @@ -1302,15 +1303,23 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
>  	if (DISPLAY_VER(display) < 9)
>  		return 0;
>  
> -	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> +					    new_crtc_state, i) {
> +		struct intel_dbuf_bw old_dbuf_bw, new_dbuf_bw;
> +
> +		skl_crtc_calc_dbuf_bw(&old_dbuf_bw, old_crtc_state);
> +		skl_crtc_calc_dbuf_bw(&new_dbuf_bw, new_crtc_state);
> +
> +		if (!intel_dbuf_bw_changed(display, &old_dbuf_bw, &new_dbuf_bw))
> +			continue;
> +
>  		new_bw_state = intel_atomic_get_bw_state(state);
>  		if (IS_ERR(new_bw_state))
>  			return PTR_ERR(new_bw_state);
>  
>  		old_bw_state = intel_atomic_get_old_bw_state(state);
>  
> -		skl_crtc_calc_dbuf_bw(&new_bw_state->dbuf_bw[crtc->pipe],
> -				      crtc_state);
> +		new_bw_state->dbuf_bw[crtc->pipe] = new_dbuf_bw;
>  	}
>  
>  	if (!old_bw_state)
Ville Syrjälä March 27, 2025, 12:06 p.m. UTC | #2 
On Thu, Mar 27, 2025 at 10:00:19AM +0200, Jani Nikula wrote:
> On Wed, 26 Mar 2025, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently intel_bw_calc_min_cdclk() always adds the bw_state
> > to the atomic state. Not only does it result in potentially
> > redundant work later, it's also currently causing unwanted cdclk
> > changes during driver load.
> 
> Can you elaborate how we currently end up changing cdclk even if the
> dbuf bw isn't changing? Different rules wrt to GOP?

Older GOP versions always use the max cdclk. IIRC newer ones
might be using a more optimal value.

> 
> Anyway, the change make sense no matter what,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> >
> > Check if the dbuf bw is actually changing before we decide to
> > pull in the bw state.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index 67d088da1f38..19b516084fac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -1294,7 +1294,8 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> >  	struct intel_bw_state *new_bw_state = NULL;
> >  	const struct intel_bw_state *old_bw_state = NULL;
> >  	const struct intel_cdclk_state *cdclk_state;
> > -	const struct intel_crtc_state *crtc_state;
> > +	const struct intel_crtc_state *old_crtc_state;
> > +	const struct intel_crtc_state *new_crtc_state;
> >  	int old_min_cdclk, new_min_cdclk;
> >  	struct intel_crtc *crtc;
> >  	int i;
> > @@ -1302,15 +1303,23 @@ int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
> >  	if (DISPLAY_VER(display) < 9)
> >  		return 0;
> >  
> > -	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> > +	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> > +					    new_crtc_state, i) {
> > +		struct intel_dbuf_bw old_dbuf_bw, new_dbuf_bw;
> > +
> > +		skl_crtc_calc_dbuf_bw(&old_dbuf_bw, old_crtc_state);
> > +		skl_crtc_calc_dbuf_bw(&new_dbuf_bw, new_crtc_state);
> > +
> > +		if (!intel_dbuf_bw_changed(display, &old_dbuf_bw, &new_dbuf_bw))
> > +			continue;
> > +
> >  		new_bw_state = intel_atomic_get_bw_state(state);
> >  		if (IS_ERR(new_bw_state))
> >  			return PTR_ERR(new_bw_state);
> >  
> >  		old_bw_state = intel_atomic_get_old_bw_state(state);
> >  
> > -		skl_crtc_calc_dbuf_bw(&new_bw_state->dbuf_bw[crtc->pipe],
> > -				      crtc_state);
> > +		new_bw_state->dbuf_bw[crtc->pipe] = new_dbuf_bw;
> >  	}
> >  
> >  	if (!old_bw_state)
> 
> -- 
> Jani Nikula, Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
index 67d088da1f38..19b516084fac 100644
--- a/drivers/gpu/drm/i915/display/intel_bw.c
+++ b/drivers/gpu/drm/i915/display/intel_bw.c
@@ -1294,7 +1294,8 @@  int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
 	struct intel_bw_state *new_bw_state = NULL;
 	const struct intel_bw_state *old_bw_state = NULL;
 	const struct intel_cdclk_state *cdclk_state;
-	const struct intel_crtc_state *crtc_state;
+	const struct intel_crtc_state *old_crtc_state;
+	const struct intel_crtc_state *new_crtc_state;
 	int old_min_cdclk, new_min_cdclk;
 	struct intel_crtc *crtc;
 	int i;
@@ -1302,15 +1303,23 @@  int intel_bw_calc_min_cdclk(struct intel_atomic_state *state,
 	if (DISPLAY_VER(display) < 9)
 		return 0;
 
-	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
+					    new_crtc_state, i) {
+		struct intel_dbuf_bw old_dbuf_bw, new_dbuf_bw;
+
+		skl_crtc_calc_dbuf_bw(&old_dbuf_bw, old_crtc_state);
+		skl_crtc_calc_dbuf_bw(&new_dbuf_bw, new_crtc_state);
+
+		if (!intel_dbuf_bw_changed(display, &old_dbuf_bw, &new_dbuf_bw))
+			continue;
+
 		new_bw_state = intel_atomic_get_bw_state(state);
 		if (IS_ERR(new_bw_state))
 			return PTR_ERR(new_bw_state);
 
 		old_bw_state = intel_atomic_get_old_bw_state(state);
 
-		skl_crtc_calc_dbuf_bw(&new_bw_state->dbuf_bw[crtc->pipe],
-				      crtc_state);
+		new_bw_state->dbuf_bw[crtc->pipe] = new_dbuf_bw;
 	}
 
 	if (!old_bw_state)