| Message ID | 20200901151036.1312357-1-kai.vehmanen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915: fix regression leading to display audio probe failure on GLK | expand |
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> > > 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
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. > > > > > 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
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. > > > > > > > > > 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
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? 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@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
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> 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@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
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
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. > > 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> Amended the commit msg with a bit more details: "[vsyrjala: It also broke the ability of planes to bump up the cdclk and thus could lead to underruns when eg. flipping from 32bpp to 64bpp framebuffer. To be clear, we still compute the new cdclk correctly but fail to actually program it to the hardware due to intel_set_cdclk_{pre,post}_plane_update() not getting called on account of state->modeset==false.]" The "Fixes" line was also a bit wrong: )" vs ") at the end. Took me a while to figure out what dim was complaining about :) Pushed to dinq. Thanks! > --- > 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
Hey, On Thu, 3 Sep 2020, 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 > > Amended the commit msg with a bit more details: > "[vsyrjala: It also broke the ability of planes to bump up the cdclk > and thus could lead to underruns when eg. flipping from 32bpp to > 64bpp framebuffer. To be clear, we still compute the new cdclk ack on that, thanks! > The "Fixes" line was also a bit wrong: )" vs ") at the end. Took > me a while to figure out what dim was complaining about :) > > Pushed to dinq. Thanks! Uh, sorry about that. I actually fixed that in some (?) version due to checkpatch warnings, but apparently it slipped though. Sorry about that, took me a while to figure it out, as well :) Br, Kai
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;
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. 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(-)