[PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[ville.syrjala]

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

[PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[ville.syrjala]

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

✓ Fi.CI.BAT: success for drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Patchwork]

== 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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Paul Bolle]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Paul Bolle]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Paul Bolle]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Paul Bolle]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Maarten Lankhorst]

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>

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Maarten Lankhorst]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Paul Bolle]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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

Re: [PATCH] drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things

[Ville Syrjälä]

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