[PATCH 0/4] cdclk fixes

[PATCH 0/4] cdclk fixes

[Maarten Lankhorst]

The first patch fixes a WARN_ON that happens when running kms_flip.
Second patch fixes a FIXME. Now that the code's atomic the cdclk is
calculated in compute_config so -EINVAL can be returned.

The third patch introduces active_crtcs which is useful to have in general,
and also calculates the state without locking other crtc's. This will allow
modesets on newer platforms without locking all planes and crtc's.

The final patch is a small removal of a FIXME for broxton. Hopefully useful
to have.

Maarten Lankhorst (4):
  drm/i915/skl: Do not allow scaling when crtc is disabled.
  drm/i915: Handle cdclk limits on broadwell.
  drm/i915: Do not acquire crtc state to check clock during modeset, v3.
  drm/i915/bxt: Use the bypass frequency if there are no active pipes.

 drivers/gpu/drm/i915/i915_drv.h      |   5 ++
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 156 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |   7 +-
 4 files changed, 119 insertions(+), 51 deletions(-)

-- 
2.1.0

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

[PATCH 1/4] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

This fixes a warning when the crtc is turned off. In that case fb
will be NULL, and crtc_clock will be 0. Because the crtc is no longer
active this is not a bug, and shouldn't trigger the WARN_ON.

Also remove handling a null crtc_state, with all transitional helpers
gone this can no longer happen.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 764eeb05492d..351ecb69e5eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13693,7 +13693,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 	struct drm_i915_private *dev_priv;
 	int crtc_clock, cdclk;
 
-	if (!intel_crtc || !crtc_state)
+	if (!intel_crtc || !crtc_state->base.enable)
 		return DRM_PLANE_HELPER_NO_SCALING;
 
 	dev = intel_crtc->base.dev;
-- 
2.1.0

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

[PATCH 2/4] drm/i915: Handle cdclk limits on broadwell.

[Maarten Lankhorst]

As the comment indicates this can only fail gracefully when
called from compute_config. Fortunately this is now what's happening,
so the fixme can be removed and the DRM_ERROR downgraded.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 351ecb69e5eb..3c46037b6e55 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9677,14 +9677,10 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
 	else
 		cdclk = 337500;
 
-	/*
-	 * FIXME move the cdclk caclulation to
-	 * compute_config() so we can fail gracegully.
-	 */
 	if (cdclk > dev_priv->max_cdclk_freq) {
-		DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
-			  cdclk, dev_priv->max_cdclk_freq);
-		cdclk = dev_priv->max_cdclk_freq;
+		DRM_INFO("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
+			 cdclk, dev_priv->max_cdclk_freq);
+		return -EINVAL;
 	}
 
 	to_intel_atomic_state(state)->cdclk = cdclk;
-- 
2.1.0

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

[PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.

[Maarten Lankhorst]

Parallel modesets are still not allowed, but this will allow updating
a different crtc during a modeset if the clock is not changed.

Additionally when all pipes are DPMS off the cdclk will be lowered
to the minimum allowed.

Changes since v1:
- Add dev_priv->active_crtcs for tracking which crtcs are active.
- Rename min_cdclk to min_pixclk and move to dev_priv.
- Add a active_crtcs mask which is updated atomically.
- Add intel_atomic_state->modeset which is set on modesets.
- Commit new pixclk/active_crtcs right after state swap.
Changes since v2:
- Make the changes related to max_pixel_rate calculations more readable.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   5 ++
 drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
 drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
 drivers/gpu/drm/i915/intel_drv.h     |   7 +-
 4 files changed, 112 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a32571f0d018..c7b76c2e8e09 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1821,8 +1821,13 @@ struct drm_i915_private {
 	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
 #endif
 
+	/* dpll and cdclk state is protected by connection_mutex */
 	int num_shared_dpll;
 	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
+
+	unsigned int active_crtcs;
+	unsigned int min_pixclk[I915_MAX_PIPES];
+
 	int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
 
 	struct i915_workarounds workarounds;
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 643f342de33b..c4eadbc928b7 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(s);
 	drm_atomic_state_default_clear(&state->base);
-	state->dpll_set = false;
+	state->dpll_set = state->modeset = false;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c46037b6e55..178a042f917e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
 static int intel_mode_max_pixclk(struct drm_device *dev,
 				 struct drm_atomic_state *state)
 {
-	struct intel_crtc *intel_crtc;
-	struct intel_crtc_state *crtc_state;
-	int max_pixclk = 0;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	unsigned max_pixclk = 0, i;
+	enum pipe pipe;
 
-	for_each_intel_crtc(dev, intel_crtc) {
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
+	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
+	       sizeof(intel_state->min_pixclk));
 
-		if (!crtc_state->base.enable)
-			continue;
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		int pixclk = 0;
+
+		if (crtc_state->enable)
+			pixclk = crtc_state->adjusted_mode.crtc_clock;
 
-		max_pixclk = max(max_pixclk,
-				 crtc_state->base.adjusted_mode.crtc_clock);
+		intel_state->min_pixclk[i] = pixclk;
 	}
 
+	if (!intel_state->active_crtcs)
+		return 0;
+
+	for_each_pipe(dev_priv, pipe)
+		max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk);
+
 	return max_pixclk;
 }
 
