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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Sep 02, 2020 at 03:34:50PM +0300, Lisovskiy, Stanislav wrote:
> On Wed, Sep 02, 2020 at 03:17:08PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 02, 2020 at 03:12:01PM +0300, Lisovskiy, Stanislav wrote:
> > > 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@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.
> > 
> > I spotted some CI logs that show underruns due to this regression,
> > but the results just got lumped in with other older underrun bugs,
> > and thus CI results were always "success" :/
> > 
> > I think we need to kill off all underrun related CI filters and
> > start from scratch. Otherwise new bugs will keep slipping through.
> 
> 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?

intel_modeset_checks() doesn't really do much anymore.

I think the old global state mess was the reason there was
some ordering requirement between the two. But if you see the
patch I just posted that old global state stuff is now dead
code that can be ripped out.

Oh, the other linkage was the cdclk vs. linetime watermark, but
I moved the linetime wm stuff elsewhere already a while ago. So
also not an issue.

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

The force cdclk stuff looks fine to me. Though there is
https://patchwork.freedesktop.org/patch/377191/?series=79480&rev=1
still unreviewed which would clean it up a bit further.

> 
> 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@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
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx