[PATCH 0/3] drm/i915: atomic_cdclk_freq fixes

[PATCH 0/3] drm/i915: atomic_cdclk_freq fixes

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

My little fix for populating intel_state->cdclk with
dev_priv->atomic_cdclk_freq became a little meatier after I realized
that we really need to protect it with what is essentially a crtc
rwlock. That is, writers need to hold all the crtc locks, whereas
readers need to hold at least one.

Ville Syrjälä (3):
  drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things
  drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks
  drm/i915: Simplify error handling in intel_modeset_all_pipes()

 drivers/gpu/drm/i915/i915_drv.h      |  9 +++++-
 drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
 2 files changed, 55 insertions(+), 14 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2 1/3] 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.

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.

v2: Drop the dev_cdclk stuff (Maarten)

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> (v1)
Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
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>
Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e48d9571c99d..70f3f0b70263 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14027,8 +14027,9 @@ 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;
+	}
 
 	intel_modeset_clear_plls(state);
 
@@ -14129,8 +14130,9 @@ 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;
+	}
 
 	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

[PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
actually touching the hardware, in which case we won't force a modeset
on all the pipes, and thus won't lock any of the other pipes either.
That means a parallel plane update on another pipe could be looking at
a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
plane configuration is invalid, or potentially reject a valid update.

To overcome this we must protect writes to atomic_cdclk_freq with
all the crtc locks, and thus for reads any single crtc lock will
be sufficient protection.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
 drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
 2 files changed, 44 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0f1dfc7119e..66d2950dc657 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1874,7 +1874,14 @@ struct drm_i915_private {
 
 	unsigned int fsb_freq, mem_freq, is_ddr3;
 	unsigned int skl_preferred_vco_freq;
-	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
+	unsigned int cdclk_freq, max_cdclk_freq;
+
+	/*
+	 * For reading holding any crtc lock is sufficient,
+	 * for writing must hold all of them.
+	 */
+	unsigned int atomic_cdclk_freq;
+
 	unsigned int max_dotclk_freq;
 	unsigned int rawclk_freq;
 	unsigned int hpll_freq;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 70f3f0b70263..d7a4bc63b05b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13946,13 +13946,32 @@ 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;
+
+	/* Add all pipes to the state */
+	for_each_crtc(state->dev, crtc) {
+		struct drm_crtc_state *crtc_state;
+
+		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;
 	struct drm_crtc_state *crtc_state;
 	int ret = 0;
 
-	/* add all active pipes to the state */
+	/*
+	 * Add all pipes to the state, and force
+	 * a modeset on all the active ones.
+	 */
 	for_each_crtc(state->dev, crtc) {
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state))
@@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 		if (ret < 0)
 			return ret;
 
+		/*
+		 * Writes to dev_priv->atomic_cdclk_freq must protected by
+		 * holding all the crtc locks, even if we don't end up
+		 * touching the hardware
+		 */
+		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
+			ret = intel_lock_all_pipes(state);
+			if (ret < 0)
+				return ret;
+		}
+
+		/* All pipes must be switched off while we change the cdclk. */
 		if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
-		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
+		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
 			ret = intel_modeset_all_pipes(state);
-
-		if (ret < 0)
-			return ret;
+			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);
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915: Simplify error handling in intel_modeset_all_pipes()

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

No need for the extra break statements and whatnot, just return the
error directly. And tighten the scope of the local variables while at
it.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d7a4bc63b05b..b158af6d89b3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13965,14 +13965,15 @@ static int intel_lock_all_pipes(struct drm_atomic_state *state)
 static int intel_modeset_all_pipes(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *crtc_state;
-	int ret = 0;
 
 	/*
 	 * Add all pipes to the state, and force
 	 * a modeset on all the active ones.
 	 */
 	for_each_crtc(state->dev, crtc) {
+		struct drm_crtc_state *crtc_state;
+		int ret;
+
 		crtc_state = drm_atomic_get_crtc_state(state, crtc);
 		if (IS_ERR(crtc_state))
 			return PTR_ERR(crtc_state);
@@ -13984,14 +13985,14 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
 
 		ret = drm_atomic_add_affected_connectors(state, crtc);
 		if (ret)
-			break;
+			return ret;
 
 		ret = drm_atomic_add_affected_planes(state, crtc);
 		if (ret)
-			break;
+			return ret;
 	}
 
-	return ret;
+	return 0;
 }
 
 static int intel_modeset_checks(struct drm_atomic_state *state)
-- 
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: atomic_cdclk_freq fixes

[Patchwork]

== Series Details ==

Series: drm/i915: atomic_cdclk_freq fixes
URL   : https://patchwork.freedesktop.org/series/15288/
State : success

== Summary ==

Series 15288v1 drm/i915: atomic_cdclk_freq fixes
https://patchwork.freedesktop.org/api/1.0/series/15288/revisions/1/mbox/


fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-bxt-t5700     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33 

8670f0f0d91190e0d090ee910c73ed83c37cfef5 drm-intel-nightly: 2016y-11m-14d-16h-10m-52s UTC integration manifest
9832f6f drm/i915: Simplify error handling in intel_modeset_all_pipes()
ede8100 drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks
97437e8 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_2985/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Daniel Vetter]

On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> actually touching the hardware, in which case we won't force a modeset
> on all the pipes, and thus won't lock any of the other pipes either.
> That means a parallel plane update on another pipe could be looking at
> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> plane configuration is invalid, or potentially reject a valid update.

It's a bit a bikeshed, but everywhere else in atomic those kinds of data
are called ->state or something like that. Diverging and giving it an
atomic_ prefix seems a bit funky.

Maybe we need a dev_priv->state substruct where we could put all these
global atomic bits into?
-Daniel

> 
> To overcome this we must protect writes to atomic_cdclk_freq with
> all the crtc locks, and thus for reads any single crtc lock will
> be sufficient protection.
> 
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0f1dfc7119e..66d2950dc657 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
>  
>  	unsigned int fsb_freq, mem_freq, is_ddr3;
>  	unsigned int skl_preferred_vco_freq;
> -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> +	unsigned int cdclk_freq, max_cdclk_freq;
> +
> +	/*
> +	 * For reading holding any crtc lock is sufficient,
> +	 * for writing must hold all of them.
> +	 */
> +	unsigned int atomic_cdclk_freq;
> +
>  	unsigned int max_dotclk_freq;
>  	unsigned int rawclk_freq;
>  	unsigned int hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 70f3f0b70263..d7a4bc63b05b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13946,13 +13946,32 @@ 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;
> +
> +	/* Add all pipes to the state */
> +	for_each_crtc(state->dev, crtc) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		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;
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> -	/* add all active pipes to the state */
> +	/*
> +	 * Add all pipes to the state, and force
> +	 * a modeset on all the active ones.
> +	 */
>  	for_each_crtc(state->dev, crtc) {
>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>  		if (IS_ERR(crtc_state))
> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  		if (ret < 0)
>  			return ret;
>  
> +		/*
> +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> +		 * holding all the crtc locks, even if we don't end up
> +		 * touching the hardware
> +		 */
> +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> +			ret = intel_lock_all_pipes(state);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
> +		/* All pipes must be switched off while we change the cdclk. */
>  		if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> -		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
> +		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
>  			ret = intel_modeset_all_pipes(state);
> -
> -		if (ret < 0)
> -			return ret;
> +			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);
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Daniel Vetter]

On Tue, Nov 15, 2016 at 10:08:10AM +0100, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> > actually touching the hardware, in which case we won't force a modeset
> > on all the pipes, and thus won't lock any of the other pipes either.
> > That means a parallel plane update on another pipe could be looking at
> > a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> > plane configuration is invalid, or potentially reject a valid update.
> 
> It's a bit a bikeshed, but everywhere else in atomic those kinds of data
> are called ->state or something like that. Diverging and giving it an
> atomic_ prefix seems a bit funky.
> 
> Maybe we need a dev_priv->state substruct where we could put all these
> global atomic bits into?

Even more bikesheds in this area: Having different names for the same
stuff in intel_state and dev_priv is imo also bad.

Another one: We unconditionally put the values from intel_state into
dev_priv when intel_state->modeset is set. I think going more full-on with
state structs (intel_global_modeset_state) and pointers would streamline
the code a lot. With a full state structure and a real pointer in
intel_state we could also use the full get_state/destroy_state pattern,
and hide the locking in there. Everyone who just needs read access could
then either look at dev_priv->modeset_state->cdclk_freq, or we could have
a copy in intel_pipe_config.

