| Message ID | 1446146763-31821-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/29/2015 12:25 PM, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We get spurious PCH FIFO underruns if we enable the reporting too soon > after enabling the crtc. Move it to be the last step, after the encoder > enable. Additionally we need an extra vblank wait, otherwise we still > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > otherwise. > > For symmetry, disable the PCH underrun reporting as the first thing, > just before encoder disable, when shutting down the crtc. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 99fb33f..d5cb899 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > intel_crtc->active = true; > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > for_each_encoder_on_crtc(dev, crtc, encoder) > if (encoder->pre_enable) > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > if (HAS_PCH_CPT(dev)) > cpt_verify_modeset(dev, intel_crtc->pipe); > + > + if (intel_crtc->config->has_pch_encoder) { > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > + intel_wait_for_vblank(dev, pipe); > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > + } > } > > /* IPS only exists on ULT machines and is tied to pipe A. */ > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > int pipe = intel_crtc->pipe; > u32 reg, temp; > > + if (intel_crtc->config->has_pch_encoder) > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > + > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > > drm_crtc_vblank_off(crtc); > assert_vblank_disabled(crtc); > > - if (intel_crtc->config->has_pch_encoder) > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > - > intel_disable_pipe(intel_crtc); > > ironlake_pfit_disable(intel_crtc, false); > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
2015-10-29 17:25 GMT-02:00 <ville.syrjala@linux.intel.com>: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We get spurious PCH FIFO underruns if we enable the reporting too soon > after enabling the crtc. Move it to be the last step, after the encoder > enable. Additionally we need an extra vblank wait, otherwise we still > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > otherwise. > > For symmetry, disable the PCH underrun reporting as the first thing, > just before encoder disable, when shutting down the crtc. Is there any place that describes where/when a FIFO underrun is expected and where/when one is an actual problem that needs to be solved? How do we know the underruns avoided by these patch are not a signal of real bugs? The fact that we don't get these 100% of the time suggests that maybe they're avoidable somehow. Perhaps you could update the commit message with the info. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 99fb33f..d5cb899 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > intel_crtc->active = true; > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > for_each_encoder_on_crtc(dev, crtc, encoder) > if (encoder->pre_enable) > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > if (HAS_PCH_CPT(dev)) > cpt_verify_modeset(dev, intel_crtc->pipe); > + > + if (intel_crtc->config->has_pch_encoder) { > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > + intel_wait_for_vblank(dev, pipe); > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > + } > } > > /* IPS only exists on ULT machines and is tied to pipe A. */ > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > int pipe = intel_crtc->pipe; > u32 reg, temp; > > + if (intel_crtc->config->has_pch_encoder) > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > + > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > > drm_crtc_vblank_off(crtc); > assert_vblank_disabled(crtc); > > - if (intel_crtc->config->has_pch_encoder) > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > - > intel_disable_pipe(intel_crtc); > > ironlake_pfit_disable(intel_crtc, false); > -- > 2.4.10 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 29, 2015 at 05:57:57PM -0200, Paulo Zanoni wrote: > 2015-10-29 17:25 GMT-02:00 <ville.syrjala@linux.intel.com>: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We get spurious PCH FIFO underruns if we enable the reporting too soon > > after enabling the crtc. Move it to be the last step, after the encoder > > enable. Additionally we need an extra vblank wait, otherwise we still > > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > > otherwise. > > > > For symmetry, disable the PCH underrun reporting as the first thing, > > just before encoder disable, when shutting down the crtc. > > Is there any place that describes where/when a FIFO underrun is > expected and where/when one is an actual problem that needs to be > solved? How do we know the underruns avoided by these patch are not a > signal of real bugs? Can't be 100% sure since its not documented anywhere. But we've been getting these since forever now and stuff still works (more or less at least), so I'm inclined to say we don't have to care about them. Also in these case we only get PCH FIFO underruns and no CPU pipe underruns, so I'm tempted to say it's not that serious. IIRC I once tracked some of these down to having the FDI PLL enabled with FDI RX/TX disabled. Or something like that, but don't quote me on that since my memory might be failing here. Obviously that can't explain it all since I still need the vblank wait to eliminate them. Anyway, this time I didn't try to narrow it down too much. Instead my aim was more to reliably eliminate them without permamently disabling the underrun detection. In any case, we can't get the bat stuff really working until we get the results to be stable, and these underruns are one big obstacle to that. > > The fact that we don't get these 100% of the time suggests that maybe > they're avoidable somehow. It might be just about timing and/or the port type. Feel free to dig into it more if you want, I won't. I think I already wasted enough time poking at my ILK in an effort to figure out the CPU eDP underruns. > > Perhaps you could update the commit message with the info. > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 99fb33f..d5cb899 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > intel_crtc->active = true; > > > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->pre_enable) > > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > > > if (HAS_PCH_CPT(dev)) > > cpt_verify_modeset(dev, intel_crtc->pipe); > > + > > + if (intel_crtc->config->has_pch_encoder) { > > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > > + intel_wait_for_vblank(dev, pipe); > > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > + } > > } > > > > /* IPS only exists on ULT machines and is tied to pipe A. */ > > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > > int pipe = intel_crtc->pipe; > > u32 reg, temp; > > > > + if (intel_crtc->config->has_pch_encoder) > > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > > + > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->disable(encoder); > > > > drm_crtc_vblank_off(crtc); > > assert_vblank_disabled(crtc); > > > > - if (intel_crtc->config->has_pch_encoder) > > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > > - > > intel_disable_pipe(intel_crtc); > > > > ironlake_pfit_disable(intel_crtc, false); > > -- > > 2.4.10 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote: > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > We get spurious PCH FIFO underruns if we enable the reporting too soon > after enabling the crtc. Move it to be the last step, after the encoder > enable. Additionally we need an extra vblank wait, otherwise we still > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > otherwise. > > For symmetry, disable the PCH underrun reporting as the first thing, > just before encoder disable, when shutting down the crtc. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 99fb33f..d5cb899 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > intel_crtc->active = true; > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > for_each_encoder_on_crtc(dev, crtc, encoder) > if (encoder->pre_enable) > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > if (HAS_PCH_CPT(dev)) > cpt_verify_modeset(dev, intel_crtc->pipe); > + > + if (intel_crtc->config->has_pch_encoder) { > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > + intel_wait_for_vblank(dev, pipe); > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); Nitpick, moving this within the if (has_pch_encoder) isn't documented in the commit message. Does that change have an impact? BR, Jani. > + } > } > > /* IPS only exists on ULT machines and is tied to pipe A. */ > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > int pipe = intel_crtc->pipe; > u32 reg, temp; > > + if (intel_crtc->config->has_pch_encoder) > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > + > for_each_encoder_on_crtc(dev, crtc, encoder) > encoder->disable(encoder); > > drm_crtc_vblank_off(crtc); > assert_vblank_disabled(crtc); > > - if (intel_crtc->config->has_pch_encoder) > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > - > intel_disable_pipe(intel_crtc); > > ironlake_pfit_disable(intel_crtc, false);
On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote: > On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote: > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > We get spurious PCH FIFO underruns if we enable the reporting too soon > > after enabling the crtc. Move it to be the last step, after the encoder > > enable. Additionally we need an extra vblank wait, otherwise we still > > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > > otherwise. > > > > For symmetry, disable the PCH underrun reporting as the first thing, > > just before encoder disable, when shutting down the crtc. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 99fb33f..d5cb899 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > intel_crtc->active = true; > > > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > if (encoder->pre_enable) > > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > > > if (HAS_PCH_CPT(dev)) > > cpt_verify_modeset(dev, intel_crtc->pipe); > > + > > + if (intel_crtc->config->has_pch_encoder) { > > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > > + intel_wait_for_vblank(dev, pipe); > > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > Nitpick, moving this within the if (has_pch_encoder) isn't documented in > the commit message. Does that change have an impact? I don't much of a real concern here. I think the following might happen (all on the same pipe): 1. enable PCH port 2. disable PCH port 3. PCH FIFO underrun just after we've re-enabled the PCH underrun reporting 4. enable port A 5. PCH FIFO underrun reporting isn't enabled anymore for this pipe But since it's driving a non-PCH port anyway, that doesn't seem like a huge worry. But I suppose I could change it to always enable PCH FIFO underrun reporting even for port A. It should do no harm at least. > > BR, > Jani. > > > + } > > } > > > > /* IPS only exists on ULT machines and is tied to pipe A. */ > > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > > int pipe = intel_crtc->pipe; > > u32 reg, temp; > > > > + if (intel_crtc->config->has_pch_encoder) > > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > > + > > for_each_encoder_on_crtc(dev, crtc, encoder) > > encoder->disable(encoder); > > > > drm_crtc_vblank_off(crtc); > > assert_vblank_disabled(crtc); > > > > - if (intel_crtc->config->has_pch_encoder) > > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > > - > > intel_disable_pipe(intel_crtc); > > > > ironlake_pfit_disable(intel_crtc, false); > > -- > Jani Nikula, Intel Open Source Technology Center
On Fri, 30 Oct 2015, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote: >> On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote: >> > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > >> > We get spurious PCH FIFO underruns if we enable the reporting too soon >> > after enabling the crtc. Move it to be the last step, after the encoder >> > enable. Additionally we need an extra vblank wait, otherwise we still >> > get the underruns. Presumably the pipe/fdi isn't yet fully up and running >> > otherwise. >> > >> > For symmetry, disable the PCH underrun reporting as the first thing, >> > just before encoder disable, when shutting down the crtc. >> > >> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- >> > 1 file changed, 9 insertions(+), 4 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> > index 99fb33f..d5cb899 100644 >> > --- a/drivers/gpu/drm/i915/intel_display.c >> > +++ b/drivers/gpu/drm/i915/intel_display.c >> > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) >> > intel_crtc->active = true; >> > >> > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); >> > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); >> > >> > for_each_encoder_on_crtc(dev, crtc, encoder) >> > if (encoder->pre_enable) >> > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) >> > >> > if (HAS_PCH_CPT(dev)) >> > cpt_verify_modeset(dev, intel_crtc->pipe); >> > + >> > + if (intel_crtc->config->has_pch_encoder) { >> > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ >> > + intel_wait_for_vblank(dev, pipe); >> > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); >> >> Nitpick, moving this within the if (has_pch_encoder) isn't documented in >> the commit message. Does that change have an impact? > > I don't much of a real concern here. I think the following might > happen (all on the same pipe): > > 1. enable PCH port > 2. disable PCH port > 3. PCH FIFO underrun just after we've re-enabled the PCH > underrun reporting > 4. enable port A > 5. PCH FIFO underrun reporting isn't enabled anymore for this pipe > > But since it's driving a non-PCH port anyway, that doesn't seem like > a huge worry. But I suppose I could change it to always enable PCH > FIFO underrun reporting even for port A. It should do no harm at least. Mostly I just wanted this change documented in the commit message. Jani. > >> >> BR, >> Jani. >> >> > + } >> > } >> > >> > /* IPS only exists on ULT machines and is tied to pipe A. */ >> > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) >> > int pipe = intel_crtc->pipe; >> > u32 reg, temp; >> > >> > + if (intel_crtc->config->has_pch_encoder) >> > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); >> > + >> > for_each_encoder_on_crtc(dev, crtc, encoder) >> > encoder->disable(encoder); >> > >> > drm_crtc_vblank_off(crtc); >> > assert_vblank_disabled(crtc); >> > >> > - if (intel_crtc->config->has_pch_encoder) >> > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); >> > - >> > intel_disable_pipe(intel_crtc); >> > >> > ironlake_pfit_disable(intel_crtc, false); >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Fri, Oct 30, 2015 at 02:08:51PM +0200, Ville Syrjälä wrote: > On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote: > > On Thu, 29 Oct 2015, ville.syrjala@linux.intel.com wrote: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > We get spurious PCH FIFO underruns if we enable the reporting too soon > > > after enabling the crtc. Move it to be the last step, after the encoder > > > enable. Additionally we need an extra vblank wait, otherwise we still > > > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > > > otherwise. > > > > > > For symmetry, disable the PCH underrun reporting as the first thing, > > > just before encoder disable, when shutting down the crtc. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 13 +++++++++---- > > > 1 file changed, 9 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 99fb33f..d5cb899 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > > intel_crtc->active = true; > > > > > > intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); > > > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > > > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > > if (encoder->pre_enable) > > > @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) > > > > > > if (HAS_PCH_CPT(dev)) > > > cpt_verify_modeset(dev, intel_crtc->pipe); > > > + > > > + if (intel_crtc->config->has_pch_encoder) { > > > + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ > > > + intel_wait_for_vblank(dev, pipe); > > > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); > > > > Nitpick, moving this within the if (has_pch_encoder) isn't documented in > > the commit message. Does that change have an impact? > > I don't much of a real concern here. I think the following might > happen (all on the same pipe): > > 1. enable PCH port > 2. disable PCH port > 3. PCH FIFO underrun just after we've re-enabled the PCH > underrun reporting > 4. enable port A > 5. PCH FIFO underrun reporting isn't enabled anymore for this pipe > > But since it's driving a non-PCH port anyway, that doesn't seem like > a huge worry. But I suppose I could change it to always enable PCH > FIFO underrun reporting even for port A. It should do no harm at least. Iirc we still fail to enable fifo underrun reporting with fastboot (should fix this now since we update watermarks on takeover). That was the reason to unconditionally enable fifo underruns even on the pch, to make it work on platforms where the pch interrupt source is shared. See the pile of hurt at the end of intel_sanitize_crtc. I'd just keep it out of the if for now. -Daniel > > > > > BR, > > Jani. > > > > > + } > > > } > > > > > > /* IPS only exists on ULT machines and is tied to pipe A. */ > > > @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) > > > int pipe = intel_crtc->pipe; > > > u32 reg, temp; > > > > > > + if (intel_crtc->config->has_pch_encoder) > > > + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > > > + > > > for_each_encoder_on_crtc(dev, crtc, encoder) > > > encoder->disable(encoder); > > > > > > drm_crtc_vblank_off(crtc); > > > assert_vblank_disabled(crtc); > > > > > > - if (intel_crtc->config->has_pch_encoder) > > > - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); > > > - > > > intel_disable_pipe(intel_crtc); > > > > > > ironlake_pfit_disable(intel_crtc, false); > > > > -- > > Jani Nikula, Intel Open Source Technology Center > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Thu, Oct 29, 2015 at 11:21:28PM +0200, Ville Syrjälä wrote: > On Thu, Oct 29, 2015 at 05:57:57PM -0200, Paulo Zanoni wrote: > > 2015-10-29 17:25 GMT-02:00 <ville.syrjala@linux.intel.com>: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > We get spurious PCH FIFO underruns if we enable the reporting too soon > > > after enabling the crtc. Move it to be the last step, after the encoder > > > enable. Additionally we need an extra vblank wait, otherwise we still > > > get the underruns. Presumably the pipe/fdi isn't yet fully up and running > > > otherwise. > > > > > > For symmetry, disable the PCH underrun reporting as the first thing, > > > just before encoder disable, when shutting down the crtc. > > > > Is there any place that describes where/when a FIFO underrun is > > expected and where/when one is an actual problem that needs to be > > solved? How do we know the underruns avoided by these patch are not a > > signal of real bugs? > > Can't be 100% sure since its not documented anywhere. But we've been > getting these since forever now and stuff still works (more or less at > least), so I'm inclined to say we don't have to care about them. Also > in these case we only get PCH FIFO underruns and no CPU pipe > underruns, so I'm tempted to say it's not that serious. > > IIRC I once tracked some of these down to having the FDI PLL enabled > with FDI RX/TX disabled. Or something like that, but don't quote me > on that since my memory might be failing here. Obviously that can't > explain it all since I still need the vblank wait to eliminate them. > Anyway, this time I didn't try to narrow it down too much. Instead > my aim was more to reliably eliminate them without permamently disabling > the underrun detection. > > In any case, we can't get the bat stuff really working until we get > the results to be stable, and these underruns are one big obstacle > to that. Agreed with Ville here, these are old platforms and we want stable BAT results first. On gen9+ we should try a bit harder, or at least make a note in JIRA that we need to look into this. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 99fb33f..d5cb899 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4874,7 +4874,6 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) intel_crtc->active = true; intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true); - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); for_each_encoder_on_crtc(dev, crtc, encoder) if (encoder->pre_enable) @@ -4912,6 +4911,12 @@ static void ironlake_crtc_enable(struct drm_crtc *crtc) if (HAS_PCH_CPT(dev)) cpt_verify_modeset(dev, intel_crtc->pipe); + + if (intel_crtc->config->has_pch_encoder) { + /* Must wait for vblank to avoid spurious PCH FIFO underruns */ + intel_wait_for_vblank(dev, pipe); + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, true); + } } /* IPS only exists on ULT machines and is tied to pipe A. */ @@ -5040,15 +5045,15 @@ static void ironlake_crtc_disable(struct drm_crtc *crtc) int pipe = intel_crtc->pipe; u32 reg, temp; + if (intel_crtc->config->has_pch_encoder) + intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); + for_each_encoder_on_crtc(dev, crtc, encoder) encoder->disable(encoder); drm_crtc_vblank_off(crtc); assert_vblank_disabled(crtc); - if (intel_crtc->config->has_pch_encoder) - intel_set_pch_fifo_underrun_reporting(dev_priv, pipe, false); - intel_disable_pipe(intel_crtc); ironlake_pfit_disable(intel_crtc, false);