@@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
 	for_each_power_domain(domain, domains)
 		intel_display_power_put(dev_priv, domain);
 	intel_crtc->enabled_power_domains = 0;
+
+	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
+	dev_priv->min_pixclk[intel_crtc->pipe] = 0;
 }
 
 /*
@@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
 /* compute the max rate for new configuration */
 static int ilk_max_pixel_rate(struct drm_atomic_state *state)
 {
-	struct intel_crtc *intel_crtc;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = state->dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *cstate;
 	struct intel_crtc_state *crtc_state;
-	int max_pixel_rate = 0;
+	unsigned max_pixel_rate = 0, i;
+	enum pipe pipe;
 
-	for_each_intel_crtc(state->dev, intel_crtc) {
-		int pixel_rate;
+	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
+	       sizeof(intel_state->min_pixclk));
 
-		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
-		if (IS_ERR(crtc_state))
-			return PTR_ERR(crtc_state);
+	for_each_crtc_in_state(state, crtc, cstate, i) {
+		int pixel_rate;
 
-		if (!crtc_state->base.enable)
+		crtc_state = to_intel_crtc_state(cstate);
+		if (!crtc_state->base.enable) {
+			intel_state->min_pixclk[i] = 0;
 			continue;
+		}
 
 		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
 
 		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
-		if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
+		if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
 			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
 
-		max_pixel_rate = max(max_pixel_rate, pixel_rate);
+		intel_state->min_pixclk[i] = pixel_rate;
 	}
 
+	if (!intel_state->active_crtcs)
+		return 0;
+
+	for_each_pipe(dev_priv, pipe)
+		max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate);
+
 	return max_pixel_rate;
 }
 
@@ -13069,15 +13093,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
 
 static int intel_modeset_checks(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = state->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	int ret;
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+	struct drm_i915_private *dev_priv = state->dev->dev_private;
+	struct drm_crtc *crtc;
+	struct drm_crtc_state *crtc_state;
+	int ret = 0, i;
 
 	if (!check_digital_port_conflicts(state)) {
 		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
 		return -EINVAL;
 	}
 
+	intel_state->modeset = true;
+	intel_state->active_crtcs = dev_priv->active_crtcs;
+
+	for_each_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->active)
+			intel_state->active_crtcs |= 1 << i;
+		else
+			intel_state->active_crtcs &= ~(1 << i);
+	}
+
 	/*
 	 * See if the config requires any additional preparation, e.g.
 	 * to adjust global state with pipes off.  We need to do this
@@ -13101,7 +13137,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
 
 	intel_modeset_clear_plls(state);
 
-	if (IS_HASWELL(dev))
+	if (IS_HASWELL(dev_priv))
 		return haswell_mode_set_planes_workaround(state);
 
 	return 0;
@@ -13318,12 +13354,12 @@ static int intel_atomic_commit(struct drm_device *dev,
 			       struct drm_atomic_state *state,
 			       bool async)
 {
+	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
-	int ret = 0;
-	int i;
-	bool any_ms = false;
+	int ret = 0, i;
+	bool hw_check = intel_state->modeset;
 
 	ret = intel_atomic_prepare_commit(dev, state, async);
 	if (ret) {
@@ -13334,13 +13370,18 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_swap_state(dev, state);
 	dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
 
+	if (intel_state->modeset) {
+		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
+		       sizeof(intel_state->min_pixclk));
+		dev_priv->active_crtcs = intel_state->active_crtcs;
+	}
+
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
 		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 
 		if (!needs_modeset(crtc->state))
 			continue;
 
-		any_ms = true;
 		intel_pre_plane_update(intel_crtc);
 
 		if (crtc_state->active) {
@@ -13355,7 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	 * update the the output configuration. */
 	intel_modeset_update_crtc_state(state);
 
