* [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused. @ 2018-05-14 20:49 Tarun Vyas 2018-05-14 20:49 ` [PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle Tarun Vyas ` (5 more replies) 0 siblings, 6 replies; 17+ messages in thread From: Tarun Vyas @ 2018-05-14 20:49 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula, dhinakaran.pandiyan, rodrigo.vivi intel_pipe_update_start also needs to wait for PSR to idle out. Need some minor modifications in psr_wait_for_idle in order to reuse it. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index db27f2faa1de..40aafc0f4513 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) i915_reg_t reg; u32 mask; int err; + bool wait = false; + + mutex_lock(&dev_priv->psr.lock); intel_dp = dev_priv->psr.enabled; if (!intel_dp) - return false; + goto unlock; + wait = true; if (HAS_DDI(dev_priv)) { if (dev_priv->psr.psr2_enabled) { reg = EDP_PSR2_STATUS; @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) mask = VLV_EDP_PSR_IN_TRANS; } +unlock: mutex_unlock(&dev_priv->psr.lock); - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); - if (err) - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); + if(wait) { + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); + if (err) { + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); + wait = false; + } + } - /* After the unlocked wait, verify that PSR is still wanted! */ - mutex_lock(&dev_priv->psr.lock); - return err == 0 && dev_priv->psr.enabled; + return wait; } static void intel_psr_work(struct work_struct *work) @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) struct drm_i915_private *dev_priv = container_of(work, typeof(*dev_priv), psr.work.work); - mutex_lock(&dev_priv->psr.lock); /* * We have to make sure PSR is ready for re-enable @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) * and be ready for re-enable. */ if (!psr_wait_for_idle(dev_priv)) - goto unlock; + return; - /* + /* After the unlocked wait, verify that PSR is still wanted! * The delayed work can race with an invalidate hence we need to * recheck. Since psr_flush first clears this and then reschedules we * won't ever miss a flush when bailing out here. */ - if (dev_priv->psr.busy_frontbuffer_bits) + mutex_lock(&dev_priv->psr.lock); + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) goto unlock; intel_psr_activate(dev_priv->psr.enabled); -- 2.13.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas @ 2018-05-14 20:49 ` Tarun Vyas 2018-05-14 20:49 ` [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas ` (4 subsequent siblings) 5 siblings, 0 replies; 17+ messages in thread From: Tarun Vyas @ 2018-05-14 20:49 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula, dhinakaran.pandiyan, rodrigo.vivi We have new users (follow up patch). So, un-statify it Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_psr.c | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d7dbca1aabff..d8f8282f1fad 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1919,6 +1919,7 @@ void intel_psr_compute_config(struct intel_dp *intel_dp, struct intel_crtc_state *crtc_state); void intel_psr_irq_control(struct drm_i915_private *dev_priv, bool debug); void intel_psr_irq_handler(struct drm_i915_private *dev_priv, u32 psr_iir); +bool psr_wait_for_idle(struct drm_i915_private *dev_priv); /* intel_runtime_pm.c */ int intel_power_domains_init(struct drm_i915_private *); diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 40aafc0f4513..e69859fa782f 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -883,7 +883,7 @@ void intel_psr_disable(struct intel_dp *intel_dp, cancel_delayed_work_sync(&dev_priv->psr.work); } -static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) +bool psr_wait_for_idle(struct drm_i915_private *dev_priv) { struct intel_dp *intel_dp; i915_reg_t reg; -- 2.13.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas 2018-05-14 20:49 ` [PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle Tarun Vyas @ 2018-05-14 20:49 ` Tarun Vyas 2018-05-14 21:16 ` Chris Wilson ` (2 more replies) 2018-05-14 21:15 ` [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Chris Wilson ` (3 subsequent siblings) 5 siblings, 3 replies; 17+ messages in thread From: Tarun Vyas @ 2018-05-14 20:49 UTC (permalink / raw) To: intel-gfx; +Cc: jani.nikula, dhinakaran.pandiyan, rodrigo.vivi The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then the pipe_update_start call schedules itself out to check back later. On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but lags w.r.t core kernel code, hot plugging an external display triggers tons of "potential atomic update errors" in the dmesg, on *pipe A*. A closer analysis reveals that we try to read the scanline 3 times and eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some* reason we loop inside intel_pipe_update start for ~2+ msec which in this case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL counter, hence no error. On the other hand, the ChromeOS kernel spends ~1.1 msec looping inside intel_pipe_update_start and hence errors out b/c the source is still in PSR. Regardless, we should wait for PSR exit (if PSR is supported and active on the current pipe) before reading the PIPEDSL, b/c if we haven't fully exited PSR, then checking for vblank evasion isn't actually applicable. This scenario applies to a configuration with an additional pipe, as of now Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index ee23613f9fd4..481d310e5c3b 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) VBLANK_EVASION_TIME_US); max = vblank_start - 1; - local_irq_disable(); - if (min <= 0 || max <= 0) return; if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) return; + if(new_crtc_state->has_psr && dev_priv->psr.active) + psr_wait_for_idle(dev_priv); + + local_irq_disable(); + crtc->debug.min_vbl = min; crtc->debug.max_vbl = max; trace_i915_pipe_update_start(crtc); -- 2.13.5 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-05-14 20:49 ` [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas @ 2018-05-14 21:16 ` Chris Wilson 2018-05-15 19:38 ` Tarun Vyas 2018-05-15 3:16 ` kbuild test robot 2018-06-19 21:27 ` Dhinakaran Pandiyan 2 siblings, 1 reply; 17+ messages in thread From: Chris Wilson @ 2018-05-14 21:16 UTC (permalink / raw) To: Tarun Vyas, intel-gfx; +Cc: jani.nikula, dhinakaran.pandiyan, rodrigo.vivi Quoting Tarun Vyas (2018-05-14 21:49:22) > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then > the pipe_update_start call schedules itself out to check back later. > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but > lags w.r.t core kernel code, hot plugging an external display triggers > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A > closer analysis reveals that we try to read the scanline 3 times and > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL > stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some* > reason we loop inside intel_pipe_update start for ~2+ msec which in this > case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL > counter, hence no error. On the other hand, the ChromeOS kernel spends > ~1.1 msec looping inside intel_pipe_update_start and hence errors out > b/c the source is still in PSR. > > Regardless, we should wait for PSR exit (if PSR is supported and active > on the current pipe) before reading the PIPEDSL, b/c if we haven't > fully exited PSR, then checking for vblank evasion isn't actually > applicable. > > This scenario applies to a configuration with an additional pipe, > as of now > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index ee23613f9fd4..481d310e5c3b 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) > VBLANK_EVASION_TIME_US); > max = vblank_start - 1; > > - local_irq_disable(); > - > if (min <= 0 || max <= 0) > return; > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > return; > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > + psr_wait_for_idle(dev_priv); > + > + local_irq_disable(); Pop quiz, does intel_pipe_update_finish() unconditionally assume it is called with irqs disabled? -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-05-14 21:16 ` Chris Wilson @ 2018-05-15 19:38 ` Tarun Vyas 0 siblings, 0 replies; 17+ messages in thread From: Tarun Vyas @ 2018-05-15 19:38 UTC (permalink / raw) To: Chris Wilson; +Cc: jani.nikula, intel-gfx, dhinakaran.pandiyan, rodrigo.vivi On Mon, May 14, 2018 at 10:16:38PM +0100, Chris Wilson wrote: > Quoting Tarun Vyas (2018-05-14 21:49:22) > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then > > the pipe_update_start call schedules itself out to check back later. > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but > > lags w.r.t core kernel code, hot plugging an external display triggers > > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A > > closer analysis reveals that we try to read the scanline 3 times and > > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for *some* > > reason we loop inside intel_pipe_update start for ~2+ msec which in this > > case is more than enough to exit PSR fully, hence an *unstuck* PIPEDSL > > counter, hence no error. On the other hand, the ChromeOS kernel spends > > ~1.1 msec looping inside intel_pipe_update_start and hence errors out > > b/c the source is still in PSR. > > > > Regardless, we should wait for PSR exit (if PSR is supported and active > > on the current pipe) before reading the PIPEDSL, b/c if we haven't > > fully exited PSR, then checking for vblank evasion isn't actually > > applicable. > > > > This scenario applies to a configuration with an additional pipe, > > as of now > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > > index ee23613f9fd4..481d310e5c3b 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) > > VBLANK_EVASION_TIME_US); > > max = vblank_start - 1; > > > > - local_irq_disable(); > > - > > if (min <= 0 || max <= 0) > > return; > > > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > return; > > > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > > + psr_wait_for_idle(dev_priv); > > + > > + local_irq_disable(); > > Pop quiz, does intel_pipe_update_finish() unconditionally assume it is > called with irqs disabled? > -Chris Unless local_irq_disable() fails, intel_pipe_update_end() should always get called with irqs disabled, from what it looks like to me. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-05-14 20:49 ` [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas 2018-05-14 21:16 ` Chris Wilson @ 2018-05-15 3:16 ` kbuild test robot 2018-06-19 21:27 ` Dhinakaran Pandiyan 2 siblings, 0 replies; 17+ messages in thread From: kbuild test robot @ 2018-05-15 3:16 UTC (permalink / raw) To: Tarun Vyas Cc: jani.nikula, intel-gfx, rodrigo.vivi, kbuild-all, dhinakaran.pandiyan [-- Attachment #1: Type: text/plain, Size: 5370 bytes --] Hi Tarun, Thank you for the patch! Yet something to improve: [auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on v4.17-rc5 next-20180514] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Tarun-Vyas/drm-i915-Wait-for-PSR-exit-before-checking-for-vblank-evasion/20180515-103355 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: i386-randconfig-x000-201819 (attached as .config) compiler: gcc-7 (Debian 7.3.0-16) 7.3.0 reproduce: # save the attached .config to linux build tree make ARCH=i386 All errors (new ones prefixed by >>): drivers/gpu/drm/i915/intel_sprite.c: In function 'intel_pipe_update_start': >> drivers/gpu/drm/i915/intel_sprite.c:117:3: error: implicit declaration of function 'psr_wait_for_idle'; did you mean 'apic_wait_icr_idle'? [-Werror=implicit-function-declaration] psr_wait_for_idle(dev_priv); ^~~~~~~~~~~~~~~~~ apic_wait_icr_idle cc1: some warnings being treated as errors vim +117 drivers/gpu/drm/i915/intel_sprite.c 76 77 /** 78 * intel_pipe_update_start() - start update of a set of display registers 79 * @new_crtc_state: the new crtc state 80 * 81 * Mark the start of an update to pipe registers that should be updated 82 * atomically regarding vblank. If the next vblank will happens within 83 * the next 100 us, this function waits until the vblank passes. 84 * 85 * After a successful call to this function, interrupts will be disabled 86 * until a subsequent call to intel_pipe_update_end(). That is done to 87 * avoid random delays. 88 */ 89 void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) 90 { 91 struct intel_crtc *crtc = to_intel_crtc(new_crtc_state->base.crtc); 92 struct drm_i915_private *dev_priv = to_i915(crtc->base.dev); 93 const struct drm_display_mode *adjusted_mode = &new_crtc_state->base.adjusted_mode; 94 long timeout = msecs_to_jiffies_timeout(1); 95 int scanline, min, max, vblank_start; 96 wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base); 97 bool need_vlv_dsi_wa = (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) && 98 intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI); 99 DEFINE_WAIT(wait); 100 101 vblank_start = adjusted_mode->crtc_vblank_start; 102 if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) 103 vblank_start = DIV_ROUND_UP(vblank_start, 2); 104 105 /* FIXME needs to be calibrated sensibly */ 106 min = vblank_start - intel_usecs_to_scanlines(adjusted_mode, 107 VBLANK_EVASION_TIME_US); 108 max = vblank_start - 1; 109 110 if (min <= 0 || max <= 0) 111 return; 112 113 if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) 114 return; 115 116 if(new_crtc_state->has_psr && dev_priv->psr.active) > 117 psr_wait_for_idle(dev_priv); 118 119 local_irq_disable(); 120 121 crtc->debug.min_vbl = min; 122 crtc->debug.max_vbl = max; 123 trace_i915_pipe_update_start(crtc); 124 125 for (;;) { 126 /* 127 * prepare_to_wait() has a memory barrier, which guarantees 128 * other CPUs can see the task state update by the time we 129 * read the scanline. 130 */ 131 prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE); 132 133 scanline = intel_get_crtc_scanline(crtc); 134 if (scanline < min || scanline > max) 135 break; 136 137 if (!timeout) { 138 DRM_ERROR("Potential atomic update failure on pipe %c\n", 139 pipe_name(crtc->pipe)); 140 break; 141 } 142 143 local_irq_enable(); 144 145 timeout = schedule_timeout(timeout); 146 147 local_irq_disable(); 148 } 149 150 finish_wait(wq, &wait); 151 152 drm_crtc_vblank_put(&crtc->base); 153 154 /* 155 * On VLV/CHV DSI the scanline counter would appear to 156 * increment approx. 1/3 of a scanline before start of vblank. 157 * The registers still get latched at start of vblank however. 158 * This means we must not write any registers on the first 159 * line of vblank (since not the whole line is actually in 160 * vblank). And unfortunately we can't use the interrupt to 161 * wait here since it will fire too soon. We could use the 162 * frame start interrupt instead since it will fire after the 163 * critical scanline, but that would require more changes 164 * in the interrupt code. So for now we'll just do the nasty 165 * thing and poll for the bad scanline to pass us by. 166 * 167 * FIXME figure out if BXT+ DSI suffers from this as well 168 */ 169 while (need_vlv_dsi_wa && scanline == vblank_start) 170 scanline = intel_get_crtc_scanline(crtc); 171 172 crtc->debug.scanline_start = scanline; 173 crtc->debug.start_vbl_time = ktime_get(); 174 crtc->debug.start_vbl_count = intel_crtc_get_vblank_counter(crtc); 175 176 trace_i915_pipe_update_vblank_evaded(crtc); 177 } 178 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 32668 bytes --] [-- Attachment #3: Type: text/plain, Size: 160 bytes --] _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-05-14 20:49 ` [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas 2018-05-14 21:16 ` Chris Wilson 2018-05-15 3:16 ` kbuild test robot @ 2018-06-19 21:27 ` Dhinakaran Pandiyan 2018-06-19 21:54 ` Dhinakaran Pandiyan 2 siblings, 1 reply; 17+ messages in thread From: Dhinakaran Pandiyan @ 2018-06-19 21:27 UTC (permalink / raw) To: Tarun Vyas, intel-gfx; +Cc: jani.nikula, rodrigo.vivi On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote: > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, then > the pipe_update_start call schedules itself out to check back later. > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 but > lags w.r.t core kernel code, hot plugging an external display > triggers > tons of "potential atomic update errors" in the dmesg, on *pipe A*. A > closer analysis reveals that we try to read the scanline 3 times and > eventually timeout, b/c PSR hasn't exited fully leading to a PIPEDSL > stuck @ 1599. This issue is not seen on upstream kernels, b/c for > *some* > reason we loop inside intel_pipe_update start for ~2+ msec which in > this > case is more than enough to exit PSR fully, hence an *unstuck* > PIPEDSL > counter, hence no error. On the other hand, the ChromeOS kernel > spends > ~1.1 msec looping inside intel_pipe_update_start and hence errors out > b/c the source is still in PSR. > > Regardless, we should wait for PSR exit (if PSR is supported and > active > on the current pipe) before reading the PIPEDSL, b/c if we haven't > fully exited PSR, then checking for vblank evasion isn't actually > applicable. > > This scenario applies to a configuration with an additional pipe, > as of now > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index ee23613f9fd4..481d310e5c3b 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct > intel_crtc_state *new_crtc_state) > VBLANK_EVASION > _TIME_US); > max = vblank_start - 1; > > - local_irq_disable(); > - > if (min <= 0 || max <= 0) > return; > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > return; > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > + psr_wait_for_idle(dev_priv); How about just waiting for PSR_STATUS to idle without grabbing any locks or checking whether PSR is active? Status should be idle if PSR was disabled or on it's way to becoming idle if it was enabled. Even if PSR did get enabled while we are in pipe_update_start(), it will not be active as long as VBIs are enabled. > + > + local_irq_disable(); > + > crtc->debug.min_vbl = min; > crtc->debug.max_vbl = max; > trace_i915_pipe_update_start(crtc); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-06-19 21:27 ` Dhinakaran Pandiyan @ 2018-06-19 21:54 ` Dhinakaran Pandiyan 2018-06-19 21:59 ` Tarun Vyas 0 siblings, 1 reply; 17+ messages in thread From: Dhinakaran Pandiyan @ 2018-06-19 21:54 UTC (permalink / raw) To: Tarun Vyas, intel-gfx; +Cc: jani.nikula, rodrigo.vivi On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote: > On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote: > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, > > then > > the pipe_update_start call schedules itself out to check back > > later. > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 > > but > > lags w.r.t core kernel code, hot plugging an external display > > triggers > > tons of "potential atomic update errors" in the dmesg, on *pipe A*. > > A > > closer analysis reveals that we try to read the scanline 3 times > > and > > eventually timeout, b/c PSR hasn't exited fully leading to a > > PIPEDSL > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for > > *some* > > reason we loop inside intel_pipe_update start for ~2+ msec which in > > this > > case is more than enough to exit PSR fully, hence an *unstuck* > > PIPEDSL > > counter, hence no error. On the other hand, the ChromeOS kernel > > spends > > ~1.1 msec looping inside intel_pipe_update_start and hence errors > > out > > b/c the source is still in PSR. > > > > Regardless, we should wait for PSR exit (if PSR is supported and > > active > > on the current pipe) before reading the PIPEDSL, b/c if we haven't > > fully exited PSR, then checking for vblank evasion isn't actually > > applicable. > > > > This scenario applies to a configuration with an additional pipe, > > as of now > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index ee23613f9fd4..481d310e5c3b 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct > > intel_crtc_state *new_crtc_state) > > VBLANK_EVASI > > ON > > _TIME_US); > > max = vblank_start - 1; > > > > - local_irq_disable(); > > - > > if (min <= 0 || max <= 0) > > return; > > > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > return; > > > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > > + psr_wait_for_idle(dev_priv); > How about just waiting for PSR_STATUS to idle without grabbing any > locks or checking whether PSR is active? > > Status should be idle if PSR was disabled or on it's way to becoming > idle if it was enabled. Even if PSR did get enabled while we are in > pipe_update_start(), it will not be active as long as VBIs are > enabled. > Correct me if this was already considered, why not wait until the scanline counter starts moving? I see we have a intel_wait_for_pipe_scanline_moving(crtc) that's used when the pipe is enabled. -DK _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-06-19 21:54 ` Dhinakaran Pandiyan @ 2018-06-19 21:59 ` Tarun Vyas 2018-06-19 23:11 ` Dhinakaran Pandiyan 2018-06-21 22:43 ` Tarun Vyas 0 siblings, 2 replies; 17+ messages in thread From: Tarun Vyas @ 2018-06-19 21:59 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: jani.nikula, intel-gfx, rodrigo.vivi On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote: > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote: > > On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote: > > > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, > > > then > > > the pipe_update_start call schedules itself out to check back > > > later. > > > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 > > > but > > > lags w.r.t core kernel code, hot plugging an external display > > > triggers > > > tons of "potential atomic update errors" in the dmesg, on *pipe A*. > > > A > > > closer analysis reveals that we try to read the scanline 3 times > > > and > > > eventually timeout, b/c PSR hasn't exited fully leading to a > > > PIPEDSL > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for > > > *some* > > > reason we loop inside intel_pipe_update start for ~2+ msec which in > > > this > > > case is more than enough to exit PSR fully, hence an *unstuck* > > > PIPEDSL > > > counter, hence no error. On the other hand, the ChromeOS kernel > > > spends > > > ~1.1 msec looping inside intel_pipe_update_start and hence errors > > > out > > > b/c the source is still in PSR. > > > > > > Regardless, we should wait for PSR exit (if PSR is supported and > > > active > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't > > > fully exited PSR, then checking for vblank evasion isn't actually > > > applicable. > > > > > > This scenario applies to a configuration with an additional pipe, > > > as of now > > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > index ee23613f9fd4..481d310e5c3b 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct > > > intel_crtc_state *new_crtc_state) > > > VBLANK_EVASI > > > ON > > > _TIME_US); > > > max = vblank_start - 1; > > > > > > - local_irq_disable(); > > > - > > > if (min <= 0 || max <= 0) > > > return; > > > > > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > > return; > > > > > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > > > + psr_wait_for_idle(dev_priv); > > How about just waiting for PSR_STATUS to idle without grabbing any > > locks or checking whether PSR is active? > > > > Status should be idle if PSR was disabled or on it's way to becoming > > idle if it was enabled. Even if PSR did get enabled while we are in > > pipe_update_start(), it will not be active as long as VBIs are > > enabled. > > Right, if we are OK with some duplication (of psr_wait_for_idle) inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done in psr_wait_for_idle and then just wait without grabbing any locks, so essentially a lockless version of psr_wait_for_idle() > Correct me if this was already considered, why not wait until the > scanline counter starts moving? I see we have a > intel_wait_for_pipe_scanline_moving(crtc) that's used when the > pipe is enabled. > > -DK Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? Heuristically, waiting for PSR idle has almost always returned within < 2 msec. Occasionally it takes upto 1 full frame. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-06-19 21:59 ` Tarun Vyas @ 2018-06-19 23:11 ` Dhinakaran Pandiyan 2018-06-21 22:43 ` Tarun Vyas 1 sibling, 0 replies; 17+ messages in thread From: Dhinakaran Pandiyan @ 2018-06-19 23:11 UTC (permalink / raw) To: Tarun Vyas; +Cc: jani.nikula, intel-gfx, rodrigo.vivi On Tue, 2018-06-19 at 14:59 -0700, Tarun Vyas wrote: > On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote: > > > > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote: > > > > > > On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote: > > > > > > > > > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully > > > > exited, > > > > then > > > > the pipe_update_start call schedules itself out to check back > > > > later. > > > > > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t > > > > drm/i915 > > > > but > > > > lags w.r.t core kernel code, hot plugging an external display > > > > triggers > > > > tons of "potential atomic update errors" in the dmesg, on *pipe > > > > A*. > > > > A > > > > closer analysis reveals that we try to read the scanline 3 > > > > times > > > > and > > > > eventually timeout, b/c PSR hasn't exited fully leading to a > > > > PIPEDSL > > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c > > > > for > > > > *some* > > > > reason we loop inside intel_pipe_update start for ~2+ msec > > > > which in > > > > this > > > > case is more than enough to exit PSR fully, hence an *unstuck* > > > > PIPEDSL > > > > counter, hence no error. On the other hand, the ChromeOS kernel > > > > spends > > > > ~1.1 msec looping inside intel_pipe_update_start and hence > > > > errors > > > > out > > > > b/c the source is still in PSR. > > > > > > > > Regardless, we should wait for PSR exit (if PSR is supported > > > > and > > > > active > > > > on the current pipe) before reading the PIPEDSL, b/c if we > > > > haven't > > > > fully exited PSR, then checking for vblank evasion isn't > > > > actually > > > > applicable. > > > > > > > > This scenario applies to a configuration with an additional > > > > pipe, > > > > as of now > > > > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index ee23613f9fd4..481d310e5c3b 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct > > > > intel_crtc_state *new_crtc_state) > > > > VBLANK_E > > > > VASI > > > > ON > > > > _TIME_US); > > > > max = vblank_start - 1; > > > > > > > > - local_irq_disable(); > > > > - > > > > if (min <= 0 || max <= 0) > > > > return; > > > > > > > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > > > return; > > > > > > > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > > > > + psr_wait_for_idle(dev_priv); > > > How about just waiting for PSR_STATUS to idle without grabbing > > > any > > > locks or checking whether PSR is active? > > > > > > Status should be idle if PSR was disabled or on it's way to > > > becoming > > > idle if it was enabled. Even if PSR did get enabled while we are > > > in > > > pipe_update_start(), it will not be active as long as VBIs are > > > enabled. > > > > Right, if we are OK with some duplication (of psr_wait_for_idle) > inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check > that's being done in psr_wait_for_idle and then just wait without > grabbing any locks, so essentially a lockless version of > psr_wait_for_idle() Yeah, you can extract the wait into psr_wait_for_idle_locked() > > > > Correct me if this was already considered, why not wait until the > > scanline counter starts moving? I see we have a > > intel_wait_for_pipe_scanline_moving(crtc) that's used when the > > pipe is enabled. > > > > -DK > Didn't consider this before, but, pipe_scanline_is_moving waits for a > minimum of 5 msec. Are we OK with a min wait of 5 msec inside > pipe_update_start ? Heuristically, waiting for PSR idle has almost > always returned within < 2 msec. Occasionally it takes upto 1 full > frame. We should be able to change that function. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion 2018-06-19 21:59 ` Tarun Vyas 2018-06-19 23:11 ` Dhinakaran Pandiyan @ 2018-06-21 22:43 ` Tarun Vyas 1 sibling, 0 replies; 17+ messages in thread From: Tarun Vyas @ 2018-06-21 22:43 UTC (permalink / raw) To: Dhinakaran Pandiyan; +Cc: jani.nikula, intel-gfx, rodrigo.vivi On Tue, Jun 19, 2018 at 02:59:54PM -0700, Tarun Vyas wrote: > On Tue, Jun 19, 2018 at 02:54:07PM -0700, Dhinakaran Pandiyan wrote: > > On Tue, 2018-06-19 at 14:27 -0700, Dhinakaran Pandiyan wrote: > > > On Mon, 2018-05-14 at 13:49 -0700, Tarun Vyas wrote: > > > > > > > > The PIPEDSL freezes on PSR entry and if PSR hasn't fully exited, > > > > then > > > > the pipe_update_start call schedules itself out to check back > > > > later. > > > > > > > > On ChromeOS-4.4 kernel, which is fairly up-to-date w.r.t drm/i915 > > > > but > > > > lags w.r.t core kernel code, hot plugging an external display > > > > triggers > > > > tons of "potential atomic update errors" in the dmesg, on *pipe A*. > > > > A > > > > closer analysis reveals that we try to read the scanline 3 times > > > > and > > > > eventually timeout, b/c PSR hasn't exited fully leading to a > > > > PIPEDSL > > > > stuck @ 1599. This issue is not seen on upstream kernels, b/c for > > > > *some* > > > > reason we loop inside intel_pipe_update start for ~2+ msec which in > > > > this > > > > case is more than enough to exit PSR fully, hence an *unstuck* > > > > PIPEDSL > > > > counter, hence no error. On the other hand, the ChromeOS kernel > > > > spends > > > > ~1.1 msec looping inside intel_pipe_update_start and hence errors > > > > out > > > > b/c the source is still in PSR. > > > > > > > > Regardless, we should wait for PSR exit (if PSR is supported and > > > > active > > > > on the current pipe) before reading the PIPEDSL, b/c if we haven't > > > > fully exited PSR, then checking for vblank evasion isn't actually > > > > applicable. > > > > > > > > This scenario applies to a configuration with an additional pipe, > > > > as of now > > > > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_sprite.c | 7 +++++-- > > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index ee23613f9fd4..481d310e5c3b 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -107,14 +107,17 @@ void intel_pipe_update_start(const struct > > > > intel_crtc_state *new_crtc_state) > > > > VBLANK_EVASI > > > > ON > > > > _TIME_US); > > > > max = vblank_start - 1; > > > > > > > > - local_irq_disable(); > > > > - > > > > if (min <= 0 || max <= 0) > > > > return; > > > > > > > > if (WARN_ON(drm_crtc_vblank_get(&crtc->base))) > > > > return; > > > > > > > > + if(new_crtc_state->has_psr && dev_priv->psr.active) > > > > + psr_wait_for_idle(dev_priv); > > > How about just waiting for PSR_STATUS to idle without grabbing any > > > locks or checking whether PSR is active? > > > > > > Status should be idle if PSR was disabled or on it's way to becoming > > > idle if it was enabled. Even if PSR did get enabled while we are in > > > pipe_update_start(), it will not be active as long as VBIs are > > > enabled. > > > > Right, if we are OK with some duplication (of psr_wait_for_idle) inside intel_psr.c, then we can duplicate the PSR2 vs. PSR check that's being done in psr_wait_for_idle and then just wait without grabbing any locks, so essentially a lockless version of psr_wait_for_idle() > > Correct me if this was already considered, why not wait until the > > scanline counter starts moving? I see we have a > > intel_wait_for_pipe_scanline_moving(crtc) that's used when the > > pipe is enabled. > > > > -DK > > Didn't consider this before, but, pipe_scanline_is_moving waits for a minimum of 5 msec. Are we OK with a min wait of 5 msec inside pipe_update_start ? Heuristically, waiting for PSR idle has almost always returned within < 2 msec. Occasionally it takes upto 1 full frame. As per some preliminary measurements Approach 1: Wait *unconditionally* (so no need to check for PSR enabled/disabled and hence no locks) for PSR_STATUS to IDLE out. This takes ~7msec when PSR is active and ~2 usec when PSR is inactive/disabled. Approach 2: Use intel_wait_for_pipe_scanline_moving to wait for PIPEDSL to start moving after PSR exit. Currently, this ends up waiting for a minimum of 5 msec but I changed this to accept a caller defined value for the delay. After the above changes, this approach takes ~7msec when PSR is active and ~25-40 usec with PSR disabled, b/c we still need to check for at least 10+ usec and see if PIPEDSL moved, if it did, we wait for longer, otherwise we move on. _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused. 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas 2018-05-14 20:49 ` [PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle Tarun Vyas 2018-05-14 20:49 ` [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas @ 2018-05-14 21:15 ` Chris Wilson 2018-05-14 22:00 ` Tarun Vyas 2018-05-14 21:35 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) Patchwork ` (2 subsequent siblings) 5 siblings, 1 reply; 17+ messages in thread From: Chris Wilson @ 2018-05-14 21:15 UTC (permalink / raw) To: Tarun Vyas, intel-gfx; +Cc: jani.nikula, dhinakaran.pandiyan, rodrigo.vivi Quoting Tarun Vyas (2018-05-14 21:49:20) > intel_pipe_update_start also needs to wait for PSR to idle > out. Need some minor modifications in psr_wait_for_idle in > order to reuse it. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > --- > drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- > 1 file changed, 18 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index db27f2faa1de..40aafc0f4513 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > i915_reg_t reg; > u32 mask; > int err; > + bool wait = false; > + > + mutex_lock(&dev_priv->psr.lock); > > intel_dp = dev_priv->psr.enabled; > if (!intel_dp) > - return false; > + goto unlock; > > + wait = true; > if (HAS_DDI(dev_priv)) { > if (dev_priv->psr.psr2_enabled) { > reg = EDP_PSR2_STATUS; > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > mask = VLV_EDP_PSR_IN_TRANS; > } > > +unlock: > mutex_unlock(&dev_priv->psr.lock); > > - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > - if (err) > - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > + if(wait) { > + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > + if (err) { > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > + wait = false; > + } > + } > > - /* After the unlocked wait, verify that PSR is still wanted! */ > - mutex_lock(&dev_priv->psr.lock); > - return err == 0 && dev_priv->psr.enabled; > + return wait; > } > > static void intel_psr_work(struct work_struct *work) > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) > struct drm_i915_private *dev_priv = > container_of(work, typeof(*dev_priv), psr.work.work); > > - mutex_lock(&dev_priv->psr.lock); > > /* > * We have to make sure PSR is ready for re-enable > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) > * and be ready for re-enable. > */ > if (!psr_wait_for_idle(dev_priv)) > - goto unlock; > + return; > > - /* > + /* After the unlocked wait, verify that PSR is still wanted! > * The delayed work can race with an invalidate hence we need to > * recheck. Since psr_flush first clears this and then reschedules we > * won't ever miss a flush when bailing out here. > */ > - if (dev_priv->psr.busy_frontbuffer_bits) > + mutex_lock(&dev_priv->psr.lock); > + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) > goto unlock; I'm not sold on the locking dropping here, doing so inside the wait is bad enough. (And do we need to there anyway?) Since you need to introduce intel_psr_wait_for_idle() anyway, how about void intel_psr_wait_for_idle(...) { mutex_lock(&i915->psr.lock); psr_wait_for_idle(); mutex_unlock(&i915->psr.lock); } -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused. 2018-05-14 21:15 ` [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Chris Wilson @ 2018-05-14 22:00 ` Tarun Vyas 2018-05-17 20:35 ` Tarun Vyas 0 siblings, 1 reply; 17+ messages in thread From: Tarun Vyas @ 2018-05-14 22:00 UTC (permalink / raw) To: Chris Wilson; +Cc: jani.nikula, intel-gfx, dhinakaran.pandiyan, rodrigo.vivi On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote: > Quoting Tarun Vyas (2018-05-14 21:49:20) > > intel_pipe_update_start also needs to wait for PSR to idle > > out. Need some minor modifications in psr_wait_for_idle in > > order to reuse it. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > index db27f2faa1de..40aafc0f4513 100644 > > --- a/drivers/gpu/drm/i915/intel_psr.c > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > i915_reg_t reg; > > u32 mask; > > int err; > > + bool wait = false; > > + > > + mutex_lock(&dev_priv->psr.lock); > > > > intel_dp = dev_priv->psr.enabled; > > if (!intel_dp) > > - return false; > > + goto unlock; > > > > + wait = true; > > if (HAS_DDI(dev_priv)) { > > if (dev_priv->psr.psr2_enabled) { > > reg = EDP_PSR2_STATUS; > > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > mask = VLV_EDP_PSR_IN_TRANS; > > } > > > > +unlock: > > mutex_unlock(&dev_priv->psr.lock); > > > > - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > - if (err) > > - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > + if(wait) { > > + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > + if (err) { > > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > + wait = false; > > + } > > + } > > > > - /* After the unlocked wait, verify that PSR is still wanted! */ > > - mutex_lock(&dev_priv->psr.lock); > > - return err == 0 && dev_priv->psr.enabled; > > + return wait; I wanted to avoid taking this additional lock b/c all we need inside intel_pipe_update_start is for PSR to go idle. So can we retain moving it to intel_psr_work ? > > } > > > > static void intel_psr_work(struct work_struct *work) > > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) > > struct drm_i915_private *dev_priv = > > container_of(work, typeof(*dev_priv), psr.work.work); > > > > - mutex_lock(&dev_priv->psr.lock); > > > > /* > > * We have to make sure PSR is ready for re-enable > > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) > > * and be ready for re-enable. > > */ > > if (!psr_wait_for_idle(dev_priv)) > > - goto unlock; > > + return; > > > > - /* > > + /* After the unlocked wait, verify that PSR is still wanted! > > * The delayed work can race with an invalidate hence we need to > > * recheck. Since psr_flush first clears this and then reschedules we > > * won't ever miss a flush when bailing out here. > > */ > > - if (dev_priv->psr.busy_frontbuffer_bits) > > + mutex_lock(&dev_priv->psr.lock); > > + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) > > goto unlock; > > I'm not sold on the locking dropping here, doing so inside the wait is > bad enough. (And do we need to there anyway?) > Thanks for the comments Chris. In that case, as suggested by Rodrigo, can we assert that the lock is held, inside psr_wait_for_idle() ? > Since you need to introduce intel_psr_wait_for_idle() anyway, how about > > void intel_psr_wait_for_idle(...) > { > mutex_lock(&i915->psr.lock); > psr_wait_for_idle(); > mutex_unlock(&i915->psr.lock); > } > -Chris >> /* After the unlocked wait, verify that PSR is still wanted! */ >> mutex_lock(&dev_priv->psr.lock); >> return err == 0 && dev_priv->psr.enabled; _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused. 2018-05-14 22:00 ` Tarun Vyas @ 2018-05-17 20:35 ` Tarun Vyas 0 siblings, 0 replies; 17+ messages in thread From: Tarun Vyas @ 2018-05-17 20:35 UTC (permalink / raw) To: Chris Wilson; +Cc: jani.nikula, intel-gfx, dhinakaran.pandiyan, rodrigo.vivi On Mon, May 14, 2018 at 03:00:15PM -0700, Tarun Vyas wrote: > On Mon, May 14, 2018 at 10:15:19PM +0100, Chris Wilson wrote: > > Quoting Tarun Vyas (2018-05-14 21:49:20) > > > intel_pipe_update_start also needs to wait for PSR to idle > > > out. Need some minor modifications in psr_wait_for_idle in > > > order to reuse it. > > > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_psr.c | 29 ++++++++++++++++++----------- > > > 1 file changed, 18 insertions(+), 11 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > > > index db27f2faa1de..40aafc0f4513 100644 > > > --- a/drivers/gpu/drm/i915/intel_psr.c > > > +++ b/drivers/gpu/drm/i915/intel_psr.c > > > @@ -889,11 +889,15 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > > i915_reg_t reg; > > > u32 mask; > > > int err; > > > + bool wait = false; > > > + > > > + mutex_lock(&dev_priv->psr.lock); > > > > > > intel_dp = dev_priv->psr.enabled; > > > if (!intel_dp) > > > - return false; > > > + goto unlock; > > > > > > + wait = true; > > > if (HAS_DDI(dev_priv)) { > > > if (dev_priv->psr.psr2_enabled) { > > > reg = EDP_PSR2_STATUS; > > > @@ -911,15 +915,18 @@ static bool psr_wait_for_idle(struct drm_i915_private *dev_priv) > > > mask = VLV_EDP_PSR_IN_TRANS; > > > } > > > > > > +unlock: > > > mutex_unlock(&dev_priv->psr.lock); > > > > > > - err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > > - if (err) > > > - DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > > + if(wait) { > > > + err = intel_wait_for_register(dev_priv, reg, mask, 0, 50); > > > + if (err) { > > > + DRM_ERROR("Timed out waiting for PSR Idle for re-enable\n"); > > > + wait = false; > > > + } > > > + } > > > > > > - /* After the unlocked wait, verify that PSR is still wanted! */ > > > - mutex_lock(&dev_priv->psr.lock); > > > - return err == 0 && dev_priv->psr.enabled; > > > + return wait; > I wanted to avoid taking this additional lock b/c all we need inside intel_pipe_update_start is for PSR to go idle. So can we retain moving it to intel_psr_work ? > > > } > > > > > > static void intel_psr_work(struct work_struct *work) > > > @@ -927,7 +934,6 @@ static void intel_psr_work(struct work_struct *work) > > > struct drm_i915_private *dev_priv = > > > container_of(work, typeof(*dev_priv), psr.work.work); > > > > > > - mutex_lock(&dev_priv->psr.lock); > > > > > > /* > > > * We have to make sure PSR is ready for re-enable > > > @@ -936,14 +942,15 @@ static void intel_psr_work(struct work_struct *work) > > > * and be ready for re-enable. > > > */ > > > if (!psr_wait_for_idle(dev_priv)) > > > - goto unlock; > > > + return; > > > > > > - /* > > > + /* After the unlocked wait, verify that PSR is still wanted! > > > * The delayed work can race with an invalidate hence we need to > > > * recheck. Since psr_flush first clears this and then reschedules we > > > * won't ever miss a flush when bailing out here. > > > */ > > > - if (dev_priv->psr.busy_frontbuffer_bits) > > > + mutex_lock(&dev_priv->psr.lock); > > > + if (dev_priv->psr.enabled && dev_priv->psr.busy_frontbuffer_bits) > > > goto unlock; > > > > I'm not sold on the locking dropping here, doing so inside the wait is > > bad enough. (And do we need to there anyway?) > > Per the commit message in "daeb725e drm/i915/psr: Chase psr.enabled only under the psr.lock", the wait_for_register is done after dropping the locks so that we don't block the modeset path. > > Since you need to introduce intel_psr_wait_for_idle() anyway, how about > > > > void intel_psr_wait_for_idle(...) > > { > > mutex_lock(&i915->psr.lock); > > psr_wait_for_idle(); > > mutex_unlock(&i915->psr.lock); On a second thought, I need to wait for PSR idle inside intel_pipe_update_start. Now, intel_psr_disable/intel_psr_enable will *not race* inside intel_pipe_update_start(), so we do not need any psr.locks, at all. The locking dropping inside psr_wait_for_idle makes it difficult to move the common wait code, so I was thinking if I can do something like: void intel_psr_wait_for_idle(...) { if (dev_priv->psr.psr2_enabled) { reg = EDP_PSR2_STATUS; mask = EDP_PSR2_STATUS_STATE_MASK; } else { reg = EDP_PSR_STATUS; mask = EDP_PSR_STATUS_STATE_MASK; } intel_wait_for_register(dev_priv, reg, mask, 0, 25); } which is essentially a lockless version of psr_wait_for_idle(). So, there is code duplication here, but I can't help it. Any thoughts ? > > } > > -Chris > >> /* After the unlocked wait, verify that PSR is still wanted! */ > >> mutex_lock(&dev_priv->psr.lock); > >> return err == 0 && dev_priv->psr.enabled; _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas ` (2 preceding siblings ...) 2018-05-14 21:15 ` [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Chris Wilson @ 2018-05-14 21:35 ` Patchwork 2018-05-14 21:36 ` ✗ Fi.CI.SPARSE: " Patchwork 2018-05-14 21:53 ` ✗ Fi.CI.BAT: failure " Patchwork 5 siblings, 0 replies; 17+ messages in thread From: Patchwork @ 2018-05-14 21:35 UTC (permalink / raw) To: Tarun Vyas; +Cc: intel-gfx == Series Details == Series: series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) URL : https://patchwork.freedesktop.org/series/43155/ State : warning == Summary == $ dim checkpatch origin/drm-tip 2e41c5eabfce drm/i915: Wait for PSR exit before checking for vblank evasion -:48: ERROR:SPACING: space required before the open parenthesis '(' #48: FILE: drivers/gpu/drm/i915/intel_sprite.c:116: + if(new_crtc_state->has_psr && dev_priv->psr.active) total: 1 errors, 0 warnings, 0 checks, 19 lines checked 199b120ec46b drm/i915: Un-statify psr_wait_for_idle _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.SPARSE: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas ` (3 preceding siblings ...) 2018-05-14 21:35 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) Patchwork @ 2018-05-14 21:36 ` Patchwork 2018-05-14 21:53 ` ✗ Fi.CI.BAT: failure " Patchwork 5 siblings, 0 replies; 17+ messages in thread From: Patchwork @ 2018-05-14 21:36 UTC (permalink / raw) To: Tarun Vyas; +Cc: intel-gfx == Series Details == Series: series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) URL : https://patchwork.freedesktop.org/series/43155/ State : warning == Summary == $ dim sparse origin/drm-tip Commit: drm/i915: Wait for PSR exit before checking for vblank evasion + ^~~~~~~~~~~~~~~~~ + apic_wait_icr_idle +cc1: all warnings being treated as errors +drivers/gpu/drm/i915/intel_sprite.c:117:17: error: undefined identifier 'psr_wait_for_idle' +drivers/gpu/drm/i915/intel_sprite.c:117:34: warning: call with no type! +drivers/gpu/drm/i915/intel_sprite.c:117:3: error: implicit declaration of function ‘psr_wait_for_idle’; did you mean ‘apic_wait_icr_idle’? [-Werror=implicit-function-declaration] +drivers/gpu/drm/i915/intel_sprite.c: In function ‘intel_pipe_update_start’: +make[1]: *** [drivers/gpu/drm/i915] Error 2 +make[2]: *** [drivers/gpu/drm/i915/intel_sprite.o] Error 1 +make: *** [drivers/gpu/drm/] Error 2 + psr_wait_for_idle(dev_priv); Commit: drm/i915: Un-statify psr_wait_for_idle - ^~~~~~~~~~~~~~~~~ - apic_wait_icr_idle -cc1: all warnings being treated as errors -drivers/gpu/drm/i915/intel_sprite.c:117:17: error: undefined identifier 'psr_wait_for_idle' -drivers/gpu/drm/i915/intel_sprite.c:117:34: warning: call with no type! -drivers/gpu/drm/i915/intel_sprite.c:117:3: error: implicit declaration of function ‘psr_wait_for_idle’; did you mean ‘apic_wait_icr_idle’? [-Werror=implicit-function-declaration] -drivers/gpu/drm/i915/intel_sprite.c: In function ‘intel_pipe_update_start’: -make[1]: *** [drivers/gpu/drm/i915] Error 2 -make[2]: *** [drivers/gpu/drm/i915/intel_sprite.o] Error 1 -make[2]: *** Waiting for unfinished jobs.... -make: *** [drivers/gpu/drm/] Error 2 - psr_wait_for_idle(dev_priv); _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
* ✗ Fi.CI.BAT: failure for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas ` (4 preceding siblings ...) 2018-05-14 21:36 ` ✗ Fi.CI.SPARSE: " Patchwork @ 2018-05-14 21:53 ` Patchwork 5 siblings, 0 replies; 17+ messages in thread From: Patchwork @ 2018-05-14 21:53 UTC (permalink / raw) To: Tarun Vyas; +Cc: intel-gfx == Series Details == Series: series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) URL : https://patchwork.freedesktop.org/series/43155/ State : failure == Summary == = CI Bug Log - changes from CI_DRM_4179 -> Patchwork_8998 = == Summary - FAILURE == Serious unknown changes coming with Patchwork_8998 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in Patchwork_8998, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://patchwork.freedesktop.org/api/1.0/series/43155/revisions/2/mbox/ == Possible new issues == Here are the unknown changes that may have been introduced in Patchwork_8998: === IGT changes === ==== Possible regressions ==== igt@kms_psr_sink_crc@psr_basic: fi-kbl-r: PASS -> INCOMPLETE fi-skl-6600u: PASS -> INCOMPLETE fi-kbl-7560u: PASS -> INCOMPLETE fi-cfl-s3: PASS -> INCOMPLETE fi-cfl-u: PASS -> INCOMPLETE ==== Warnings ==== igt@gem_exec_gttfill@basic: fi-pnv-d510: PASS -> SKIP == Known issues == Here are the changes found in Patchwork_8998 that come from known issues: === IGT changes === ==== Issues hit ==== igt@kms_psr_sink_crc@psr_basic: fi-cnl-y3: PASS -> INCOMPLETE (fdo#105086) fi-cnl-psr: PASS -> INCOMPLETE (fdo#105086) ==== Possible fixes ==== igt@debugfs_test@read_all_entries: fi-snb-2520m: INCOMPLETE (fdo#103713) -> PASS igt@gem_exec_suspend@basic-s4-devices: fi-kbl-7500u: DMESG-WARN (fdo#105128) -> PASS igt@kms_pipe_crc_basic@suspend-read-crc-pipe-b: fi-snb-2600: DMESG-WARN (fdo#102365) -> PASS fdo#102365 https://bugs.freedesktop.org/show_bug.cgi?id=102365 fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713 fdo#105086 https://bugs.freedesktop.org/show_bug.cgi?id=105086 fdo#105128 https://bugs.freedesktop.org/show_bug.cgi?id=105128 == Participating hosts (41 -> 37) == Additional (1): fi-byt-j1900 Missing (5): fi-ctg-p8600 fi-ilk-m540 fi-byt-squawks fi-bsw-cyan fi-skl-6700hq == Build changes == * Linux: CI_DRM_4179 -> Patchwork_8998 CI_DRM_4179: be6d36ea8d6130f54ab5ec816555f1a46bd95f7b @ git://anongit.freedesktop.org/gfx-ci/linux IGT_4479: 89ae332745e31a075747a63ac5acc5baccf75769 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools Patchwork_8998: 199b120ec46b363ca9d0d44db972a0b1ad3779f7 @ git://anongit.freedesktop.org/gfx-ci/linux piglit_4479: 3ba0657bff4216d1ec7179935590261855f1651e @ git://anongit.freedesktop.org/piglit == Linux commits == 199b120ec46b drm/i915: Un-statify psr_wait_for_idle 2e41c5eabfce drm/i915: Wait for PSR exit before checking for vblank evasion == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8998/issues.html _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2018-06-21 22:43 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-05-14 20:49 [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Tarun Vyas 2018-05-14 20:49 ` [PATCH 2/2] drm/i915: Un-statify psr_wait_for_idle Tarun Vyas 2018-05-14 20:49 ` [PATCH v2] drm/i915: Wait for PSR exit before checking for vblank evasion Tarun Vyas 2018-05-14 21:16 ` Chris Wilson 2018-05-15 19:38 ` Tarun Vyas 2018-05-15 3:16 ` kbuild test robot 2018-06-19 21:27 ` Dhinakaran Pandiyan 2018-06-19 21:54 ` Dhinakaran Pandiyan 2018-06-19 21:59 ` Tarun Vyas 2018-06-19 23:11 ` Dhinakaran Pandiyan 2018-06-21 22:43 ` Tarun Vyas 2018-05-14 21:15 ` [PATCH 1/2] drm/i915: Modify psr_wait_for_idle to be reused Chris Wilson 2018-05-14 22:00 ` Tarun Vyas 2018-05-17 20:35 ` Tarun Vyas 2018-05-14 21:35 ` ✗ Fi.CI.CHECKPATCH: warning for series starting with [v2] drm/i915: Wait for PSR exit before checking for vblank evasion (rev2) Patchwork 2018-05-14 21:36 ` ✗ Fi.CI.SPARSE: " Patchwork 2018-05-14 21:53 ` ✗ Fi.CI.BAT: failure " Patchwork
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.