| Message ID | 20180709014636.29607-1-tarun.vyas@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote: > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > In commit "drm/i915: Wait for PSR exit before checking for vblank > > evasion", the idea was to limit the PSR IDLE checks when PSR is > > actually supported. While CAN_PSR does do that check, it doesn't > > applies on a per-crtc basis. crtc_state->has_psr is a more granular > > check that avoids everything but pipe A, for the PSR IDLE check. > > > > With this, the PSR IDLE check should be a *no-op* for all but pipe A > > which is what was intended originally. > > > > So, the problem is when we update a non-PSR pipe (B or C) and PSR is > active on another pipe(A, specifically), we end up waiting for the pipe > A MMIO to become idle. > > Can you please update the commit message as the commit message makes > the per-pipe check sound like an optimization? > > This also points to a gap in our testing, I don't see a two pipe PSR > related IGT. > That's right. On my KBL chromebook that's running the drm-tip, when I plug-in an external display, so pipe B, I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out, atomic update may fail on pipe B", Iadded the pipe name in the DRM_ERROR, may be I should make that change in the v3 of this patch along with updating the commit message. But, yea, this proves that with the CAN_PSR check, the non-PSR pipes (B/C) wait on pipe-A to exit PSR which doesn't have any reason to do so at that moment, hence the error. I'll make the commit message changes and add the pipe name in the DRM_ERROR as well ? > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for > > vblank evasion") > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index 4990d6e84ddf..83880e3a5f3d 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > intel_crtc_state *new_crtc_state) > > * VBL interrupts will start the PSR exit and prevent a PSR > > * re-entry as well. > > */ > > - if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv)) > > + if (new_crtc_state->has_psr && > > intel_psr_wait_for_idle(dev_priv)) > > DRM_ERROR("PSR idle timed out, atomic update may > > fail\n"); > > > > local_irq_disable();
On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > In commit "drm/i915: Wait for PSR exit before checking for vblank > evasion", the idea was to limit the PSR IDLE checks when PSR is > actually supported. While CAN_PSR does do that check, it doesn't > applies on a per-crtc basis. crtc_state->has_psr is a more granular > check that avoids everything but pipe A, for the PSR IDLE check. > > With this, the PSR IDLE check should be a *no-op* for all but pipe A > which is what was intended originally. > So, the problem is when we update a non-PSR pipe (B or C) and PSR is active on another pipe(A, specifically), we end up waiting for the pipe A MMIO to become idle. Can you please update the commit message as the commit message makes the per-pipe check sound like an optimization? This also points to a gap in our testing, I don't see a two pipe PSR related IGT. > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for > vblank evasion") > > v2: Remove unnecessary parantheses, make checkpatch happy. > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > --- > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 4990d6e84ddf..83880e3a5f3d 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > intel_crtc_state *new_crtc_state) > * VBL interrupts will start the PSR exit and prevent a PSR > * re-entry as well. > */ > - if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv)) > + if (new_crtc_state->has_psr && > intel_psr_wait_for_idle(dev_priv)) > DRM_ERROR("PSR idle timed out, atomic update may > fail\n"); > > local_irq_disable();
On Mon, 2018-07-09 at 11:16 -0700, Tarun Vyas wrote: > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote: > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > > > > > In commit "drm/i915: Wait for PSR exit before checking for vblank > > > evasion", the idea was to limit the PSR IDLE checks when PSR is > > > actually supported. While CAN_PSR does do that check, it doesn't > > > applies on a per-crtc basis. crtc_state->has_psr is a more > > > granular > > > check that avoids everything but pipe A, for the PSR IDLE check. > > > > > > With this, the PSR IDLE check should be a *no-op* for all but > > > pipe A > > > which is what was intended originally. > > > > > So, the problem is when we update a non-PSR pipe (B or C) and PSR > > is > > active on another pipe(A, specifically), we end up waiting for the > > pipe > > A MMIO to become idle. > > > > Can you please update the commit message as the commit message > > makes > > the per-pipe check sound like an optimization? > > > > This also points to a gap in our testing, I don't see a two pipe > > PSR > > related IGT. > > > That's right. On my KBL chromebook that's running the drm-tip, when I > plug-in an external display, so pipe B, > I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out, > atomic update may fail on pipe B", Iadded the pipe > name in the DRM_ERROR, may be I should make that change in the v3 of > this patch along with updating the commit message. > > But, yea, this proves that with the CAN_PSR check, the non-PSR pipes > (B/C) wait on pipe-A to exit PSR which doesn't have > any reason to do so at that moment, hence the error. > > I'll make the commit message changes and add the pipe name in the > DRM_ERROR as well ? I am thinking you could pass crtc_state to intel_psr_wait_for_idle() and then check inside the implementation if the argument is the same as the pipe PSR was enabled on and then wait. intel_psr_wait_for_idle(crtc_state) { if (!CAN_PSR() || !crtc_state->has_psr) return; ... } I don't like how intel_psr_wait_for_idle() doesn't care which pipe (transcoder actually) MMIO it should wait on. > > > > > > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking > > > for > > > vblank evasion") > > > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > index 4990d6e84ddf..83880e3a5f3d 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > > intel_crtc_state *new_crtc_state) > > > * VBL interrupts will start the PSR exit and prevent a > > > PSR > > > * re-entry as well. > > > */ > > > - if (CAN_PSR(dev_priv) && > > > intel_psr_wait_for_idle(dev_priv)) > > > + if (new_crtc_state->has_psr && > > > intel_psr_wait_for_idle(dev_priv)) > > > DRM_ERROR("PSR idle timed out, atomic update may > > > fail\n"); > > > > > > local_irq_disable(); > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote: > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > In commit "drm/i915: Wait for PSR exit before checking for vblank > > evasion", the idea was to limit the PSR IDLE checks when PSR is > > actually supported. While CAN_PSR does do that check, it doesn't > > applies on a per-crtc basis. crtc_state->has_psr is a more granular > > check that avoids everything but pipe A, for the PSR IDLE check. > > > > With this, the PSR IDLE check should be a *no-op* for all but pipe A > > which is what was intended originally. > > > > So, the problem is when we update a non-PSR pipe (B or C) and PSR is > active on another pipe(A, specifically), we end up waiting for the pipe > A MMIO to become idle. > > Can you please update the commit message as the commit message makes > the per-pipe check sound like an optimization? I truly doubt that multiple PSR pipes case doesn't work in our driver. if that works I'd assume it is by coincidence :P > This also points to a gap in our testing, I don't see a two pipe PSR > related IGT. The almost impossible mission here is to find any design with 2 eDP connectors and both panels with PSR. > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for > > vblank evasion") > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > --- > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > b/drivers/gpu/drm/i915/intel_sprite.c > > index 4990d6e84ddf..83880e3a5f3d 100644 > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > intel_crtc_state *new_crtc_state) > > * VBL interrupts will start the PSR exit and prevent a PSR > > * re-entry as well. > > */ > > - if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv)) > > + if (new_crtc_state->has_psr && > > intel_psr_wait_for_idle(dev_priv)) > > DRM_ERROR("PSR idle timed out, atomic update may > > fail\n"); > > > > local_irq_disable();
On Mon, Jul 09, 2018 at 12:58:28PM -0700, Dhinakaran Pandiyan wrote: > On Mon, 2018-07-09 at 12:24 -0700, Rodrigo Vivi wrote: > > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote: > > > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > > > > > > > In commit "drm/i915: Wait for PSR exit before checking for vblank > > > > evasion", the idea was to limit the PSR IDLE checks when PSR is > > > > actually supported. While CAN_PSR does do that check, it doesn't > > > > applies on a per-crtc basis. crtc_state->has_psr is a more > > > > granular > > > > check that avoids everything but pipe A, for the PSR IDLE check. > > > > > > > > With this, the PSR IDLE check should be a *no-op* for all but > > > > pipe A > > > > which is what was intended originally. > > > > > > > So, the problem is when we update a non-PSR pipe (B or C) and PSR > > > is > > > active on another pipe(A, specifically), we end up waiting for the > > > pipe > > > A MMIO to become idle. > > > > > > Can you please update the commit message as the commit message > > > makes > > > the per-pipe check sound like an optimization? > > I truly doubt that multiple PSR pipes case doesn't work in our > > driver. > > if that works I'd assume it is by coincidence :P > > > > > > > > This also points to a gap in our testing, I don't see a two pipe > > > PSR > > > related IGT. > > The almost impossible mission here is to find any design with 2 eDP > > connectors and both panels with PSR. > > > I meant, two pipes with PSR on one of them. I looked at the frontbuffer > _tracking@psr subtests in https://intel-gfx-ci.01.org/tree/drm-tip/drmt > ip.html, none of them were "2p". Ideally, a pipe update on a non-PSR > pipe would have triggered this failure in CI. oh I see... you are right... > > > > > > > > > > > > > > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking > > > > for > > > > vblank evasion") > > > > > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> I take it back, sorry ;) > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index 4990d6e84ddf..83880e3a5f3d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > > > intel_crtc_state *new_crtc_state) > > > > * VBL interrupts will start the PSR exit and prevent a > > > > PSR > > > > * re-entry as well. > > > > */ > > > > - if (CAN_PSR(dev_priv) && > > > > intel_psr_wait_for_idle(dev_priv)) > > > > + if (new_crtc_state->has_psr && > > > > intel_psr_wait_for_idle(dev_priv)) > > > > DRM_ERROR("PSR idle timed out, atomic update may > > > > fail\n"); > > > > > > > > local_irq_disable();
On Mon, Jul 09, 2018 at 11:58:52AM -0700, Dhinakaran Pandiyan wrote: > On Mon, 2018-07-09 at 11:16 -0700, Tarun Vyas wrote: > > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote: > > > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > > > > > > > In commit "drm/i915: Wait for PSR exit before checking for vblank > > > > evasion", the idea was to limit the PSR IDLE checks when PSR is > > > > actually supported. While CAN_PSR does do that check, it doesn't > > > > applies on a per-crtc basis. crtc_state->has_psr is a more > > > > granular > > > > check that avoids everything but pipe A, for the PSR IDLE check. > > > > > > > > With this, the PSR IDLE check should be a *no-op* for all but > > > > pipe A > > > > which is what was intended originally. > > > > > > > So, the problem is when we update a non-PSR pipe (B or C) and PSR > > > is > > > active on another pipe(A, specifically), we end up waiting for the > > > pipe > > > A MMIO to become idle. > > > > > > Can you please update the commit message as the commit message > > > makes > > > the per-pipe check sound like an optimization? > > > > > > This also points to a gap in our testing, I don't see a two pipe > > > PSR > > > related IGT. > > > > > That's right. On my KBL chromebook that's running the drm-tip, when I > > plug-in an external display, so pipe B, > > I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out, > > atomic update may fail on pipe B", Iadded the pipe > > name in the DRM_ERROR, may be I should make that change in the v3 of > > this patch along with updating the commit message. > > > > But, yea, this proves that with the CAN_PSR check, the non-PSR pipes > > (B/C) wait on pipe-A to exit PSR which doesn't have > > any reason to do so at that moment, hence the error. > > > > I'll make the commit message changes and add the pipe name in the > > DRM_ERROR as well ? > > I am thinking you could pass crtc_state to intel_psr_wait_for_idle() > and then check inside the implementation if the argument is the same as > the pipe PSR was enabled on and then wait. > > intel_psr_wait_for_idle(crtc_state) { > if (!CAN_PSR() || !crtc_state->has_psr) > return; > ... > } Hmm, but the CAN_PSR check is already taken care of by intel_psr_compute_config() which then sets has_psr, so just if (!crtc_state->has_psr) return; should suffice, right ? But then, we incur a function call for non-PSR pipes, which will return right away. > > I don't like how intel_psr_wait_for_idle() doesn't care which pipe > (transcoder actually) MMIO it should wait on. > > > > > > > > > > > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking > > > > for > > > > vblank evasion") > > > > > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > index 4990d6e84ddf..83880e3a5f3d 100644 > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > > > intel_crtc_state *new_crtc_state) > > > > * VBL interrupts will start the PSR exit and prevent a > > > > PSR > > > > * re-entry as well. > > > > */ > > > > - if (CAN_PSR(dev_priv) && > > > > intel_psr_wait_for_idle(dev_priv)) > > > > + if (new_crtc_state->has_psr && > > > > intel_psr_wait_for_idle(dev_priv)) > > > > DRM_ERROR("PSR idle timed out, atomic update may > > > > fail\n"); > > > > > > > > local_irq_disable(); > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2018-07-09 at 12:24 -0700, Rodrigo Vivi wrote: > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan wrote: > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > > > > > In commit "drm/i915: Wait for PSR exit before checking for vblank > > > evasion", the idea was to limit the PSR IDLE checks when PSR is > > > actually supported. While CAN_PSR does do that check, it doesn't > > > applies on a per-crtc basis. crtc_state->has_psr is a more > > > granular > > > check that avoids everything but pipe A, for the PSR IDLE check. > > > > > > With this, the PSR IDLE check should be a *no-op* for all but > > > pipe A > > > which is what was intended originally. > > > > > So, the problem is when we update a non-PSR pipe (B or C) and PSR > > is > > active on another pipe(A, specifically), we end up waiting for the > > pipe > > A MMIO to become idle. > > > > Can you please update the commit message as the commit message > > makes > > the per-pipe check sound like an optimization? > I truly doubt that multiple PSR pipes case doesn't work in our > driver. > if that works I'd assume it is by coincidence :P > > > > > This also points to a gap in our testing, I don't see a two pipe > > PSR > > related IGT. > The almost impossible mission here is to find any design with 2 eDP > connectors and both panels with PSR. > I meant, two pipes with PSR on one of them. I looked at the frontbuffer _tracking@psr subtests in https://intel-gfx-ci.01.org/tree/drm-tip/drmt ip.html, none of them were "2p". Ideally, a pipe update on a non-PSR pipe would have triggered this failure in CI. > > > > > > > > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking > > > for > > > vblank evasion") > > > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > index 4990d6e84ddf..83880e3a5f3d 100644 > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > > intel_crtc_state *new_crtc_state) > > > * VBL interrupts will start the PSR exit and prevent a > > > PSR > > > * re-entry as well. > > > */ > > > - if (CAN_PSR(dev_priv) && > > > intel_psr_wait_for_idle(dev_priv)) > > > + if (new_crtc_state->has_psr && > > > intel_psr_wait_for_idle(dev_priv)) > > > DRM_ERROR("PSR idle timed out, atomic update may > > > fail\n"); > > > > > > local_irq_disable();
On Mon, Jul 09, 2018 at 01:31:52PM -0700, Dhinakaran Pandiyan wrote: > On Mon, 2018-07-09 at 12:52 -0700, Tarun Vyas wrote: > > On Mon, Jul 09, 2018 at 11:58:52AM -0700, Dhinakaran Pandiyan wrote: > > > > > > On Mon, 2018-07-09 at 11:16 -0700, Tarun Vyas wrote: > > > > > > > > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan > > > > wrote: > > > > > > > > > > > > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > > > > > > > > > > > > > > > > > In commit "drm/i915: Wait for PSR exit before checking for > > > > > > vblank > > > > > > evasion", the idea was to limit the PSR IDLE checks when PSR > > > > > > is > > > > > > actually supported. While CAN_PSR does do that check, it > > > > > > doesn't > > > > > > applies on a per-crtc basis. crtc_state->has_psr is a more > > > > > > granular > > > > > > check that avoids everything but pipe A, for the PSR IDLE > > > > > > check. > > I looked at the code and spec again, PSR isn't tied to "pipe A" The > driver allows PSR only on "port A" + "transcoder eDP", but the pipe > itself can be any one of the possible options. > I'll remove the pipe A part from the message and specify that at the moment we assume port A + eDP, b/c at least in the code we populate the registers with EDP_PSR base directly. > > > > > > > > > > > > With this, the PSR IDLE check should be a *no-op* for all but > > > > > > pipe A > > > > > > which is what was intended originally. > > > > > > > > > > > So, the problem is when we update a non-PSR pipe (B or C) and > > > > > PSR > > > > > is > > > > > active on another pipe(A, specifically), we end up waiting for > > > > > the > > > > > pipe > > > > > A MMIO to become idle. > > > > > > > > > > Can you please update the commit message as the commit message > > > > > makes > > > > > the per-pipe check sound like an optimization? > > > > > > > > > > This also points to a gap in our testing, I don't see a two > > > > > pipe > > > > > PSR > > > > > related IGT. > > > > > > > > > That's right. On my KBL chromebook that's running the drm-tip, > > > > when I > > > > plug-in an external display, so pipe B, > > > > I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out, > > > > atomic update may fail on pipe B", Iadded the pipe > > > > name in the DRM_ERROR, may be I should make that change in the v3 > > > > of > > > > this patch along with updating the commit message. > > > > > > > > But, yea, this proves that with the CAN_PSR check, the non-PSR > > > > pipes > > > > (B/C) wait on pipe-A to exit PSR which doesn't have > > > > any reason to do so at that moment, hence the error. > > > > > > > > I'll make the commit message changes and add the pipe name in the > > > > DRM_ERROR as well ? > > > I am thinking you could pass crtc_state > > > to intel_psr_wait_for_idle() > > > and then check inside the implementation if the argument is the > > > same as > > > the pipe PSR was enabled on and then wait. > > > > > > intel_psr_wait_for_idle(crtc_state) { > > > if (!CAN_PSR() || !crtc_state->has_psr) > > > return; > > > ... > > > } > > Hmm, but the CAN_PSR check is already taken care of by > > intel_psr_compute_config() which then sets has_psr, so just > > if (!crtc_state->has_psr) > > return; > > should suffice, right ? > > > Yeah, we can assume at this point state->has_psr is set correctly. > > > But then, we incur a function call for non-PSR pipes, which will > > return right away. > > That should be okay, having the PSR related check inside the function > looks cleaner IMO. Sounds good. > > > > > > > > > > I don't like how intel_psr_wait_for_idle() doesn't care which pipe > > > (transcoder actually) MMIO it should wait on. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before > > > > > > checking > > > > > > for > > > > > > vblank evasion") > > > > > > > > > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > > > --- > > > > > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > > > index 4990d6e84ddf..83880e3a5f3d 100644 > > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > > > > > intel_crtc_state *new_crtc_state) > > > > > > * VBL interrupts will start the PSR exit and > > > > > > prevent a > > > > > > PSR > > > > > > * re-entry as well. > > > > > > */ > > > > > > - if (CAN_PSR(dev_priv) && > > > > > > intel_psr_wait_for_idle(dev_priv)) > > > > > > + if (new_crtc_state->has_psr && > > > > > > intel_psr_wait_for_idle(dev_priv)) > > > > > > DRM_ERROR("PSR idle timed out, atomic update > > > > > > may > > > > > > fail\n"); > > > > > > > > > > > > local_irq_disable(); > > > > _______________________________________________ > > > > Intel-gfx mailing list > > > > Intel-gfx@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 2018-07-09 at 12:52 -0700, Tarun Vyas wrote: > On Mon, Jul 09, 2018 at 11:58:52AM -0700, Dhinakaran Pandiyan wrote: > > > > On Mon, 2018-07-09 at 11:16 -0700, Tarun Vyas wrote: > > > > > > On Mon, Jul 09, 2018 at 11:30:00AM -0700, Dhinakaran Pandiyan > > > wrote: > > > > > > > > > > > > On Sun, 2018-07-08 at 18:46 -0700, Tarun Vyas wrote: > > > > > > > > > > > > > > > In commit "drm/i915: Wait for PSR exit before checking for > > > > > vblank > > > > > evasion", the idea was to limit the PSR IDLE checks when PSR > > > > > is > > > > > actually supported. While CAN_PSR does do that check, it > > > > > doesn't > > > > > applies on a per-crtc basis. crtc_state->has_psr is a more > > > > > granular > > > > > check that avoids everything but pipe A, for the PSR IDLE > > > > > check. I looked at the code and spec again, PSR isn't tied to "pipe A" The driver allows PSR only on "port A" + "transcoder eDP", but the pipe itself can be any one of the possible options. > > > > > > > > > > With this, the PSR IDLE check should be a *no-op* for all but > > > > > pipe A > > > > > which is what was intended originally. > > > > > > > > > So, the problem is when we update a non-PSR pipe (B or C) and > > > > PSR > > > > is > > > > active on another pipe(A, specifically), we end up waiting for > > > > the > > > > pipe > > > > A MMIO to become idle. > > > > > > > > Can you please update the commit message as the commit message > > > > makes > > > > the per-pipe check sound like an optimization? > > > > > > > > This also points to a gap in our testing, I don't see a two > > > > pipe > > > > PSR > > > > related IGT. > > > > > > > That's right. On my KBL chromebook that's running the drm-tip, > > > when I > > > plug-in an external display, so pipe B, > > > I see "[drm:intel_pipe_update_start] *ERROR* PSR idle timed out, > > > atomic update may fail on pipe B", Iadded the pipe > > > name in the DRM_ERROR, may be I should make that change in the v3 > > > of > > > this patch along with updating the commit message. > > > > > > But, yea, this proves that with the CAN_PSR check, the non-PSR > > > pipes > > > (B/C) wait on pipe-A to exit PSR which doesn't have > > > any reason to do so at that moment, hence the error. > > > > > > I'll make the commit message changes and add the pipe name in the > > > DRM_ERROR as well ? > > I am thinking you could pass crtc_state > > to intel_psr_wait_for_idle() > > and then check inside the implementation if the argument is the > > same as > > the pipe PSR was enabled on and then wait. > > > > intel_psr_wait_for_idle(crtc_state) { > > if (!CAN_PSR() || !crtc_state->has_psr) > > return; > > ... > > } > Hmm, but the CAN_PSR check is already taken care of by > intel_psr_compute_config() which then sets has_psr, so just > if (!crtc_state->has_psr) > return; > should suffice, right ? > Yeah, we can assume at this point state->has_psr is set correctly. > But then, we incur a function call for non-PSR pipes, which will > return right away. That should be okay, having the PSR related check inside the function looks cleaner IMO. > > > > > > I don't like how intel_psr_wait_for_idle() doesn't care which pipe > > (transcoder actually) MMIO it should wait on. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before > > > > > checking > > > > > for > > > > > vblank evasion") > > > > > > > > > > v2: Remove unnecessary parantheses, make checkpatch happy. > > > > > > > > > > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> > > > > > Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> > > > > > --- > > > > > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > > > > > b/drivers/gpu/drm/i915/intel_sprite.c > > > > > index 4990d6e84ddf..83880e3a5f3d 100644 > > > > > --- a/drivers/gpu/drm/i915/intel_sprite.c > > > > > +++ b/drivers/gpu/drm/i915/intel_sprite.c > > > > > @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct > > > > > intel_crtc_state *new_crtc_state) > > > > > * VBL interrupts will start the PSR exit and > > > > > prevent a > > > > > PSR > > > > > * re-entry as well. > > > > > */ > > > > > - if (CAN_PSR(dev_priv) && > > > > > intel_psr_wait_for_idle(dev_priv)) > > > > > + if (new_crtc_state->has_psr && > > > > > intel_psr_wait_for_idle(dev_priv)) > > > > > DRM_ERROR("PSR idle timed out, atomic update > > > > > may > > > > > fail\n"); > > > > > > > > > > local_irq_disable(); > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 4990d6e84ddf..83880e3a5f3d 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -118,7 +118,7 @@ void intel_pipe_update_start(const struct intel_crtc_state *new_crtc_state) * VBL interrupts will start the PSR exit and prevent a PSR * re-entry as well. */ - if (CAN_PSR(dev_priv) && intel_psr_wait_for_idle(dev_priv)) + if (new_crtc_state->has_psr && intel_psr_wait_for_idle(dev_priv)) DRM_ERROR("PSR idle timed out, atomic update may fail\n"); local_irq_disable();
In commit "drm/i915: Wait for PSR exit before checking for vblank evasion", the idea was to limit the PSR IDLE checks when PSR is actually supported. While CAN_PSR does do that check, it doesn't applies on a per-crtc basis. crtc_state->has_psr is a more granular check that avoids everything but pipe A, for the PSR IDLE check. With this, the PSR IDLE check should be a *no-op* for all but pipe A which is what was intended originally. Fixes: a608987970b9 ("drm/i915: Wait for PSR exit before checking for vblank evasion") v2: Remove unnecessary parantheses, make checkpatch happy. Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Tarun Vyas <tarun.vyas@intel.com> --- drivers/gpu/drm/i915/intel_sprite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)