On Fri, 30 Oct 2015, Ville Syrjälä <ville.syrj...@linux.intel.com> wrote:
> On Fri, Oct 30, 2015 at 12:06:09PM +0200, Jani Nikula wrote:
>> On Thu, 29 Oct 2015, ville.syrj...@linux.intel.com wrote:
>> > From: Ville Syrjälä <ville.syrj...@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.syrj...@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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
