Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Ville Syrjälä (2018-07-20 15:19:20) > On Fri, Jul 20, 2018 at 03:03:09PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjälä (2018-07-20 14:50:25) > > > On Fri, Jul 20, 2018 at 02:38:32PM +0100, Chris Wilson wrote: > > > > Quoting Ville Syrjälä (2018-07-20 14:32:40) > > > > > Another question is what happens where there are parallel flips > > > > > happening? One could undo the boost from the other AFAICS. But maybe > > > > > we don't care enough to protect against that? > > > > > > > > It's a counter, so the "interactive" mode remains high until all > > > > concurrent flips are completed. > > > > > > Ah. I guess the bool in the atomic state threw me off. I suppose that > > > one is just an optimization to avoid calling the function more than > > > once? > > > > Yes, it's that I caught the RPS counter going negative. We have more > > cleanup_plane than we prepared I believe that's from > > find_initial_plane. > > Hmm. I don't quite see how that could happen. I believe > prepare_fb/cleanup_fb should be fully paired up within a single commit. The way I read intel_find_initial_plane_obj() is that it writes the current configuration into the current intel_crtc->base.primary->state. I think that is where the initial mismatch comes from. Also we seem to be quite adapt at not holding rps_interactive high after modeset completion, which suggests to me that something sneaky is happening with the plane_state duplication. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Tvrtko Ursulin (2018-07-20 15:58:51) > True, it only catches the imbalance in one direction quickly. If suspend > idea works go with that, but what's so bad about some log messages? > Assuming leak towards the overflow direction on each flip it could be > reached in ~18 hours which is realistic to get bug reports of. A log message means handling bugs reports for years after you fix the bug :) Silence is bliss. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On 20/07/2018 14:59, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-07-20 14:29:52)
On 20/07/2018 14:02, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-07-20 13:49:09)
On 12/07/2018 18:38, Chris Wilson wrote:
+ if (rps->interactive)
+ new_power = HIGH_POWER;
+ rps_set_power(dev_priv, new_power);
+ mutex_unlock(>power_lock);
>
> rps->last_adj = 0;
This block should go up to the beginning of the function since when
rps->interactive all preceding calculations are for nothing. And can
immediately return then.
We have to lock around rps_set_power, so you're not going to avoid
taking the lock here, so I don't think it makes any difference.
Certainly neater than locking everything.
Not to avoid the lock but to avoid all the trouble of calculating
new_power to override it all if rps->interactive is set. Just looks a
bit weird. I suspect read of rps->interactive doesn't even need to be
under the lock so maybe:
if (rps->interactive)
new_power = HIGH_POWER;
} else {
switch (...)
}
There is a race there, so you do need to explain why we wouldn't care.
(There is a reasonable argument about it being periodic and we don't care
about the first vs interactive.) I thought it came out quite neat as the
atomic override.
Putting it all under the lock works for me then. But okay, if you so
like the override idea, which I wouldn't call neat, but unusual, it's
not the end of the world.
+{
+ struct intel_rps *rps = _priv->gt_pm.rps;
+
+ if (INTEL_GEN(dev_priv) < 6)
+ return;
+
+ mutex_lock(>power_lock);
+ if (state) {
+ if (!rps->interactive++ && READ_ONCE(dev_priv->gt.awake))
+ rps_set_power(dev_priv, HIGH_POWER);
+ } else {
+ GEM_BUG_ON(!rps->interactive);
+ rps->interactive--;
Hm maybe protect this in production code so some missed code flows in
the atomic paths do not cause underflow and so permanent interactive mode.
Are you suggesting testing is inadequate? ;) Underflow here isn't a big
deal, it just locks in the HIGH_POWER (faster sampling, bias towards
upclocking). Or worse not allow HIGH_POWER, status quo returned.
Testing for this probably is inadequate, no? Would need to be looking at
the new debugfs flag from many test cases. And in real world probably
quite difficult to debug too.
Whether or not it would be too much to add a DRM_WARN for this.. I am
leaning towards saying to have it, since it is about two systems
communicating together so it would be easier to notice a broken
contract. But I don't insist on it.
Just checking underflow is not going to catch many problems. If we can
identify some points where we know what the value should be... I wonder
if we can assert it is zero at runtime/system suspend.
True, it only catches the imbalance in one direction quickly. If suspend
idea works go with that, but what's so bad about some log messages?
Assuming leak towards the overflow direction on each flip it could be
reached in ~18 hours which is realistic to get bug reports of.
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On Fri, Jul 20, 2018 at 03:03:09PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-07-20 14:50:25) > > On Fri, Jul 20, 2018 at 02:38:32PM +0100, Chris Wilson wrote: > > > Quoting Ville Syrjälä (2018-07-20 14:32:40) > > > > Another question is what happens where there are parallel flips > > > > happening? One could undo the boost from the other AFAICS. But maybe > > > > we don't care enough to protect against that? > > > > > > It's a counter, so the "interactive" mode remains high until all > > > concurrent flips are completed. > > > > Ah. I guess the bool in the atomic state threw me off. I suppose that > > one is just an optimization to avoid calling the function more than > > once? > > Yes, it's that I caught the RPS counter going negative. We have more > cleanup_plane than we prepared I believe that's from > find_initial_plane. Hmm. I don't quite see how that could happen. I believe prepare_fb/cleanup_fb should be fully paired up within a single commit. -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Ville Syrjälä (2018-07-20 14:50:25) > On Fri, Jul 20, 2018 at 02:38:32PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjälä (2018-07-20 14:32:40) > > > Another question is what happens where there are parallel flips > > > happening? One could undo the boost from the other AFAICS. But maybe > > > we don't care enough to protect against that? > > > > It's a counter, so the "interactive" mode remains high until all > > concurrent flips are completed. > > Ah. I guess the bool in the atomic state threw me off. I suppose that > one is just an optimization to avoid calling the function more than > once? Yes, it's that I caught the RPS counter going negative. We have more cleanup_plane than we prepared I believe that's from find_initial_plane. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Tvrtko Ursulin (2018-07-20 14:29:52)
>
> On 20/07/2018 14:02, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-07-20 13:49:09)
> >>
> >> On 12/07/2018 18:38, Chris Wilson wrote:
> >>> + if (rps->interactive)
> >>> + new_power = HIGH_POWER;
> >>> + rps_set_power(dev_priv, new_power);
> >>> + mutex_unlock(>power_lock);
> >> >
> >> > rps->last_adj = 0;
> >>
> >> This block should go up to the beginning of the function since when
> >> rps->interactive all preceding calculations are for nothing. And can
> >> immediately return then.
> >
> > We have to lock around rps_set_power, so you're not going to avoid
> > taking the lock here, so I don't think it makes any difference.
> > Certainly neater than locking everything.
>
> Not to avoid the lock but to avoid all the trouble of calculating
> new_power to override it all if rps->interactive is set. Just looks a
> bit weird. I suspect read of rps->interactive doesn't even need to be
> under the lock so maybe:
>
> if (rps->interactive)
> new_power = HIGH_POWER;
> } else {
> switch (...)
> }
There is a race there, so you do need to explain why we wouldn't care.
(There is a reasonable argument about it being periodic and we don't care
about the first vs interactive.) I thought it came out quite neat as the
atomic override.
> >>> +{
> >>> + struct intel_rps *rps = _priv->gt_pm.rps;
> >>> +
> >>> + if (INTEL_GEN(dev_priv) < 6)
> >>> + return;
> >>> +
> >>> + mutex_lock(>power_lock);
> >>> + if (state) {
> >>> + if (!rps->interactive++ && READ_ONCE(dev_priv->gt.awake))
> >>> + rps_set_power(dev_priv, HIGH_POWER);
> >>> + } else {
> >>> + GEM_BUG_ON(!rps->interactive);
> >>> + rps->interactive--;
> >>
> >> Hm maybe protect this in production code so some missed code flows in
> >> the atomic paths do not cause underflow and so permanent interactive mode.
> >
> > Are you suggesting testing is inadequate? ;) Underflow here isn't a big
> > deal, it just locks in the HIGH_POWER (faster sampling, bias towards
> > upclocking). Or worse not allow HIGH_POWER, status quo returned.
>
> Testing for this probably is inadequate, no? Would need to be looking at
> the new debugfs flag from many test cases. And in real world probably
> quite difficult to debug too.
>
> Whether or not it would be too much to add a DRM_WARN for this.. I am
> leaning towards saying to have it, since it is about two systems
> communicating together so it would be easier to notice a broken
> contract. But I don't insist on it.
Just checking underflow is not going to catch many problems. If we can
identify some points where we know what the value should be... I wonder
if we can assert it is zero at runtime/system suspend.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On Fri, Jul 20, 2018 at 02:38:32PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-07-20 14:32:40) > > On Fri, Jul 20, 2018 at 02:14:11PM +0100, Chris Wilson wrote: > > > Quoting Ville Syrjälä (2018-07-20 14:07:31) > > > > On Fri, Jul 20, 2018 at 02:02:34PM +0100, Chris Wilson wrote: > > > > Doing this kind of global thing from the plane hooks seems a bit > > > > strange. How about just doing this directly from commit_tail() > > > > etc.? > > > > > > We want it upfront in prepare (so that it's set before any wait) or > > > somewhere around there (atomic_state setup?). cleanup was chosen for the > > > symmetry with prepare. > > > > Looks like we have intel_atomic_prepare_commit() which I guess would be > > a decent spot then. And introduce intel_atomic_cleanup_commit() to do > > the reverse? > > The only other point is I started from prepare_plane for being next to > both the reprioritisation and the add_rps_boost_after_vblank. So that's > quite nice. Ok, I guess that's quite reasonable. > > > Another question is what happens where there are parallel flips > > happening? One could undo the boost from the other AFAICS. But maybe > > we don't care enough to protect against that? > > It's a counter, so the "interactive" mode remains high until all > concurrent flips are completed. Ah. I guess the bool in the atomic state threw me off. I suppose that one is just an optimization to avoid calling the function more than once? -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Ville Syrjälä (2018-07-20 14:32:40) > On Fri, Jul 20, 2018 at 02:14:11PM +0100, Chris Wilson wrote: > > Quoting Ville Syrjälä (2018-07-20 14:07:31) > > > On Fri, Jul 20, 2018 at 02:02:34PM +0100, Chris Wilson wrote: > > > Doing this kind of global thing from the plane hooks seems a bit > > > strange. How about just doing this directly from commit_tail() > > > etc.? > > > > We want it upfront in prepare (so that it's set before any wait) or > > somewhere around there (atomic_state setup?). cleanup was chosen for the > > symmetry with prepare. > > Looks like we have intel_atomic_prepare_commit() which I guess would be > a decent spot then. And introduce intel_atomic_cleanup_commit() to do > the reverse? The only other point is I started from prepare_plane for being next to both the reprioritisation and the add_rps_boost_after_vblank. So that's quite nice. > Another question is what happens where there are parallel flips > happening? One could undo the boost from the other AFAICS. But maybe > we don't care enough to protect against that? It's a counter, so the "interactive" mode remains high until all concurrent flips are completed. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On Fri, Jul 20, 2018 at 02:14:11PM +0100, Chris Wilson wrote: > Quoting Ville Syrjälä (2018-07-20 14:07:31) > > On Fri, Jul 20, 2018 at 02:02:34PM +0100, Chris Wilson wrote: > > Doing this kind of global thing from the plane hooks seems a bit > > strange. How about just doing this directly from commit_tail() > > etc.? > > We want it upfront in prepare (so that it's set before any wait) or > somewhere around there (atomic_state setup?). cleanup was chosen for the > symmetry with prepare. Looks like we have intel_atomic_prepare_commit() which I guess would be a decent spot then. And introduce intel_atomic_cleanup_commit() to do the reverse? Another question is what happens where there are parallel flips happening? One could undo the boost from the other AFAICS. But maybe we don't care enough to protect against that? -- Ville Syrjälä Intel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On 20/07/2018 14:02, Chris Wilson wrote:
Quoting Tvrtko Ursulin (2018-07-20 13:49:09)
On 12/07/2018 18:38, Chris Wilson wrote:
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7998e70a3174..5809366ff9f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13104,6 +13104,19 @@ intel_prepare_plane_fb(struct drm_plane *plane,
add_rps_boost_after_vblank(new_state->crtc, new_state->fence);
}
+ /*
+ * We declare pageflips to be interactive and so merit a small bias
+ * towards upclocking to deliver the frame on time. By only changing
+ * the RPS thresholds to sample more regularly and aim for higher
+ * clocks we can hopefully deliver low power workloads (like kodi)
+ * that are not quite steady state without resorting to forcing
+ * maximum clocks following a vblank miss (see do_rps_boost()).
+ */
+ if (!intel_state->rps_interactive) {
+ intel_rps_set_interactive(dev_priv, true);
+ intel_state->rps_interactive = true;
+ }
+
return 0;
}
@@ -13120,8 +13133,15 @@ void
intel_cleanup_plane_fb(struct drm_plane *plane,
struct drm_plane_state *old_state)
{
+ struct intel_atomic_state *intel_state =
+ to_intel_atomic_state(old_state->state);
struct drm_i915_private *dev_priv = to_i915(plane->dev);
+ if (intel_state->rps_interactive) {
+ intel_rps_set_interactive(dev_priv, false);
+ intel_state->rps_interactive = false;
+ }
Why is the rps_intearctive flag needed in plane state? I wanted to
assume prepare & cleanup hooks are fully symmetric, but this flags makes
me unsure. A reviewer with more display knowledge will be needed here I
think.
It's so that when we call intel_cleanup_plane_fb() on something that
wasn't first prepared, we don't decrement the counter. There's a little
bit of asymmetry at the start where we inherit the plane state.
+
/* Should only be called after a successful intel_prepare_plane_fb()! */
mutex_lock(_priv->drm.struct_mutex);
intel_plane_unpin_fb(to_intel_plane_state(old_state));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 61e715ddd0d5..544812488821 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -482,6 +482,8 @@ struct intel_atomic_state {
*/
bool skip_intermediate_wm;
+ bool rps_interactive;
Should the name at this level be something more tied to the subsystem
and not implying wider relationships? Like page flip pending? fb_prepared?
But we are in the plane state so anything plane/flip is implied. I think
saying whether or not we've called out to rps is a reasonable name for
the state.
Okay fair enough. Maybe just call it rps_interactive_requested?
+ /* Max/min bins are special */
+ if (val <= rps->min_freq_softlimit)
+ new_power = LOW_POWER;
+ if (val >= rps->max_freq_softlimit)
+ new_power = HIGH_POWER;
+
+ mutex_lock(>power_lock);
Is it worth avoiding the atomic here when GEN < 6?
We don't get here when not !RPS. You mean GEN < 5 ;)
No, okay, just incomplete picture based on locking in the other helper.
Ignore.
+ if (rps->interactive)
+ new_power = HIGH_POWER;
+ rps_set_power(dev_priv, new_power);
+ mutex_unlock(>power_lock);
>
> rps->last_adj = 0;
This block should go up to the beginning of the function since when
rps->interactive all preceding calculations are for nothing. And can
immediately return then.
We have to lock around rps_set_power, so you're not going to avoid
taking the lock here, so I don't think it makes any difference.
Certainly neater than locking everything.
Not to avoid the lock but to avoid all the trouble of calculating
new_power to override it all if rps->interactive is set. Just looks a
bit weird. I suspect read of rps->interactive doesn't even need to be
under the lock so maybe:
if (rps->interactive)
new_power = HIGH_POWER;
} else {
switch (...)
}
mutex_lock
...
mutex_unlock
+void intel_rps_set_interactive(struct drm_i915_private *dev_priv, bool state)
s/state/interactive/ for more self-documenting function body?
And not s/dev_priv/i915/ ?!? :)
+{
+ struct intel_rps *rps = _priv->gt_pm.rps;
+
+ if (INTEL_GEN(dev_priv) < 6)
+ return;
+
+ mutex_lock(>power_lock);
+ if (state) {
+ if (!rps->interactive++ && READ_ONCE(dev_priv->gt.awake))
+ rps_set_power(dev_priv, HIGH_POWER);
+ } else {
+ GEM_BUG_ON(!rps->interactive);
+ rps->interactive--;
Hm maybe protect this in production code so some missed code flows in
the atomic paths do not cause underflow and so permanent interactive mode.
Are you suggesting testing is inadequate? ;)
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Ville Syrjälä (2018-07-20 14:07:31) > On Fri, Jul 20, 2018 at 02:02:34PM +0100, Chris Wilson wrote: > Doing this kind of global thing from the plane hooks seems a bit > strange. How about just doing this directly from commit_tail() > etc.? We want it upfront in prepare (so that it's set before any wait) or somewhere around there (atomic_state setup?). cleanup was chosen for the symmetry with prepare. Pick a spot for the increment for that tells us where to decrement. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On Fri, Jul 20, 2018 at 02:02:34PM +0100, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-07-20 13:49:09)
> >
> > On 12/07/2018 18:38, Chris Wilson wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 7998e70a3174..5809366ff9f0 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13104,6 +13104,19 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > > add_rps_boost_after_vblank(new_state->crtc,
> > > new_state->fence);
> > > }
> > >
> > > + /*
> > > + * We declare pageflips to be interactive and so merit a small bias
> > > + * towards upclocking to deliver the frame on time. By only changing
> > > + * the RPS thresholds to sample more regularly and aim for higher
> > > + * clocks we can hopefully deliver low power workloads (like kodi)
> > > + * that are not quite steady state without resorting to forcing
> > > + * maximum clocks following a vblank miss (see do_rps_boost()).
> > > + */
> > > + if (!intel_state->rps_interactive) {
> > > + intel_rps_set_interactive(dev_priv, true);
> > > + intel_state->rps_interactive = true;
> > > + }
> > > +
> > > return 0;
> > > }
> > >
> > > @@ -13120,8 +13133,15 @@ void
> > > intel_cleanup_plane_fb(struct drm_plane *plane,
> > > struct drm_plane_state *old_state)
> > > {
> > > + struct intel_atomic_state *intel_state =
> > > + to_intel_atomic_state(old_state->state);
> > > struct drm_i915_private *dev_priv = to_i915(plane->dev);
> > >
> > > + if (intel_state->rps_interactive) {
> > > + intel_rps_set_interactive(dev_priv, false);
> > > + intel_state->rps_interactive = false;
> > > + }
> >
> > Why is the rps_intearctive flag needed in plane state? I wanted to
> > assume prepare & cleanup hooks are fully symmetric, but this flags makes
> > me unsure. A reviewer with more display knowledge will be needed here I
> > think.
>
> It's so that when we call intel_cleanup_plane_fb() on something that
> wasn't first prepared, we don't decrement the counter. There's a little
> bit of asymmetry at the start where we inherit the plane state.
Doing this kind of global thing from the plane hooks seems a bit
strange. How about just doing this directly from commit_tail()
etc.?
--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
Quoting Tvrtko Ursulin (2018-07-20 13:49:09)
>
> On 12/07/2018 18:38, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 7998e70a3174..5809366ff9f0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13104,6 +13104,19 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > add_rps_boost_after_vblank(new_state->crtc, new_state->fence);
> > }
> >
> > + /*
> > + * We declare pageflips to be interactive and so merit a small bias
> > + * towards upclocking to deliver the frame on time. By only changing
> > + * the RPS thresholds to sample more regularly and aim for higher
> > + * clocks we can hopefully deliver low power workloads (like kodi)
> > + * that are not quite steady state without resorting to forcing
> > + * maximum clocks following a vblank miss (see do_rps_boost()).
> > + */
> > + if (!intel_state->rps_interactive) {
> > + intel_rps_set_interactive(dev_priv, true);
> > + intel_state->rps_interactive = true;
> > + }
> > +
> > return 0;
> > }
> >
> > @@ -13120,8 +13133,15 @@ void
> > intel_cleanup_plane_fb(struct drm_plane *plane,
> > struct drm_plane_state *old_state)
> > {
> > + struct intel_atomic_state *intel_state =
> > + to_intel_atomic_state(old_state->state);
> > struct drm_i915_private *dev_priv = to_i915(plane->dev);
> >
> > + if (intel_state->rps_interactive) {
> > + intel_rps_set_interactive(dev_priv, false);
> > + intel_state->rps_interactive = false;
> > + }
>
> Why is the rps_intearctive flag needed in plane state? I wanted to
> assume prepare & cleanup hooks are fully symmetric, but this flags makes
> me unsure. A reviewer with more display knowledge will be needed here I
> think.
It's so that when we call intel_cleanup_plane_fb() on something that
wasn't first prepared, we don't decrement the counter. There's a little
bit of asymmetry at the start where we inherit the plane state.
> > +
> > /* Should only be called after a successful intel_prepare_plane_fb()!
> > */
> > mutex_lock(_priv->drm.struct_mutex);
> > intel_plane_unpin_fb(to_intel_plane_state(old_state));
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 61e715ddd0d5..544812488821 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -482,6 +482,8 @@ struct intel_atomic_state {
> >*/
> > bool skip_intermediate_wm;
> >
> > + bool rps_interactive;
>
> Should the name at this level be something more tied to the subsystem
> and not implying wider relationships? Like page flip pending? fb_prepared?
But we are in the plane state so anything plane/flip is implied. I think
saying whether or not we've called out to rps is a reasonable name for
the state.
> > + /* Max/min bins are special */
> > + if (val <= rps->min_freq_softlimit)
> > + new_power = LOW_POWER;
> > + if (val >= rps->max_freq_softlimit)
> > + new_power = HIGH_POWER;
> > +
> > + mutex_lock(>power_lock);
>
> Is it worth avoiding the atomic here when GEN < 6?
We don't get here when not !RPS. You mean GEN < 5 ;)
> > + if (rps->interactive)
> > + new_power = HIGH_POWER;
> > + rps_set_power(dev_priv, new_power);
> > + mutex_unlock(>power_lock);
> >
> > rps->last_adj = 0;
>
> This block should go up to the beginning of the function since when
> rps->interactive all preceding calculations are for nothing. And can
> immediately return then.
We have to lock around rps_set_power, so you're not going to avoid
taking the lock here, so I don't think it makes any difference.
Certainly neater than locking everything.
> > +void intel_rps_set_interactive(struct drm_i915_private *dev_priv, bool
> > state)
>
> s/state/interactive/ for more self-documenting function body?
>
> And not s/dev_priv/i915/ ?!? :)
>
> > +{
> > + struct intel_rps *rps = _priv->gt_pm.rps;
> > +
> > + if (INTEL_GEN(dev_priv) < 6)
> > + return;
> > +
> > + mutex_lock(>power_lock);
> > + if (state) {
> > + if (!rps->interactive++ && READ_ONCE(dev_priv->gt.awake))
> > + rps_set_power(dev_priv, HIGH_POWER);
> > + } else {
> > + GEM_BUG_ON(!rps->interactive);
> > + rps->interactive--;
>
> Hm maybe protect this in production code so some missed code flows in
> the atomic paths do not cause underflow and so permanent interactive mode.
Are you suggesting testing is inadequate? ;) Underflow here isn't a big
deal, it just locks in the HIGH_POWER (faster sampling, bias towards
upclocking). Or worse not allow HIGH_POWER, status quo returned.
-Chris
___
Re: [Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
On 12/07/2018 18:38, Chris Wilson wrote:
RPS provides a feedback loop where we use the load during the previous
evaluation interval to decide whether to up or down clock the GPU
frequency. Our responsiveness is split into 3 regimes, a high and low
plateau with the intent to keep the gpu clocked high to cover occasional
stalls under high load, and low despite occasional glitches under steady
low load, and inbetween. However, we run into situations like kodi where
we want to stay at low power (video decoding is done efficiently
inside the fixed function HW and doesn't need high clocks even for high
bitrate streams), but just occasionally the pipeline is more complex
than a video decode and we need a smidgen of extra GPU power to present
on time. In the high power regime, we sample at sub frame intervals with
a bias to upclocking, and conversely at low power we sample over a few
frames worth to provide what we consider to be the right levels of
responsiveness respectively. At low power, we more or less expect to be
kicked out to high power at the start of a busy sequence by waitboosting.
Prior to commit e9af4ea2b9e7 ("drm/i915: Avoid waitboosting on the active
request") whenever we missed the frame or stalled, we would immediate go
full throttle and upclock the GPU to max. But in commit e9af4ea2b9e7, we
relaxed the waitboosting to only apply if the pipeline was deep to avoid
over-committing resources for a near miss. Sadly though, a near miss is
still a miss, and perceptible as jitter in the frame delivery.
To try and prevent the near miss before having to resort to boosting
after the fact, we use the pageflip queue as an indication that we are
in an "interactive" regime and so should sample the load more frequently
to provide power before the frame misses it vblank. This will make us
more favorable to providing a small power increase (one or two bins) as
required rather than going all the way to maximum and then having to
work back down again. (We still keep the waitboosting mechanism around
just in case a dramatic change in system load requires urgent uplocking,
faster than we can provide in a few evaluation intervals.)
v2: Reduce rps_set_interactive to a boolean parameter to avoid the
confusion of what if they wanted a new power mode after pinning to a
different mode (which to choose?)
v3: Only reprogram RPS while the GT is awake, it will be set when we
wake the GT, and while off warns about being used outside of rpm.
v4: Fix deferred application of interactive mode
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107111
Fixes: e9af4ea2b9e7 ("drm/i915: Avoid waitboosting on the active request")
Signed-off-by: Chris Wilson
Cc: Joonas Lahtinen
Cc: Tvrtko Ursulin
Cc: Radoslaw Szwichtenberg
---
drivers/gpu/drm/i915/i915_debugfs.c | 1 +
drivers/gpu/drm/i915/i915_drv.h | 4 ++
drivers/gpu/drm/i915/intel_display.c | 20 ++
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_pm.c | 91 +++-
5 files changed, 89 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 099f97ef2303..ac019bb927d0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2218,6 +2218,7 @@ static int i915_rps_boost_info(struct seq_file *m, void
*data)
seq_printf(m, "CPU waiting? %d\n", count_irq_waiters(dev_priv));
seq_printf(m, "Boosts outstanding? %d\n",
atomic_read(>num_waiters));
+ seq_printf(m, "Interactive? %d\n", READ_ONCE(rps->interactive));
seq_printf(m, "Frequency requested %d\n",
intel_gpu_freq(dev_priv, rps->cur_freq));
seq_printf(m, " min hard:%d, soft:%d; max soft:%d, hard:%d\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01dd29837233..f02fbeee553f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -784,6 +784,8 @@ struct intel_rps {
int last_adj;
enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
+ unsigned int interactive;
+ struct mutex power_lock;
bool enabled;
atomic_t num_waiters;
@@ -3429,6 +3431,8 @@ extern void i915_redisable_vga_power_on(struct
drm_i915_private *dev_priv);
extern bool ironlake_set_drps(struct drm_i915_private *dev_priv, u8 val);
extern void intel_init_pch_refclk(struct drm_i915_private *dev_priv);
extern int intel_set_rps(struct drm_i915_private *dev_priv, u8 val);
+extern void intel_rps_set_interactive(struct drm_i915_private *dev_priv,
+ bool state);
extern bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
bool enable);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7998e70a3174..5809366ff9f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++
[Intel-gfx] [RESEND] drm/i915: Interactive RPS mode
RPS provides a feedback loop where we use the load during the previous
evaluation interval to decide whether to up or down clock the GPU
frequency. Our responsiveness is split into 3 regimes, a high and low
plateau with the intent to keep the gpu clocked high to cover occasional
stalls under high load, and low despite occasional glitches under steady
low load, and inbetween. However, we run into situations like kodi where
we want to stay at low power (video decoding is done efficiently
inside the fixed function HW and doesn't need high clocks even for high
bitrate streams), but just occasionally the pipeline is more complex
than a video decode and we need a smidgen of extra GPU power to present
on time. In the high power regime, we sample at sub frame intervals with
a bias to upclocking, and conversely at low power we sample over a few
frames worth to provide what we consider to be the right levels of
responsiveness respectively. At low power, we more or less expect to be
kicked out to high power at the start of a busy sequence by waitboosting.
Prior to commit e9af4ea2b9e7 ("drm/i915: Avoid waitboosting on the active
request") whenever we missed the frame or stalled, we would immediate go
full throttle and upclock the GPU to max. But in commit e9af4ea2b9e7, we
relaxed the waitboosting to only apply if the pipeline was deep to avoid
over-committing resources for a near miss. Sadly though, a near miss is
still a miss, and perceptible as jitter in the frame delivery.
To try and prevent the near miss before having to resort to boosting
after the fact, we use the pageflip queue as an indication that we are
in an "interactive" regime and so should sample the load more frequently
to provide power before the frame misses it vblank. This will make us
more favorable to providing a small power increase (one or two bins) as
required rather than going all the way to maximum and then having to
work back down again. (We still keep the waitboosting mechanism around
just in case a dramatic change in system load requires urgent uplocking,
faster than we can provide in a few evaluation intervals.)
v2: Reduce rps_set_interactive to a boolean parameter to avoid the
confusion of what if they wanted a new power mode after pinning to a
different mode (which to choose?)
v3: Only reprogram RPS while the GT is awake, it will be set when we
wake the GT, and while off warns about being used outside of rpm.
v4: Fix deferred application of interactive mode
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107111
Fixes: e9af4ea2b9e7 ("drm/i915: Avoid waitboosting on the active request")
Signed-off-by: Chris Wilson
Cc: Joonas Lahtinen
Cc: Tvrtko Ursulin
Cc: Radoslaw Szwichtenberg
---
drivers/gpu/drm/i915/i915_debugfs.c | 1 +
drivers/gpu/drm/i915/i915_drv.h | 4 ++
drivers/gpu/drm/i915/intel_display.c | 20 ++
drivers/gpu/drm/i915/intel_drv.h | 2 +
drivers/gpu/drm/i915/intel_pm.c | 91 +++-
5 files changed, 89 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 099f97ef2303..ac019bb927d0 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2218,6 +2218,7 @@ static int i915_rps_boost_info(struct seq_file *m, void
*data)
seq_printf(m, "CPU waiting? %d\n", count_irq_waiters(dev_priv));
seq_printf(m, "Boosts outstanding? %d\n",
atomic_read(>num_waiters));
+ seq_printf(m, "Interactive? %d\n", READ_ONCE(rps->interactive));
seq_printf(m, "Frequency requested %d\n",
intel_gpu_freq(dev_priv, rps->cur_freq));
seq_printf(m, " min hard:%d, soft:%d; max soft:%d, hard:%d\n",
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 01dd29837233..f02fbeee553f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -784,6 +784,8 @@ struct intel_rps {
int last_adj;
enum { LOW_POWER, BETWEEN, HIGH_POWER } power;
+ unsigned int interactive;
+ struct mutex power_lock;
bool enabled;
atomic_t num_waiters;
@@ -3429,6 +3431,8 @@ extern void i915_redisable_vga_power_on(struct
drm_i915_private *dev_priv);
extern bool ironlake_set_drps(struct drm_i915_private *dev_priv, u8 val);
extern void intel_init_pch_refclk(struct drm_i915_private *dev_priv);
extern int intel_set_rps(struct drm_i915_private *dev_priv, u8 val);
+extern void intel_rps_set_interactive(struct drm_i915_private *dev_priv,
+ bool state);
extern bool intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
bool enable);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7998e70a3174..5809366ff9f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13104,6 +13104,19 @@
