On 2019-09-08 at 20:55:17 +0300, Imre Deak wrote:
Hi Imre ,
Thanks for review, could you please provide your response on below
comments.
> On Sat, Sep 07, 2019 at 10:44:42PM +0530, Anshuman Gupta wrote:
> > DC3CO is useful power state, when DMC detects PSR2 idle frame
> > while an active video playback, playing 30fps video on 60hz panel
> > is the classic example of this use case.
> > DC5 and DC6 saves more power, but can't be entered during video
> > playback because there are not enough idle frames in a row to meet
> > most PSR2 panel deep sleep entry requirement typically 4 frames.
>
> Please also explain why DC3co will be enabled only for flips but not for
> other frontbuffer flush events (ORIGIN_CS/DIRTYFB etc.) and that we
> could enable it for those too by switching to manual PSR tracking and
> programming only 1 idle frame for deep sleep (see below).
>
> Also explaining that the frontbuffer invalidate event doesn't need to be
> acted on (b/c of PSR exit) would be helpful.
>
> >
> > It will be worthy to enable DC3CO after completion of each flip
> > and switch back to DC5 when display is idle, as driver doesn't
> > differentiate between video playback and a normal flip.
> > It is safer to allow DC5 after 6 idle frame, as PSR2 requires
> > minimum 6 idle frame.
>
> It would be clearer to say here that after a flip we enable DC3co, after
> which we wait manually 6 frames (by scheduling the idle frame work) at
> which point we enable PSR deep sleep with 6 idle frames. After this 6
> idle frames the HW will enter deep sleep and DC5 will be entered
> after this by DMC at some point.
>
> The claim that we _have_ to make the HW wait for 6 idle frames before it
> enters deep sleep doesn't make much sense to me, would be good to see a
> reference to that if it really exists. That setting seems to only serve
> the purpose to avoid update lags, but in the future (as discussed with
> Ville) we should switch to manual PSR tracking and for that we would
> program the HW to wait only 1 idle frame before entering deep sleep and
> rely only on the manual 6 idle frame wait (via the idle frame work) to
> avoid update lags.
>
> >
> > v2: calculated s/w state to switch over dc3co when there is an
> > update. [Imre]
> > used cancel_delayed_work_sync() in order to avoid any race
> > with already scheduled delayed work. [Imre]
> > v3: cancel_delayed_work_sync() may blocked the commit work.
> > Hence dropping it, dc5_idle_thread() checks the valid wakeref before
> > putting the reference count, which avoids any chances of dropping
> > a zero wakeref. [Imre (IRC)]
> > v4: use frontbuffer flush mechanism. [Imre]
> >
> > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > Cc: Animesh Manna <animesh.manna@xxxxxxxxx>
> > Signed-off-by: Anshuman Gupta <anshuman.gupta@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c | 2 +
> > .../drm/i915/display/intel_display_power.c | 79 +++++++++++++++++++
> > .../drm/i915/display/intel_display_power.h | 6 ++
> > .../gpu/drm/i915/display/intel_frontbuffer.c | 1 +
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > 5 files changed, 89 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 84488f87d058..2754e8ee693f 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -16206,6 +16206,7 @@ int intel_modeset_init(struct drm_device *dev)
> > init_llist_head(&dev_priv->atomic_helper.free_list);
> > INIT_WORK(&dev_priv->atomic_helper.free_work,
> > intel_atomic_helper_free_state_worker);
> > + INIT_DELAYED_WORK(&dev_priv->csr.idle_work, tgl_dc5_idle_thread);
> >
> > intel_init_quirks(dev_priv);
> >
> > @@ -17147,6 +17148,7 @@ void intel_modeset_driver_remove(struct drm_device *dev)
> > flush_workqueue(dev_priv->modeset_wq);
> >
> > flush_work(&dev_priv->atomic_helper.free_work);
> > + flush_delayed_work(&dev_priv->csr.idle_work);
>
> This is racy as the work could be still running, but also would leave a
> few other places with the work running like suspend, so let's just make
> sure that it's not running any more after encoder disabling.
>
> > WARN_ON(!llist_empty(&dev_priv->atomic_helper.free_list));
> >
> > /*
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.c b/drivers/gpu/drm/i915/display/intel_display_power.c
> > index ecce118b5b0e..c110f04c9733 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.c
> > @@ -20,6 +20,7 @@
> > #include "intel_sideband.h"
> > #include "intel_tc.h"
> > #include "intel_pm.h"
> > +#include "intel_psr.h"
> >
> > bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
> > enum i915_power_well_id power_well_id);
> > @@ -773,6 +774,27 @@ static void gen9_set_dc_state(struct drm_i915_private *dev_priv, u32 state)
> > dev_priv->csr.dc_state = val & mask;
> > }
> >
> > +static u32 intel_get_frame_time_us(const struct intel_crtc_state *cstate)
> > +{
> > + u32 pixel_rate, crtc_htotal, crtc_vtotal;
> > + u32 frametime_us;
> > +
> > + if (!cstate || !cstate->base.active)
> > + return 0;
> > +
> > + pixel_rate = cstate->pixel_rate;
> > +
> > + if (WARN_ON(pixel_rate == 0))
> > + return 0;
> > +
> > + crtc_htotal = cstate->base.adjusted_mode.crtc_htotal;
> > + crtc_vtotal = cstate->base.adjusted_mode.crtc_vtotal;
> > + frametime_us = DIV_ROUND_UP(crtc_htotal * crtc_vtotal * 1000ULL,
> > + pixel_rate);
> > +
> > + return frametime_us;
> > +}
> > +
> > void tgl_disable_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> > {
> > struct drm_i915_private *dev_priv = to_i915(cstate->base.crtc->dev);
> > @@ -817,6 +839,49 @@ void tgl_enable_psr2_transcoder_exitline(const struct intel_crtc_state *cstate)
> > I915_WRITE(EXITLINE(cstate->cpu_transcoder), val);
> > }
> >
> > +void tgl_dc3co_flush(struct drm_i915_private *dev_priv,
> > + unsigned int frontbuffer_bits, enum fb_op_origin origin)
> > +{
> > + struct intel_crtc_state *cstate;
> > + u32 delay;
> > + unsigned int busy_frontbuffer_bits;
> > +
> > + if (!IS_TIGERLAKE(dev_priv))
> > + return;
> > +
> > + if (origin != ORIGIN_FLIP)
> > + return;
> > +
> > + if (!dev_priv->csr.dc3co_crtc)
> > + return;
> > +
> > + cstate = to_intel_crtc_state(dev_priv->csr.dc3co_crtc->base.state);
> > + frontbuffer_bits &=
> > + INTEL_FRONTBUFFER_ALL_MASK(dev_priv->csr.dc3co_crtc->pipe);
> > + busy_frontbuffer_bits &= ~frontbuffer_bits;
>
> Hrm, this looks wrong. Also it depends on the PSR mechanism, so this
> whole DC3co flush logic should rather be done from the PSR flush func,
> using psr.pipe.
>
Hmm initially i have planned to have entire logic under psr flush func
but PSR invalidate/flush logic for ORIGIN_FLIP relies on H/W tracking,
PSR invalidate and flush call has early return for ORIGIN_FLIP
unlike DC3CO case where we are only interested in ORIGIN_FLIP requests.
> > +
> > + mutex_lock(&dev_priv->psr.lock);
> > +
> > + if (!dev_priv->psr.psr2_enabled || !dev_priv->psr.active)
> > + goto unlock;
> > +
> > + /*
> > + * At every flip frontbuffer flush first cancel the delayed work,
> > + * when delayed schedules that means display has been idle
> > + * for the 6 idle frame.
> > + */
> > + if (!busy_frontbuffer_bits) {
> > + cancel_delayed_work(&dev_priv->csr.idle_work);
>
> The above is racy.
Yes tgl_dc5_idle_thread() can even run after this but in that case also
tgl_set_target_state() is protected by the power domain locks.
Do u see any other harm of it apart from setting target_dc_state
immediately to DC5/DC6 after it sets to DC3CO (IMO this would be a little
penalty for a VBLANK in next page flip it will again set target_dc_state to DC3CO).
I can think of two possible solutions to avoid this race.
1. cancel_delayed_work_sync().
2. Having a flag to indicate that delayed work has been canceled
and same can be used by tgl_dc5_idle_thread() to have an early return.
Please suggest your opinion on it.
>
> > + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_DC3CO, false);
> > + delay = DC5_REQ_IDLE_FRAMES * intel_get_frame_time_us(cstate);
> > + schedule_delayed_work(&dev_priv->csr.idle_work,
> > + usecs_to_jiffies(delay));
> > + }
> > +
> > +unlock:
> > + mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > static bool tgl_dc3co_is_edp_connected(struct intel_crtc_state *crtc_state)
> > {
> > struct drm_atomic_state *state = crtc_state->base.state;
> > @@ -880,6 +945,14 @@ void tgl_dc3co_crtc_get_config(struct intel_crtc_state *crtc_state)
> > }
> > }
> >
> > +void tgl_dc5_idle_thread(struct work_struct *work)
> > +{
> > + struct drm_i915_private *dev_priv =
> > + container_of(work, typeof(*dev_priv), csr.idle_work.work);
> > +
> > + tgl_set_target_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6, true);
>
> So it would result in enabling deep sleep, but without the PSR lock.
> That's one reason we should really keep the PSR specific parts here.
>
> > +}
> > +
> > static void tgl_allow_dc3co(struct drm_i915_private *dev_priv)
> > {
> > if (!dev_priv->psr.sink_psr2_support)
> > @@ -1155,6 +1228,9 @@ void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > if (state == dev_priv->csr.max_dc_state)
> > goto unlock;
> >
> > + if (!psr2_deep_sleep)
> > + tgl_psr2_deep_sleep_disable(dev_priv);
> > +
> > if (!dc_off_enabled) {
> > /*
> > * Need to disable the DC off power well to
> > @@ -1167,6 +1243,9 @@ void tgl_set_target_dc_state(struct drm_i915_private *dev_priv, u32 state,
> > }
> > dev_priv->csr.max_dc_state = state;
> >
> > + if (psr2_deep_sleep)
> > + tgl_psr2_deep_sleep_enable(dev_priv);
> > +
> > unlock:
> > mutex_unlock(&power_domains->lock);
> > }
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_power.h b/drivers/gpu/drm/i915/display/intel_display_power.h
> > index d77a5a53f543..df096d64c744 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_power.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_power.h
> > @@ -9,6 +9,9 @@
> > #include "intel_display.h"
> > #include "intel_runtime_pm.h"
> > #include "i915_reg.h"
> > +#include "intel_frontbuffer.h"
> > +
> > +#define DC5_REQ_IDLE_FRAMES 6
>
> No need for a define for this.
>
> >
> > struct drm_i915_private;
> > struct intel_encoder;
> > @@ -266,6 +269,9 @@ void tgl_dc3co_crtc_compute_config(struct drm_i915_private *dev_priv,
> > void tgl_dc3co_crtc_get_config(struct intel_crtc_state *crtc_state);
> > void tgl_disable_psr2_transcoder_exitline(const struct intel_crtc_state *state);
> > void tgl_enable_psr2_transcoder_exitline(const struct intel_crtc_state *state);
> > +void tgl_dc3co_flush(struct drm_i915_private *dev_priv,
> > + unsigned int frontbuffer_bits, enum fb_op_origin origin);
> > +void tgl_dc5_idle_thread(struct work_struct *work);
> >
> > const char *
> > intel_display_power_domain_str(enum intel_display_power_domain domain);
> > diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > index fc40dc1fdbcc..c3b10f6e4382 100644
> > --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
> > @@ -90,6 +90,7 @@ static void frontbuffer_flush(struct drm_i915_private *i915,
> > might_sleep();
> > intel_edp_drrs_flush(i915, frontbuffer_bits);
> > intel_psr_flush(i915, frontbuffer_bits, origin);
> > + tgl_dc3co_flush(i915, frontbuffer_bits, origin);
> > intel_fbc_flush(i915, frontbuffer_bits, origin);
> > }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 3218b0319852..fe71119a458e 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -338,6 +338,7 @@ struct intel_csr {
> > u32 dc_state;
> > u32 max_dc_state;
> > u32 allowed_dc_mask;
> > + struct delayed_work idle_work;
> > intel_wakeref_t wakeref;
> > /* cache the crtc on which DC3CO will be allowed */
> > struct intel_crtc *dc3co_crtc;
> > --
> > 2.21.0
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
