From: "Ville Syrjälä" <ville.syrjala@linux.intel.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, Mika Kahola <mika.kahola@intel.com>, bruno.pagani@ens-lyon.org, Daniel J Blueman <daniel.blueman@gmail.com>, Paul Bolle <pebolle@tiscali.nl>, Joseph Yasi <joe.yasi@gmail.com>, stable@vger.kernel.org Subject: Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things Date: Tue, 1 Nov 2016 14:53:13 +0200 [thread overview] Message-ID: <20161101125313.GU4617@intel.com> (raw) In-Reply-To: <20161101124035.GS4617@intel.com> On Tue, Nov 01, 2016 at 02:40:35PM +0200, Ville Syrj�l� wrote: > On Tue, Nov 01, 2016 at 09:57:43AM +0100, Maarten Lankhorst wrote: > > Op 28-10-16 om 18:59 schreef ville.syrjala@linux.intel.com: > > > From: Ville Syrj�l� <ville.syrjala@linux.intel.com> > > > > > > When we end up not recomputing the cdclk, we need to populate > > > intel_state->cdclk with the "atomic_cdclk_freq" instead of the > > > current cdclk_freq. When no pipes are active, the actual cdclk_freq > > > may be lower than what the configuration of the planes and > > > pipes would require from the point of view of the software state. > > > > > > intel_state->dev_cdclk is the computed actual cdclk in such cases, > > > so let's populate that with the current cdclk value. Although basically > > > nothing should ever use dev_cdclk for any checks and whatnot. > > > > > > This fixes bogus WARNS from skl_max_scale() which is trying to check > > > the plane software state against the cdclk frequency. So any time > > > it got called during DPMS off for instance, we might have tripped > > > the warn if the current mode would have required a higher than > > > minimum cdclk. > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Cc: bruno.pagani@ens-lyon.org > > > Cc: Daniel J Blueman <daniel.blueman@gmail.com> > > > Cc: Paul Bolle <pebolle@tiscali.nl> > > > Cc: Joseph Yasi <joe.yasi@gmail.com> > > > Tested-by: Paul Bolle <pebolle@tiscali.nl> > > > Tested-by: Joseph Yasi <joe.yasi@gmail.com> > > > Cc: stable@vger.kernel.org > > > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.") > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98214 > > > Signed-off-by: Ville Syrj�l� <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 895b3dc50e3f..f010e154e33e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -14040,8 +14040,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state) > > > > > > DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n", > > > intel_state->cdclk, intel_state->dev_cdclk); > > > - } else > > > + } else { > > > to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq; > > > + to_intel_atomic_state(state)->dev_cdclk = dev_priv->cdclk_freq; > > > + } > > This shouldn't be required in this case, but might as well do so since it doesn't hurt either. > > > intel_modeset_clear_plls(state); > > > > > > @@ -14142,8 +14144,10 @@ static int intel_atomic_check(struct drm_device *dev, > > > > > > if (ret) > > > return ret; > > > - } else > > > - intel_state->cdclk = dev_priv->cdclk_freq; > > > + } else { > > > + intel_state->cdclk = dev_priv->atomic_cdclk_freq; > > > + intel_state->dev_cdclk = dev_priv->cdclk_freq; > > > + } > > We shouldn't rely on dev_cdclk being valid for the !modeset case. > > Best to keep it zero there, the global cdclk can't be changed and the non-modeset case shouldn't rely on the current setting. > > It should pretty much be protected by any of the crtc locks, at least > for now since we don't allow changing it w/o modesetting all the pipes. > But yeah, nothing should be using it for any checks so could just leave > it unset. > > But this got me thinking about dev_priv->atomic_cdclk_freq. Essentially > that one is protected by connection_mutex, which we won't be holding > for the !modeset case. So I think using it there is a bit dubious. I > guess it would require a modeset on one pipe that doesn't actually > end up changing the cdclk frequency but which changes atomic_cdclk_freq, > and a parallel plane update on another pipe. I guess that would mean > both pipes would have be !active at the time so that dev_cdclk remains > stable. Seems to me that we'd need to lock all the crtcs (without > forcing a modeset on them) when atomic_cdclk changes. Something like this: diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 093af6e4ab40..532a932a1ff9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13921,6 +13921,21 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state) return 0; } +static int intel_lock_all_pipes(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + /* add all pipes to the state */ + for_each_crtc(state->dev, crtc) { + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + } + + return 0; +} + static int intel_modeset_all_pipes(struct drm_atomic_state *state) { struct drm_crtc *crtc; @@ -13993,17 +14008,33 @@ static int intel_modeset_checks(struct drm_atomic_state *state) if (ret < 0) return ret; + /* + * All pipes must be switched off while we change the cdclk. + * + * Even if we don't end up changing the actual cdclk frequency + * (dev_priv->cdclk_freq) we must lock all the crtcs if the + * logical cdclk frequency (dev_priv->atomic_cdclk_freq) needs to + * be updated since non-modeset operations will depend on its + * current value. + */ if (intel_state->dev_cdclk != dev_priv->cdclk_freq || intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) ret = intel_modeset_all_pipes(state); + else if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) + ret = intel_lock_all_pipes(state); if (ret < 0) return ret; DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n", intel_state->cdclk, intel_state->dev_cdclk); - } else + } else { + /* + * We know that this well never change, + * so no need to lock all the crtcs here. + */ to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq; + } intel_modeset_clear_plls(state); > > > > > Otherwise looks sane, I have a similar patch in my tree. I didn't submit it yet but the fix was similar. Except for the > > dev_cdclk stuff. > > > > With the last dev_cdclk assignment removed: > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > -- > Ville Syrj�l� > Intel OTC -- Ville Syrj�l� Intel OTC
WARNING: multiple messages have this Message-ID (diff)
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: intel-gfx@lists.freedesktop.org, Mika Kahola <mika.kahola@intel.com>, bruno.pagani@ens-lyon.org, Daniel J Blueman <daniel.blueman@gmail.com>, Paul Bolle <pebolle@tiscali.nl>, Joseph Yasi <joe.yasi@gmail.com>, stable@vger.kernel.org Subject: Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things Date: Tue, 1 Nov 2016 14:53:13 +0200 [thread overview] Message-ID: <20161101125313.GU4617@intel.com> (raw) In-Reply-To: <20161101124035.GS4617@intel.com> On Tue, Nov 01, 2016 at 02:40:35PM +0200, Ville Syrjälä wrote: > On Tue, Nov 01, 2016 at 09:57:43AM +0100, Maarten Lankhorst wrote: > > Op 28-10-16 om 18:59 schreef ville.syrjala@linux.intel.com: > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > > > > When we end up not recomputing the cdclk, we need to populate > > > intel_state->cdclk with the "atomic_cdclk_freq" instead of the > > > current cdclk_freq. When no pipes are active, the actual cdclk_freq > > > may be lower than what the configuration of the planes and > > > pipes would require from the point of view of the software state. > > > > > > intel_state->dev_cdclk is the computed actual cdclk in such cases, > > > so let's populate that with the current cdclk value. Although basically > > > nothing should ever use dev_cdclk for any checks and whatnot. > > > > > > This fixes bogus WARNS from skl_max_scale() which is trying to check > > > the plane software state against the cdclk frequency. So any time > > > it got called during DPMS off for instance, we might have tripped > > > the warn if the current mode would have required a higher than > > > minimum cdclk. > > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > > Cc: Mika Kahola <mika.kahola@intel.com> > > > Cc: bruno.pagani@ens-lyon.org > > > Cc: Daniel J Blueman <daniel.blueman@gmail.com> > > > Cc: Paul Bolle <pebolle@tiscali.nl> > > > Cc: Joseph Yasi <joe.yasi@gmail.com> > > > Tested-by: Paul Bolle <pebolle@tiscali.nl> > > > Tested-by: Joseph Yasi <joe.yasi@gmail.com> > > > Cc: stable@vger.kernel.org > > > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.") > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98214 > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_display.c | 10 +++++++--- > > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 895b3dc50e3f..f010e154e33e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -14040,8 +14040,10 @@ static int intel_modeset_checks(struct drm_atomic_state *state) > > > > > > DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n", > > > intel_state->cdclk, intel_state->dev_cdclk); > > > - } else > > > + } else { > > > to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq; > > > + to_intel_atomic_state(state)->dev_cdclk = dev_priv->cdclk_freq; > > > + } > > This shouldn't be required in this case, but might as well do so since it doesn't hurt either. > > > intel_modeset_clear_plls(state); > > > > > > @@ -14142,8 +14144,10 @@ static int intel_atomic_check(struct drm_device *dev, > > > > > > if (ret) > > > return ret; > > > - } else > > > - intel_state->cdclk = dev_priv->cdclk_freq; > > > + } else { > > > + intel_state->cdclk = dev_priv->atomic_cdclk_freq; > > > + intel_state->dev_cdclk = dev_priv->cdclk_freq; > > > + } > > We shouldn't rely on dev_cdclk being valid for the !modeset case. > > Best to keep it zero there, the global cdclk can't be changed and the non-modeset case shouldn't rely on the current setting. > > It should pretty much be protected by any of the crtc locks, at least > for now since we don't allow changing it w/o modesetting all the pipes. > But yeah, nothing should be using it for any checks so could just leave > it unset. > > But this got me thinking about dev_priv->atomic_cdclk_freq. Essentially > that one is protected by connection_mutex, which we won't be holding > for the !modeset case. So I think using it there is a bit dubious. I > guess it would require a modeset on one pipe that doesn't actually > end up changing the cdclk frequency but which changes atomic_cdclk_freq, > and a parallel plane update on another pipe. I guess that would mean > both pipes would have be !active at the time so that dev_cdclk remains > stable. Seems to me that we'd need to lock all the crtcs (without > forcing a modeset on them) when atomic_cdclk changes. Something like this: diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 093af6e4ab40..532a932a1ff9 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13921,6 +13921,21 @@ static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state) return 0; } +static int intel_lock_all_pipes(struct drm_atomic_state *state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + + /* add all pipes to the state */ + for_each_crtc(state->dev, crtc) { + crtc_state = drm_atomic_get_crtc_state(state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + } + + return 0; +} + static int intel_modeset_all_pipes(struct drm_atomic_state *state) { struct drm_crtc *crtc; @@ -13993,17 +14008,33 @@ static int intel_modeset_checks(struct drm_atomic_state *state) if (ret < 0) return ret; + /* + * All pipes must be switched off while we change the cdclk. + * + * Even if we don't end up changing the actual cdclk frequency + * (dev_priv->cdclk_freq) we must lock all the crtcs if the + * logical cdclk frequency (dev_priv->atomic_cdclk_freq) needs to + * be updated since non-modeset operations will depend on its + * current value. + */ if (intel_state->dev_cdclk != dev_priv->cdclk_freq || intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) ret = intel_modeset_all_pipes(state); + else if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) + ret = intel_lock_all_pipes(state); if (ret < 0) return ret; DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n", intel_state->cdclk, intel_state->dev_cdclk); - } else + } else { + /* + * We know that this well never change, + * so no need to lock all the crtcs here. + */ to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq; + } intel_modeset_clear_plls(state); > > > > > Otherwise looks sane, I have a similar patch in my tree. I didn't submit it yet but the fix was similar. Except for the > > dev_cdclk stuff. > > > > With the last dev_cdclk assignment removed: > > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC
next prev parent reply other threads:[~2016-11-01 12:53 UTC|newest] Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-10-28 16:59 [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things ville.syrjala 2016-10-28 16:59 ` ville.syrjala 2016-10-28 17:15 ` ✓ Fi.CI.BAT: success for " Patchwork 2016-10-28 21:05 ` [PATCH] " Paul Bolle 2016-10-28 21:05 ` Paul Bolle 2016-10-28 21:30 ` Ville Syrjälä 2016-10-28 21:30 ` Ville Syrjälä 2016-10-28 21:38 ` Paul Bolle 2016-10-28 21:38 ` Paul Bolle 2016-10-28 22:01 ` Ville Syrjälä 2016-10-28 22:01 ` Ville Syrjälä 2016-11-01 8:57 ` Maarten Lankhorst 2016-11-01 8:57 ` Maarten Lankhorst 2016-11-01 9:07 ` Paul Bolle 2016-11-01 12:41 ` Ville Syrjälä 2016-11-01 12:41 ` Ville Syrjälä 2016-11-01 12:40 ` Ville Syrjälä 2016-11-01 12:40 ` Ville Syrjälä 2016-11-01 12:53 ` Ville Syrjälä [this message] 2016-11-01 12:53 ` Ville Syrjälä
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20161101125313.GU4617@intel.com \ --to=ville.syrjala@linux.intel.com \ --cc=bruno.pagani@ens-lyon.org \ --cc=daniel.blueman@gmail.com \ --cc=intel-gfx@lists.freedesktop.org \ --cc=joe.yasi@gmail.com \ --cc=maarten.lankhorst@linux.intel.com \ --cc=mika.kahola@intel.com \ --cc=pebolle@tiscali.nl \ --cc=stable@vger.kernel.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.