-	if (any_ms) {
+	if (intel_state->modeset) {
 		intel_shared_dpll_commit(state);
 
 		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
@@ -13382,7 +13423,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 			put_domains = modeset_get_crtc_power_domains(crtc);
 
 			/* make sure intel_modeset_check_state runs */
-			any_ms = true;
+			hw_check = true;
 		}
 
 		if (!modeset)
@@ -13409,7 +13450,7 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_helper_cleanup_planes(dev, state);
 	mutex_unlock(&dev->struct_mutex);
 
-	if (any_ms)
+	if (hw_check)
 		intel_modeset_check_state(dev, state);
 
 	drm_atomic_state_free(state);
@@ -15417,16 +15458,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	struct intel_connector *connector;
 	int i;
 
+	dev_priv->active_crtcs = 0;
+
 	for_each_intel_crtc(dev, crtc) {
-		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
-		memset(crtc->config, 0, sizeof(*crtc->config));
-		crtc->config->base.crtc = &crtc->base;
+		struct intel_crtc_state *crtc_state = crtc->config;
+		int pixclk = 0;
 
-		crtc->active = dev_priv->display.get_pipe_config(crtc,
-								 crtc->config);
+		__drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base);
+		memset(crtc_state, 0, sizeof(*crtc_state));
+		crtc_state->base.crtc = &crtc->base;
 
-		crtc->base.state->active = crtc->active;
-		crtc->base.enabled = crtc->active;
+		crtc_state->base.active = crtc_state->base.enable =
+			dev_priv->display.get_pipe_config(crtc, crtc_state);
+
+		crtc->base.enabled = crtc_state->base.enable;
+		crtc->active = crtc_state->base.active;
+
+		if (crtc_state->base.active) {
+			dev_priv->active_crtcs |= 1 << crtc->pipe;
+
+			if (IS_BROADWELL(dev_priv)) {
+				pixclk = ilk_pipe_pixel_rate(crtc_state);
+
+				/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
+				if (crtc_state->ips_enabled)
+					pixclk = DIV_ROUND_UP(pixclk * 100, 95);
+			} else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv))
+				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
+		}
+
+		dev_priv->min_pixclk[crtc->pipe] = pixclk;
 
 		readout_plane_state(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4dfca58d37bf..8cfc599fd413 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -246,7 +246,12 @@ struct intel_atomic_state {
 	struct drm_atomic_state base;
 
 	unsigned int cdclk;
-	bool dpll_set;
+
+	bool dpll_set, modeset;
+
+	unsigned int active_crtcs;
+	unsigned int min_pixclk[I915_MAX_PIPES];
+
 	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
 	struct intel_wm_config wm_config;
 };
-- 
2.1.0

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

[PATCH 4/4] drm/i915/bxt: Use the bypass frequency if there are no active pipes.

[Maarten Lankhorst]

Now that pixel clock is set to 0 when there are no active pipes it's
easy to set the bypass frequency for this case.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 178a042f917e..8a825f24ce51 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5972,7 +5972,6 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
 	/*
 	 * FIXME:
 	 * - remove the guardband, it's not needed on BXT
-	 * - set 19.2MHz bypass frequency if there are no active pipes
 	 */
 	if (max_pixclk > 576000*9/10)
 		return 624000;
@@ -5982,8 +5981,10 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
 		return 384000;
 	else if (max_pixclk > 144000*9/10)
 		return 288000;
-	else
+	else if (max_pixclk)
 		return 144000;
+	else
+		return 19200;
 }
 
 /* Compute the max pixel clock for new configuration. Uses atomic state if
-- 
2.1.0

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

Re: [PATCH 2/4] drm/i915: Handle cdclk limits on broadwell.

[Chris Wilson]

On Tue, Nov 24, 2015 at 11:29:03AM +0100, Maarten Lankhorst wrote:
> As the comment indicates this can only fail gracefully when
> called from compute_config. Fortunately this is now what's happening,
> so the fixme can be removed and the DRM_ERROR downgraded.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 351ecb69e5eb..3c46037b6e55 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9677,14 +9677,10 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
>  	else
>  		cdclk = 337500;
>  
> -	/*
> -	 * FIXME move the cdclk caclulation to
> -	 * compute_config() so we can fail gracegully.
> -	 */
>  	if (cdclk > dev_priv->max_cdclk_freq) {
> -		DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> -			  cdclk, dev_priv->max_cdclk_freq);
> -		cdclk = dev_priv->max_cdclk_freq;
> +		DRM_INFO("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> +			 cdclk, dev_priv->max_cdclk_freq);

This is a validation step, right? So the invalid value is never applied
to hardware and so doesn't really exist for anybody but the calling
userspace. Ergo, this shouldn't be logged for the user but only when
debugging the application -> DRM_DEBUG_KMS

Dreaming of an application-centric debug log,
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Mika Kahola]

