All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Only look for matching clocks for LVDS downclock
@ 2012-01-10 23:09 Sean Paul
  2012-01-16 20:20 ` Jesse Barnes
  0 siblings, 1 reply; 5+ messages in thread
From: Sean Paul @ 2012-01-10 23:09 UTC (permalink / raw)
  To: intel-gfx

This patch enforces that the downclock clock source is the same as the preferred
clock source for LVDS. This fixes a bug where the driver chooses a downclock
clock source with a different P than the preferred mode clock source. This
happened even if the preferred clock source implemented an acceptable rate for
the downclock. The result of this bug is that downclock is disabled.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 drivers/gpu/drm/i915/intel_display.c |   74 +++++++++++++++++++---------------
 1 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 29743de..15f2b52 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -75,7 +75,7 @@ struct intel_limit {
 	intel_range_t   dot, vco, n, m, m1, m2, p, p1;
 	intel_p2_t	    p2;
 	bool (* find_pll)(const intel_limit_t *, struct drm_crtc *,
-			int, int, intel_clock_t *);
+			int, int, intel_clock_t *, intel_clock_t *);
 };
 
 /* FDI */
@@ -83,17 +83,21 @@ struct intel_limit {
 
 static bool
 intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
-		    int target, int refclk, intel_clock_t *best_clock);
+		    int target, int refclk, intel_clock_t *match_clock,
+		    intel_clock_t *best_clock);
 static bool
 intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
-			int target, int refclk, intel_clock_t *best_clock);
+			int target, int refclk, intel_clock_t *match_clock,
+			intel_clock_t *best_clock);
 
 static bool
 intel_find_pll_g4x_dp(const intel_limit_t *, struct drm_crtc *crtc,
-		      int target, int refclk, intel_clock_t *best_clock);
+		      int target, int refclk, intel_clock_t *match_clock,
+		      intel_clock_t *best_clock);
 static bool
 intel_find_pll_ironlake_dp(const intel_limit_t *, struct drm_crtc *crtc,
-			   int target, int refclk, intel_clock_t *best_clock);
+			   int target, int refclk, intel_clock_t *match_clock,
+			   intel_clock_t *best_clock);
 
 static inline u32 /* units of 100MHz */
 intel_fdi_link_freq(struct drm_device *dev)
@@ -515,7 +519,8 @@ static bool intel_PLL_is_valid(struct drm_device *dev,
 
 static bool
 intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
-		    int target, int refclk, intel_clock_t *best_clock)
+		    int target, int refclk, intel_clock_t *match_clock,
+		    intel_clock_t *best_clock)
 
 {
 	struct drm_device *dev = crtc->dev;
@@ -562,6 +567,9 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
 						continue;
+					if (match_clock &&
+					    clock.p != match_clock->p)
+						continue;
 
 					this_err = abs(clock.dot - target);
 					if (this_err < err) {
@@ -578,7 +586,8 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 
 static bool
 intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
-			int target, int refclk, intel_clock_t *best_clock)
+			int target, int refclk, intel_clock_t *match_clock,
+			intel_clock_t *best_clock)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -625,6 +634,9 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
 						continue;
+					if (match_clock &&
+					    clock.p != match_clock->p)
+						continue;
 
 					this_err = abs(clock.dot - target);
 					if (this_err < err_most) {
@@ -642,7 +654,8 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 
 static bool
 intel_find_pll_ironlake_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
-			   int target, int refclk, intel_clock_t *best_clock)
+			   int target, int refclk, intel_clock_t *match_clock,
+			   intel_clock_t *best_clock)
 {
 	struct drm_device *dev = crtc->dev;
 	intel_clock_t clock;
@@ -668,7 +681,8 @@ intel_find_pll_ironlake_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
 /* DisplayPort has only two frequencies, 162MHz and 270MHz */
 static bool
 intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
-		      int target, int refclk, intel_clock_t *best_clock)
+		      int target, int refclk, intel_clock_t *match_clock,
+		      intel_clock_t *best_clock)
 {
 	intel_clock_t clock;
 	if (target < 200000) {
@@ -5038,7 +5052,8 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
 	 */
 	limit = intel_limit(crtc, refclk);
-	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, &clock);
+	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
+			     &clock);
 	if (!ok) {
 		DRM_ERROR("Couldn't find PLL settings for mode!\n");
 		return -EINVAL;
@@ -5048,21 +5063,17 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	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 = limit->find_pll(limit, crtc,
 						    dev_priv->lvds_downclock,
 						    refclk,
+						    &clock,
 						    &reduced_clock);
-		if (has_reduced_clock && (clock.p != reduced_clock.p)) {
-			/*
-			 * If the different P is found, it means that we can't
-			 * switch the display clock by using the FP0/FP1.
-			 * In such case we will disable the LVDS downclock
-			 * feature.
-			 */
-			DRM_DEBUG_KMS("Different P is found for "
-				      "LVDS clock/downclock\n");
-			has_reduced_clock = 0;
-		}
 	}
 	/* SDVO TV has fixed PLL values depend on its clock range,
 	   this mirrors vbios setting. */
@@ -5583,7 +5594,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
 	 */
 	limit = intel_limit(crtc, refclk);
-	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, &clock);
+	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
+			     &clock);
 	if (!ok) {
 		DRM_ERROR("Couldn't find PLL settings for mode!\n");
 		return -EINVAL;
@@ -5593,21 +5605,17 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	intel_crtc_update_cursor(crtc, true);
 
 	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 = limit->find_pll(limit, crtc,
 						    dev_priv->lvds_downclock,
 						    refclk,
+						    &clock,
 						    &reduced_clock);
-		if (has_reduced_clock && (clock.p != reduced_clock.p)) {
-			/*
-			 * If the different P is found, it means that we can't
-			 * switch the display clock by using the FP0/FP1.
-			 * In such case we will disable the LVDS downclock
-			 * feature.
-			 */
-			DRM_DEBUG_KMS("Different P is found for "
-				      "LVDS clock/downclock\n");
-			has_reduced_clock = 0;
-		}
 	}
 	/* SDVO TV has fixed PLL values depend on its clock range,
 	   this mirrors vbios setting. */
-- 
1.7.3.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Only look for matching clocks for LVDS downclock
  2012-01-10 23:09 [PATCH] drm/i915: Only look for matching clocks for LVDS downclock Sean Paul
@ 2012-01-16 20:20 ` Jesse Barnes
  2012-01-16 20:21   ` Daniel Vetter
  0 siblings, 1 reply; 5+ messages in thread
From: Jesse Barnes @ 2012-01-16 20:20 UTC (permalink / raw)
  To: Sean Paul; +Cc: intel-gfx


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

On Tue, 10 Jan 2012 15:09:36 -0800
Sean Paul <seanpaul@chromium.org> wrote:

> This patch enforces that the downclock clock source is the same as the preferred
> clock source for LVDS. This fixes a bug where the driver chooses a downclock
> clock source with a different P than the preferred mode clock source. This
> happened even if the preferred clock source implemented an acceptable rate for
> the downclock. The result of this bug is that downclock is disabled.
> 
> Signed-off-by: Sean Paul <seanpaul@chromium.org>
> ---

Yeah looks like a good cleanup that also makes downclocking more likely.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Only look for matching clocks for LVDS downclock
  2012-01-16 20:20 ` Jesse Barnes
