[Intel-gfx] [PATCH] drm/i915: fix regression leading to display audio probe failure on GLK

[Lisovskiy, Stanislav]

On Wed, Sep 02, 2020 at 01:31:09PM +0300, Ville Syrjälä wrote:
> On Tue, Sep 01, 2020 at 06:10:36PM +0300, Kai Vehmanen wrote:
> > In commit 4f0b4352bd26 ("drm/i915: Extract cdclk requirements checking
> > to separate function") the order of force_min_cdclk_changed check and
> > intel_modeset_checks(), was reversed. This broke the mechanism to
> > immediately force a new CDCLK minimum, and lead to driver probe
> > errors for display audio on GLK platform with 5.9-rc1 kernel. Fix
> > the issue by moving intel_modeset_checks() call later.
> 
> Yep. I eyeed this same code recently and noticed the same bug.
> The one thing I didn't yet figure out is whether there is some
> subtle ordering requirement that was the reason for the change.
> But considering intel_modeset_checks() doesn't really do much
> anymore I think it should be safe.
> 
> Sadly CI has been lumping all underrun errors under some ancient
> bugs, so no one noticed that things started to fail when this
> regression was introduced :(
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

What surprises me here, is that the actual patch has been sent
and merged during late spring I think and we figure out that there was
a regression only by now. 
For example I figured out this only today. When I was doing that change, 
was actually aware that the change is actually quite significant as 
it changes the way how we deal with CDCLK, however those were necessary 
as we had a massive FIFO underrun issues at the moment. However CI didn't
show any problems, so we went ahead with this.

> 
> > 
> > Fixes: 4f0b4352bd26 ("drm/i915: Extract cdclk requirements checking to separate function)"
> > BugLink: https://github.com/thesofproject/linux/issues/2410
> > Signed-off-by: Kai Vehmanen <kai.vehmanen at linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 10 ++++------
> >  1 file changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index 7d50b7177d40..8caeed23037c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -15009,12 +15009,6 @@ static int intel_atomic_check(struct drm_device *dev,
> >  	if (dev_priv->wm.distrust_bios_wm)
> >  		any_ms = true;
> >  
> > -	if (any_ms) {
> > -		ret = intel_modeset_checks(state);
> > -		if (ret)
> > -			goto fail;
> > -	}
> > -
> >  	intel_fbc_choose_crtc(dev_priv, state);
> >  	ret = calc_watermark_data(state);
> >  	if (ret)
> > @@ -15029,6 +15023,10 @@ static int intel_atomic_check(struct drm_device *dev,
> >  		goto fail;
> >  
> >  	if (any_ms) {
> > +		ret = intel_modeset_checks(state);
> > +		if (ret)
> > +			goto fail;
> > +
> >  		ret = intel_modeset_calc_cdclk(state);
> >  		if (ret)
> >  			return ret;
> > -- 
> > 2.27.0
> 
> -- 
> Ville Syrjälä
> Intel