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.syrj...@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.vehma...@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx