* [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-10-28 16:59 ` ville.syrjala 0 siblings, 0 replies; 20+ messages in thread From: ville.syrjala @ 2016-10-28 16:59 UTC (permalink / raw) To: intel-gfx Cc: Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle, Joseph Yasi, stable 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; + } 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; + } ret = drm_atomic_helper_check_planes(dev, state); if (ret) -- 2.7.4 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-10-28 16:59 ` ville.syrjala 0 siblings, 0 replies; 20+ messages in thread From: ville.syrjala @ 2016-10-28 16:59 UTC (permalink / raw) To: intel-gfx; +Cc: bruno.pagani, Daniel J Blueman, Paul Bolle, stable, Joseph Yasi 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; + } 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; + } ret = drm_atomic_helper_check_planes(dev, state); if (ret) -- 2.7.4 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 20+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-10-28 16:59 ` ville.syrjala (?) @ 2016-10-28 17:15 ` Patchwork -1 siblings, 0 replies; 20+ messages in thread From: Patchwork @ 2016-10-28 17:15 UTC (permalink / raw) To: ville.syrjala; +Cc: intel-gfx == Series Details == Series: drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things URL : https://patchwork.freedesktop.org/series/14540/ State : success == Summary == Series 14540v1 drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things https://patchwork.freedesktop.org/api/1.0/series/14540/revisions/1/mbox/ Test drv_module_reload_basic: skip -> PASS (fi-skl-6260u) fi-bdw-5557u total:239 pass:224 dwarn:0 dfail:0 fail:0 skip:15 fi-bsw-n3050 total:239 pass:199 dwarn:0 dfail:0 fail:0 skip:40 fi-bxt-t5700 total:239 pass:211 dwarn:0 dfail:0 fail:0 skip:28 fi-byt-j1900 total:239 pass:211 dwarn:0 dfail:0 fail:0 skip:28 fi-byt-n2820 total:239 pass:207 dwarn:0 dfail:0 fail:0 skip:32 fi-hsw-4770 total:239 pass:219 dwarn:0 dfail:0 fail:0 skip:20 fi-hsw-4770r total:239 pass:218 dwarn:0 dfail:0 fail:0 skip:21 fi-ilk-650 total:239 pass:185 dwarn:0 dfail:0 fail:0 skip:54 fi-ivb-3520m total:239 pass:216 dwarn:0 dfail:0 fail:0 skip:23 fi-ivb-3770 total:239 pass:216 dwarn:0 dfail:0 fail:0 skip:23 fi-kbl-7200u total:239 pass:217 dwarn:0 dfail:0 fail:0 skip:22 fi-skl-6260u total:239 pass:225 dwarn:0 dfail:0 fail:0 skip:14 fi-skl-6700hq total:239 pass:218 dwarn:0 dfail:0 fail:0 skip:21 fi-skl-6700k total:239 pass:217 dwarn:1 dfail:0 fail:0 skip:21 fi-skl-6770hq total:239 pass:225 dwarn:0 dfail:0 fail:0 skip:14 fi-snb-2520m total:239 pass:206 dwarn:0 dfail:0 fail:0 skip:33 fi-snb-2600 total:239 pass:205 dwarn:0 dfail:0 fail:0 skip:34 c15686c36cae1cac4af0807213a9bb4bf79b8cdb drm-intel-nightly: 2016y-10m-28d-12h-29m-33s UTC integration manifest ee9a76e drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things == Logs == For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_2852/ _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-10-28 16:59 ` ville.syrjala @ 2016-10-28 21:05 ` Paul Bolle -1 siblings, 0 replies; 20+ messages in thread From: Paul Bolle @ 2016-10-28 21:05 UTC (permalink / raw) To: ville.syrjala, intel-gfx Cc: Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Fri, 2016-10-28 at 19:59 +0300, ville.syrjala@linux.intel.com wrote: > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.") Obviously, I'm pretty happy with this patch. One question though: this fixes a commit that shipped in v4.6. Do you have any idea why this issue apparently never surfaced before v4.8? Thanks, Paul Bolle ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-10-28 21:05 ` Paul Bolle 0 siblings, 0 replies; 20+ messages in thread From: Paul Bolle @ 2016-10-28 21:05 UTC (permalink / raw) To: ville.syrjala, intel-gfx Cc: bruno.pagani, Daniel J Blueman, stable, Joseph Yasi On Fri, 2016-10-28 at 19:59 +0300, ville.syrjala@linux.intel.com wrote: > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.") Obviously, I'm pretty happy with this patch. One question though: this fixes a commit that shipped in v4.6. Do you have any idea why this issue apparently never surfaced before v4.8? Thanks, Paul Bolle _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-10-28 21:05 ` Paul Bolle @ 2016-10-28 21:30 ` Ville Syrjälä -1 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-10-28 21:30 UTC (permalink / raw) To: Paul Bolle Cc: intel-gfx, Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Fri, Oct 28, 2016 at 11:05:48PM +0200, Paul Bolle wrote: > On Fri, 2016-10-28 at 19:59 +0300, ville.syrjala@linux.intel.com wrote: > > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.") > > Obviously, I'm pretty happy with this patch. One question though: this > fixes a commit that shipped in v4.6. Do you have any idea why this > issue apparently never surfaced before v4.8? Pop quiz time, eh? :) I think it was probably due to commit 44d1240d006c9cd0249263b5449c8e4752500f6a Author: Marek Szyprowski <m.szyprowski@samsung.com> Date: Mon Jun 13 11:11:26 2016 +0200 drm: add generic zpos property If you want want to grade my answer all you have to do is revert that on 4.8 and see what happens. Might be interesting to see if I'm right actually since the link between the WARN and that commit isn't entirely obvious. -- Ville Syrj�l� Intel OTC ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-10-28 21:30 ` Ville Syrjälä 0 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-10-28 21:30 UTC (permalink / raw) To: Paul Bolle; +Cc: bruno.pagani, Daniel J Blueman, intel-gfx, stable, Joseph Yasi On Fri, Oct 28, 2016 at 11:05:48PM +0200, Paul Bolle wrote: > On Fri, 2016-10-28 at 19:59 +0300, ville.syrjala@linux.intel.com wrote: > > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.") > > Obviously, I'm pretty happy with this patch. One question though: this > fixes a commit that shipped in v4.6. Do you have any idea why this > issue apparently never surfaced before v4.8? Pop quiz time, eh? :) I think it was probably due to commit 44d1240d006c9cd0249263b5449c8e4752500f6a Author: Marek Szyprowski <m.szyprowski@samsung.com> Date: Mon Jun 13 11:11:26 2016 +0200 drm: add generic zpos property If you want want to grade my answer all you have to do is revert that on 4.8 and see what happens. Might be interesting to see if I'm right actually since the link between the WARN and that commit isn't entirely obvious. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-10-28 21:30 ` Ville Syrjälä @ 2016-10-28 21:38 ` Paul Bolle -1 siblings, 0 replies; 20+ messages in thread From: Paul Bolle @ 2016-10-28 21:38 UTC (permalink / raw) To: Ville Syrjälä Cc: intel-gfx, Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Sat, 2016-10-29 at 00:30 +0300, Ville Syrjälä wrote: > I think it was probably due to > > commit 44d1240d006c9cd0249263b5449c8e4752500f6a > Author: Marek Szyprowski <m.szyprowski@samsung.com> > Date: Mon Jun 13 11:11:26 2016 +0200 > > drm: add generic zpos property > > If you want want to grade my answer all you have to do is revert that > on 4.8 and see what happens. Might be interesting to see if I'm right > actually since the link between the WARN and that commit isn't entirely > obvious. You mean reverting commit 44d1240d006c ("drm: add generic zpos property") and not applying this fix, right? Anyway, I'm happy to grade your answer in a few days. Please prod if notifying you of your grade takes too long. Paul Bolle ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-10-28 21:38 ` Paul Bolle 0 siblings, 0 replies; 20+ messages in thread From: Paul Bolle @ 2016-10-28 21:38 UTC (permalink / raw) To: Ville Syrjälä Cc: bruno.pagani, Daniel J Blueman, intel-gfx, stable, Joseph Yasi On Sat, 2016-10-29 at 00:30 +0300, Ville Syrjälä wrote: > I think it was probably due to > > commit 44d1240d006c9cd0249263b5449c8e4752500f6a > Author: Marek Szyprowski <m.szyprowski@samsung.com> > Date: Mon Jun 13 11:11:26 2016 +0200 > > drm: add generic zpos property > > If you want want to grade my answer all you have to do is revert that > on 4.8 and see what happens. Might be interesting to see if I'm right > actually since the link between the WARN and that commit isn't entirely > obvious. You mean reverting commit 44d1240d006c ("drm: add generic zpos property") and not applying this fix, right? Anyway, I'm happy to grade your answer in a few days. Please prod if notifying you of your grade takes too long. Paul Bolle _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-10-28 21:38 ` Paul Bolle @ 2016-10-28 22:01 ` Ville Syrjälä -1 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-10-28 22:01 UTC (permalink / raw) To: Paul Bolle Cc: intel-gfx, Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Fri, Oct 28, 2016 at 11:38:14PM +0200, Paul Bolle wrote: > On Sat, 2016-10-29 at 00:30 +0300, Ville Syrj�l� wrote: > > I think it was probably due to > > > > commit 44d1240d006c9cd0249263b5449c8e4752500f6a > > Author: Marek Szyprowski <m.szyprowski@samsung.com> > > Date:���Mon Jun 13 11:11:26 2016 +0200 > > > > ����drm: add generic zpos property > > > > If you want want to grade my answer all you have to do is revert that > > on 4.8 and see what happens. Might be interesting to see if I'm right > > actually since the link between the WARN and that commit isn't entirely > > obvious. > > You mean reverting commit�44d1240d006c ("drm: add generic zpos > property") and not applying this fix, right? Yep. > Anyway, I'm happy to grade your answer in a few days. Please prod if > notifying you of your grade takes too long. > > > Paul Bolle -- Ville Syrj�l� Intel OTC ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-10-28 22:01 ` Ville Syrjälä 0 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-10-28 22:01 UTC (permalink / raw) To: Paul Bolle Cc: intel-gfx, Maarten Lankhorst, Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Fri, Oct 28, 2016 at 11:38:14PM +0200, Paul Bolle wrote: > On Sat, 2016-10-29 at 00:30 +0300, Ville Syrjälä wrote: > > I think it was probably due to > > > > commit 44d1240d006c9cd0249263b5449c8e4752500f6a > > Author: Marek Szyprowski <m.szyprowski@samsung.com> > > Date: Mon Jun 13 11:11:26 2016 +0200 > > > > drm: add generic zpos property > > > > If you want want to grade my answer all you have to do is revert that > > on 4.8 and see what happens. Might be interesting to see if I'm right > > actually since the link between the WARN and that commit isn't entirely > > obvious. > > You mean reverting commit 44d1240d006c ("drm: add generic zpos > property") and not applying this fix, right? Yep. > Anyway, I'm happy to grade your answer in a few days. Please prod if > notifying you of your grade takes too long. > > > Paul Bolle -- Ville Syrjälä Intel OTC ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-10-28 16:59 ` ville.syrjala @ 2016-11-01 8:57 ` Maarten Lankhorst -1 siblings, 0 replies; 20+ messages in thread From: Maarten Lankhorst @ 2016-11-01 8:57 UTC (permalink / raw) To: ville.syrjala, intel-gfx Cc: Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle, Joseph Yasi, stable 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. 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> ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-11-01 8:57 ` Maarten Lankhorst 0 siblings, 0 replies; 20+ messages in thread From: Maarten Lankhorst @ 2016-11-01 8:57 UTC (permalink / raw) To: ville.syrjala, intel-gfx Cc: bruno.pagani, Daniel J Blueman, Paul Bolle, stable, Joseph Yasi 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. 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> _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-11-01 8:57 ` Maarten Lankhorst (?) @ 2016-11-01 9:07 ` Paul Bolle 2016-11-01 12:41 ` Ville Syrjälä -1 siblings, 1 reply; 20+ messages in thread From: Paul Bolle @ 2016-11-01 9:07 UTC (permalink / raw) To: Maarten Lankhorst, ville.syrjala, intel-gfx Cc: Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Tue, 2016-11-01 at 09:57 +0100, Maarten Lankhorst wrote: > 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> So I've been running this patch for a few days now. First I tested it on top of v4.8.4. Now I'm running it on top of v4.8.5. My current v4.8.5 boot saw this new (for me) *ERROR*, twice: <3>[43483.521341] [drm:skl_set_cdclk [i915]] *ERROR* failed to inform PCU about cdclk change <3>[108639.090776] [drm:skl_set_cdclk [i915]] *ERROR* failed to inform PCU about cdclk change Related or a coincidence? Thanks, Paul Bolle ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-11-01 9:07 ` Paul Bolle @ 2016-11-01 12:41 ` Ville Syrjälä 0 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-11-01 12:41 UTC (permalink / raw) To: Paul Bolle Cc: Maarten Lankhorst, intel-gfx, Mika Kahola, bruno.pagani, Daniel J Blueman, Joseph Yasi, stable On Tue, Nov 01, 2016 at 10:07:51AM +0100, Paul Bolle wrote: > On Tue, 2016-11-01 at 09:57 +0100, Maarten Lankhorst wrote: > > 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> > > So I've been running this patch for a few days now. First I tested it > on top of v4.8.4. Now I'm running it on top of v4.8.5. > > My current v4.8.5 boot saw this new (for me) *ERROR*, twice: > � ��<3>[43483.521341] [drm:skl_set_cdclk [i915]] *ERROR* failed to inform PCU about cdclk change > <3>[108639.090776] [drm:skl_set_cdclk [i915]] *ERROR* failed to inform PCU about cdclk change > > Related or a coincidence? Not directly related. That's actually a more serious problem we really need to figure out. I already tried to fix it but apparently I failed. -- Ville Syrj�l� Intel OTC ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-11-01 12:41 ` Ville Syrjälä 0 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-11-01 12:41 UTC (permalink / raw) To: Paul Bolle; +Cc: bruno.pagani, Daniel J Blueman, intel-gfx, stable, Joseph Yasi On Tue, Nov 01, 2016 at 10:07:51AM +0100, Paul Bolle wrote: > On Tue, 2016-11-01 at 09:57 +0100, Maarten Lankhorst wrote: > > 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> > > So I've been running this patch for a few days now. First I tested it > on top of v4.8.4. Now I'm running it on top of v4.8.5. > > My current v4.8.5 boot saw this new (for me) *ERROR*, twice: > <3>[43483.521341] [drm:skl_set_cdclk [i915]] *ERROR* failed to inform PCU about cdclk change > <3>[108639.090776] [drm:skl_set_cdclk [i915]] *ERROR* failed to inform PCU about cdclk change > > Related or a coincidence? Not directly related. That's actually a more serious problem we really need to figure out. I already tried to fix it but apparently I failed. -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-11-01 8:57 ` Maarten Lankhorst @ 2016-11-01 12:40 ` Ville Syrjälä -1 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-11-01 12:40 UTC (permalink / raw) To: Maarten Lankhorst Cc: intel-gfx, Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle, Joseph Yasi, stable 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. > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-11-01 12:40 ` Ville Syrjälä 0 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-11-01 12:40 UTC (permalink / raw) To: Maarten Lankhorst Cc: intel-gfx, Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle, Joseph Yasi, stable 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. > > 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 ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things 2016-11-01 12:40 ` Ville Syrjälä @ 2016-11-01 12:53 ` Ville Syrjälä -1 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-11-01 12:53 UTC (permalink / raw) To: Maarten Lankhorst Cc: intel-gfx, Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle, Joseph Yasi, stable 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things @ 2016-11-01 12:53 ` Ville Syrjälä 0 siblings, 0 replies; 20+ messages in thread From: Ville Syrjälä @ 2016-11-01 12:53 UTC (permalink / raw) To: Maarten Lankhorst Cc: intel-gfx, Mika Kahola, bruno.pagani, Daniel J Blueman, Paul Bolle, Joseph Yasi, stable 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 ^ permalink raw reply related [flat|nested] 20+ messages in thread
end of thread, other threads:[~2016-11-01 12:53 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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ä 2016-11-01 12:53 ` Ville Syrjälä
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.