On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote:
> This fixes a warning when the crtc is turned off. In that case fb
> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> active this is not a bug, and shouldn't trigger the WARN_ON.
> 
> Also remove handling a null crtc_state, with all transitional helpers
> gone this can no longer happen.
> 
Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 764eeb05492d..351ecb69e5eb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13693,7 +13693,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
>  	struct drm_i915_private *dev_priv;
>  	int crtc_clock, cdclk;
>  
> -	if (!intel_crtc || !crtc_state)
> +	if (!intel_crtc || !crtc_state->base.enable)
>  		return DRM_PLANE_HELPER_NO_SCALING;
>  
>  	dev = intel_crtc->base.dev;


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

Re: [PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.

[Mika Kahola]

On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote:
> Parallel modesets are still not allowed, but this will allow updating
> a different crtc during a modeset if the clock is not changed.
> 
> Additionally when all pipes are DPMS off the cdclk will be lowered
> to the minimum allowed.
> 
> Changes since v1:
> - Add dev_priv->active_crtcs for tracking which crtcs are active.
> - Rename min_cdclk to min_pixclk and move to dev_priv.
> - Add a active_crtcs mask which is updated atomically.
> - Add intel_atomic_state->modeset which is set on modesets.
> - Commit new pixclk/active_crtcs right after state swap.
> Changes since v2:
> - Make the changes related to max_pixel_rate calculations more readable.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   5 ++
>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>  drivers/gpu/drm/i915/intel_drv.h     |   7 +-
>  4 files changed, 112 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a32571f0d018..c7b76c2e8e09 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1821,8 +1821,13 @@ struct drm_i915_private {
>  	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
>  #endif
>  
> +	/* dpll and cdclk state is protected by connection_mutex */
>  	int num_shared_dpll;
>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
> +
> +	unsigned int active_crtcs;
> +	unsigned int min_pixclk[I915_MAX_PIPES];
> +
>  	int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
>  
>  	struct i915_workarounds workarounds;
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 643f342de33b..c4eadbc928b7 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
>  {
>  	struct intel_atomic_state *state = to_intel_atomic_state(s);
>  	drm_atomic_state_default_clear(&state->base);
> -	state->dpll_set = false;
> +	state->dpll_set = state->modeset = false;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c46037b6e55..178a042f917e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
>  static int intel_mode_max_pixclk(struct drm_device *dev,
>  				 struct drm_atomic_state *state)
>  {
> -	struct intel_crtc *intel_crtc;
> -	struct intel_crtc_state *crtc_state;
> -	int max_pixclk = 0;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
This is a nitpick but we should use nowadays to_i915() 
 
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	unsigned max_pixclk = 0, i;
> +	enum pipe pipe;
>  
> -	for_each_intel_crtc(dev, intel_crtc) {
> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> -		if (IS_ERR(crtc_state))
> -			return PTR_ERR(crtc_state);
> +	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> +	       sizeof(intel_state->min_pixclk));
>  
> -		if (!crtc_state->base.enable)
> -			continue;
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		int pixclk = 0;
> +
> +		if (crtc_state->enable)
> +			pixclk = crtc_state->adjusted_mode.crtc_clock;
>  
> -		max_pixclk = max(max_pixclk,
> -				 crtc_state->base.adjusted_mode.crtc_clock);
> +		intel_state->min_pixclk[i] = pixclk;
>  	}
>  
> +	if (!intel_state->active_crtcs)
> +		return 0;
> +
> +	for_each_pipe(dev_priv, pipe)
> +		max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk);
> +
>  	return max_pixclk;
>  }
>  
> @@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>  	for_each_power_domain(domain, domains)
>  		intel_display_power_put(dev_priv, domain);
>  	intel_crtc->enabled_power_domains = 0;
> +
> +	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> +	dev_priv->min_pixclk[intel_crtc->pipe] = 0;
>  }
>  
>  /*
> @@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>  /* compute the max rate for new configuration */
>  static int ilk_max_pixel_rate(struct drm_atomic_state *state)
>  {
> -	struct intel_crtc *intel_crtc;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_i915_private *dev_priv = state->dev->dev_private;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *cstate;
>  	struct intel_crtc_state *crtc_state;
> -	int max_pixel_rate = 0;
> +	unsigned max_pixel_rate = 0, i;
> +	enum pipe pipe;
>  
> -	for_each_intel_crtc(state->dev, intel_crtc) {
> -		int pixel_rate;
> +	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> +	       sizeof(intel_state->min_pixclk));
>  
> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> -		if (IS_ERR(crtc_state))
> -			return PTR_ERR(crtc_state);
> +	for_each_crtc_in_state(state, crtc, cstate, i) {
> +		int pixel_rate;
>  
> -		if (!crtc_state->base.enable)
> +		crtc_state = to_intel_crtc_state(cstate);
> +		if (!crtc_state->base.enable) {
> +			intel_state->min_pixclk[i] = 0;
>  			continue;
> +		}
>  
>  		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
>  
>  		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> -		if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
> +		if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
>  			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>  
> -		max_pixel_rate = max(max_pixel_rate, pixel_rate);
> +		intel_state->min_pixclk[i] = pixel_rate;
>  	}
>  
> +	if (!intel_state->active_crtcs)
> +		return 0;
> +
> +	for_each_pipe(dev_priv, pipe)
> +		max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate);
> +
>  	return max_pixel_rate;
>  }
>  
> @@ -13069,15 +13093,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
>  
>  static int intel_modeset_checks(struct drm_atomic_state *state)
>  {
> -	struct drm_device *dev = state->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	int ret;
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> +	struct drm_i915_private *dev_priv = state->dev->dev_private;
> +	struct drm_crtc *crtc;
> +	struct drm_crtc_state *crtc_state;
> +	int ret = 0, i;
>  
>  	if (!check_digital_port_conflicts(state)) {
>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>  		return -EINVAL;
>  	}
>  
> +	intel_state->modeset = true;
> +	intel_state->active_crtcs = dev_priv->active_crtcs;
> +
> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->active)
> +			intel_state->active_crtcs |= 1 << i;
> +		else
> +			intel_state->active_crtcs &= ~(1 << i);
> +	}
> +
>  	/*
>  	 * See if the config requires any additional preparation, e.g.
>  	 * to adjust global state with pipes off.  We need to do this
> @@ -13101,7 +13137,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>  
>  	intel_modeset_clear_plls(state);
>  
> -	if (IS_HASWELL(dev))
> +	if (IS_HASWELL(dev_priv))
>  		return haswell_mode_set_planes_workaround(state);
>  
>  	return 0;
> @@ -13318,12 +13354,12 @@ static int intel_atomic_commit(struct drm_device *dev,
>  			       struct drm_atomic_state *state,
>  			       bool async)
>  {
> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc_state *crtc_state;
>  	struct drm_crtc *crtc;
> -	int ret = 0;
> -	int i;
> -	bool any_ms = false;
> +	int ret = 0, i;
> +	bool hw_check = intel_state->modeset;
>  
>  	ret = intel_atomic_prepare_commit(dev, state, async);
>  	if (ret) {
> @@ -13334,13 +13370,18 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	drm_atomic_helper_swap_state(dev, state);
>  	dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
>  
> +	if (intel_state->modeset) {
> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> +		       sizeof(intel_state->min_pixclk));
> +		dev_priv->active_crtcs = intel_state->active_crtcs;
> +	}
> +
>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>  
>  		if (!needs_modeset(crtc->state))
>  			continue;
>  
> -		any_ms = true;
>  		intel_pre_plane_update(intel_crtc);
>  
>  		if (crtc_state->active) {
> @@ -13355,7 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	 * update the the output configuration. */
>  	intel_modeset_update_crtc_state(state);
>  
> -	if (any_ms) {
> +	if (intel_state->modeset) {
>  		intel_shared_dpll_commit(state);
>  
>  		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
> @@ -13382,7 +13423,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  			put_domains = modeset_get_crtc_power_domains(crtc);
>  
>  			/* make sure intel_modeset_check_state runs */
> -			any_ms = true;
> +			hw_check = true;
>  		}
>  
>  		if (!modeset)
> @@ -13409,7 +13450,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	drm_atomic_helper_cleanup_planes(dev, state);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	if (any_ms)
> +	if (hw_check)
>  		intel_modeset_check_state(dev, state);
>  
>  	drm_atomic_state_free(state);
> @@ -15417,16 +15458,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  	struct intel_connector *connector;
>  	int i;
>  
> +	dev_priv->active_crtcs = 0;
> +
>  	for_each_intel_crtc(dev, crtc) {
> -		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
> -		memset(crtc->config, 0, sizeof(*crtc->config));
> -		crtc->config->base.crtc = &crtc->base;
> +		struct intel_crtc_state *crtc_state = crtc->config;
> +		int pixclk = 0;
>  
> -		crtc->active = dev_priv->display.get_pipe_config(crtc,
> -								 crtc->config);
> +		__drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base);
> +		memset(crtc_state, 0, sizeof(*crtc_state));
> +		crtc_state->base.crtc = &crtc->base;
>  
> -		crtc->base.state->active = crtc->active;
> -		crtc->base.enabled = crtc->active;
> +		crtc_state->base.active = crtc_state->base.enable =
> +			dev_priv->display.get_pipe_config(crtc, crtc_state);
> +
> +		crtc->base.enabled = crtc_state->base.enable;
> +		crtc->active = crtc_state->base.active;
> +
> +		if (crtc_state->base.active) {
> +			dev_priv->active_crtcs |= 1 << crtc->pipe;
> +
> +			if (IS_BROADWELL(dev_priv)) {
> +				pixclk = ilk_pipe_pixel_rate(crtc_state);
> +
> +				/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> +				if (crtc_state->ips_enabled)
> +					pixclk = DIV_ROUND_UP(pixclk * 100, 95);
> +			} else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv))
> +				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
What happens with the other platforms?

