| Message ID | 20180524190406.2973-1-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Ville Syrjala (2018-05-24 20:04:05) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > My ILK seems to generate a spurious PCH underrun with most interlaced > HDMI modes. Add a second vblank wait to avoid it. > > We have seen some spurious PCH underruns still in CI as well, some > of which seem to be progressive DP. The logs also point towards some > spurious underrins with progressive HDMI on SNB. While I don't have > a solid explanation for those let's try to kill all the birds with one > stone and always do the double wait. > > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 7604fbda0607..b5fa4943372a 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5528,9 +5528,16 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, > if (HAS_PCH_CPT(dev_priv)) > cpt_verify_modeset(dev, intel_crtc->pipe); > > - /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > - if (intel_crtc->config->has_pch_encoder) > + /* > + * Must wait for vblank to avoid spurious PCH FIFO underruns. > + * And a second vblank wait is needed at least on ILK with > + * some interlaced HDMI modes. Let's do the double wait always > + * in case there are more corner cases we don't know about. > + */ > + if (intel_crtc->config->has_pch_encoder) { > + intel_wait_for_vblank(dev_priv, pipe); > intel_wait_for_vblank(dev_priv, pipe); The only purpose for the double wait here is for delaying the switching on of underrun reporting, right? It doesn't accidentally fix anything else? -Chris
Quoting Chris Wilson (2018-05-24 21:14:23) > Quoting Ville Syrjala (2018-05-24 20:04:05) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > My ILK seems to generate a spurious PCH underrun with most interlaced > > HDMI modes. Add a second vblank wait to avoid it. > > > > We have seen some spurious PCH underruns still in CI as well, some > > of which seem to be progressive DP. The logs also point towards some > > spurious underrins with progressive HDMI on SNB. While I don't have > > a solid explanation for those let's try to kill all the birds with one > > stone and always do the double wait. > > > > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 7604fbda0607..b5fa4943372a 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5528,9 +5528,16 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, > > if (HAS_PCH_CPT(dev_priv)) > > cpt_verify_modeset(dev, intel_crtc->pipe); > > > > - /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > > - if (intel_crtc->config->has_pch_encoder) > > + /* > > + * Must wait for vblank to avoid spurious PCH FIFO underruns. > > + * And a second vblank wait is needed at least on ILK with > > + * some interlaced HDMI modes. Let's do the double wait always > > + * in case there are more corner cases we don't know about. > > + */ > > + if (intel_crtc->config->has_pch_encoder) { > > + intel_wait_for_vblank(dev_priv, pipe); > > intel_wait_for_vblank(dev_priv, pipe); > > The only purpose for the double wait here is for delaying the switching > on of underrun reporting, right? It doesn't accidentally fix anything > else? E.g. in a multi-crtc setup, that's a big delay between switching on a pair of pipes. -Chris
On Thu, May 24, 2018 at 09:15:37PM +0100, Chris Wilson wrote: > Quoting Chris Wilson (2018-05-24 21:14:23) > > Quoting Ville Syrjala (2018-05-24 20:04:05) > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > My ILK seems to generate a spurious PCH underrun with most interlaced > > > HDMI modes. Add a second vblank wait to avoid it. > > > > > > We have seen some spurious PCH underruns still in CI as well, some > > > of which seem to be progressive DP. The logs also point towards some > > > spurious underrins with progressive HDMI on SNB. While I don't have > > > a solid explanation for those let's try to kill all the birds with one > > > stone and always do the double wait. > > > > > > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 7604fbda0607..b5fa4943372a 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -5528,9 +5528,16 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, > > > if (HAS_PCH_CPT(dev_priv)) > > > cpt_verify_modeset(dev, intel_crtc->pipe); > > > > > > - /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > > > - if (intel_crtc->config->has_pch_encoder) > > > + /* > > > + * Must wait for vblank to avoid spurious PCH FIFO underruns. > > > + * And a second vblank wait is needed at least on ILK with > > > + * some interlaced HDMI modes. Let's do the double wait always > > > + * in case there are more corner cases we don't know about. > > > + */ > > > + if (intel_crtc->config->has_pch_encoder) { > > > + intel_wait_for_vblank(dev_priv, pipe); > > > intel_wait_for_vblank(dev_priv, pipe); > > > > The only purpose for the double wait here is for delaying the switching > > on of underrun reporting, right? It doesn't accidentally fix anything > > else? > > E.g. in a multi-crtc setup, that's a big delay between switching on a > pair of pipes. I was testing with one pipe only. So at least it's not meant to do anything else.
Quoting Ville Syrjala (2018-05-24 20:04:05) > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > My ILK seems to generate a spurious PCH underrun with most interlaced > HDMI modes. Add a second vblank wait to avoid it. Fwiw, a second vblank because of interlacing is very believable. > We have seen some spurious PCH underruns still in CI as well, some > of which seem to be progressive DP. The logs also point towards some > spurious underrins with progressive HDMI on SNB. While I don't have > a solid explanation for those let's try to kill all the birds with one > stone and always do the double wait. > > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Acked-by: Chris Wilson <chris@chris-wilson.co.uk> No point waiting for a vblank worker? ;) -Chris
On Thu, May 24, 2018 at 10:19:02PM +0100, Chris Wilson wrote: > Quoting Ville Syrjala (2018-05-24 20:04:05) > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > My ILK seems to generate a spurious PCH underrun with most interlaced > > HDMI modes. Add a second vblank wait to avoid it. > > Fwiw, a second vblank because of interlacing is very believable. > > > We have seen some spurious PCH underruns still in CI as well, some > > of which seem to be progressive DP. The logs also point towards some > > spurious underrins with progressive HDMI on SNB. While I don't have > > a solid explanation for those let's try to kill all the birds with one > > stone and always do the double wait. > > > > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Acked-by: Chris Wilson <chris@chris-wilson.co.uk>a Thanks. Pushed to dinq. > > No point waiting for a vblank worker? ;) That might take a while. Also I'm not sure we'd want to use it here because we'd probably want underrun reporting to be active by the time we enable the planes. So we'd either have to enable planes from the worker as well, or we'd just sample the vblank counter at the end of crtc_enable and wait for n+2 just before we start to enable the planes. Not sure if that latter approach would gain us any practical parallelism though.
On Fri, 25 May 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Thu, May 24, 2018 at 10:19:02PM +0100, Chris Wilson wrote: >> Quoting Ville Syrjala (2018-05-24 20:04:05) >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > My ILK seems to generate a spurious PCH underrun with most interlaced >> > HDMI modes. Add a second vblank wait to avoid it. >> >> Fwiw, a second vblank because of interlacing is very believable. /me .oO( could we wait for the 2nd vblank based on interlace? ) >> >> > We have seen some spurious PCH underruns still in CI as well, some >> > of which seem to be progressive DP. The logs also point towards some >> > spurious underrins with progressive HDMI on SNB. While I don't have >> > a solid explanation for those let's try to kill all the birds with one >> > stone and always do the double wait. >> > >> > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>a > > Thanks. Pushed to dinq. > >> >> No point waiting for a vblank worker? ;) > > That might take a while. Also I'm not sure we'd want to use it here > because we'd probably want underrun reporting to be active by the > time we enable the planes. So we'd either have to enable planes from > the worker as well, or we'd just sample the vblank counter at the > end of crtc_enable and wait for n+2 just before we start to enable the > planes. Not sure if that latter approach would gain us any practical > parallelism though.
On Fri, May 25, 2018 at 06:19:17PM +0300, Jani Nikula wrote: > On Fri, 25 May 2018, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Thu, May 24, 2018 at 10:19:02PM +0100, Chris Wilson wrote: > >> Quoting Ville Syrjala (2018-05-24 20:04:05) > >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> > > >> > My ILK seems to generate a spurious PCH underrun with most interlaced > >> > HDMI modes. Add a second vblank wait to avoid it. > >> > >> Fwiw, a second vblank because of interlacing is very believable. > > /me .oO( could we wait for the 2nd vblank based on interlace? ) We certainly could. However as explained below we have seen some similar looking underruns in CI with progressive DP as well, and so I chose to try the shotgun approach in the hopes of hitting a few more barn doors. > > >> > >> > We have seen some spurious PCH underruns still in CI as well, some > >> > of which seem to be progressive DP. The logs also point towards some > >> > spurious underrins with progressive HDMI on SNB. While I don't have > >> > a solid explanation for those let's try to kill all the birds with one > >> > stone and always do the double wait. > >> > > >> > Buzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106387 > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> Acked-by: Chris Wilson <chris@chris-wilson.co.uk>a > > > > Thanks. Pushed to dinq. > > > >> > >> No point waiting for a vblank worker? ;) > > > > That might take a while. Also I'm not sure we'd want to use it here > > because we'd probably want underrun reporting to be active by the > > time we enable the planes. So we'd either have to enable planes from > > the worker as well, or we'd just sample the vblank counter at the > > end of crtc_enable and wait for n+2 just before we start to enable the > > planes. Not sure if that latter approach would gain us any practical > > parallelism though. > > -- > Jani Nikula, Intel Open Source Graphics Center
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7604fbda0607..b5fa4943372a 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5528,9 +5528,16 @@ static void ironlake_crtc_enable(struct intel_crtc_state *pipe_config, if (HAS_PCH_CPT(dev_priv)) cpt_verify_modeset(dev, intel_crtc->pipe); - /* Must wait for vblank to avoid spurious PCH FIFO underruns */ - if (intel_crtc->config->has_pch_encoder) + /* + * Must wait for vblank to avoid spurious PCH FIFO underruns. + * And a second vblank wait is needed at least on ILK with + * some interlaced HDMI modes. Let's do the double wait always + * in case there are more corner cases we don't know about. + */ + if (intel_crtc->config->has_pch_encoder) { + intel_wait_for_vblank(dev_priv, pipe); intel_wait_for_vblank(dev_priv, pipe); + } intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); }