[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>

Replying to that thread as well:

Another concern I have here, as I understand now intel_modeset_checks
will be put again after wm/ddb and bw calculations - won't this
cause any additional issues?

Also you now have modeset checks still before that force_min_cdclk
condition check which is now in intel_modeset_calc_cdclk.
My idea was to put all CDCLK related calculations and checks into
same function. However this could be a bad idea, so should you may
be just extract force_min_cdclk check condition back from intel_modeset_calc_cdclk
to the original place where it was?

I'm just now thinking in terms of not breaking anything else now...

Stan

> 
> > 
> > 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