> +		}
> +
> +		dev_priv->min_pixclk[crtc->pipe] = pixclk;
>  
>  		readout_plane_state(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4dfca58d37bf..8cfc599fd413 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -246,7 +246,12 @@ struct intel_atomic_state {
>  	struct drm_atomic_state base;
>  
>  	unsigned int cdclk;
> -	bool dpll_set;
> +
> +	bool dpll_set, modeset;
> +
> +	unsigned int active_crtcs;
> +	unsigned int min_pixclk[I915_MAX_PIPES];
> +
>  	struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>  	struct intel_wm_config wm_config;
>  };

-- 
Mika Kahola - Intel OTC

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

Re: [PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.

[Chris Wilson]

On Tue, Dec 15, 2015 at 12:22:40PM +0200, Mika Kahola wrote:
> On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 3c46037b6e55..178a042f917e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> >  static int intel_mode_max_pixclk(struct drm_device *dev,
> >  				 struct drm_atomic_state *state)
> >  {
> > -	struct intel_crtc *intel_crtc;
> > -	struct intel_crtc_state *crtc_state;
> > -	int max_pixclk = 0;
> > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> This is a nitpick but we should use nowadays to_i915() 

If you're going to bring that up, we should be passing in the right
pointer to begin with!
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.

[Maarten Lankhorst]

Op 15-12-15 om 11:22 schreef Mika Kahola:
> On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote:
>> Parallel modesets are still not allowed, but this will allow updating
>> a different crtc during a modeset if the clock is not changed.
>>
>> Additionally when all pipes are DPMS off the cdclk will be lowered
>> to the minimum allowed.
>>
>> Changes since v1:
>> - Add dev_priv->active_crtcs for tracking which crtcs are active.
>> - Rename min_cdclk to min_pixclk and move to dev_priv.
>> - Add a active_crtcs mask which is updated atomically.
>> - Add intel_atomic_state->modeset which is set on modesets.
>> - Commit new pixclk/active_crtcs right after state swap.
>> Changes since v2:
>> - Make the changes related to max_pixel_rate calculations more readable.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |   5 ++
>>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>>  drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_drv.h     |   7 +-
>>  4 files changed, 112 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index a32571f0d018..c7b76c2e8e09 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -1821,8 +1821,13 @@ struct drm_i915_private {
>>  	struct intel_pipe_crc pipe_crc[I915_MAX_PIPES];
>>  #endif
>>  
>> +	/* dpll and cdclk state is protected by connection_mutex */
>>  	int num_shared_dpll;
>>  	struct intel_shared_dpll shared_dplls[I915_NUM_PLLS];
>> +
>> +	unsigned int active_crtcs;
>> +	unsigned int min_pixclk[I915_MAX_PIPES];
>> +
>>  	int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
>>  
>>  	struct i915_workarounds workarounds;
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
>> index 643f342de33b..c4eadbc928b7 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s)
>>  {
>>  	struct intel_atomic_state *state = to_intel_atomic_state(s);
>>  	drm_atomic_state_default_clear(&state->base);
>> -	state->dpll_set = false;
>> +	state->dpll_set = state->modeset = false;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 3c46037b6e55..178a042f917e 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
>>  static int intel_mode_max_pixclk(struct drm_device *dev,
>>  				 struct drm_atomic_state *state)
>>  {
>> -	struct intel_crtc *intel_crtc;
>> -	struct intel_crtc_state *crtc_state;
>> -	int max_pixclk = 0;
>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
> This is a nitpick but we should use nowadays to_i915() 
>  
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +	unsigned max_pixclk = 0, i;
>> +	enum pipe pipe;
>>  
>> -	for_each_intel_crtc(dev, intel_crtc) {
>> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> +	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
>> +	       sizeof(intel_state->min_pixclk));
>>  
>> -		if (!crtc_state->base.enable)
>> -			continue;
>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		int pixclk = 0;
>> +
>> +		if (crtc_state->enable)
>> +			pixclk = crtc_state->adjusted_mode.crtc_clock;
>>  
>> -		max_pixclk = max(max_pixclk,
>> -				 crtc_state->base.adjusted_mode.crtc_clock);
>> +		intel_state->min_pixclk[i] = pixclk;
>>  	}
>>  
>> +	if (!intel_state->active_crtcs)
>> +		return 0;
>> +
>> +	for_each_pipe(dev_priv, pipe)
>> +		max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk);
>> +
>>  	return max_pixclk;
>>  }
>>  
>> @@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc)
>>  	for_each_power_domain(domain, domains)
>>  		intel_display_power_put(dev_priv, domain);
>>  	intel_crtc->enabled_power_domains = 0;
>> +
>> +	dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
>> +	dev_priv->min_pixclk[intel_crtc->pipe] = 0;
>>  }
>>  
>>  /*
>> @@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
>>  /* compute the max rate for new configuration */
>>  static int ilk_max_pixel_rate(struct drm_atomic_state *state)
>>  {
>> -	struct intel_crtc *intel_crtc;
>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> +	struct drm_i915_private *dev_priv = state->dev->dev_private;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *cstate;
>>  	struct intel_crtc_state *crtc_state;
>> -	int max_pixel_rate = 0;
>> +	unsigned max_pixel_rate = 0, i;
>> +	enum pipe pipe;
>>  
>> -	for_each_intel_crtc(state->dev, intel_crtc) {
>> -		int pixel_rate;
>> +	memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
>> +	       sizeof(intel_state->min_pixclk));
>>  
>> -		crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
>> -		if (IS_ERR(crtc_state))
>> -			return PTR_ERR(crtc_state);
>> +	for_each_crtc_in_state(state, crtc, cstate, i) {
>> +		int pixel_rate;
>>  
>> -		if (!crtc_state->base.enable)
>> +		crtc_state = to_intel_crtc_state(cstate);
>> +		if (!crtc_state->base.enable) {
>> +			intel_state->min_pixclk[i] = 0;
>>  			continue;
>> +		}
>>  
>>  		pixel_rate = ilk_pipe_pixel_rate(crtc_state);
>>  
>>  		/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> -		if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
>> +		if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
>>  			pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
>>  
>> -		max_pixel_rate = max(max_pixel_rate, pixel_rate);
>> +		intel_state->min_pixclk[i] = pixel_rate;
>>  	}
>>  
>> +	if (!intel_state->active_crtcs)
>> +		return 0;
>> +
>> +	for_each_pipe(dev_priv, pipe)
>> +		max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate);
>> +
>>  	return max_pixel_rate;
>>  }
>>  
>> @@ -13069,15 +13093,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state)
>>  
>>  static int intel_modeset_checks(struct drm_atomic_state *state)
>>  {
>> -	struct drm_device *dev = state->dev;
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>> -	int ret;
>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>> +	struct drm_i915_private *dev_priv = state->dev->dev_private;
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +	int ret = 0, i;
>>  
>>  	if (!check_digital_port_conflicts(state)) {
>>  		DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n");
>>  		return -EINVAL;
>>  	}
>>  
>> +	intel_state->modeset = true;
>> +	intel_state->active_crtcs = dev_priv->active_crtcs;
>> +
>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		if (crtc_state->active)
>> +			intel_state->active_crtcs |= 1 << i;
>> +		else
>> +			intel_state->active_crtcs &= ~(1 << i);
>> +	}
>> +
>>  	/*
>>  	 * See if the config requires any additional preparation, e.g.
>>  	 * to adjust global state with pipes off.  We need to do this
>> @@ -13101,7 +13137,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>>  
>>  	intel_modeset_clear_plls(state);
>>  
>> -	if (IS_HASWELL(dev))
>> +	if (IS_HASWELL(dev_priv))
>>  		return haswell_mode_set_planes_workaround(state);
>>  
>>  	return 0;
>> @@ -13318,12 +13354,12 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  			       struct drm_atomic_state *state,
>>  			       bool async)
>>  {
>> +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_crtc_state *crtc_state;
>>  	struct drm_crtc *crtc;
>> -	int ret = 0;
>> -	int i;
>> -	bool any_ms = false;
>> +	int ret = 0, i;
>> +	bool hw_check = intel_state->modeset;
>>  
>>  	ret = intel_atomic_prepare_commit(dev, state, async);
>>  	if (ret) {
>> @@ -13334,13 +13370,18 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  	drm_atomic_helper_swap_state(dev, state);
>>  	dev_priv->wm.config = to_intel_atomic_state(state)->wm_config;
>>  
>> +	if (intel_state->modeset) {
>> +		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
>> +		       sizeof(intel_state->min_pixclk));
>> +		dev_priv->active_crtcs = intel_state->active_crtcs;
>> +	}
>> +
>>  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>  		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  
>>  		if (!needs_modeset(crtc->state))
>>  			continue;
>>  
>> -		any_ms = true;
>>  		intel_pre_plane_update(intel_crtc);
>>  
>>  		if (crtc_state->active) {
>> @@ -13355,7 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  	 * update the the output configuration. */
>>  	intel_modeset_update_crtc_state(state);
>>  
>> -	if (any_ms) {
>> +	if (intel_state->modeset) {
>>  		intel_shared_dpll_commit(state);
>>  
>>  		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>> @@ -13382,7 +13423,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  			put_domains = modeset_get_crtc_power_domains(crtc);
>>  
>>  			/* make sure intel_modeset_check_state runs */
>> -			any_ms = true;
>> +			hw_check = true;
>>  		}
>>  
>>  		if (!modeset)
>> @@ -13409,7 +13450,7 @@ static int intel_atomic_commit(struct drm_device *dev,
>>  	drm_atomic_helper_cleanup_planes(dev, state);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>> -	if (any_ms)
>> +	if (hw_check)
>>  		intel_modeset_check_state(dev, state);
>>  
>>  	drm_atomic_state_free(state);
>> @@ -15417,16 +15458,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>>  	struct intel_connector *connector;
>>  	int i;
>>  
>> +	dev_priv->active_crtcs = 0;
>> +
>>  	for_each_intel_crtc(dev, crtc) {
>> -		__drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state);
>> -		memset(crtc->config, 0, sizeof(*crtc->config));
>> -		crtc->config->base.crtc = &crtc->base;
>> +		struct intel_crtc_state *crtc_state = crtc->config;
>> +		int pixclk = 0;
>>  
>> -		crtc->active = dev_priv->display.get_pipe_config(crtc,
>> -								 crtc->config);
>> +		__drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base);
>> +		memset(crtc_state, 0, sizeof(*crtc_state));
>> +		crtc_state->base.crtc = &crtc->base;
>>  
>> -		crtc->base.state->active = crtc->active;
>> -		crtc->base.enabled = crtc->active;
>> +		crtc_state->base.active = crtc_state->base.enable =
>> +			dev_priv->display.get_pipe_config(crtc, crtc_state);
>> +
>> +		crtc->base.enabled = crtc_state->base.enable;
>> +		crtc->active = crtc_state->base.active;
>> +
>> +		if (crtc_state->base.active) {
>> +			dev_priv->active_crtcs |= 1 << crtc->pipe;
>> +
>> +			if (IS_BROADWELL(dev_priv)) {
>> +				pixclk = ilk_pipe_pixel_rate(crtc_state);
>> +
>> +				/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
>> +				if (crtc_state->ips_enabled)
>> +					pixclk = DIV_ROUND_UP(pixclk * 100, 95);
>> +			} else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv))
>> +				pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> What happens with the other platforms?
>
Well the other platforms have no support for cdclk adjustment, but it seems I would need to add a IS_CHERRYVIEW() on that if branch,
and perhaps a else WARN_ON(dev_priv->display.modeset_calc_cdclk); as well to make sure any future platform is covered.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.

[Mika Kahola]

On Tue, 2015-12-15 at 10:26 +0000, Chris Wilson wrote:
> On Tue, Dec 15, 2015 at 12:22:40PM +0200, Mika Kahola wrote:
> > On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 3c46037b6e55..178a042f917e 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> > >  static int intel_mode_max_pixclk(struct drm_device *dev,
> > >  				 struct drm_atomic_state *state)
> > >  {
> > > -	struct intel_crtc *intel_crtc;
> > > -	struct intel_crtc_state *crtc_state;
> > > -	int max_pixclk = 0;
> > > +	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > This is a nitpick but we should use nowadays to_i915() 
> 
> If you're going to bring that up, we should be passing in the right
> pointer to begin with!
> -Chris
This was more meant be as a notification. I was advised that we should
use 'to_i915()' in a new code. Personally, I don't have any preference one
way or another. Both ways will work. Maybe this would be a topic for a follow
up patch?

Cheers,
Mika


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