Re: [Intel-gfx] [PATCH 2/2] drm/i915: Stop adding planes to the commit needlessly
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ä
> > >
> > > 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
>
> >
> >
> > 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
> > > Signed-off-by: Ville Syrjälä
> > > ---
> > > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Stop adding planes to the commit needlessly
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ä
> >
> > 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
> > Signed-off-by: Ville Syrjälä
> > ---
> > 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Stop adding planes to the commit needlessly
On Thu, Mar 25, 2021 at 02:44:15AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä
>
> 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
> Signed-off-by: Ville Syrjälä
> ---
> 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
>
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