@ 2012-01-16 20:21   ` Daniel Vetter
  2012-01-16 21:56     ` Keith Packard
  0 siblings, 1 reply; 5+ messages in thread
From: Daniel Vetter @ 2012-01-16 20:21 UTC (permalink / raw)
  To: Jesse Barnes; +Cc: intel-gfx

On Mon, Jan 16, 2012 at 12:20:03PM -0800, Jesse Barnes wrote:
> On Tue, 10 Jan 2012 15:09:36 -0800
> Sean Paul <seanpaul@chromium.org> wrote:
> 
> > This patch enforces that the downclock clock source is the same as the preferred
> > clock source for LVDS. This fixes a bug where the driver chooses a downclock
> > clock source with a different P than the preferred mode clock source. This
> > happened even if the preferred clock source implemented an acceptable rate for
> > the downclock. The result of this bug is that downclock is disabled.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> 
> Yeah looks like a good cleanup that also makes downclocking more likely.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Only look for matching clocks for LVDS downclock
  2012-01-16 20:21   ` Daniel Vetter
@ 2012-01-16 21:56     ` Keith Packard
  2012-01-16 22:10       ` Jesse Barnes
  0 siblings, 1 reply; 5+ messages in thread
From: Keith Packard @ 2012-01-16 21:56 UTC (permalink / raw)
  To: Daniel Vetter, Jesse Barnes; +Cc: intel-gfx


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

On Mon, 16 Jan 2012 21:21:45 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jan 16, 2012 at 12:20:03PM -0800, Jesse Barnes wrote:

> > Yeah looks like a good cleanup that also makes downclocking more likely.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Queued for -next, thanks for the patch.

Jesse -- do you think this patch should go into -fixes for 3.3? Or
should we leave it for 3.4?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: Only look for matching clocks for LVDS downclock
  2012-01-16 21:56     ` Keith Packard
@ 2012-01-16 22:10       ` Jesse Barnes
  0 siblings, 0 replies; 5+ messages in thread
From: Jesse Barnes @ 2012-01-16 22:10 UTC (permalink / raw)
  To: Keith Packard; +Cc: intel-gfx


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

On Mon, 16 Jan 2012 13:56:34 -0800
Keith Packard <keithp@keithp.com> wrote:

> On Mon, 16 Jan 2012 21:21:45 +0100, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Jan 16, 2012 at 12:20:03PM -0800, Jesse Barnes wrote:
> 
> > > Yeah looks like a good cleanup that also makes downclocking more likely.
> > > 
> > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > 
> > Queued for -next, thanks for the patch.
> 
> Jesse -- do you think this patch should go into -fixes for 3.3? Or
> should we leave it for 3.4?

I'd say leave it for 3.4.  Downclocking is disabled by default anyway
so I don't think this one is urgent.

-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-01-16 22:10 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-01-10 23:09 [PATCH] drm/i915: Only look for matching clocks for LVDS downclock Sean Paul
2012-01-16 20:20 ` Jesse Barnes
2012-01-16 20:21   ` Daniel Vetter
2012-01-16 21:56     ` Keith Packard
2012-01-16 22:10       ` Jesse Barnes

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.