Re: [Intel-gfx] [PATCH v8 1/2] drm/i915: Refactor intel_can_enable_sagv
On Fri, 2019-10-25 at 10:44 +, Lisovskiy, Stanislav wrote:
> On Fri, 2019-10-25 at 13:24 +0300, Ville Syrjälä wrote:
> > On Fri, Oct 25, 2019 at 12:53:51PM +0300, Stanislav Lisovskiy
> > wrote:
> > > Currently intel_can_enable_sagv function contains
> > > a mix of workarounds for different platforms
> > > some of them are not valid for gens >= 11 already,
> > > so lets split it into separate functions.
> > >
> > > v2:
> > > - Rework watermark calculation algorithm to
> > > attempt to calculate Level 0 watermark
> > > with added sagv block time latency and
> > > check if it fits in DBuf in order to
> > > determine if SAGV can be enabled already
> > > at this stage, just as BSpec 49325 states.
> > > if that fails rollback to usual Level 0
> > > latency and disable SAGV.
> > > - Remove unneeded tabs(James Ausmus)
> > >
> > > v3: Rebased the patch
> > >
> > > v4: - Added back interlaced check for Gen12 and
> > > added separate function for TGL SAGV check
> > > (thanks to James Ausmus for spotting)
> > > - Removed unneeded gen check
> > > - Extracted Gen12 SAGV decision making code
> > > to a separate function from skl_compute_wm
> > >
> > > Signed-off-by: Stanislav Lisovskiy > > >
> > > Cc: Ville Syrjälä
> > > Cc: James Ausmus
> > > ---
> > > .../drm/i915/display/intel_display_types.h| 8 +
> > > drivers/gpu/drm/i915/intel_pm.c | 254
> > > +-
> > > 2 files changed, 254 insertions(+), 8 deletions(-)
> > >
> > >
> >
> > >
> > >
> > > + /*
> > > + * Lets assume we can tolerate SAGV for now,
> > > + * until watermark calculations prove the opposite
> > > + * if any of the pipe planes in the state will
> > > + * fail the requirements it will be assigned to false
> > > + * in skl_compute_ddb.
> > > + */
> > > + state->gen12_can_sagv = true;
> > > +
> > > + for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > + new_crtc_state, i) {
> > > + ret = tgl_check_pipe_fits_sagv_wm(new_crtc_state, ddb);
> > > + if (ret) {
> > > + state->gen12_can_sagv = false;
> > > + break;
> >
> > + }
> >
> > This is not going to work. We need the infromation from _all_
> > pipes,
> > not
> > just the ones in the state. We probably want to make that can_sagv
> > thing
> > a bitmask of pipes so that we don't have to have all pipes in the
> > state.
>
> But isn't it so that even if at least one plane/pipe can't tolerate
> SAGV, we can't enable it already? So what is the point of checking
> other planes/pipes then?
> Or may be I'm missing something here.
Ok, I think I get your point actually. As we don't have all pipes in
the state we might wrongly come to conclusion that we can enable SAGV
here. Also probably it really means that I will have to move
gen12_can_sagv from intel_atomic_state to our favourite dev_priv struct
as we will have to track all of the pipes in global state.
Regarding 3 different code paths from can_enable_sagv problem is that
in reality as I understand those are different, for example we disable
SAGV for SKL if there multiple active pipes, while for ICL we don't.
Also as we discussed ICL and TGL have completely different ways of
treating sagv_block_time(at least according to current BSpec) I could
unite all of those functions to one however it
would then contains multiple platform checks and stuff like that.
>
> >
> > > + }
> > > +
> > > + if (state->gen12_can_sagv) {
> > > + /*
> > > + * If we determined that we can actually enable SAGV,
> > > then
> > > + * actually use those levels
> > > tgl_check_pipe_fits_sagv_wm
> > > + * has already taken care of checking if L0 + sagv
> > > block time
> > > + * fits into ddb.
> > > + */
> > > + for_each_oldnew_intel_crtc_in_state(state, crtc,
> > > old_crtc_state,
> > > + new_crtc_state, i) {
> > > + struct intel_plane *plane;
> > > + for_each_intel_plane_on_crtc(_priv->drm,
> > > crtc, plane) {
> > > + enum plane_id plane_id = plane->id;
> > > + struct skl_plane_wm *plane_wm = \
> > > + _crtc_state-
> > > > wm.skl.optimal.planes[plane_id];
> > >
> > > + struct skl_wm_level *sagv_wm0 =
> > > _wm->sagv_wm_l0;
> > > + struct skl_wm_level *l0_wm0 =
> > > _wm->wm[0];
> > > +
> > > + memcpy(l0_wm0, sagv_wm0, sizeof(struct
> > > skl_wm_level));
> > > + }
> > > + }
> > > + }
> > > +}
> > > +
> > > static int
> > > skl_compute_wm(struct intel_atomic_state *state)
> > > {
> > > + struct drm_device *dev = state->base.dev;
> > > + const struct drm_i915_private *dev_priv = to_i915(dev);
> > > struct intel_crtc *crtc;
> > > struct
Re: [Intel-gfx] [PATCH v8 1/2] drm/i915: Refactor intel_can_enable_sagv
On Fri, 2019-10-25 at 13:24 +0300, Ville Syrjälä wrote:
> On Fri, Oct 25, 2019 at 12:53:51PM +0300, Stanislav Lisovskiy wrote:
> > Currently intel_can_enable_sagv function contains
> > a mix of workarounds for different platforms
> > some of them are not valid for gens >= 11 already,
> > so lets split it into separate functions.
> >
> > v2:
> > - Rework watermark calculation algorithm to
> > attempt to calculate Level 0 watermark
> > with added sagv block time latency and
> > check if it fits in DBuf in order to
> > determine if SAGV can be enabled already
> > at this stage, just as BSpec 49325 states.
> > if that fails rollback to usual Level 0
> > latency and disable SAGV.
> > - Remove unneeded tabs(James Ausmus)
> >
> > v3: Rebased the patch
> >
> > v4: - Added back interlaced check for Gen12 and
> > added separate function for TGL SAGV check
> > (thanks to James Ausmus for spotting)
> > - Removed unneeded gen check
> > - Extracted Gen12 SAGV decision making code
> > to a separate function from skl_compute_wm
> >
> > Signed-off-by: Stanislav Lisovskiy
> > Cc: Ville Syrjälä
> > Cc: James Ausmus
> > ---
> > .../drm/i915/display/intel_display_types.h| 8 +
> > drivers/gpu/drm/i915/intel_pm.c | 254
> > +-
> > 2 files changed, 254 insertions(+), 8 deletions(-)
> >
> >
> Do we use active or enable elsewhere to decide whether to compute wms
> for a pipe? Should be consistent here so we don't get into some wonky
> state where we didn't compute normal wms but are computing the sagv
> wm.
Good question, I have seen it either this or that everywhere, so we
probably need to discuss which one I should use.
>
> > +
> > + for_each_plane_id_on_crtc(crtc, plane_id) {
> > + struct skl_plane_wm *wm =
> > + _crtc_state-
> > >wm.skl.optimal.planes[plane_id];
> > +
> > + /* Skip this plane if it's not enabled */
> > + if (!wm->wm[0].plane_en)
> > + continue;
> > +
> > + /* Find the highest enabled wm level for this
> > plane */
> > + for (level = ilk_wm_max_level(dev_priv);
> > +!wm->wm[level].plane_en; --level) {
> > + }
> > +
> > + latency = dev_priv->wm.skl_latency[level];
> > +
> > + /*
> > +* If any of the planes on this pipe don't
> > enable
> > +* wm levels that incur memory latencies higher
> > than
> > +* sagv_block_time_us we can't enable SAGV.
> > +*/
> > + if (latency < dev_priv->sagv_block_time_us)
> > + return false;
> > + }
> > + }
> > +
> > + return true;
> > +}
> > +
> >
> > +bool intel_can_enable_sagv(struct intel_atomic_state *state)
> > +{
> > + struct drm_device *dev = state->base.dev;
> > + struct drm_i915_private *dev_priv = to_i915(dev);
> > +
> > + if (INTEL_GEN(dev_priv) >= 12)
> > + return tgl_can_enable_sagv(state);
> > + else if (INTEL_GEN(dev_priv) == 11)
> > + return icl_can_enable_sagv(state);
> > +
> > + return skl_can_enable_sagv(state);
>
> Why do we have three separate code paths now? I believe there should
> be
> just two.
>
> Also if you go to the trouble of adding dev_priv->..can_sagv just
> make
> it work for all platforms.
..can_sagv is not in dev_priv - it is part of intel_atomic_state,
so at least here I avoided using dev_priv.
>
> >
> >
> > + /*
> > +* Lets assume we can tolerate SAGV for now,
> > +* until watermark calculations prove the opposite
> > +* if any of the pipe planes in the state will
> > +* fail the requirements it will be assigned to false
> > +* in skl_compute_ddb.
> > +*/
> > + state->gen12_can_sagv = true;
> > +
> > + for_each_oldnew_intel_crtc_in_state(state, crtc,
> > old_crtc_state,
> > + new_crtc_state, i) {
> > + ret = tgl_check_pipe_fits_sagv_wm(new_crtc_state, ddb);
> > + if (ret) {
> > + state->gen12_can_sagv = false;
> > + break;
> + }
>
> This is not going to work. We need the infromation from _all_ pipes,
> not
> just the ones in the state. We probably want to make that can_sagv
> thing
> a bitmask of pipes so that we don't have to have all pipes in the
> state.
But isn't it so that even if at least one plane/pipe can't tolerate
SAGV, we can't enable it already? So what is the point of checking
other planes/pipes then?
Or may be I'm missing something here.
>
> > + }
> > +
> > + if (state->gen12_can_sagv) {
> > + /*
> > +* If we determined that we can actually enable SAGV,
> > then
> > +* actually use those levels
> > tgl_check_pipe_fits_sagv_wm
> > +
Re: [Intel-gfx] [PATCH v8 1/2] drm/i915: Refactor intel_can_enable_sagv
On Fri, Oct 25, 2019 at 12:53:51PM +0300, Stanislav Lisovskiy wrote:
> Currently intel_can_enable_sagv function contains
> a mix of workarounds for different platforms
> some of them are not valid for gens >= 11 already,
> so lets split it into separate functions.
>
> v2:
> - Rework watermark calculation algorithm to
> attempt to calculate Level 0 watermark
> with added sagv block time latency and
> check if it fits in DBuf in order to
> determine if SAGV can be enabled already
> at this stage, just as BSpec 49325 states.
> if that fails rollback to usual Level 0
> latency and disable SAGV.
> - Remove unneeded tabs(James Ausmus)
>
> v3: Rebased the patch
>
> v4: - Added back interlaced check for Gen12 and
> added separate function for TGL SAGV check
> (thanks to James Ausmus for spotting)
> - Removed unneeded gen check
> - Extracted Gen12 SAGV decision making code
> to a separate function from skl_compute_wm
>
> Signed-off-by: Stanislav Lisovskiy
> Cc: Ville Syrjälä
> Cc: James Ausmus
> ---
> .../drm/i915/display/intel_display_types.h| 8 +
> drivers/gpu/drm/i915/intel_pm.c | 254 +-
> 2 files changed, 254 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
> b/drivers/gpu/drm/i915/display/intel_display_types.h
> index 8358152e403e..f09c80c96470 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> @@ -490,6 +490,13 @@ struct intel_atomic_state {
>*/
> u8 active_pipe_changes;
>
> + /*
> + * For Gen12 only after calculating watermarks with
> + * additional latency, we can determine if SAGV can be enabled
> + * or not for that particular configuration.
> + */
> + bool gen12_can_sagv;
> +
> u8 active_pipes;
> /* minimum acceptable cdclk for each pipe */
> int min_cdclk[I915_MAX_PIPES];
> @@ -642,6 +649,7 @@ struct skl_plane_wm {
> struct skl_wm_level wm[8];
> struct skl_wm_level uv_wm[8];
> struct skl_wm_level trans_wm;
> + struct skl_wm_level sagv_wm_l0;
sagv_wm0 (or maybe even just sagv_wm) would be a bit less ugly
name I think.
> bool is_planar;
> };
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 362234449087..b61eb6aaa89b 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3751,7 +3751,7 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> return 0;
> }
>
> -bool intel_can_enable_sagv(struct intel_atomic_state *state)
> +bool skl_can_enable_sagv(struct intel_atomic_state *state)
> {
> struct drm_device *dev = state->base.dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> @@ -3817,6 +3817,95 @@ bool intel_can_enable_sagv(struct intel_atomic_state
> *state)
> return true;
> }
>
> +bool icl_can_enable_sagv(struct intel_atomic_state *state)
> +{
> + struct drm_device *dev = state->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *crtc;
> + struct intel_crtc_state *new_crtc_state;
> + int level, latency;
> + int i;
> + int plane_id;
> +
> + if (!intel_has_sagv(dev_priv))
> + return false;
> +
> + /*
> + * If there are no active CRTCs, no additional checks need be performed
> + */
> + if (hweight8(state->active_pipes) == 0)
> + return true;
> +
> + for_each_new_intel_crtc_in_state(state, crtc,
> + new_crtc_state, i) {
> + unsigned int flags = crtc->base.state->adjusted_mode.flags;
> +
> + if (flags & DRM_MODE_FLAG_INTERLACE)
> + return false;
> +
> + if (!new_crtc_state->base.enable)
> + continue;
Do we use active or enable elsewhere to decide whether to compute wms
for a pipe? Should be consistent here so we don't get into some wonky
state where we didn't compute normal wms but are computing the sagv wm.
> +
> + for_each_plane_id_on_crtc(crtc, plane_id) {
> + struct skl_plane_wm *wm =
> +
> _crtc_state->wm.skl.optimal.planes[plane_id];
> +
> + /* Skip this plane if it's not enabled */
> + if (!wm->wm[0].plane_en)
> + continue;
> +
> + /* Find the highest enabled wm level for this plane */
> + for (level = ilk_wm_max_level(dev_priv);
> + !wm->wm[level].plane_en; --level) {
> + }
> +
> + latency = dev_priv->wm.skl_latency[level];
> +
> + /*
> + * If any of the planes on this pipe don't enable
> + * wm levels that incur memory
[Intel-gfx] [PATCH v8 1/2] drm/i915: Refactor intel_can_enable_sagv
Currently intel_can_enable_sagv function contains
a mix of workarounds for different platforms
some of them are not valid for gens >= 11 already,
so lets split it into separate functions.
v2:
- Rework watermark calculation algorithm to
attempt to calculate Level 0 watermark
with added sagv block time latency and
check if it fits in DBuf in order to
determine if SAGV can be enabled already
at this stage, just as BSpec 49325 states.
if that fails rollback to usual Level 0
latency and disable SAGV.
- Remove unneeded tabs(James Ausmus)
v3: Rebased the patch
v4: - Added back interlaced check for Gen12 and
added separate function for TGL SAGV check
(thanks to James Ausmus for spotting)
- Removed unneeded gen check
- Extracted Gen12 SAGV decision making code
to a separate function from skl_compute_wm
Signed-off-by: Stanislav Lisovskiy
Cc: Ville Syrjälä
Cc: James Ausmus
---
.../drm/i915/display/intel_display_types.h| 8 +
drivers/gpu/drm/i915/intel_pm.c | 254 +-
2 files changed, 254 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h
b/drivers/gpu/drm/i915/display/intel_display_types.h
index 8358152e403e..f09c80c96470 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -490,6 +490,13 @@ struct intel_atomic_state {
*/
u8 active_pipe_changes;
+ /*
+* For Gen12 only after calculating watermarks with
+* additional latency, we can determine if SAGV can be enabled
+* or not for that particular configuration.
+*/
+ bool gen12_can_sagv;
+
u8 active_pipes;
/* minimum acceptable cdclk for each pipe */
int min_cdclk[I915_MAX_PIPES];
@@ -642,6 +649,7 @@ struct skl_plane_wm {
struct skl_wm_level wm[8];
struct skl_wm_level uv_wm[8];
struct skl_wm_level trans_wm;
+ struct skl_wm_level sagv_wm_l0;
bool is_planar;
};
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 362234449087..b61eb6aaa89b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3751,7 +3751,7 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
return 0;
}
-bool intel_can_enable_sagv(struct intel_atomic_state *state)
+bool skl_can_enable_sagv(struct intel_atomic_state *state)
{
struct drm_device *dev = state->base.dev;
struct drm_i915_private *dev_priv = to_i915(dev);
@@ -3817,6 +3817,95 @@ bool intel_can_enable_sagv(struct intel_atomic_state
*state)
return true;
}
+bool icl_can_enable_sagv(struct intel_atomic_state *state)
+{
+ struct drm_device *dev = state->base.dev;
+ struct drm_i915_private *dev_priv = to_i915(dev);
+ struct intel_crtc *crtc;
+ struct intel_crtc_state *new_crtc_state;
+ int level, latency;
+ int i;
+ int plane_id;
+
+ if (!intel_has_sagv(dev_priv))
+ return false;
+
+ /*
+* If there are no active CRTCs, no additional checks need be performed
+*/
+ if (hweight8(state->active_pipes) == 0)
+ return true;
+
+ for_each_new_intel_crtc_in_state(state, crtc,
+new_crtc_state, i) {
+ unsigned int flags = crtc->base.state->adjusted_mode.flags;
+
+ if (flags & DRM_MODE_FLAG_INTERLACE)
+ return false;
+
+ if (!new_crtc_state->base.enable)
+ continue;
+
+ for_each_plane_id_on_crtc(crtc, plane_id) {
+ struct skl_plane_wm *wm =
+
_crtc_state->wm.skl.optimal.planes[plane_id];
+
+ /* Skip this plane if it's not enabled */
+ if (!wm->wm[0].plane_en)
+ continue;
+
+ /* Find the highest enabled wm level for this plane */
+ for (level = ilk_wm_max_level(dev_priv);
+!wm->wm[level].plane_en; --level) {
+ }
+
+ latency = dev_priv->wm.skl_latency[level];
+
+ /*
+* If any of the planes on this pipe don't enable
+* wm levels that incur memory latencies higher than
+* sagv_block_time_us we can't enable SAGV.
+*/
+ if (latency < dev_priv->sagv_block_time_us)
+ return false;
+ }
+ }
+
+ return true;
+}
+
+bool tgl_can_enable_sagv(struct intel_atomic_state *state)
+{
+ struct intel_crtc *crtc;
+ struct intel_crtc_state *new_crtc_state;
+ int i;
+
+ if (!state->gen12_can_sagv)
+ return
