Re: [PATCH 03/14] drm/i915: Enable PCH FIFO underruns later on ILK/SNB/IVB

[Jani Nikula <jani.nikula@linux.intel.com>]

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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx