From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com ([192.55.52.93]:35647 "EHLO mga11.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1169005AbcKAMxU (ORCPT ); Tue, 1 Nov 2016 08:53:20 -0400 Date: Tue, 1 Nov 2016 14:53:13 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maarten Lankhorst Cc: intel-gfx@lists.freedesktop.org, Mika Kahola , bruno.pagani@ens-lyon.org, Daniel J Blueman , Paul Bolle , Joseph Yasi , stable@vger.kernel.org Subject: Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things Message-ID: <20161101125313.GU4617@intel.com> References: <1477673960-3274-1-git-send-email-ville.syrjala@linux.intel.com> <1354f7bd-a473-f941-cd79-d1fc7259fa38@linux.intel.com> <20161101124035.GS4617@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20161101124035.GS4617@intel.com> Sender: stable-owner@vger.kernel.org List-ID: 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� > > > > > > 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 > > > Cc: Mika Kahola > > > Cc: bruno.pagani@ens-lyon.org > > > Cc: Daniel J Blueman > > > Cc: Paul Bolle > > > Cc: Joseph Yasi > > > Tested-by: Paul Bolle > > > Tested-by: Joseph Yasi > > > 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� > > > --- > > > 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 > > -- > Ville Syrj�l� > Intel OTC -- Ville Syrj�l� Intel OTC From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things Date: Tue, 1 Nov 2016 14:53:13 +0200 Message-ID: <20161101125313.GU4617@intel.com> References: <1477673960-3274-1-git-send-email-ville.syrjala@linux.intel.com> <1354f7bd-a473-f941-cd79-d1fc7259fa38@linux.intel.com> <20161101124035.GS4617@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <20161101124035.GS4617@intel.com> Sender: stable-owner@vger.kernel.org To: Maarten Lankhorst Cc: intel-gfx@lists.freedesktop.org, Mika Kahola , bruno.pagani@ens-lyon.org, Daniel J Blueman , Paul Bolle , Joseph Yasi , stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org 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ä > > > > > > 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 > > > Cc: Mika Kahola > > > Cc: bruno.pagani@ens-lyon.org > > > Cc: Daniel J Blueman > > > Cc: Paul Bolle > > > Cc: Joseph Yasi > > > Tested-by: Paul Bolle > > > Tested-by: Joseph Yasi > > > 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ä > > > --- > > > 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 > > -- > Ville Syrjälä > Intel OTC -- Ville Syrjälä Intel OTC