Oh well, I guess cleaning that up is something for another day. Patch
itself looks correct.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> -Daniel
> 
> > 
> > To overcome this we must protect writes to atomic_cdclk_freq with
> > all the crtc locks, and thus for reads any single crtc lock will
> > be sufficient protection.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
> >  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c0f1dfc7119e..66d2950dc657 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1874,7 +1874,14 @@ struct drm_i915_private {
> >  
> >  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >  	unsigned int skl_preferred_vco_freq;
> > -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> > +	unsigned int cdclk_freq, max_cdclk_freq;
> > +
> > +	/*
> > +	 * For reading holding any crtc lock is sufficient,
> > +	 * for writing must hold all of them.
> > +	 */
> > +	unsigned int atomic_cdclk_freq;
> > +
> >  	unsigned int max_dotclk_freq;
> >  	unsigned int rawclk_freq;
> >  	unsigned int hpll_freq;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 70f3f0b70263..d7a4bc63b05b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13946,13 +13946,32 @@ 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;
> > +
> > +	/* Add all pipes to the state */
> > +	for_each_crtc(state->dev, crtc) {
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		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;
> >  	struct drm_crtc_state *crtc_state;
> >  	int ret = 0;
> >  
> > -	/* add all active pipes to the state */
> > +	/*
> > +	 * Add all pipes to the state, and force
> > +	 * a modeset on all the active ones.
> > +	 */
> >  	for_each_crtc(state->dev, crtc) {
> >  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >  		if (IS_ERR(crtc_state))
> > @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >  		if (ret < 0)
> >  			return ret;
> >  
> > +		/*
> > +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> > +		 * holding all the crtc locks, even if we don't end up
> > +		 * touching the hardware
> > +		 */
> > +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> > +			ret = intel_lock_all_pipes(state);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		/* All pipes must be switched off while we change the cdclk. */
> >  		if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> > -		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
> > +		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
> >  			ret = intel_modeset_all_pipes(state);
> > -
> > -		if (ret < 0)
> > -			return ret;
> > +			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);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Simplify error handling in intel_modeset_all_pipes()

[Daniel Vetter]

On Mon, Nov 14, 2016 at 06:35:11PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> No need for the extra break statements and whatnot, just return the
> error directly. And tighten the scope of the local variables while at
> it.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d7a4bc63b05b..b158af6d89b3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13965,14 +13965,15 @@ static int intel_lock_all_pipes(struct drm_atomic_state *state)
>  static int intel_modeset_all_pipes(struct drm_atomic_state *state)
>  {
>  	struct drm_crtc *crtc;
> -	struct drm_crtc_state *crtc_state;
> -	int ret = 0;
>  
>  	/*
>  	 * Add all pipes to the state, and force
>  	 * a modeset on all the active ones.
>  	 */
>  	for_each_crtc(state->dev, crtc) {
> +		struct drm_crtc_state *crtc_state;
> +		int ret;
> +
>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>  		if (IS_ERR(crtc_state))
>  			return PTR_ERR(crtc_state);
> @@ -13984,14 +13985,14 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
>  
>  		ret = drm_atomic_add_affected_connectors(state, crtc);
>  		if (ret)
> -			break;
> +			return ret;
>  
>  		ret = drm_atomic_add_affected_planes(state, crtc);
>  		if (ret)
> -			break;
> +			return ret;
>  	}
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int intel_modeset_checks(struct drm_atomic_state *state)
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 10:08:10AM +0100, Daniel Vetter wrote:
> On Mon, Nov 14, 2016 at 06:35:10PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> > actually touching the hardware, in which case we won't force a modeset
> > on all the pipes, and thus won't lock any of the other pipes either.
> > That means a parallel plane update on another pipe could be looking at
> > a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> > plane configuration is invalid, or potentially reject a valid update.
> 
> It's a bit a bikeshed, but everywhere else in atomic those kinds of data
> are called ->state or something like that. Diverging and giving it an
> atomic_ prefix seems a bit funky.
> 
> Maybe we need a dev_priv->state substruct where we could put all these
> global atomic bits into?

Yeah, I've been complaining about this stuff for quite a while now.
But so far I wasn't able to bait Maarten into doing the work ;)

> -Daniel
> 
> > 
> > To overcome this we must protect writes to atomic_cdclk_freq with
> > all the crtc locks, and thus for reads any single crtc lock will
> > be sufficient protection.
> > 
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
> >  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c0f1dfc7119e..66d2950dc657 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1874,7 +1874,14 @@ struct drm_i915_private {
> >  
> >  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >  	unsigned int skl_preferred_vco_freq;
> > -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> > +	unsigned int cdclk_freq, max_cdclk_freq;
> > +
> > +	/*
> > +	 * For reading holding any crtc lock is sufficient,
> > +	 * for writing must hold all of them.
> > +	 */
> > +	unsigned int atomic_cdclk_freq;
> > +
> >  	unsigned int max_dotclk_freq;
> >  	unsigned int rawclk_freq;
> >  	unsigned int hpll_freq;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 70f3f0b70263..d7a4bc63b05b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13946,13 +13946,32 @@ 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;
> > +
> > +	/* Add all pipes to the state */
> > +	for_each_crtc(state->dev, crtc) {
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		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;
> >  	struct drm_crtc_state *crtc_state;
> >  	int ret = 0;
> >  
> > -	/* add all active pipes to the state */
> > +	/*
> > +	 * Add all pipes to the state, and force
> > +	 * a modeset on all the active ones.
> > +	 */
> >  	for_each_crtc(state->dev, crtc) {
> >  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >  		if (IS_ERR(crtc_state))
> > @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >  		if (ret < 0)
> >  			return ret;
> >  
> > +		/*
> > +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> > +		 * holding all the crtc locks, even if we don't end up
> > +		 * touching the hardware
> > +		 */
> > +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> > +			ret = intel_lock_all_pipes(state);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> > +		/* All pipes must be switched off while we change the cdclk. */
> >  		if (intel_state->dev_cdclk != dev_priv->cdclk_freq ||
> > -		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)
> > +		    intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco) {
> >  			ret = intel_modeset_all_pipes(state);
> > -
> > -		if (ret < 0)
> > -			return ret;
> > +			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);
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Maarten Lankhorst]

Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> actually touching the hardware, in which case we won't force a modeset
> on all the pipes, and thus won't lock any of the other pipes either.
> That means a parallel plane update on another pipe could be looking at
> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> plane configuration is invalid, or potentially reject a valid update.
>
> To overcome this we must protect writes to atomic_cdclk_freq with
> all the crtc locks, and thus for reads any single crtc lock will
> be sufficient protection.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
>  2 files changed, 44 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c0f1dfc7119e..66d2950dc657 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
>  
>  	unsigned int fsb_freq, mem_freq, is_ddr3;
>  	unsigned int skl_preferred_vco_freq;
> -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> +	unsigned int cdclk_freq, max_cdclk_freq;
> +
> +	/*
> +	 * For reading holding any crtc lock is sufficient,
> +	 * for writing must hold all of them.
> +	 */
> +	unsigned int atomic_cdclk_freq;
> +
>  	unsigned int max_dotclk_freq;
>  	unsigned int rawclk_freq;
>  	unsigned int hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 70f3f0b70263..d7a4bc63b05b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13946,13 +13946,32 @@ 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;
> +
> +	/* Add all pipes to the state */
> +	for_each_crtc(state->dev, crtc) {
> +		struct drm_crtc_state *crtc_state;
> +
> +		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;
>  	struct drm_crtc_state *crtc_state;
>  	int ret = 0;
>  
> -	/* add all active pipes to the state */
> +	/*
> +	 * Add all pipes to the state, and force
> +	 * a modeset on all the active ones.
> +	 */
>  	for_each_crtc(state->dev, crtc) {
>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>  		if (IS_ERR(crtc_state))
> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  		if (ret < 0)
>  			return ret;
>  
> +		/*
> +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> +		 * holding all the crtc locks, even if we don't end up
> +		 * touching the hardware
> +		 */
> +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> +			ret = intel_lock_all_pipes(state);
> +			if (ret < 0)
> +				return ret;
> +		}
> +
Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
it won't matter much.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

Op 14-11-16 om 17:35 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.
>
> 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.
>
> v2: Drop the dev_cdclk stuff (Maarten)
>
> 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> (v1)
> Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
> 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>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e48d9571c99d..70f3f0b70263 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14027,8 +14027,9 @@ 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;
> +	}
>  
>  	intel_modeset_clear_plls(state);
>  
> @@ -14129,8 +14130,9 @@ 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;
> +	}
>  
>  	ret = drm_atomic_helper_check_planes(dev, state);
>  	if (ret)

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
> Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> > actually touching the hardware, in which case we won't force a modeset
> > on all the pipes, and thus won't lock any of the other pipes either.
> > That means a parallel plane update on another pipe could be looking at
> > a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> > plane configuration is invalid, or potentially reject a valid update.
> >
> > To overcome this we must protect writes to atomic_cdclk_freq with
> > all the crtc locks, and thus for reads any single crtc lock will
> > be sufficient protection.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
> >  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
> >  2 files changed, 44 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index c0f1dfc7119e..66d2950dc657 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1874,7 +1874,14 @@ struct drm_i915_private {
> >  
> >  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >  	unsigned int skl_preferred_vco_freq;
> > -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> > +	unsigned int cdclk_freq, max_cdclk_freq;
> > +
> > +	/*
> > +	 * For reading holding any crtc lock is sufficient,
> > +	 * for writing must hold all of them.
> > +	 */
> > +	unsigned int atomic_cdclk_freq;
> > +
> >  	unsigned int max_dotclk_freq;
> >  	unsigned int rawclk_freq;
> >  	unsigned int hpll_freq;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 70f3f0b70263..d7a4bc63b05b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13946,13 +13946,32 @@ 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;
> > +
> > +	/* Add all pipes to the state */
> > +	for_each_crtc(state->dev, crtc) {
> > +		struct drm_crtc_state *crtc_state;
> > +
> > +		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;
> >  	struct drm_crtc_state *crtc_state;
> >  	int ret = 0;
> >  
> > -	/* add all active pipes to the state */
> > +	/*
> > +	 * Add all pipes to the state, and force
> > +	 * a modeset on all the active ones.
> > +	 */
> >  	for_each_crtc(state->dev, crtc) {
> >  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >  		if (IS_ERR(crtc_state))
> > @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >  		if (ret < 0)
> >  			return ret;
> >  
> > +		/*
> > +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> > +		 * holding all the crtc locks, even if we don't end up
> > +		 * touching the hardware
> > +		 */
> > +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> > +			ret = intel_lock_all_pipes(state);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +
> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
> it won't matter much.

Is there any benefit in doing that? A bit confusing IMO to force a
modeset when you don't have to.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Maarten Lankhorst]

Op 15-11-16 om 14:41 schreef Ville Syrjälä:
> On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
>> Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
>>> actually touching the hardware, in which case we won't force a modeset
>>> on all the pipes, and thus won't lock any of the other pipes either.
>>> That means a parallel plane update on another pipe could be looking at
>>> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
>>> plane configuration is invalid, or potentially reject a valid update.
>>>
>>> To overcome this we must protect writes to atomic_cdclk_freq with
>>> all the crtc locks, and thus for reads any single crtc lock will
>>> be sufficient protection.
>>>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
>>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
>>>  2 files changed, 44 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index c0f1dfc7119e..66d2950dc657 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
>>>  
>>>  	unsigned int fsb_freq, mem_freq, is_ddr3;
>>>  	unsigned int skl_preferred_vco_freq;
>>> -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
>>> +	unsigned int cdclk_freq, max_cdclk_freq;
>>> +
>>> +	/*
>>> +	 * For reading holding any crtc lock is sufficient,
>>> +	 * for writing must hold all of them.
>>> +	 */
>>> +	unsigned int atomic_cdclk_freq;
>>> +
>>>  	unsigned int max_dotclk_freq;
>>>  	unsigned int rawclk_freq;
>>>  	unsigned int hpll_freq;
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>> index 70f3f0b70263..d7a4bc63b05b 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -13946,13 +13946,32 @@ 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;
>>> +
>>> +	/* Add all pipes to the state */
>>> +	for_each_crtc(state->dev, crtc) {
>>> +		struct drm_crtc_state *crtc_state;
>>> +
>>> +		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;
>>>  	struct drm_crtc_state *crtc_state;
>>>  	int ret = 0;
>>>  
>>> -	/* add all active pipes to the state */
>>> +	/*
>>> +	 * Add all pipes to the state, and force
>>> +	 * a modeset on all the active ones.
>>> +	 */
>>>  	for_each_crtc(state->dev, crtc) {
>>>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>>  		if (IS_ERR(crtc_state))
>>> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>>  		if (ret < 0)
>>>  			return ret;
>>>  
>>> +		/*
>>> +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
>>> +		 * holding all the crtc locks, even if we don't end up
>>> +		 * touching the hardware
>>> +		 */
>>> +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
>>> +			ret = intel_lock_all_pipes(state);
>>> +			if (ret < 0)
>>> +				return ret;
>>> +		}
>>> +
>> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
>> it won't matter much.
> Is there any benefit in doing that? A bit confusing IMO to force a
> modeset when you don't have to.
>
The case where atomic cdclk changes, but dev_cdclk stays the same can only happen
if you configure a crtc, but all crtc's stay !active. In all other cases dev_cdclk
will change too.

intel_modeset_all_pipes will only set mode_changed on active crtc's, but it will
add all crtc's to the atomic state regardless to make sure the cdclk stays consistent.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Paul Bolle]

On Mon, 2016-11-14 at 18:35 +0200, ville.syrjala@linux.intel.com wrote:
> 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.
> 
> 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.
> 
> v2: Drop the dev_cdclk stuff (Maarten)
> 
> 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> (v1)

I've run v2 of this patch (on top of v4.8.8) for over a day now without
hitting the WARN_ON_ONCE. Of course, my machine was suspended for large parts
of that period. But still, the WARN_ON_ONCE used to be triggered much quicker.
So in short: you can drop "(v1)" as I tested both versions now.

By the way, the scary i915 *ERROR*s are gone now too, as are the visual
glitches that accompanied those *ERROR*s. Apparently the v4.8.y series picked
up a few fixes. Those made i915 a much better experience. Nice! 

> Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
> Cc: stable@vger.kernel.org
> Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's were active.")

(I seem to remember discussing the reasons why a v4.6 bug was first noticed on
v4.8. I haven't looked into that yet. By now it's unlikely I ever will. Sorry
about that.)

> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98214
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks,


Paul Bolle

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

[Joseph Yasi]

[-- Attachment #1.1: Type: text/plain, Size: 2700 bytes --]

On Thu, Nov 17, 2016 at 3:17 AM, Paul Bolle <pebolle@tiscali.nl> wrote:

> On Mon, 2016-11-14 at 18:35 +0200, ville.syrjala@linux.intel.com wrote:
> > 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.
> >
> > 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.
> >
> > v2: Drop the dev_cdclk stuff (Maarten)
> >
> > 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> (v1)
>
> I've run v2 of this patch (on top of v4.8.8) for over a day now without
> hitting the WARN_ON_ONCE. Of course, my machine was suspended for large
> parts
> of that period. But still, the WARN_ON_ONCE used to be triggered much
> quicker.
> So in short: you can drop "(v1)" as I tested both versions now.
>
> By the way, the scary i915 *ERROR*s are gone now too, as are the visual
> glitches that accompanied those *ERROR*s. Apparently the v4.8.y series
> picked
> up a few fixes. Those made i915 a much better experience. Nice!
>
> > Tested-by: Joseph Yasi <joe.yasi@gmail.com> (v1)
>

I've also applied this patch (and the other two in the series) on top of
4.8.8. I haven't hit the WARN_ON_ONCE with the patches either.

The flickering I was experiencing earlier turned out to be due to a
motherboard failure. Replacing the motherboard fixed that problem.


> > Cc: stable@vger.kernel.org
> > Fixes: 1a617b77658e ("drm/i915: Keep track of the cdclk as if all crtc's
> were active.")
>
> (I seem to remember discussing the reasons why a v4.6 bug was first
> noticed on
> v4.8. I haven't looked into that yet. By now it's unlikely I ever will.
> Sorry
> about that.)
>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98214
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> Thanks,
>
>
> Paul Bolle
>

[-- Attachment #1.2: Type: text/html, Size: 4267 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Ville Syrjälä]

On Tue, Nov 15, 2016 at 02:53:00PM +0100, Maarten Lankhorst wrote:
> Op 15-11-16 om 14:41 schreef Ville Syrjälä:
> > On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
> >> Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
> >>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>
> >>> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> >>> actually touching the hardware, in which case we won't force a modeset
> >>> on all the pipes, and thus won't lock any of the other pipes either.
> >>> That means a parallel plane update on another pipe could be looking at
> >>> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> >>> plane configuration is invalid, or potentially reject a valid update.
> >>>
> >>> To overcome this we must protect writes to atomic_cdclk_freq with
> >>> all the crtc locks, and thus for reads any single crtc lock will
> >>> be sufficient protection.
> >>>
> >>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> ---
> >>>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
> >>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
> >>>  2 files changed, 44 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>> index c0f1dfc7119e..66d2950dc657 100644
> >>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
> >>>  
> >>>  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >>>  	unsigned int skl_preferred_vco_freq;
> >>> -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> >>> +	unsigned int cdclk_freq, max_cdclk_freq;
> >>> +
> >>> +	/*
> >>> +	 * For reading holding any crtc lock is sufficient,
> >>> +	 * for writing must hold all of them.
> >>> +	 */
> >>> +	unsigned int atomic_cdclk_freq;
> >>> +
> >>>  	unsigned int max_dotclk_freq;
> >>>  	unsigned int rawclk_freq;
> >>>  	unsigned int hpll_freq;
> >>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>> index 70f3f0b70263..d7a4bc63b05b 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -13946,13 +13946,32 @@ 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;
> >>> +
> >>> +	/* Add all pipes to the state */
> >>> +	for_each_crtc(state->dev, crtc) {
> >>> +		struct drm_crtc_state *crtc_state;
> >>> +
> >>> +		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;
> >>>  	struct drm_crtc_state *crtc_state;
> >>>  	int ret = 0;
> >>>  
> >>> -	/* add all active pipes to the state */
> >>> +	/*
> >>> +	 * Add all pipes to the state, and force
> >>> +	 * a modeset on all the active ones.
> >>> +	 */
> >>>  	for_each_crtc(state->dev, crtc) {
> >>>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >>>  		if (IS_ERR(crtc_state))
> >>> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >>>  		if (ret < 0)
> >>>  			return ret;
> >>>  
> >>> +		/*
> >>> +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> >>> +		 * holding all the crtc locks, even if we don't end up
> >>> +		 * touching the hardware
> >>> +		 */
> >>> +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> >>> +			ret = intel_lock_all_pipes(state);
> >>> +			if (ret < 0)
> >>> +				return ret;
> >>> +		}
> >>> +
> >> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
> >> it won't matter much.
> > Is there any benefit in doing that? A bit confusing IMO to force a
> > modeset when you don't have to.
> >
> The case where atomic cdclk changes, but dev_cdclk stays the same can only happen
> if you configure a crtc, but all crtc's stay !active. In all other cases dev_cdclk
> will change too.
> 
> intel_modeset_all_pipes will only set mode_changed on active crtc's, but it will
> add all crtc's to the atomic state regardless to make sure the cdclk stays consistent.

I still don't see what the benefit is. IMO it's just confusing to say
that we're going to force a modeset on a disabled pipe.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Maarten Lankhorst]

Op 17-11-16 om 16:06 schreef Ville Syrjälä:
> On Tue, Nov 15, 2016 at 02:53:00PM +0100, Maarten Lankhorst wrote:
>> Op 15-11-16 om 14:41 schreef Ville Syrjälä:
>>> On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
>>>> Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
>>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>>
>>>>> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
>>>>> actually touching the hardware, in which case we won't force a modeset
>>>>> on all the pipes, and thus won't lock any of the other pipes either.
>>>>> That means a parallel plane update on another pipe could be looking at
>>>>> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
>>>>> plane configuration is invalid, or potentially reject a valid update.
>>>>>
>>>>> To overcome this we must protect writes to atomic_cdclk_freq with
>>>>> all the crtc locks, and thus for reads any single crtc lock will
>>>>> be sufficient protection.
>>>>>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>> ---
>>>>>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
>>>>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
>>>>>  2 files changed, 44 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>>>> index c0f1dfc7119e..66d2950dc657 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>>>> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
>>>>>  
>>>>>  	unsigned int fsb_freq, mem_freq, is_ddr3;
>>>>>  	unsigned int skl_preferred_vco_freq;
>>>>> -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
>>>>> +	unsigned int cdclk_freq, max_cdclk_freq;
>>>>> +
>>>>> +	/*
>>>>> +	 * For reading holding any crtc lock is sufficient,
>>>>> +	 * for writing must hold all of them.
>>>>> +	 */
>>>>> +	unsigned int atomic_cdclk_freq;
>>>>> +
>>>>>  	unsigned int max_dotclk_freq;
>>>>>  	unsigned int rawclk_freq;
>>>>>  	unsigned int hpll_freq;
>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>> index 70f3f0b70263..d7a4bc63b05b 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>> @@ -13946,13 +13946,32 @@ 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;
>>>>> +
>>>>> +	/* Add all pipes to the state */
>>>>> +	for_each_crtc(state->dev, crtc) {
>>>>> +		struct drm_crtc_state *crtc_state;
>>>>> +
>>>>> +		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;
>>>>>  	struct drm_crtc_state *crtc_state;
>>>>>  	int ret = 0;
>>>>>  
>>>>> -	/* add all active pipes to the state */
>>>>> +	/*
>>>>> +	 * Add all pipes to the state, and force
>>>>> +	 * a modeset on all the active ones.
>>>>> +	 */
>>>>>  	for_each_crtc(state->dev, crtc) {
>>>>>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
>>>>>  		if (IS_ERR(crtc_state))
>>>>> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>>>>  		if (ret < 0)
>>>>>  			return ret;
>>>>>  
>>>>> +		/*
>>>>> +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
>>>>> +		 * holding all the crtc locks, even if we don't end up
>>>>> +		 * touching the hardware
>>>>> +		 */
>>>>> +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
>>>>> +			ret = intel_lock_all_pipes(state);
>>>>> +			if (ret < 0)
>>>>> +				return ret;
>>>>> +		}
>>>>> +
>>>> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
>>>> it won't matter much.
>>> Is there any benefit in doing that? A bit confusing IMO to force a
>>> modeset when you don't have to.
>>>
>> The case where atomic cdclk changes, but dev_cdclk stays the same can only happen
>> if you configure a crtc, but all crtc's stay !active. In all other cases dev_cdclk
>> will change too.
>>
>> intel_modeset_all_pipes will only set mode_changed on active crtc's, but it will
>> add all crtc's to the atomic state regardless to make sure the cdclk stays consistent.
> I still don't see what the benefit is. IMO it's just confusing to say
> that we're going to force a modeset on a disabled pipe.
>
if (!active || needs_modeset(crtc_state)) continue; crtc_state->mode_changed= true; It's not doing a modeset on a disabled pipe, it only makes sure it's part of the state.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks

[Ville Syrjälä]

On Fri, Nov 18, 2016 at 11:26:18AM +0100, Maarten Lankhorst wrote:
> Op 17-11-16 om 16:06 schreef Ville Syrjälä:
> > On Tue, Nov 15, 2016 at 02:53:00PM +0100, Maarten Lankhorst wrote:
> >> Op 15-11-16 om 14:41 schreef Ville Syrjälä:
> >>> On Tue, Nov 15, 2016 at 11:14:29AM +0100, Maarten Lankhorst wrote:
> >>>> Op 14-11-16 om 17:35 schreef ville.syrjala@linux.intel.com:
> >>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>>
> >>>>> A modeset on one pipe can update dev_priv->atomic_cdclk_freq without
> >>>>> actually touching the hardware, in which case we won't force a modeset
> >>>>> on all the pipes, and thus won't lock any of the other pipes either.
> >>>>> That means a parallel plane update on another pipe could be looking at
> >>>>> a stale dev_priv->atomic_cdcdlk_freq and thus fail to notice when the
> >>>>> plane configuration is invalid, or potentially reject a valid update.
> >>>>>
> >>>>> To overcome this we must protect writes to atomic_cdclk_freq with
> >>>>> all the crtc locks, and thus for reads any single crtc lock will
> >>>>> be sufficient protection.
> >>>>>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>> ---
> >>>>>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++++-
> >>>>>  drivers/gpu/drm/i915/intel_display.c | 41 +++++++++++++++++++++++++++++++-----
> >>>>>  2 files changed, 44 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >>>>> index c0f1dfc7119e..66d2950dc657 100644
> >>>>> --- a/drivers/gpu/drm/i915/i915_drv.h
> >>>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >>>>> @@ -1874,7 +1874,14 @@ struct drm_i915_private {
> >>>>>  
> >>>>>  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >>>>>  	unsigned int skl_preferred_vco_freq;
> >>>>> -	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> >>>>> +	unsigned int cdclk_freq, max_cdclk_freq;
> >>>>> +
> >>>>> +	/*
> >>>>> +	 * For reading holding any crtc lock is sufficient,
> >>>>> +	 * for writing must hold all of them.
> >>>>> +	 */
> >>>>> +	unsigned int atomic_cdclk_freq;
> >>>>> +
> >>>>>  	unsigned int max_dotclk_freq;
> >>>>>  	unsigned int rawclk_freq;
> >>>>>  	unsigned int hpll_freq;
> >>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>>> index 70f3f0b70263..d7a4bc63b05b 100644
> >>>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>>> @@ -13946,13 +13946,32 @@ 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;
> >>>>> +
> >>>>> +	/* Add all pipes to the state */
> >>>>> +	for_each_crtc(state->dev, crtc) {
> >>>>> +		struct drm_crtc_state *crtc_state;
> >>>>> +
> >>>>> +		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;
> >>>>>  	struct drm_crtc_state *crtc_state;
> >>>>>  	int ret = 0;
> >>>>>  
> >>>>> -	/* add all active pipes to the state */
> >>>>> +	/*
> >>>>> +	 * Add all pipes to the state, and force
> >>>>> +	 * a modeset on all the active ones.
> >>>>> +	 */
> >>>>>  	for_each_crtc(state->dev, crtc) {
> >>>>>  		crtc_state = drm_atomic_get_crtc_state(state, crtc);
> >>>>>  		if (IS_ERR(crtc_state))
> >>>>> @@ -14018,12 +14037,24 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> >>>>>  		if (ret < 0)
> >>>>>  			return ret;
> >>>>>  
> >>>>> +		/*
> >>>>> +		 * Writes to dev_priv->atomic_cdclk_freq must protected by
> >>>>> +		 * holding all the crtc locks, even if we don't end up
> >>>>> +		 * touching the hardware
> >>>>> +		 */
> >>>>> +		if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> >>>>> +			ret = intel_lock_all_pipes(state);
> >>>>> +			if (ret < 0)
> >>>>> +				return ret;
> >>>>> +		}
> >>>>> +
> >>>> Would it be terrible to just use intel_modeset_all_pipes here? Since this can only be different in the all crtc's disabled case
> >>>> it won't matter much.
> >>> Is there any benefit in doing that? A bit confusing IMO to force a
> >>> modeset when you don't have to.
> >>>
> >> The case where atomic cdclk changes, but dev_cdclk stays the same can only happen
> >> if you configure a crtc, but all crtc's stay !active. In all other cases dev_cdclk
> >> will change too.
> >>
> >> intel_modeset_all_pipes will only set mode_changed on active crtc's, but it will
> >> add all crtc's to the atomic state regardless to make sure the cdclk stays consistent.
> > I still don't see what the benefit is. IMO it's just confusing to say
> > that we're going to force a modeset on a disabled pipe.
> >
> if (!active || needs_modeset(crtc_state)) continue; crtc_state->mode_changed= true; It's not doing a modeset on a disabled pipe, it only makes sure it's part of the state.

But the code will still *read* if (blah) modeset_all_pipes();
which is nonsense.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 0/3] drm/i915: atomic_cdclk_freq fixes

[Ville Syrjälä]

On Mon, Nov 14, 2016 at 06:35:08PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> My little fix for populating intel_state->cdclk with
> dev_priv->atomic_cdclk_freq became a little meatier after I realized
> that we really need to protect it with what is essentially a crtc
> rwlock. That is, writers need to hold all the crtc locks, whereas
> readers need to hold at least one.
> 
> Ville Syrjälä (3):
>   drm/i915: Fix cdclk vs. dev_cdclk mess when not recomputing things
>   drm/i915: Protect dev_priv->atomic_cdclk_freq with all the crtc locks
>   drm/i915: Simplify error handling in intel_modeset_all_pipes()

Entire series pushed to dinq. Thanks for the reviews.

> 
>  drivers/gpu/drm/i915/i915_drv.h      |  9 +++++-
>  drivers/gpu/drm/i915/intel_display.c | 60 ++++++++++++++++++++++++++++--------
>  2 files changed, 55 insertions(+), 14 deletions(-)
> 
> -- 
> 2.7.4

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx