diff mbox series

[2/2] drm/i915: Stop adding planes to the commit needlessly

Message ID 20210325004415.17432-2-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Fix transposed arguments to skl_plane_wm_level() | expand

Commit Message

Ville Syrjälä March 25, 2021, 12:44 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The dbuf bandwidth calculations don't need the planes to be
added to the state. Each plane's data rate has already been
precalculated and stored in the crtc state, and that with
the dbuf slice usage for each plane is all the dbuf bandwidth
code needs to figure out what the minimum cdclk is.

What we're trying to do here is make sure each plane recalculates
its minimum cdclk (ie. plane->min_cdclk()) on those platforms where
the number of active planes affects the result of said calculation.
Nothing to do with any dbuf cdclk requirements.

Not sure if we had stuff in slightly different order or what,
but at least in the current scheme this is not necessary.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

Lisovskiy, Stanislav March 25, 2021, 9:35 a.m. UTC | #1 
On Thu, Mar 25, 2021 at 02:44:15AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> The dbuf bandwidth calculations don't need the planes to be
> added to the state. Each plane's data rate has already been
> precalculated and stored in the crtc state, and that with
> the dbuf slice usage for each plane is all the dbuf bandwidth
> code needs to figure out what the minimum cdclk is.
> 
> What we're trying to do here is make sure each plane recalculates
> its minimum cdclk (ie. plane->min_cdclk()) on those platforms where
> the number of active planes affects the result of said calculation.
> Nothing to do with any dbuf cdclk requirements.

So does it mean that if we lets say had active plane mask as
011(planes 0, 1 were active) and new active planes are 101(planes 0, 2
are active) - we should not add plane 2 to the state?
Because hamming weight will be obviously same, however I think it would
be wrong not have plane 2 in the state at all then..

Or will it be added somewhere else?


Stan

> 
> Not sure if we had stuff in slightly different order or what,
> but at least in the current scheme this is not necessary.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 17490d29dc13..2300d58ba47f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -9811,7 +9811,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
>  	/* See {hsw,vlv,ivb}_plane_ratio() */
>  	return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
>  		IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> -		IS_IVYBRIDGE(dev_priv) || (DISPLAY_VER(dev_priv) >= 11);
> +		IS_IVYBRIDGE(dev_priv);
>  }
>  
>  static int intel_crtc_add_bigjoiner_planes(struct intel_atomic_state *state,
> @@ -9898,13 +9898,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
>  		old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
>  		new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
>  
> -		/*
> -		 * Not only the number of planes, but if the plane configuration had
> -		 * changed might already mean we need to recompute min CDCLK,
> -		 * because different planes might consume different amount of Dbuf bandwidth
> -		 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
> -		 */
> -		if (old_active_planes == new_active_planes)
> +		if (hweight8(old_active_planes) == hweight8(new_active_planes))
>  			continue;
>  
>  		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> -- 
> 2.26.2
>
Ville Syrjälä March 25, 2021, 11:37 a.m. UTC | #2 
On Thu, Mar 25, 2021 at 11:35:53AM +0200, Lisovskiy, Stanislav wrote:
> On Thu, Mar 25, 2021 at 02:44:15AM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > The dbuf bandwidth calculations don't need the planes to be
> > added to the state. Each plane's data rate has already been
> > precalculated and stored in the crtc state, and that with
> > the dbuf slice usage for each plane is all the dbuf bandwidth
> > code needs to figure out what the minimum cdclk is.
> > 
> > What we're trying to do here is make sure each plane recalculates
> > its minimum cdclk (ie. plane->min_cdclk()) on those platforms where
> > the number of active planes affects the result of said calculation.
> > Nothing to do with any dbuf cdclk requirements.
> 
> So does it mean that if we lets say had active plane mask as
> 011(planes 0, 1 were active) and new active planes are 101(planes 0, 2
> are active) - we should not add plane 2 to the state?
> Because hamming weight will be obviously same, however I think it would
> be wrong not have plane 2 in the state at all then..
> 
> Or will it be added somewhere else?

If someone is asking to disable plane 2 then it will be added to
the state already during the atomic/setplance ioctl handling.

> 
> 
> Stan
> 
> > 
> > Not sure if we had stuff in slightly different order or what,
> > but at least in the current scheme this is not necessary.
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++--------
> >  1 file changed, 2 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 17490d29dc13..2300d58ba47f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -9811,7 +9811,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
> >  	/* See {hsw,vlv,ivb}_plane_ratio() */
> >  	return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
> >  		IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> > -		IS_IVYBRIDGE(dev_priv) || (DISPLAY_VER(dev_priv) >= 11);
> > +		IS_IVYBRIDGE(dev_priv);
> >  }
> >  
> >  static int intel_crtc_add_bigjoiner_planes(struct intel_atomic_state *state,
> > @@ -9898,13 +9898,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> >  		old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> >  		new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> >  
> > -		/*
> > -		 * Not only the number of planes, but if the plane configuration had
> > -		 * changed might already mean we need to recompute min CDCLK,
> > -		 * because different planes might consume different amount of Dbuf bandwidth
> > -		 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
> > -		 */
> > -		if (old_active_planes == new_active_planes)
> > +		if (hweight8(old_active_planes) == hweight8(new_active_planes))
> >  			continue;
> >  
> >  		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> > -- 
> > 2.26.2
> >
Lisovskiy, Stanislav March 29, 2021, 9:01 a.m. UTC | #3 
On Thu, Mar 25, 2021 at 01:37:31PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 25, 2021 at 11:35:53AM +0200, Lisovskiy, Stanislav wrote:
> > On Thu, Mar 25, 2021 at 02:44:15AM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > The dbuf bandwidth calculations don't need the planes to be
> > > added to the state. Each plane's data rate has already been
> > > precalculated and stored in the crtc state, and that with
> > > the dbuf slice usage for each plane is all the dbuf bandwidth
> > > code needs to figure out what the minimum cdclk is.
> > > 
> > > What we're trying to do here is make sure each plane recalculates
> > > its minimum cdclk (ie. plane->min_cdclk()) on those platforms where
> > > the number of active planes affects the result of said calculation.
> > > Nothing to do with any dbuf cdclk requirements.
> > 
> > So does it mean that if we lets say had active plane mask as
> > 011(planes 0, 1 were active) and new active planes are 101(planes 0, 2
> > are active) - we should not add plane 2 to the state?
> > Because hamming weight will be obviously same, however I think it would
> > be wrong not have plane 2 in the state at all then..
> > 
> > Or will it be added somewhere else?
> 
> If someone is asking to disable plane 2 then it will be added to
> the state already during the atomic/setplance ioctl handling.

Ok, I thought intel_atomic_check_planes function is intended for checking
which planes had changed and need to be added to the state.
It is a bit non-obvious as we are adding them only when their amount changes,
but not themself.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> > 
> > 
> > Stan
> > 
> > > 
> > > Not sure if we had stuff in slightly different order or what,
> > > but at least in the current scheme this is not necessary.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++--------
> > >  1 file changed, 2 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 17490d29dc13..2300d58ba47f 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -9811,7 +9811,7 @@ static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
> > >  	/* See {hsw,vlv,ivb}_plane_ratio() */
> > >  	return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
> > >  		IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
> > > -		IS_IVYBRIDGE(dev_priv) || (DISPLAY_VER(dev_priv) >= 11);
> > > +		IS_IVYBRIDGE(dev_priv);
> > >  }
> > >  
> > >  static int intel_crtc_add_bigjoiner_planes(struct intel_atomic_state *state,
> > > @@ -9898,13 +9898,7 @@ static int intel_atomic_check_planes(struct intel_atomic_state *state)
> > >  		old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> > >  		new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
> > >  
> > > -		/*
> > > -		 * Not only the number of planes, but if the plane configuration had
> > > -		 * changed might already mean we need to recompute min CDCLK,
> > > -		 * because different planes might consume different amount of Dbuf bandwidth
> > > -		 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
> > > -		 */
> > > -		if (old_active_planes == new_active_planes)
> > > +		if (hweight8(old_active_planes) == hweight8(new_active_planes))
> > >  			continue;
> > >  
> > >  		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);
> > > -- 
> > > 2.26.2
> > > 
> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 17490d29dc13..2300d58ba47f 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -9811,7 +9811,7 @@  static bool active_planes_affects_min_cdclk(struct drm_i915_private *dev_priv)
 	/* See {hsw,vlv,ivb}_plane_ratio() */
 	return IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv) ||
 		IS_CHERRYVIEW(dev_priv) || IS_VALLEYVIEW(dev_priv) ||
-		IS_IVYBRIDGE(dev_priv) || (DISPLAY_VER(dev_priv) >= 11);
+		IS_IVYBRIDGE(dev_priv);
 }
 
 static int intel_crtc_add_bigjoiner_planes(struct intel_atomic_state *state,
@@ -9898,13 +9898,7 @@  static int intel_atomic_check_planes(struct intel_atomic_state *state)
 		old_active_planes = old_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
 		new_active_planes = new_crtc_state->active_planes & ~BIT(PLANE_CURSOR);
 
-		/*
-		 * Not only the number of planes, but if the plane configuration had
-		 * changed might already mean we need to recompute min CDCLK,
-		 * because different planes might consume different amount of Dbuf bandwidth
-		 * according to formula: Bw per plane = Pixel rate * bpp * pipe/plane scale factor
-		 */
-		if (old_active_planes == new_active_planes)
+		if (hweight8(old_active_planes) == hweight8(new_active_planes))
 			continue;
 
 		ret = intel_crtc_add_planes_to_state(state, crtc, new_active_planes);