* [PATCH v2 2/3] drm/i915: do not update cursor in crtc mode set
2013-09-04 11:20 [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set Jani Nikula
@ 2013-09-04 11:20 ` Jani Nikula
2013-09-04 18:32 ` Ville Syrjälä
2013-09-04 11:20 ` [PATCH v2 3/3] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula
2013-09-04 12:11 ` [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set Ville Syrjälä
2 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2013-09-04 11:20 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
The cursor is disabled before crtc mode set in crtc disable (and we
assert this is the case), and enabled afterwards in crtc enable. Do not
update it in crtc mode set.
Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 89243cc..4939683 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4910,9 +4910,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
}
}
- /* Ensure that the cursor is valid for the new mode before changing... */
- intel_crtc_update_cursor(crtc, true);
-
if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
/*
* Ensure we match the reduced clock's P to the target clock.
@@ -5806,9 +5803,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
intel_crtc->config.dpll.p2 = clock.p2;
}
- /* Ensure that the cursor is valid for the new mode before changing... */
- intel_crtc_update_cursor(crtc, true);
-
/* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
if (intel_crtc->config.has_pch_encoder) {
fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
@@ -6299,9 +6293,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
if (!intel_ddi_pll_mode_set(crtc))
return -EINVAL;
- /* Ensure that the cursor is valid for the new mode before changing... */
- intel_crtc_update_cursor(crtc, true);
-
if (intel_crtc->config.has_dp_encoder)
intel_dp_set_m_n(intel_crtc);
--
1.7.9.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 2/3] drm/i915: do not update cursor in crtc mode set
2013-09-04 11:20 ` [PATCH v2 2/3] drm/i915: do not update cursor " Jani Nikula
@ 2013-09-04 18:32 ` Ville Syrjälä
0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2013-09-04 18:32 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Wed, Sep 04, 2013 at 02:20:09PM +0300, Jani Nikula wrote:
> The cursor is disabled before crtc mode set in crtc disable (and we
> assert this is the case), and enabled afterwards in crtc enable. Do not
> update it in crtc mode set.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 89243cc..4939683 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4910,9 +4910,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> }
> }
>
> - /* Ensure that the cursor is valid for the new mode before changing... */
> - intel_crtc_update_cursor(crtc, true);
> -
> if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
> /*
> * Ensure we match the reduced clock's P to the target clock.
> @@ -5806,9 +5803,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> intel_crtc->config.dpll.p2 = clock.p2;
> }
>
> - /* Ensure that the cursor is valid for the new mode before changing... */
> - intel_crtc_update_cursor(crtc, true);
> -
> /* CPU eDP is the only output that doesn't need a PCH PLL of its own. */
> if (intel_crtc->config.has_pch_encoder) {
> fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
> @@ -6299,9 +6293,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> if (!intel_ddi_pll_mode_set(crtc))
> return -EINVAL;
>
> - /* Ensure that the cursor is valid for the new mode before changing... */
> - intel_crtc_update_cursor(crtc, true);
> -
> if (intel_crtc->config.has_dp_encoder)
> intel_dp_set_m_n(intel_crtc);
>
> --
> 1.7.9.5
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 3/3] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling
2013-09-04 11:20 [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set Jani Nikula
2013-09-04 11:20 ` [PATCH v2 2/3] drm/i915: do not update cursor " Jani Nikula
@ 2013-09-04 11:20 ` Jani Nikula
2013-09-05 11:04 ` Ville Syrjälä
2013-09-04 12:11 ` [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set Ville Syrjälä
2 siblings, 1 reply; 6+ messages in thread
From: Jani Nikula @ 2013-09-04 11:20 UTC (permalink / raw)
To: intel-gfx; +Cc: jani.nikula
Flat out skip anything to do with PLL if we have a DSI encoder (and thus
DSI PLL). Also skip PLL computation if the encoder has already set
clocks. This allows for some tidying up of the code, including a
superfluous call to intel_limit() for LVDS downclock path.
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++-----------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4939683..0646d14 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4891,9 +4891,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
num_connectors++;
}
- refclk = i9xx_get_refclk(crtc, num_connectors);
+ if (is_dsi)
+ goto skip_dpll;
+
+ if (!intel_crtc->config.clock_set) {
+ refclk = i9xx_get_refclk(crtc, num_connectors);
- if (!is_dsi && !intel_crtc->config.clock_set) {
/*
* Returns a set of divisors for the desired target clock with
* the given refclk, or FALSE. The returned values represent
@@ -4904,28 +4907,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
ok = dev_priv->display.find_dpll(limit, crtc,
intel_crtc->config.port_clock,
refclk, NULL, &clock);
- if (!ok && !intel_crtc->config.clock_set) {
+ if (!ok) {
DRM_ERROR("Couldn't find PLL settings for mode!\n");
return -EINVAL;
}
- }
- if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
- /*
- * Ensure we match the reduced clock's P to the target clock.
- * If the clocks don't match, we can't switch the display clock
- * by using the FP0/FP1. In such case we will disable the LVDS
- * downclock feature.
- */
- limit = intel_limit(crtc, refclk);
- has_reduced_clock =
- dev_priv->display.find_dpll(limit, crtc,
- dev_priv->lvds_downclock,
- refclk, &clock,
- &reduced_clock);
- }
- /* Compat-code for transition, will disappear. */
- if (!intel_crtc->config.clock_set) {
+ if (is_lvds && dev_priv->lvds_downclock_avail) {
+ /*
+ * Ensure we match the reduced clock's P to the target
+ * clock. If the clocks don't match, we can't switch
+ * the display clock by using the FP0/FP1. In such case
+ * we will disable the LVDS downclock feature.
+ */
+ has_reduced_clock =
+ dev_priv->display.find_dpll(limit, crtc,
+ dev_priv->lvds_downclock,
+ refclk, &clock,
+ &reduced_clock);
+ }
+ /* Compat-code for transition, will disappear. */
intel_crtc->config.dpll.n = clock.n;
intel_crtc->config.dpll.m1 = clock.m1;
intel_crtc->config.dpll.m2 = clock.m2;
@@ -4938,14 +4938,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
has_reduced_clock ? &reduced_clock : NULL,
num_connectors);
} else if (IS_VALLEYVIEW(dev)) {
- if (!is_dsi)
- vlv_update_pll(intel_crtc);
+ vlv_update_pll(intel_crtc);
} else {
i9xx_update_pll(intel_crtc,
has_reduced_clock ? &reduced_clock : NULL,
num_connectors);
}
+skip_dpll:
/* Set up the display plane register */
dspcntr = DISPPLANE_GAMMA_ENABLE;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 3/3] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling
2013-09-04 11:20 ` [PATCH v2 3/3] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula
@ 2013-09-05 11:04 ` Ville Syrjälä
0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2013-09-05 11:04 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Wed, Sep 04, 2013 at 02:20:10PM +0300, Jani Nikula wrote:
> Flat out skip anything to do with PLL if we have a DSI encoder (and thus
> DSI PLL). Also skip PLL computation if the encoder has already set
> clocks. This allows for some tidying up of the code, including a
> superfluous call to intel_limit() for LVDS downclock path.
>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 44 +++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4939683..0646d14 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4891,9 +4891,12 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> num_connectors++;
> }
>
> - refclk = i9xx_get_refclk(crtc, num_connectors);
> + if (is_dsi)
> + goto skip_dpll;
> +
> + if (!intel_crtc->config.clock_set) {
> + refclk = i9xx_get_refclk(crtc, num_connectors);
>
> - if (!is_dsi && !intel_crtc->config.clock_set) {
> /*
> * Returns a set of divisors for the desired target clock with
> * the given refclk, or FALSE. The returned values represent
> @@ -4904,28 +4907,25 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> ok = dev_priv->display.find_dpll(limit, crtc,
> intel_crtc->config.port_clock,
> refclk, NULL, &clock);
> - if (!ok && !intel_crtc->config.clock_set) {
> + if (!ok) {
> DRM_ERROR("Couldn't find PLL settings for mode!\n");
> return -EINVAL;
> }
> - }
>
> - if (!is_dsi && is_lvds && dev_priv->lvds_downclock_avail) {
> - /*
> - * Ensure we match the reduced clock's P to the target clock.
> - * If the clocks don't match, we can't switch the display clock
> - * by using the FP0/FP1. In such case we will disable the LVDS
> - * downclock feature.
> - */
> - limit = intel_limit(crtc, refclk);
> - has_reduced_clock =
> - dev_priv->display.find_dpll(limit, crtc,
> - dev_priv->lvds_downclock,
> - refclk, &clock,
> - &reduced_clock);
> - }
> - /* Compat-code for transition, will disappear. */
> - if (!intel_crtc->config.clock_set) {
> + if (is_lvds && dev_priv->lvds_downclock_avail) {
> + /*
> + * Ensure we match the reduced clock's P to the target
> + * clock. If the clocks don't match, we can't switch
> + * the display clock by using the FP0/FP1. In such case
> + * we will disable the LVDS downclock feature.
> + */
> + has_reduced_clock =
> + dev_priv->display.find_dpll(limit, crtc,
> + dev_priv->lvds_downclock,
> + refclk, &clock,
> + &reduced_clock);
> + }
> + /* Compat-code for transition, will disappear. */
> intel_crtc->config.dpll.n = clock.n;
> intel_crtc->config.dpll.m1 = clock.m1;
> intel_crtc->config.dpll.m2 = clock.m2;
> @@ -4938,14 +4938,14 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> has_reduced_clock ? &reduced_clock : NULL,
> num_connectors);
> } else if (IS_VALLEYVIEW(dev)) {
> - if (!is_dsi)
> - vlv_update_pll(intel_crtc);
> + vlv_update_pll(intel_crtc);
> } else {
> i9xx_update_pll(intel_crtc,
> has_reduced_clock ? &reduced_clock : NULL,
> num_connectors);
> }
This looks all right.
But I was thinking that we could just set .clock_set=true in DSI code
and move the DSI PLL enable here. But we anyway have to move the DSI PLL
calculations out from the PLL enable func, so I guess we can leave all
that for later.
So in the meantime:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> +skip_dpll:
> /* Set up the display plane register */
> dspcntr = DISPPLANE_GAMMA_ENABLE;
>
> --
> 1.7.9.5
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set
2013-09-04 11:20 [PATCH v2 1/3] drm/i915: add asserts for cursor state in crtc mode set Jani Nikula
2013-09-04 11:20 ` [PATCH v2 2/3] drm/i915: do not update cursor " Jani Nikula
2013-09-04 11:20 ` [PATCH v2 3/3] drm/i915: clean up and simplify i9xx_crtc_mode_set wrt PLL handling Jani Nikula
@ 2013-09-04 12:11 ` Ville Syrjälä
2 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2013-09-04 12:11 UTC (permalink / raw)
To: Jani Nikula; +Cc: intel-gfx
On Wed, Sep 04, 2013 at 02:20:08PM +0300, Jani Nikula wrote:
> The cursor is supposed to be disabled during crtc mode set (disabled by
> ctrc disable). Assert this is the case.
>
> Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d88057e..89243cc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1069,6 +1069,26 @@ static void assert_panel_unlocked(struct drm_i915_private *dev_priv,
> pipe_name(pipe));
> }
>
> +static void assert_cursor(struct drm_i915_private *dev_priv,
> + enum pipe pipe, bool state)
> +{
> + struct drm_device *dev = dev_priv->dev;
> + bool cur_state;
> +
> + if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
This could be (gen >=7 && !VLV), but we have the same check in
intel_crtc_update_cursor() so that should be changed as well. Can
be left for another patch I suppose.
> + cur_state = I915_READ(CURCNTR_IVB(pipe)) & CURSOR_MODE;
> + else if (IS_845G(dev) || IS_I865G(dev))
> + cur_state = I915_READ(_CURACNTR) & CURSOR_ENABLE;
> + else
> + cur_state = I915_READ(CURCNTR(pipe)) & CURSOR_MODE;
First I was thingking that can't be right, but looks like it is. Weird
how they stuffed the cursor registers differently on mobile and desktop
during the gen2 days.
> +
> + WARN(cur_state != state,
> + "cursor on pipe %c assertion failure (expected %s, current %s)\n",
> + pipe_name(pipe), state_string(state), state_string(cur_state));
> +}
> +#define assert_cursor_enabled(d, p) assert_cursor(d, p, true)
> +#define assert_cursor_disabled(d, p) assert_cursor(d, p, false)
I guess we don't really need assert_cursor_enabled() but no real harm in
having it.
> +
> void assert_pipe(struct drm_i915_private *dev_priv,
> enum pipe pipe, bool state)
> {
> @@ -4856,6 +4876,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
> const intel_limit_t *limit;
> int ret;
>
> + assert_cursor_disabled(dev_priv, pipe);
> +
> for_each_encoder_on_crtc(dev, crtc, encoder) {
> switch (encoder->type) {
> case INTEL_OUTPUT_LVDS:
> @@ -5754,6 +5776,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
> struct intel_shared_dpll *pll;
> int ret;
>
> + assert_cursor_disabled(dev_priv, pipe);
> +
> for_each_encoder_on_crtc(dev, crtc, encoder) {
> switch (encoder->type) {
> case INTEL_OUTPUT_LVDS:
> @@ -6270,6 +6294,8 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
> int plane = intel_crtc->plane;
> int ret;
>
> + assert_cursor_disabled(dev_priv, intel_crtc->pipe);
> +
> if (!intel_ddi_pll_mode_set(crtc))
> return -EINVAL;
I'd slap the asserts to the same places as the asserts for other planes.
But maybe we'd want to have pipe and plane asserts in mode_set as well,
just in case we manage to mess things up and call mode_set w/o disabling
the pipe first.
Also looks like the plane assert should be killed from
ironlake_fdi_link_train(). We shouldn't need a plain to train the FDI
link as the pipe will anyway pump out pixels.
--
Ville Syrjälä
Intel OTC
^ permalink raw reply [flat|nested] 6+ messages in thread