* [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
@ 2017-07-10 19:33 ville.syrjala
2017-07-10 19:33 ` [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk() ville.syrjala
` (5 more replies)
0 siblings, 6 replies; 16+ messages in thread
From: ville.syrjala @ 2017-07-10 19:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Paulo Zanoni, Rodrigo Vivi
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Make the min_pixclk thing less confusing by changing it to track
the minimum acceptable cdclk frequency instead. This means moving
the application of the guardbands to a slightly higher level from
the low level platform specific calc_cdclk() functions.
The immediate benefit is elimination of the confusing 2x factors
on GLK/CNL+ in the audio workarounds (which stems from the fact
that the pipes produce two pixels per clock).
v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
v3: Squash with the CNL cdclk limits patch (DK)
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
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 | 12 ++-
drivers/gpu/drm/i915/intel_cdclk.c | 202 ++++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_display.c | 21 ++--
drivers/gpu/drm/i915/intel_drv.h | 4 +-
4 files changed, 125 insertions(+), 114 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 81cd21ecfa7d..c80176424d84 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -563,6 +563,15 @@ struct i915_hotplug {
(__i)++) \
for_each_if (plane_state)
+#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, __i) \
+ for ((__i) = 0; \
+ (__i) < (__state)->base.dev->mode_config.num_crtc && \
+ ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+ (new_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \
+ (__i)++) \
+ for_each_if (crtc)
+
+
struct drm_i915_private;
struct i915_mm_struct;
struct i915_mmu_object;
@@ -2268,7 +2277,8 @@ struct drm_i915_private {
struct mutex dpll_lock;
unsigned int active_crtcs;
- unsigned int min_pixclk[I915_MAX_PIPES];
+ /* minimum acceptable cdclk for each pipe */
+ int min_cdclk[I915_MAX_PIPES];
int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 1241e5891b29..50f153dbea14 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -417,24 +417,21 @@ static void hsw_get_cdclk(struct drm_i915_private *dev_priv,
cdclk_state->cdclk = 540000;
}
-static int vlv_calc_cdclk(struct drm_i915_private *dev_priv,
- int max_pixclk)
+static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
{
int freq_320 = (dev_priv->hpll_freq << 1) % 320000 != 0 ?
333333 : 320000;
- int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
/*
* We seem to get an unstable or solid color picture at 200MHz.
* Not sure what's wrong. For now use 200MHz only when all pipes
* are off.
*/
- if (!IS_CHERRYVIEW(dev_priv) &&
- max_pixclk > freq_320*limit/100)
+ if (IS_VALLEYVIEW(dev_priv) && min_cdclk > freq_320)
return 400000;
- else if (max_pixclk > 266667*limit/100)
+ else if (min_cdclk > 266667)
return freq_320;
- else if (max_pixclk > 0)
+ else if (min_cdclk > 0)
return 266667;
else
return 200000;
@@ -612,13 +609,13 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
}
-static int bdw_calc_cdclk(int max_pixclk)
+static int bdw_calc_cdclk(int min_cdclk)
{
- if (max_pixclk > 540000)
+ if (min_cdclk > 540000)
return 675000;
- else if (max_pixclk > 450000)
+ else if (min_cdclk > 450000)
return 540000;
- else if (max_pixclk > 337500)
+ else if (min_cdclk > 337500)
return 450000;
else
return 337500;
@@ -724,23 +721,23 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
cdclk, dev_priv->cdclk.hw.cdclk);
}
-static int skl_calc_cdclk(int max_pixclk, int vco)
+static int skl_calc_cdclk(int min_cdclk, int vco)
{
if (vco == 8640000) {
- if (max_pixclk > 540000)
+ if (min_cdclk > 540000)
return 617143;
- else if (max_pixclk > 432000)
+ else if (min_cdclk > 432000)
return 540000;
- else if (max_pixclk > 308571)
+ else if (min_cdclk > 308571)
return 432000;
else
return 308571;
} else {
- if (max_pixclk > 540000)
+ if (min_cdclk > 540000)
return 675000;
- else if (max_pixclk > 450000)
+ else if (min_cdclk > 450000)
return 540000;
- else if (max_pixclk > 337500)
+ else if (min_cdclk > 337500)
return 450000;
else
return 337500;
@@ -1075,31 +1072,25 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
skl_set_cdclk(dev_priv, &cdclk_state);
}
-static int bxt_calc_cdclk(int max_pixclk)
+static int bxt_calc_cdclk(int min_cdclk)
{
- if (max_pixclk > 576000)
+ if (min_cdclk > 576000)
return 624000;
- else if (max_pixclk > 384000)
+ else if (min_cdclk > 384000)
return 576000;
- else if (max_pixclk > 288000)
+ else if (min_cdclk > 288000)
return 384000;
- else if (max_pixclk > 144000)
+ else if (min_cdclk > 144000)
return 288000;
else
return 144000;
}
-static int glk_calc_cdclk(int max_pixclk)
+static int glk_calc_cdclk(int min_cdclk)
{
- /*
- * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
- * as a temporary workaround. Use a higher cdclk instead. (Note that
- * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
- * cdclk.)
- */
- if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100))
+ if (min_cdclk > 158400)
return 316800;
- else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100))
+ else if (min_cdclk > 79200)
return 158400;
else
return 79200;
@@ -1420,11 +1411,11 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
bxt_set_cdclk(dev_priv, &cdclk_state);
}
-static int cnl_calc_cdclk(int max_pixclk)
+static int cnl_calc_cdclk(int min_cdclk)
{
- if (max_pixclk > 336000)
+ if (min_cdclk > 336000)
return 528000;
- else if (max_pixclk > 168000)
+ else if (min_cdclk > 168000)
return 336000;
else
return 168000;
@@ -1732,98 +1723,106 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
dev_priv->display.set_cdclk(dev_priv, cdclk_state);
}
-static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
- int pixel_rate)
+static int intel_min_cdclk(struct drm_i915_private *dev_priv,
+ int pixel_rate)
+{
+ if (INTEL_GEN(dev_priv) >= 10)
+ /*
+ * FIXME: Switch to DIV_ROUND_UP(pixel_rate, 2)
+ * once DDI clock voltage requirements are
+ * handled correctly.
+ */
+ return pixel_rate;
+ else if (IS_GEMINILAKE(dev_priv))
+ /*
+ * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
+ * as a temporary workaround. Use a higher cdclk instead. (Note that
+ * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
+ * cdclk.)
+ */
+ return DIV_ROUND_UP(pixel_rate * 100, 2 * 99);
+ else if (IS_GEN9(dev_priv) ||
+ IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
+ return pixel_rate;
+ else if (IS_CHERRYVIEW(dev_priv))
+ return DIV_ROUND_UP(pixel_rate * 100, 95);
+ else
+ return DIV_ROUND_UP(pixel_rate * 100, 90);
+}
+
+int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
{
struct drm_i915_private *dev_priv =
to_i915(crtc_state->base.crtc->dev);
+ int min_cdclk;
+
+ if (!crtc_state->base.enable)
+ return 0;
+
+ min_cdclk = intel_min_cdclk(dev_priv, crtc_state->pixel_rate);
/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
- pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
+ min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
/* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
* audio enabled, port width x4, and link rate HBR2 (5.4 GHz), or else
* there may be audio corruption or screen corruption." This cdclk
- * restriction for GLK is 316.8 MHz and since GLK can output two
- * pixels per clock, the pixel rate becomes 2 * 316.8 MHz.
+ * restriction for GLK is 316.8 MHz.
*/
if (intel_crtc_has_dp_encoder(crtc_state) &&
crtc_state->has_audio &&
crtc_state->port_clock >= 540000 &&
crtc_state->lane_count == 4) {
- if (IS_CANNONLAKE(dev_priv))
- pixel_rate = max(316800, pixel_rate);
- else if (IS_GEMINILAKE(dev_priv))
- pixel_rate = max(2 * 316800, pixel_rate);
- else
- pixel_rate = max(432000, pixel_rate);
+ if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
+ /* Display WA #1145: glk,cnl */
+ min_cdclk = max(316800, min_cdclk);
+ } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
+ /* Display WA #1144: skl,bxt */
+ min_cdclk = max(432000, min_cdclk);
+ }
}
/* According to BSpec, "The CD clock frequency must be at least twice
* the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
- * The check for GLK has to be adjusted as the platform can output
- * two pixels per clock.
*/
- if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
- if (IS_GEMINILAKE(dev_priv))
- pixel_rate = max(2 * 2 * 96000, pixel_rate);
- else
- pixel_rate = max(2 * 96000, pixel_rate);
- }
+ if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
+ min_cdclk = max(2 * 96000, min_cdclk);
- return pixel_rate;
+ return min_cdclk;
}
-/* compute the max rate for new configuration */
-static int intel_max_pixel_rate(struct drm_atomic_state *state)
+static int intel_compute_min_cdclk(struct drm_atomic_state *state)
{
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(state->dev);
- struct drm_crtc *crtc;
- struct drm_crtc_state *cstate;
+ struct intel_crtc *crtc;
struct intel_crtc_state *crtc_state;
- unsigned int max_pixel_rate = 0, i;
+ int min_cdclk = 0, i;
enum pipe pipe;
- memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
- sizeof(intel_state->min_pixclk));
-
- for_each_new_crtc_in_state(state, crtc, cstate, i) {
- int pixel_rate;
-
- crtc_state = to_intel_crtc_state(cstate);
- if (!crtc_state->base.enable) {
- intel_state->min_pixclk[i] = 0;
- continue;
- }
+ memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
+ sizeof(intel_state->min_cdclk));
- pixel_rate = crtc_state->pixel_rate;
-
- if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
- pixel_rate =
- bdw_adjust_min_pipe_pixel_rate(crtc_state,
- pixel_rate);
-
- intel_state->min_pixclk[i] = pixel_rate;
- }
+ for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
+ intel_state->min_cdclk[i] =
+ intel_crtc_compute_min_cdclk(crtc_state);
for_each_pipe(dev_priv, pipe)
- max_pixel_rate = max(intel_state->min_pixclk[pipe],
- max_pixel_rate);
+ min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
- return max_pixel_rate;
+ return min_cdclk;
}
static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
int cdclk;
- cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
+ cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
if (cdclk > dev_priv->max_cdclk_freq) {
DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1849,14 +1848,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
int cdclk;
/*
* FIXME should also account for plane ratio
* once 64bpp pixel formats are supported.
*/
- cdclk = bdw_calc_cdclk(max_pixclk);
+ cdclk = bdw_calc_cdclk(min_cdclk);
if (cdclk > dev_priv->max_cdclk_freq) {
DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1882,7 +1881,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(state->dev);
- const int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
int cdclk, vco;
vco = intel_state->cdclk.logical.vco;
@@ -1893,7 +1892,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
* FIXME should also account for plane ratio
* once 64bpp pixel formats are supported.
*/
- cdclk = skl_calc_cdclk(max_pixclk, vco);
+ cdclk = skl_calc_cdclk(min_cdclk, vco);
if (cdclk > dev_priv->max_cdclk_freq) {
DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1920,16 +1919,16 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
int cdclk, vco;
if (IS_GEMINILAKE(dev_priv)) {
- cdclk = glk_calc_cdclk(max_pixclk);
+ cdclk = glk_calc_cdclk(min_cdclk);
vco = glk_de_pll_vco(dev_priv, cdclk);
} else {
- cdclk = bxt_calc_cdclk(max_pixclk);
+ cdclk = bxt_calc_cdclk(min_cdclk);
vco = bxt_de_pll_vco(dev_priv, cdclk);
}
@@ -1966,10 +1965,10 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
int cdclk, vco;
- cdclk = cnl_calc_cdclk(max_pixclk);
+ cdclk = cnl_calc_cdclk(min_cdclk);
vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
if (cdclk > dev_priv->max_cdclk_freq) {
@@ -1999,14 +1998,21 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
{
int max_cdclk_freq = dev_priv->max_cdclk_freq;
- if (IS_GEMINILAKE(dev_priv))
+ if (INTEL_GEN(dev_priv) >= 10)
+ /*
+ * FIXME: Allow '2 * max_cdclk_freq'
+ * once DDI clock voltage requirements are
+ * handled correctly.
+ */
+ return max_cdclk_freq;
+ else if (IS_GEMINILAKE(dev_priv))
/*
* FIXME: Limiting to 99% as a temporary workaround. See
- * glk_calc_cdclk() for details.
+ * intel_min_cdclk() for details.
*/
return 2 * max_cdclk_freq * 99 / 100;
- else if (INTEL_INFO(dev_priv)->gen >= 9 ||
- IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+ else if (IS_GEN9(dev_priv) ||
+ IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
return max_cdclk_freq;
else if (IS_CHERRYVIEW(dev_priv))
return max_cdclk_freq*95/100;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2144adc5b1d5..b47535f5d95d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5920,7 +5920,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
intel_crtc->enabled_power_domains = 0;
dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
- dev_priv->min_pixclk[intel_crtc->pipe] = 0;
+ dev_priv->min_cdclk[intel_crtc->pipe] = 0;
}
/*
@@ -13292,8 +13292,8 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_atomic_track_fbs(state);
if (intel_state->modeset) {
- memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
- sizeof(intel_state->min_pixclk));
+ memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
+ sizeof(intel_state->min_cdclk));
dev_priv->active_crtcs = intel_state->active_crtcs;
dev_priv->cdclk.logical = intel_state->cdclk.logical;
dev_priv->cdclk.actual = intel_state->cdclk.actual;
@@ -15569,7 +15569,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
for_each_intel_crtc(dev, crtc) {
struct intel_crtc_state *crtc_state =
to_intel_crtc_state(crtc->base.state);
- int pixclk = 0;
+ int min_cdclk = 0;
memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
if (crtc_state->base.active) {
@@ -15590,22 +15590,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
intel_crtc_compute_pixel_rate(crtc_state);
- if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv) ||
- IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
- pixclk = crtc_state->pixel_rate;
- else
- WARN_ON(dev_priv->display.modeset_calc_cdclk);
-
- /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
- if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
- pixclk = DIV_ROUND_UP(pixclk * 100, 95);
+ if (dev_priv->display.modeset_calc_cdclk)
+ min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
drm_calc_timestamping_constants(&crtc->base,
&crtc_state->base.adjusted_mode);
update_scanline_offset(crtc);
}
- dev_priv->min_pixclk[crtc->pipe] = pixclk;
+ dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
intel_pipe_config_sanity_check(dev_priv, crtc_state);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d17a32437f07..8cc1b86b799a 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -383,7 +383,8 @@ struct intel_atomic_state {
unsigned int active_pipe_changes;
unsigned int active_crtcs;
- unsigned int min_pixclk[I915_MAX_PIPES];
+ /* minimum acceptable cdclk for each pipe */
+ int min_cdclk[I915_MAX_PIPES];
struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
@@ -1308,6 +1309,7 @@ void intel_audio_init(struct drm_i915_private *dev_priv);
void intel_audio_deinit(struct drm_i915_private *dev_priv);
/* intel_cdclk.c */
+int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
void skl_init_cdclk(struct drm_i915_private *dev_priv);
void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
void cnl_init_cdclk(struct drm_i915_private *dev_priv);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
@ 2017-07-10 19:33 ` ville.syrjala
2017-07-11 20:21 ` Pandiyan, Dhinakaran
2017-07-10 19:58 ` ✗ Fi.CI.BAT: warning for series starting with [v3,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" Patchwork
` (4 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: ville.syrjala @ 2017-07-10 19:33 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Paulo Zanoni, Rodrigo Vivi
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Currently the .modeset_calc_cdclk() hooks check the final cdclk value
against the max allowed. That's not really sufficient since the low
level calc_cdclk() functions effectively clamp the minimum required
cdclk to the max supported by the platform. Hence if the minimum
required exceeds the platforms capabilities we'd keep going anyway
using the max cdclk frequency.
To fix that let's move the check earlier into
intel_crtc_compute_min_cdclk() and we'll check the minimum required
cdclk of the pipe against the maximum supported by the platform.
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/intel_cdclk.c | 96 +++++++++++++++++-------------------
drivers/gpu/drm/i915/intel_display.c | 5 +-
2 files changed, 48 insertions(+), 53 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 50f153dbea14..603950f1b87f 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -1789,6 +1789,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
min_cdclk = max(2 * 96000, min_cdclk);
+ if (min_cdclk > dev_priv->max_cdclk_freq) {
+ DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
+ min_cdclk, dev_priv->max_cdclk_freq);
+ return -EINVAL;
+ }
+
return min_cdclk;
}
@@ -1798,16 +1804,21 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_crtc *crtc;
struct intel_crtc_state *crtc_state;
- int min_cdclk = 0, i;
+ int min_cdclk, i;
enum pipe pipe;
memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
sizeof(intel_state->min_cdclk));
- for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
- intel_state->min_cdclk[i] =
- intel_crtc_compute_min_cdclk(crtc_state);
+ for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
+ min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
+ if (min_cdclk < 0)
+ return min_cdclk;
+
+ intel_state->min_cdclk[i] = min_cdclk;
+ }
+ min_cdclk = 0;
for_each_pipe(dev_priv, pipe)
min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
@@ -1817,18 +1828,14 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int min_cdclk = intel_compute_min_cdclk(state);
- struct intel_atomic_state *intel_state =
- to_intel_atomic_state(state);
- int cdclk;
+ struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+ int min_cdclk, cdclk;
- cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
+ min_cdclk = intel_compute_min_cdclk(state);
+ if (min_cdclk < 0)
+ return min_cdclk;
- if (cdclk > dev_priv->max_cdclk_freq) {
- DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
- cdclk, dev_priv->max_cdclk_freq);
- return -EINVAL;
- }
+ cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
intel_state->cdclk.logical.cdclk = cdclk;
@@ -1846,10 +1853,12 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
{
- struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
- int min_cdclk = intel_compute_min_cdclk(state);
- int cdclk;
+ int min_cdclk, cdclk;
+
+ min_cdclk = intel_compute_min_cdclk(state);
+ if (min_cdclk < 0)
+ return min_cdclk;
/*
* FIXME should also account for plane ratio
@@ -1857,12 +1866,6 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
*/
cdclk = bdw_calc_cdclk(min_cdclk);
- if (cdclk > dev_priv->max_cdclk_freq) {
- DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
- cdclk, dev_priv->max_cdclk_freq);
- return -EINVAL;
- }
-
intel_state->cdclk.logical.cdclk = cdclk;
if (!intel_state->active_crtcs) {
@@ -1879,10 +1882,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
{
- struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int min_cdclk = intel_compute_min_cdclk(state);
- int cdclk, vco;
+ struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+ int min_cdclk, cdclk, vco;
+
+ min_cdclk = intel_compute_min_cdclk(state);
+ if (min_cdclk < 0)
+ return min_cdclk;
vco = intel_state->cdclk.logical.vco;
if (!vco)
@@ -1894,12 +1900,6 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
*/
cdclk = skl_calc_cdclk(min_cdclk, vco);
- if (cdclk > dev_priv->max_cdclk_freq) {
- DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
- cdclk, dev_priv->max_cdclk_freq);
- return -EINVAL;
- }
-
intel_state->cdclk.logical.vco = vco;
intel_state->cdclk.logical.cdclk = cdclk;
@@ -1919,10 +1919,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int min_cdclk = intel_compute_min_cdclk(state);
- struct intel_atomic_state *intel_state =
- to_intel_atomic_state(state);
- int cdclk, vco;
+ struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+ int min_cdclk, cdclk, vco;
+
+ min_cdclk = intel_compute_min_cdclk(state);
+ if (min_cdclk < 0)
+ return min_cdclk;
if (IS_GEMINILAKE(dev_priv)) {
cdclk = glk_calc_cdclk(min_cdclk);
@@ -1932,12 +1934,6 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
vco = bxt_de_pll_vco(dev_priv, cdclk);
}
- if (cdclk > dev_priv->max_cdclk_freq) {
- DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
- cdclk, dev_priv->max_cdclk_freq);
- return -EINVAL;
- }
-
intel_state->cdclk.logical.vco = vco;
intel_state->cdclk.logical.cdclk = cdclk;
@@ -1963,20 +1959,16 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- struct intel_atomic_state *intel_state =
- to_intel_atomic_state(state);
- int min_cdclk = intel_compute_min_cdclk(state);
- int cdclk, vco;
+ struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
+ int min_cdclk, cdclk, vco;
+
+ min_cdclk = intel_compute_min_cdclk(state);
+ if (min_cdclk < 0)
+ return min_cdclk;
cdclk = cnl_calc_cdclk(min_cdclk);
vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
- if (cdclk > dev_priv->max_cdclk_freq) {
- DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
- cdclk, dev_priv->max_cdclk_freq);
- return -EINVAL;
- }
-
intel_state->cdclk.logical.vco = vco;
intel_state->cdclk.logical.cdclk = cdclk;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b47535f5d95d..1caf0ef82e36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15590,8 +15590,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
intel_crtc_compute_pixel_rate(crtc_state);
- if (dev_priv->display.modeset_calc_cdclk)
+ if (dev_priv->display.modeset_calc_cdclk) {
min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
+ if (WARN_ON(min_cdclk < 0))
+ min_cdclk = 0;
+ }
drm_calc_timestamping_constants(&crtc->base,
&crtc_state->base.adjusted_mode);
--
2.13.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ✗ Fi.CI.BAT: warning for series starting with [v3,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
2017-07-10 19:33 ` [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk() ville.syrjala
@ 2017-07-10 19:58 ` Patchwork
2017-07-11 9:47 ` [PATCH v3 1/2] " Dhinakaran Pandiyan
` (3 subsequent siblings)
5 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-07-10 19:58 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v3,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
URL : https://patchwork.freedesktop.org/series/27078/
State : warning
== Summary ==
Series 27078v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/27078/revisions/1/mbox/
Test kms_flip:
Subgroup basic-flip-vs-modeset:
pass -> SKIP (fi-skl-x1585l)
Test kms_pipe_crc_basic:
Subgroup hang-read-crc-pipe-a:
pass -> DMESG-WARN (fi-pnv-d510) fdo#101597
Subgroup suspend-read-crc-pipe-b:
dmesg-warn -> PASS (fi-byt-j1900) fdo#101705
fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597
fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705
fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:442s
fi-bdw-gvtdvm total:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:426s
fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:356s
fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:523s
fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:506s
fi-byt-j1900 total:279 pass:255 dwarn:0 dfail:0 fail:0 skip:24 time:493s
fi-byt-n2820 total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:485s
fi-glk-2a total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:593s
fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:433s
fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:416s
fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:419s
fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:493s
fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:478s
fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:459s
fi-kbl-7560u total:279 pass:268 dwarn:1 dfail:0 fail:0 skip:10 time:573s
fi-kbl-r total:279 pass:260 dwarn:1 dfail:0 fail:0 skip:18 time:581s
fi-pnv-d510 total:279 pass:222 dwarn:2 dfail:0 fail:0 skip:55 time:561s
fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:452s
fi-skl-6700hq total:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:591s
fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:468s
fi-skl-6770hq total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:480s
fi-skl-gvtdvm total:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:438s
fi-skl-x1585l total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:470s
fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:542s
fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:405s
db404caab8d6ac51787ff715d8c7fbf920d2136e drm-tip: 2017y-07m-10d-19h-21m-26s UTC integration manifest
d33fa21 drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()
968160f drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_5158/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
2017-07-10 19:33 ` [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk() ville.syrjala
2017-07-10 19:58 ` ✗ Fi.CI.BAT: warning for series starting with [v3,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" Patchwork
@ 2017-07-11 9:47 ` Dhinakaran Pandiyan
2017-07-11 13:00 ` Ville Syrjälä
2017-08-30 18:57 ` [PATCH v4 " ville.syrjala
` (2 subsequent siblings)
5 siblings, 1 reply; 16+ messages in thread
From: Dhinakaran Pandiyan @ 2017-07-11 9:47 UTC (permalink / raw)
To: intel-gfx; +Cc: Rodrigo Vivi, Dhinakaran Pandiyan, Paulo Zanoni
On Monday, July 10, 2017 10:33:46 PM PDT ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the min_pixclk thing less confusing by changing it to track
> the minimum acceptable cdclk frequency instead. This means moving
> the application of the guardbands to a slightly higher level from
> the low level platform specific calc_cdclk() functions.
>
> The immediate benefit is elimination of the confusing 2x factors
> on GLK/CNL+ in the audio workarounds (which stems from the fact
> that the pipes produce two pixels per clock).
>
> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage
> handling v3: Squash with the CNL cdclk limits patch (DK)
>
Looks good to me, I only have some bikesheds and nitpicks below. Will leave it
to you to decide if you want to address them.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 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 | 12 ++-
> drivers/gpu/drm/i915/intel_cdclk.c | 202
> ++++++++++++++++++----------------- drivers/gpu/drm/i915/intel_display.c |
> 21 ++--
> drivers/gpu/drm/i915/intel_drv.h | 4 +-
> 4 files changed, 125 insertions(+), 114 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index 81cd21ecfa7d..c80176424d84 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -563,6 +563,15 @@ struct i915_hotplug {
> (__i)++) \
> for_each_if (plane_state)
>
> +#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state,
> __i) \ + for ((__i) = 0; \
> + (__i) < (__state)->base.dev->mode_config.num_crtc && \
> + ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
> + (new_crtc_state) =
> to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \ +
> (__i)++) \
> + for_each_if (crtc)
> +
> +
I am not sure how useful adding this macro is. The loop looks concise with
this macro, but we end up doing unnecessary to_intel_crtc() conversions in the
loop. Looks like all we need is a to_intel_crtc_state().
> struct drm_i915_private;
> struct i915_mm_struct;
> struct i915_mmu_object;
> @@ -2268,7 +2277,8 @@ struct drm_i915_private {
> struct mutex dpll_lock;
>
> unsigned int active_crtcs;
> - unsigned int min_pixclk[I915_MAX_PIPES];
> + /* minimum acceptable cdclk for each pipe */
> + int min_cdclk[I915_MAX_PIPES];
>
> int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> b/drivers/gpu/drm/i915/intel_cdclk.c index 1241e5891b29..50f153dbea14
> 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -417,24 +417,21 @@ static void hsw_get_cdclk(struct drm_i915_private
> *dev_priv, cdclk_state->cdclk = 540000;
> }
>
> -static int vlv_calc_cdclk(struct drm_i915_private *dev_priv,
> - int max_pixclk)
> +static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> {
> int freq_320 = (dev_priv->hpll_freq << 1) % 320000 != 0 ?
> 333333 : 320000;
> - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
>
> /*
> * We seem to get an unstable or solid color picture at 200MHz.
> * Not sure what's wrong. For now use 200MHz only when all pipes
> * are off.
> */
> - if (!IS_CHERRYVIEW(dev_priv) &&
> - max_pixclk > freq_320*limit/100)
> + if (IS_VALLEYVIEW(dev_priv) && min_cdclk > freq_320)
> return 400000;
> - else if (max_pixclk > 266667*limit/100)
> + else if (min_cdclk > 266667)
> return freq_320;
> - else if (max_pixclk > 0)
> + else if (min_cdclk > 0)
> return 266667;
> else
> return 200000;
> @@ -612,13 +609,13 @@ static void chv_set_cdclk(struct drm_i915_private
> *dev_priv, intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
> }
>
> -static int bdw_calc_cdclk(int max_pixclk)
> +static int bdw_calc_cdclk(int min_cdclk)
> {
> - if (max_pixclk > 540000)
> + if (min_cdclk > 540000)
> return 675000;
> - else if (max_pixclk > 450000)
> + else if (min_cdclk > 450000)
> return 540000;
> - else if (max_pixclk > 337500)
> + else if (min_cdclk > 337500)
> return 450000;
> else
> return 337500;
> @@ -724,23 +721,23 @@ static void bdw_set_cdclk(struct drm_i915_private
> *dev_priv, cdclk, dev_priv->cdclk.hw.cdclk);
> }
>
> -static int skl_calc_cdclk(int max_pixclk, int vco)
> +static int skl_calc_cdclk(int min_cdclk, int vco)
> {
> if (vco == 8640000) {
> - if (max_pixclk > 540000)
> + if (min_cdclk > 540000)
> return 617143;
> - else if (max_pixclk > 432000)
> + else if (min_cdclk > 432000)
> return 540000;
> - else if (max_pixclk > 308571)
> + else if (min_cdclk > 308571)
> return 432000;
> else
> return 308571;
> } else {
> - if (max_pixclk > 540000)
> + if (min_cdclk > 540000)
> return 675000;
> - else if (max_pixclk > 450000)
> + else if (min_cdclk > 450000)
> return 540000;
> - else if (max_pixclk > 337500)
> + else if (min_cdclk > 337500)
> return 450000;
> else
> return 337500;
> @@ -1075,31 +1072,25 @@ void skl_uninit_cdclk(struct drm_i915_private
> *dev_priv) skl_set_cdclk(dev_priv, &cdclk_state);
> }
>
> -static int bxt_calc_cdclk(int max_pixclk)
> +static int bxt_calc_cdclk(int min_cdclk)
> {
> - if (max_pixclk > 576000)
> + if (min_cdclk > 576000)
> return 624000;
> - else if (max_pixclk > 384000)
> + else if (min_cdclk > 384000)
> return 576000;
> - else if (max_pixclk > 288000)
> + else if (min_cdclk > 288000)
> return 384000;
> - else if (max_pixclk > 144000)
> + else if (min_cdclk > 144000)
> return 288000;
> else
> return 144000;
> }
>
> -static int glk_calc_cdclk(int max_pixclk)
> +static int glk_calc_cdclk(int min_cdclk)
> {
> - /*
> - * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
> - * as a temporary workaround. Use a higher cdclk instead. (Note that
> - * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
> - * cdclk.)
> - */
> - if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100))
> + if (min_cdclk > 158400)
> return 316800;
> - else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100))
> + else if (min_cdclk > 79200)
> return 158400;
> else
> return 79200;
> @@ -1420,11 +1411,11 @@ void bxt_uninit_cdclk(struct drm_i915_private
> *dev_priv) bxt_set_cdclk(dev_priv, &cdclk_state);
> }
>
> -static int cnl_calc_cdclk(int max_pixclk)
> +static int cnl_calc_cdclk(int min_cdclk)
> {
> - if (max_pixclk > 336000)
> + if (min_cdclk > 336000)
> return 528000;
> - else if (max_pixclk > 168000)
> + else if (min_cdclk > 168000)
> return 336000;
> else
> return 168000;
> @@ -1732,98 +1723,106 @@ void intel_set_cdclk(struct drm_i915_private
> *dev_priv, dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> }
>
> -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state
> *crtc_state, - int pixel_rate)
> +static int intel_min_cdclk(struct drm_i915_private *dev_priv,
The names have started to sound similar.
intel_min_cdclk
intel_crtc_compute_min_cdclk
intel_compute_min_cdclk
I can't think of anything better. How about something like
intel_pixel_rate_to_cdclk() ?
> + int pixel_rate)
> +{
> + if (INTEL_GEN(dev_priv) >= 10)
> + /*
> + * FIXME: Switch to DIV_ROUND_UP(pixel_rate, 2)
> + * once DDI clock voltage requirements are
> + * handled correctly.
> + */
> + return pixel_rate;
> + else if (IS_GEMINILAKE(dev_priv))
> + /*
> + * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
> + * as a temporary workaround. Use a higher cdclk instead. (Note that
> + * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
> + * cdclk.)
> + */
> + return DIV_ROUND_UP(pixel_rate * 100, 2 * 99);
> + else if (IS_GEN9(dev_priv) ||
> + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
I don't see why IS_HASWELL() is needed here. The callers seem to need a
.modeset_calc_cdclk() hook and Haswell does not have one.
> + return pixel_rate;
> + else if (IS_CHERRYVIEW(dev_priv))
> + return DIV_ROUND_UP(pixel_rate * 100, 95);
> + else
> + return DIV_ROUND_UP(pixel_rate * 100, 90);
Type out IS_VALLEYVIEW() explicitly for documentation?
> +}
> +
> +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> {
> struct drm_i915_private *dev_priv =
> to_i915(crtc_state->base.crtc->dev);
> + int min_cdclk;
> +
> + if (!crtc_state->base.enable)
> + return 0;
> +
> + min_cdclk = intel_min_cdclk(dev_priv, crtc_state->pixel_rate);
>
> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> + min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
>
> /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
> * audio enabled, port width x4, and link rate HBR2 (5.4 GHz), or else
> * there may be audio corruption or screen corruption." This cdclk
> - * restriction for GLK is 316.8 MHz and since GLK can output two
> - * pixels per clock, the pixel rate becomes 2 * 316.8 MHz.
> + * restriction for GLK is 316.8 MHz.
> */
> if (intel_crtc_has_dp_encoder(crtc_state) &&
> crtc_state->has_audio &&
> crtc_state->port_clock >= 540000 &&
> crtc_state->lane_count == 4) {
> - if (IS_CANNONLAKE(dev_priv))
> - pixel_rate = max(316800, pixel_rate);
> - else if (IS_GEMINILAKE(dev_priv))
> - pixel_rate = max(2 * 316800, pixel_rate);
> - else
> - pixel_rate = max(432000, pixel_rate);
> + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> + /* Display WA #1145: glk,cnl */
> + min_cdclk = max(316800, min_cdclk);
> + } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> + /* Display WA #1144: skl,bxt */
> + min_cdclk = max(432000, min_cdclk);
> + }
> }
>
> /* According to BSpec, "The CD clock frequency must be at least twice
> * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
> - * The check for GLK has to be adjusted as the platform can output
> - * two pixels per clock.
> */
> - if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> - if (IS_GEMINILAKE(dev_priv))
> - pixel_rate = max(2 * 2 * 96000, pixel_rate);
> - else
> - pixel_rate = max(2 * 96000, pixel_rate);
> - }
> + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> + min_cdclk = max(2 * 96000, min_cdclk);
>
> - return pixel_rate;
> + return min_cdclk;
> }
>
> -/* compute the max rate for new configuration */
> -static int intel_max_pixel_rate(struct drm_atomic_state *state)
> +static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> {
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - struct drm_crtc *crtc;
> - struct drm_crtc_state *cstate;
> + struct intel_crtc *crtc;
> struct intel_crtc_state *crtc_state;
> - unsigned int max_pixel_rate = 0, i;
> + int min_cdclk = 0, i;
> enum pipe pipe;
>
> - memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> - sizeof(intel_state->min_pixclk));
> -
> - for_each_new_crtc_in_state(state, crtc, cstate, i) {
> - int pixel_rate;
> -
> - crtc_state = to_intel_crtc_state(cstate);
> - if (!crtc_state->base.enable) {
> - intel_state->min_pixclk[i] = 0;
> - continue;
> - }
> + memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> + sizeof(intel_state->min_cdclk));
>
> - pixel_rate = crtc_state->pixel_rate;
> -
> - if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
> - pixel_rate =
> - bdw_adjust_min_pipe_pixel_rate(crtc_state,
> - pixel_rate);
> -
> - intel_state->min_pixclk[i] = pixel_rate;
> - }
> + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> + intel_state->min_cdclk[i] =
> + intel_crtc_compute_min_cdclk(crtc_state);
>
Like I wrote above, we could just reuse for_each_new_crtc_in_state()
> for_each_pipe(dev_priv, pipe)
> - max_pixel_rate = max(intel_state->min_pixclk[pipe],
> - max_pixel_rate);
> + min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>
> - return max_pixel_rate;
> + return min_cdclk;
> }
>
> static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_compute_min_cdclk(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> int cdclk;
>
> - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1849,14 +1848,14 @@ static int bdw_modeset_calc_cdclk(struct
> drm_atomic_state *state) {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> - int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_compute_min_cdclk(state);
> int cdclk;
>
> /*
> * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = bdw_calc_cdclk(max_pixclk);
> + cdclk = bdw_calc_cdclk(min_cdclk);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1882,7 +1881,7 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state) {
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - const int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_compute_min_cdclk(state);
> int cdclk, vco;
>
> vco = intel_state->cdclk.logical.vco;
> @@ -1893,7 +1892,7 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state) * FIXME should also account for plane ratio
> * once 64bpp pixel formats are supported.
> */
> - cdclk = skl_calc_cdclk(max_pixclk, vco);
> + cdclk = skl_calc_cdclk(min_cdclk, vco);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> @@ -1920,16 +1919,16 @@ static int skl_modeset_calc_cdclk(struct
> drm_atomic_state *state) static int bxt_modeset_calc_cdclk(struct
> drm_atomic_state *state) {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_compute_min_cdclk(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> int cdclk, vco;
>
> if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(max_pixclk);
> + cdclk = glk_calc_cdclk(min_cdclk);
> vco = glk_de_pll_vco(dev_priv, cdclk);
> } else {
> - cdclk = bxt_calc_cdclk(max_pixclk);
> + cdclk = bxt_calc_cdclk(min_cdclk);
> vco = bxt_de_pll_vco(dev_priv, cdclk);
> }
>
> @@ -1966,10 +1965,10 @@ static int cnl_modeset_calc_cdclk(struct
> drm_atomic_state *state) struct drm_i915_private *dev_priv =
> to_i915(state->dev);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> - int max_pixclk = intel_max_pixel_rate(state);
> + int min_cdclk = intel_compute_min_cdclk(state);
> int cdclk, vco;
>
> - cdclk = cnl_calc_cdclk(max_pixclk);
> + cdclk = cnl_calc_cdclk(min_cdclk);
> vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>
> if (cdclk > dev_priv->max_cdclk_freq) {
> @@ -1999,14 +1998,21 @@ static int intel_compute_max_dotclk(struct
> drm_i915_private *dev_priv) {
> int max_cdclk_freq = dev_priv->max_cdclk_freq;
>
> - if (IS_GEMINILAKE(dev_priv))
> + if (INTEL_GEN(dev_priv) >= 10)
> + /*
> + * FIXME: Allow '2 * max_cdclk_freq'
> + * once DDI clock voltage requirements are
> + * handled correctly.
> + */
> + return max_cdclk_freq;
> + else if (IS_GEMINILAKE(dev_priv))
> /*
> * FIXME: Limiting to 99% as a temporary workaround. See
> - * glk_calc_cdclk() for details.
> + * intel_min_cdclk() for details.
> */
> return 2 * max_cdclk_freq * 99 / 100;
> - else if (INTEL_INFO(dev_priv)->gen >= 9 ||
> - IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> + else if (IS_GEN9(dev_priv) ||
> + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> return max_cdclk_freq;
> else if (IS_CHERRYVIEW(dev_priv))
> return max_cdclk_freq*95/100;
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c index 2144adc5b1d5..b47535f5d95d
> 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5920,7 +5920,7 @@ static void intel_crtc_disable_noatomic(struct
> drm_crtc *crtc, intel_crtc->enabled_power_domains = 0;
>
> dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> - dev_priv->min_pixclk[intel_crtc->pipe] = 0;
> + dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> }
>
> /*
> @@ -13292,8 +13292,8 @@ static int intel_atomic_commit(struct drm_device
> *dev, intel_atomic_track_fbs(state);
>
> if (intel_state->modeset) {
> - memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> - sizeof(intel_state->min_pixclk));
> + memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
> + sizeof(intel_state->min_cdclk));
> dev_priv->active_crtcs = intel_state->active_crtcs;
> dev_priv->cdclk.logical = intel_state->cdclk.logical;
> dev_priv->cdclk.actual = intel_state->cdclk.actual;
> @@ -15569,7 +15569,7 @@ static void intel_modeset_readout_hw_state(struct
> drm_device *dev) for_each_intel_crtc(dev, crtc) {
> struct intel_crtc_state *crtc_state =
> to_intel_crtc_state(crtc->base.state);
> - int pixclk = 0;
> + int min_cdclk = 0;
>
> memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> if (crtc_state->base.active) {
> @@ -15590,22 +15590,15 @@ static void intel_modeset_readout_hw_state(struct
> drm_device *dev)
>
> intel_crtc_compute_pixel_rate(crtc_state);
>
> - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv) ||
> - IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> - pixclk = crtc_state->pixel_rate;
> - else
> - WARN_ON(dev_priv->display.modeset_calc_cdclk);
> -
> - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> - pixclk = DIV_ROUND_UP(pixclk * 100, 95);
> + if (dev_priv->display.modeset_calc_cdclk)
> + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
Hmm. So we were not applying the audio workarounds here. Wonder why it did not
cause any trouble.
I believe, the .modeset_calc_cdclk() functions are not called when this
hw_state is committed. Is that right?
>
> drm_calc_timestamping_constants(&crtc->base,
> &crtc_state->base.adjusted_mode);
> update_scanline_offset(crtc);
> }
>
> - dev_priv->min_pixclk[crtc->pipe] = pixclk;
> + dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
>
> intel_pipe_config_sanity_check(dev_priv, crtc_state);
> }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h index d17a32437f07..8cc1b86b799a 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -383,7 +383,8 @@ struct intel_atomic_state {
> unsigned int active_pipe_changes;
>
> unsigned int active_crtcs;
> - unsigned int min_pixclk[I915_MAX_PIPES];
> + /* minimum acceptable cdclk for each pipe */
> + int min_cdclk[I915_MAX_PIPES];
>
> struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
>
> @@ -1308,6 +1309,7 @@ void intel_audio_init(struct drm_i915_private
> *dev_priv); void intel_audio_deinit(struct drm_i915_private *dev_priv);
>
> /* intel_cdclk.c */
> +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state
> *crtc_state); void skl_init_cdclk(struct drm_i915_private *dev_priv);
> void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> void cnl_init_cdclk(struct drm_i915_private *dev_priv);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-07-11 9:47 ` [PATCH v3 1/2] " Dhinakaran Pandiyan
@ 2017-07-11 13:00 ` Ville Syrjälä
2017-07-11 20:35 ` Pandiyan, Dhinakaran
0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2017-07-11 13:00 UTC (permalink / raw)
To: Dhinakaran Pandiyan
Cc: intel-gfx, Rodrigo Vivi, Dhinakaran Pandiyan, Paulo Zanoni
On Tue, Jul 11, 2017 at 02:47:42AM -0700, Dhinakaran Pandiyan wrote:
> On Monday, July 10, 2017 10:33:46 PM PDT ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Make the min_pixclk thing less confusing by changing it to track
> > the minimum acceptable cdclk frequency instead. This means moving
> > the application of the guardbands to a slightly higher level from
> > the low level platform specific calc_cdclk() functions.
> >
> > The immediate benefit is elimination of the confusing 2x factors
> > on GLK/CNL+ in the audio workarounds (which stems from the fact
> > that the pipes produce two pixels per clock).
> >
> > v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage
> > handling v3: Squash with the CNL cdclk limits patch (DK)
> >
>
> Looks good to me, I only have some bikesheds and nitpicks below. Will leave it
> to you to decide if you want to address them.
>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 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 | 12 ++-
> > drivers/gpu/drm/i915/intel_cdclk.c | 202
> > ++++++++++++++++++----------------- drivers/gpu/drm/i915/intel_display.c |
> > 21 ++--
> > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > 4 files changed, 125 insertions(+), 114 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h index 81cd21ecfa7d..c80176424d84 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -563,6 +563,15 @@ struct i915_hotplug {
> > (__i)++) \
> > for_each_if (plane_state)
> >
> > +#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state,
> > __i) \ + for ((__i) = 0; \
> > + (__i) < (__state)->base.dev->mode_config.num_crtc && \
> > + ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
> > + (new_crtc_state) =
> > to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \ +
> > (__i)++) \
> > + for_each_if (crtc)
> > +
> > +
>
> I am not sure how useful adding this macro is. The loop looks concise with
> this macro, but we end up doing unnecessary to_intel_crtc() conversions in the
> loop.
Those are free.
> Looks like all we need is a to_intel_crtc_state().
I want to move away from the drm_ types as much as possible. Ideally
I'd rather not see those in the i915 code at all, but that's slightly
unrealistic thanks to the function pointers we hook into the
core/helpers. I've been pondering if we could automagically generate
some kind of wrappers for those that would give us the intel_ types
directly, but not sure if that's doable.
>
> > struct drm_i915_private;
> > struct i915_mm_struct;
> > struct i915_mmu_object;
> > @@ -2268,7 +2277,8 @@ struct drm_i915_private {
> > struct mutex dpll_lock;
> >
> > unsigned int active_crtcs;
> > - unsigned int min_pixclk[I915_MAX_PIPES];
> > + /* minimum acceptable cdclk for each pipe */
> > + int min_cdclk[I915_MAX_PIPES];
> >
> > int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > b/drivers/gpu/drm/i915/intel_cdclk.c index 1241e5891b29..50f153dbea14
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -417,24 +417,21 @@ static void hsw_get_cdclk(struct drm_i915_private
> > *dev_priv, cdclk_state->cdclk = 540000;
> > }
> >
> > -static int vlv_calc_cdclk(struct drm_i915_private *dev_priv,
> > - int max_pixclk)
> > +static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> > {
> > int freq_320 = (dev_priv->hpll_freq << 1) % 320000 != 0 ?
> > 333333 : 320000;
> > - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
> >
> > /*
> > * We seem to get an unstable or solid color picture at 200MHz.
> > * Not sure what's wrong. For now use 200MHz only when all pipes
> > * are off.
> > */
> > - if (!IS_CHERRYVIEW(dev_priv) &&
> > - max_pixclk > freq_320*limit/100)
> > + if (IS_VALLEYVIEW(dev_priv) && min_cdclk > freq_320)
> > return 400000;
> > - else if (max_pixclk > 266667*limit/100)
> > + else if (min_cdclk > 266667)
> > return freq_320;
> > - else if (max_pixclk > 0)
> > + else if (min_cdclk > 0)
> > return 266667;
> > else
> > return 200000;
> > @@ -612,13 +609,13 @@ static void chv_set_cdclk(struct drm_i915_private
> > *dev_priv, intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
> > }
> >
> > -static int bdw_calc_cdclk(int max_pixclk)
> > +static int bdw_calc_cdclk(int min_cdclk)
> > {
> > - if (max_pixclk > 540000)
> > + if (min_cdclk > 540000)
> > return 675000;
> > - else if (max_pixclk > 450000)
> > + else if (min_cdclk > 450000)
> > return 540000;
> > - else if (max_pixclk > 337500)
> > + else if (min_cdclk > 337500)
> > return 450000;
> > else
> > return 337500;
> > @@ -724,23 +721,23 @@ static void bdw_set_cdclk(struct drm_i915_private
> > *dev_priv, cdclk, dev_priv->cdclk.hw.cdclk);
> > }
> >
> > -static int skl_calc_cdclk(int max_pixclk, int vco)
> > +static int skl_calc_cdclk(int min_cdclk, int vco)
> > {
> > if (vco == 8640000) {
> > - if (max_pixclk > 540000)
> > + if (min_cdclk > 540000)
> > return 617143;
> > - else if (max_pixclk > 432000)
> > + else if (min_cdclk > 432000)
> > return 540000;
> > - else if (max_pixclk > 308571)
> > + else if (min_cdclk > 308571)
> > return 432000;
> > else
> > return 308571;
> > } else {
> > - if (max_pixclk > 540000)
> > + if (min_cdclk > 540000)
> > return 675000;
> > - else if (max_pixclk > 450000)
> > + else if (min_cdclk > 450000)
> > return 540000;
> > - else if (max_pixclk > 337500)
> > + else if (min_cdclk > 337500)
> > return 450000;
> > else
> > return 337500;
> > @@ -1075,31 +1072,25 @@ void skl_uninit_cdclk(struct drm_i915_private
> > *dev_priv) skl_set_cdclk(dev_priv, &cdclk_state);
> > }
> >
> > -static int bxt_calc_cdclk(int max_pixclk)
> > +static int bxt_calc_cdclk(int min_cdclk)
> > {
> > - if (max_pixclk > 576000)
> > + if (min_cdclk > 576000)
> > return 624000;
> > - else if (max_pixclk > 384000)
> > + else if (min_cdclk > 384000)
> > return 576000;
> > - else if (max_pixclk > 288000)
> > + else if (min_cdclk > 288000)
> > return 384000;
> > - else if (max_pixclk > 144000)
> > + else if (min_cdclk > 144000)
> > return 288000;
> > else
> > return 144000;
> > }
> >
> > -static int glk_calc_cdclk(int max_pixclk)
> > +static int glk_calc_cdclk(int min_cdclk)
> > {
> > - /*
> > - * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
> > - * as a temporary workaround. Use a higher cdclk instead. (Note that
> > - * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
> > - * cdclk.)
> > - */
> > - if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100))
> > + if (min_cdclk > 158400)
> > return 316800;
> > - else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100))
> > + else if (min_cdclk > 79200)
> > return 158400;
> > else
> > return 79200;
> > @@ -1420,11 +1411,11 @@ void bxt_uninit_cdclk(struct drm_i915_private
> > *dev_priv) bxt_set_cdclk(dev_priv, &cdclk_state);
> > }
> >
> > -static int cnl_calc_cdclk(int max_pixclk)
> > +static int cnl_calc_cdclk(int min_cdclk)
> > {
> > - if (max_pixclk > 336000)
> > + if (min_cdclk > 336000)
> > return 528000;
> > - else if (max_pixclk > 168000)
> > + else if (min_cdclk > 168000)
> > return 336000;
> > else
> > return 168000;
> > @@ -1732,98 +1723,106 @@ void intel_set_cdclk(struct drm_i915_private
> > *dev_priv, dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > }
> >
> > -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state
> > *crtc_state, - int pixel_rate)
> > +static int intel_min_cdclk(struct drm_i915_private *dev_priv,
>
> The names have started to sound similar.
> intel_min_cdclk
> intel_crtc_compute_min_cdclk
> intel_compute_min_cdclk
>
> I can't think of anything better. How about something like
> intel_pixel_rate_to_cdclk() ?
Sure, I can respin with that.
>
>
> > + int pixel_rate)
> > +{
> > + if (INTEL_GEN(dev_priv) >= 10)
> > + /*
> > + * FIXME: Switch to DIV_ROUND_UP(pixel_rate, 2)
> > + * once DDI clock voltage requirements are
> > + * handled correctly.
> > + */
> > + return pixel_rate;
> > + else if (IS_GEMINILAKE(dev_priv))
> > + /*
> > + * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
> > + * as a temporary workaround. Use a higher cdclk instead. (Note that
> > + * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
> > + * cdclk.)
> > + */
> > + return DIV_ROUND_UP(pixel_rate * 100, 2 * 99);
> > + else if (IS_GEN9(dev_priv) ||
> > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
>
> I don't see why IS_HASWELL() is needed here. The callers seem to need a
> .modeset_calc_cdclk() hook and Haswell does not have one.
I still have the HSW code tucked away in a branch ;)
>
>
> > + return pixel_rate;
> > + else if (IS_CHERRYVIEW(dev_priv))
> > + return DIV_ROUND_UP(pixel_rate * 100, 95);
> > + else
> > + return DIV_ROUND_UP(pixel_rate * 100, 90);
>
> Type out IS_VALLEYVIEW() explicitly for documentation?
That 90% would apply to all remaining platforms. Not that we currently
implement cdclk frequency scaling for those, but we could (at least for
some gmch platforms).
Even if we never add the cdclk code fo HSW/GMCH platforms I think keeping
this in sync with the max_dotclock() thing makes it easier to cross
check things.
>
> > +}
> > +
> > +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > {
> > struct drm_i915_private *dev_priv =
> > to_i915(crtc_state->base.crtc->dev);
> > + int min_cdclk;
> > +
> > + if (!crtc_state->base.enable)
> > + return 0;
> > +
> > + min_cdclk = intel_min_cdclk(dev_priv, crtc_state->pixel_rate);
> >
> > /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> > - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> > + min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
> >
> > /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
> > * audio enabled, port width x4, and link rate HBR2 (5.4 GHz), or else
> > * there may be audio corruption or screen corruption." This cdclk
> > - * restriction for GLK is 316.8 MHz and since GLK can output two
> > - * pixels per clock, the pixel rate becomes 2 * 316.8 MHz.
> > + * restriction for GLK is 316.8 MHz.
> > */
> > if (intel_crtc_has_dp_encoder(crtc_state) &&
> > crtc_state->has_audio &&
> > crtc_state->port_clock >= 540000 &&
> > crtc_state->lane_count == 4) {
> > - if (IS_CANNONLAKE(dev_priv))
> > - pixel_rate = max(316800, pixel_rate);
> > - else if (IS_GEMINILAKE(dev_priv))
> > - pixel_rate = max(2 * 316800, pixel_rate);
> > - else
> > - pixel_rate = max(432000, pixel_rate);
> > + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> > + /* Display WA #1145: glk,cnl */
> > + min_cdclk = max(316800, min_cdclk);
> > + } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> > + /* Display WA #1144: skl,bxt */
> > + min_cdclk = max(432000, min_cdclk);
> > + }
> > }
> >
> > /* According to BSpec, "The CD clock frequency must be at least twice
> > * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
> > - * The check for GLK has to be adjusted as the platform can output
> > - * two pixels per clock.
> > */
> > - if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> > - if (IS_GEMINILAKE(dev_priv))
> > - pixel_rate = max(2 * 2 * 96000, pixel_rate);
> > - else
> > - pixel_rate = max(2 * 96000, pixel_rate);
> > - }
> > + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > + min_cdclk = max(2 * 96000, min_cdclk);
> >
> > - return pixel_rate;
> > + return min_cdclk;
> > }
> >
> > -/* compute the max rate for new configuration */
> > -static int intel_max_pixel_rate(struct drm_atomic_state *state)
> > +static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > {
> > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - struct drm_crtc *crtc;
> > - struct drm_crtc_state *cstate;
> > + struct intel_crtc *crtc;
> > struct intel_crtc_state *crtc_state;
> > - unsigned int max_pixel_rate = 0, i;
> > + int min_cdclk = 0, i;
> > enum pipe pipe;
> >
> > - memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> > - sizeof(intel_state->min_pixclk));
> > -
> > - for_each_new_crtc_in_state(state, crtc, cstate, i) {
> > - int pixel_rate;
> > -
> > - crtc_state = to_intel_crtc_state(cstate);
> > - if (!crtc_state->base.enable) {
> > - intel_state->min_pixclk[i] = 0;
> > - continue;
> > - }
> > + memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> > + sizeof(intel_state->min_cdclk));
> >
> > - pixel_rate = crtc_state->pixel_rate;
> > -
> > - if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
> > - pixel_rate =
> > - bdw_adjust_min_pipe_pixel_rate(crtc_state,
> > - pixel_rate);
> > -
> > - intel_state->min_pixclk[i] = pixel_rate;
> > - }
> > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> > + intel_state->min_cdclk[i] =
> > + intel_crtc_compute_min_cdclk(crtc_state);
> >
>
> Like I wrote above, we could just reuse for_each_new_crtc_in_state()
As mentioned that doesn't agree with the direction where we want to go.
>
> > for_each_pipe(dev_priv, pipe)
> > - max_pixel_rate = max(intel_state->min_pixclk[pipe],
> > - max_pixel_rate);
> > + min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> >
> > - return max_pixel_rate;
> > + return min_cdclk;
> > }
> >
> > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_compute_min_cdclk(state);
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > int cdclk;
> >
> > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > @@ -1849,14 +1848,14 @@ static int bdw_modeset_calc_cdclk(struct
> > drm_atomic_state *state) {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > - int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_compute_min_cdclk(state);
> > int cdclk;
> >
> > /*
> > * FIXME should also account for plane ratio
> > * once 64bpp pixel formats are supported.
> > */
> > - cdclk = bdw_calc_cdclk(max_pixclk);
> > + cdclk = bdw_calc_cdclk(min_cdclk);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > @@ -1882,7 +1881,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state) {
> > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - const int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_compute_min_cdclk(state);
> > int cdclk, vco;
> >
> > vco = intel_state->cdclk.logical.vco;
> > @@ -1893,7 +1892,7 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state) * FIXME should also account for plane ratio
> > * once 64bpp pixel formats are supported.
> > */
> > - cdclk = skl_calc_cdclk(max_pixclk, vco);
> > + cdclk = skl_calc_cdclk(min_cdclk, vco);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > @@ -1920,16 +1919,16 @@ static int skl_modeset_calc_cdclk(struct
> > drm_atomic_state *state) static int bxt_modeset_calc_cdclk(struct
> > drm_atomic_state *state) {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_compute_min_cdclk(state);
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > int cdclk, vco;
> >
> > if (IS_GEMINILAKE(dev_priv)) {
> > - cdclk = glk_calc_cdclk(max_pixclk);
> > + cdclk = glk_calc_cdclk(min_cdclk);
> > vco = glk_de_pll_vco(dev_priv, cdclk);
> > } else {
> > - cdclk = bxt_calc_cdclk(max_pixclk);
> > + cdclk = bxt_calc_cdclk(min_cdclk);
> > vco = bxt_de_pll_vco(dev_priv, cdclk);
> > }
> >
> > @@ -1966,10 +1965,10 @@ static int cnl_modeset_calc_cdclk(struct
> > drm_atomic_state *state) struct drm_i915_private *dev_priv =
> > to_i915(state->dev);
> > struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > - int max_pixclk = intel_max_pixel_rate(state);
> > + int min_cdclk = intel_compute_min_cdclk(state);
> > int cdclk, vco;
> >
> > - cdclk = cnl_calc_cdclk(max_pixclk);
> > + cdclk = cnl_calc_cdclk(min_cdclk);
> > vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> >
> > if (cdclk > dev_priv->max_cdclk_freq) {
> > @@ -1999,14 +1998,21 @@ static int intel_compute_max_dotclk(struct
> > drm_i915_private *dev_priv) {
> > int max_cdclk_freq = dev_priv->max_cdclk_freq;
> >
> > - if (IS_GEMINILAKE(dev_priv))
> > + if (INTEL_GEN(dev_priv) >= 10)
> > + /*
> > + * FIXME: Allow '2 * max_cdclk_freq'
> > + * once DDI clock voltage requirements are
> > + * handled correctly.
> > + */
> > + return max_cdclk_freq;
> > + else if (IS_GEMINILAKE(dev_priv))
> > /*
> > * FIXME: Limiting to 99% as a temporary workaround. See
> > - * glk_calc_cdclk() for details.
> > + * intel_min_cdclk() for details.
> > */
> > return 2 * max_cdclk_freq * 99 / 100;
> > - else if (INTEL_INFO(dev_priv)->gen >= 9 ||
> > - IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > + else if (IS_GEN9(dev_priv) ||
> > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > return max_cdclk_freq;
> > else if (IS_CHERRYVIEW(dev_priv))
> > return max_cdclk_freq*95/100;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c index 2144adc5b1d5..b47535f5d95d
> > 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5920,7 +5920,7 @@ static void intel_crtc_disable_noatomic(struct
> > drm_crtc *crtc, intel_crtc->enabled_power_domains = 0;
> >
> > dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> > - dev_priv->min_pixclk[intel_crtc->pipe] = 0;
> > + dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> > }
> >
> > /*
> > @@ -13292,8 +13292,8 @@ static int intel_atomic_commit(struct drm_device
> > *dev, intel_atomic_track_fbs(state);
> >
> > if (intel_state->modeset) {
> > - memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> > - sizeof(intel_state->min_pixclk));
> > + memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
> > + sizeof(intel_state->min_cdclk));
> > dev_priv->active_crtcs = intel_state->active_crtcs;
> > dev_priv->cdclk.logical = intel_state->cdclk.logical;
> > dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > @@ -15569,7 +15569,7 @@ static void intel_modeset_readout_hw_state(struct
> > drm_device *dev) for_each_intel_crtc(dev, crtc) {
> > struct intel_crtc_state *crtc_state =
> > to_intel_crtc_state(crtc->base.state);
> > - int pixclk = 0;
> > + int min_cdclk = 0;
> >
> > memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> > if (crtc_state->base.active) {
> > @@ -15590,22 +15590,15 @@ static void intel_modeset_readout_hw_state(struct
> > drm_device *dev)
> >
> > intel_crtc_compute_pixel_rate(crtc_state);
> >
> > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv) ||
> > - IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > - pixclk = crtc_state->pixel_rate;
> > - else
> > - WARN_ON(dev_priv->display.modeset_calc_cdclk);
> > -
> > - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> > - pixclk = DIV_ROUND_UP(pixclk * 100, 95);
> > + if (dev_priv->display.modeset_calc_cdclk)
> > + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
>
> Hmm. So we were not applying the audio workarounds here. Wonder why it did not
> cause any trouble.
I guess usually there's a modeset happening at some point.
>
> I believe, the .modeset_calc_cdclk() functions are not called when this
> hw_state is committed. Is that right?
This is the state readout for init/resume. So not sure which commit
you're referring to here?
>
>
> >
> > drm_calc_timestamping_constants(&crtc->base,
> > &crtc_state->base.adjusted_mode);
> > update_scanline_offset(crtc);
> > }
> >
> > - dev_priv->min_pixclk[crtc->pipe] = pixclk;
> > + dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
> >
> > intel_pipe_config_sanity_check(dev_priv, crtc_state);
> > }
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h index d17a32437f07..8cc1b86b799a 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -383,7 +383,8 @@ struct intel_atomic_state {
> > unsigned int active_pipe_changes;
> >
> > unsigned int active_crtcs;
> > - unsigned int min_pixclk[I915_MAX_PIPES];
> > + /* minimum acceptable cdclk for each pipe */
> > + int min_cdclk[I915_MAX_PIPES];
> >
> > struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
> >
> > @@ -1308,6 +1309,7 @@ void intel_audio_init(struct drm_i915_private
> > *dev_priv); void intel_audio_deinit(struct drm_i915_private *dev_priv);
> >
> > /* intel_cdclk.c */
> > +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state
> > *crtc_state); void skl_init_cdclk(struct drm_i915_private *dev_priv);
> > void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> > void cnl_init_cdclk(struct drm_i915_private *dev_priv);
>
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()
2017-07-10 19:33 ` [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk() ville.syrjala
@ 2017-07-11 20:21 ` Pandiyan, Dhinakaran
2017-07-12 10:18 ` Ville Syrjälä
0 siblings, 1 reply; 16+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-07-11 20:21 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Zanoni, Paulo R, Vivi, Rodrigo
On Mon, 2017-07-10 at 22:33 +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Currently the .modeset_calc_cdclk() hooks check the final cdclk value
> against the max allowed. That's not really sufficient since the low
> level calc_cdclk() functions effectively clamp the minimum required
> cdclk to the max supported by the platform. Hence if the minimum
> required exceeds the platforms capabilities we'd keep going anyway
> using the max cdclk frequency.
>
> To fix that let's move the check earlier into
> intel_crtc_compute_min_cdclk() and we'll check the minimum required
> cdclk of the pipe against the maximum supported by the platform.
>
I suppose this should save some power in the case where one of the
CRTC's pixel rate exceeds platform capabilities. And failing the
atomic_check instead of going with max_cdclk will help the userspace try
a lower mode. Is that the idea?
Moving the checks makes sense to me because that seems like the original
intention anyway, but I think it's a good idea to get someone else to
take a look too.
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_cdclk.c | 96 +++++++++++++++++-------------------
> drivers/gpu/drm/i915/intel_display.c | 5 +-
> 2 files changed, 48 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 50f153dbea14..603950f1b87f 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1789,6 +1789,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> min_cdclk = max(2 * 96000, min_cdclk);
>
> + if (min_cdclk > dev_priv->max_cdclk_freq) {
> + DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
> + min_cdclk, dev_priv->max_cdclk_freq);
> + return -EINVAL;
> + }
> +
> return min_cdclk;
> }
>
> @@ -1798,16 +1804,21 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_crtc *crtc;
> struct intel_crtc_state *crtc_state;
> - int min_cdclk = 0, i;
> + int min_cdclk, i;
> enum pipe pipe;
>
> memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> sizeof(intel_state->min_cdclk));
>
> - for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> - intel_state->min_cdclk[i] =
> - intel_crtc_compute_min_cdclk(crtc_state);
> + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
> + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> + if (min_cdclk < 0)
> + return min_cdclk;
> +
> + intel_state->min_cdclk[i] = min_cdclk;
> + }
>
> + min_cdclk = 0;
> for_each_pipe(dev_priv, pipe)
> min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
>
> @@ -1817,18 +1828,14 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int cdclk;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk;
>
> - cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
>
> intel_state->cdclk.logical.cdclk = cdclk;
>
> @@ -1846,10 +1853,12 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->dev);
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - int cdclk;
> + int min_cdclk, cdclk;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> /*
> * FIXME should also account for plane ratio
> @@ -1857,12 +1866,6 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> */
> cdclk = bdw_calc_cdclk(min_cdclk);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.cdclk = cdclk;
>
> if (!intel_state->active_crtcs) {
> @@ -1879,10 +1882,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
>
> static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - int cdclk, vco;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk, vco;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> vco = intel_state->cdclk.logical.vco;
> if (!vco)
> @@ -1894,12 +1900,6 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> */
> cdclk = skl_calc_cdclk(min_cdclk, vco);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
>
> @@ -1919,10 +1919,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int cdclk, vco;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk, vco;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> if (IS_GEMINILAKE(dev_priv)) {
> cdclk = glk_calc_cdclk(min_cdclk);
> @@ -1932,12 +1934,6 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> vco = bxt_de_pll_vco(dev_priv, cdclk);
> }
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
>
> @@ -1963,20 +1959,16 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int min_cdclk = intel_compute_min_cdclk(state);
> - int cdclk, vco;
> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> + int min_cdclk, cdclk, vco;
> +
> + min_cdclk = intel_compute_min_cdclk(state);
> + if (min_cdclk < 0)
> + return min_cdclk;
>
> cdclk = cnl_calc_cdclk(min_cdclk);
> vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - return -EINVAL;
> - }
> -
> intel_state->cdclk.logical.vco = vco;
> intel_state->cdclk.logical.cdclk = cdclk;
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b47535f5d95d..1caf0ef82e36 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15590,8 +15590,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>
> intel_crtc_compute_pixel_rate(crtc_state);
>
> - if (dev_priv->display.modeset_calc_cdclk)
> + if (dev_priv->display.modeset_calc_cdclk) {
> min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> + if (WARN_ON(min_cdclk < 0))
> + min_cdclk = 0;
> + }
>
> drm_calc_timestamping_constants(&crtc->base,
> &crtc_state->base.adjusted_mode);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-07-11 13:00 ` Ville Syrjälä
@ 2017-07-11 20:35 ` Pandiyan, Dhinakaran
0 siblings, 0 replies; 16+ messages in thread
From: Pandiyan, Dhinakaran @ 2017-07-11 20:35 UTC (permalink / raw)
To: ville.syrjala; +Cc: intel-gfx, Zanoni, Paulo R, Vivi, Rodrigo
On Tue, 2017-07-11 at 16:00 +0300, Ville Syrjälä wrote:
> On Tue, Jul 11, 2017 at 02:47:42AM -0700, Dhinakaran Pandiyan wrote:
> > On Monday, July 10, 2017 10:33:46 PM PDT ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Make the min_pixclk thing less confusing by changing it to track
> > > the minimum acceptable cdclk frequency instead. This means moving
> > > the application of the guardbands to a slightly higher level from
> > > the low level platform specific calc_cdclk() functions.
> > >
> > > The immediate benefit is elimination of the confusing 2x factors
> > > on GLK/CNL+ in the audio workarounds (which stems from the fact
> > > that the pipes produce two pixels per clock).
> > >
> > > v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage
> > > handling v3: Squash with the CNL cdclk limits patch (DK)
> > >
> >
> > Looks good to me, I only have some bikesheds and nitpicks below. Will leave it
> > to you to decide if you want to address them.
> >
> > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >
> > > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > 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 | 12 ++-
> > > drivers/gpu/drm/i915/intel_cdclk.c | 202
> > > ++++++++++++++++++----------------- drivers/gpu/drm/i915/intel_display.c |
> > > 21 ++--
> > > drivers/gpu/drm/i915/intel_drv.h | 4 +-
> > > 4 files changed, 125 insertions(+), 114 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h index 81cd21ecfa7d..c80176424d84 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -563,6 +563,15 @@ struct i915_hotplug {
> > > (__i)++) \
> > > for_each_if (plane_state)
> > >
> > > +#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state,
> > > __i) \ + for ((__i) = 0; \
> > > + (__i) < (__state)->base.dev->mode_config.num_crtc && \
> > > + ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
> > > + (new_crtc_state) =
> > > to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \ +
> > > (__i)++) \
> > > + for_each_if (crtc)
> > > +
> > > +
> >
> > I am not sure how useful adding this macro is. The loop looks concise with
> > this macro, but we end up doing unnecessary to_intel_crtc() conversions in the
> > loop.
>
> Those are free.
>
> > Looks like all we need is a to_intel_crtc_state().
>
> I want to move away from the drm_ types as much as possible. Ideally
> I'd rather not see those in the i915 code at all, but that's slightly
> unrealistic thanks to the function pointers we hook into the
> core/helpers. I've been pondering if we could automagically generate
> some kind of wrappers for those that would give us the intel_ types
> directly, but not sure if that's doable.
>
> >
> > > struct drm_i915_private;
> > > struct i915_mm_struct;
> > > struct i915_mmu_object;
> > > @@ -2268,7 +2277,8 @@ struct drm_i915_private {
> > > struct mutex dpll_lock;
> > >
> > > unsigned int active_crtcs;
> > > - unsigned int min_pixclk[I915_MAX_PIPES];
> > > + /* minimum acceptable cdclk for each pipe */
> > > + int min_cdclk[I915_MAX_PIPES];
> > >
> > > int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c
> > > b/drivers/gpu/drm/i915/intel_cdclk.c index 1241e5891b29..50f153dbea14
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > > @@ -417,24 +417,21 @@ static void hsw_get_cdclk(struct drm_i915_private
> > > *dev_priv, cdclk_state->cdclk = 540000;
> > > }
> > >
> > > -static int vlv_calc_cdclk(struct drm_i915_private *dev_priv,
> > > - int max_pixclk)
> > > +static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
> > > {
> > > int freq_320 = (dev_priv->hpll_freq << 1) % 320000 != 0 ?
> > > 333333 : 320000;
> > > - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
> > >
> > > /*
> > > * We seem to get an unstable or solid color picture at 200MHz.
> > > * Not sure what's wrong. For now use 200MHz only when all pipes
> > > * are off.
> > > */
> > > - if (!IS_CHERRYVIEW(dev_priv) &&
> > > - max_pixclk > freq_320*limit/100)
> > > + if (IS_VALLEYVIEW(dev_priv) && min_cdclk > freq_320)
> > > return 400000;
> > > - else if (max_pixclk > 266667*limit/100)
> > > + else if (min_cdclk > 266667)
> > > return freq_320;
> > > - else if (max_pixclk > 0)
> > > + else if (min_cdclk > 0)
> > > return 266667;
> > > else
> > > return 200000;
> > > @@ -612,13 +609,13 @@ static void chv_set_cdclk(struct drm_i915_private
> > > *dev_priv, intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
> > > }
> > >
> > > -static int bdw_calc_cdclk(int max_pixclk)
> > > +static int bdw_calc_cdclk(int min_cdclk)
> > > {
> > > - if (max_pixclk > 540000)
> > > + if (min_cdclk > 540000)
> > > return 675000;
> > > - else if (max_pixclk > 450000)
> > > + else if (min_cdclk > 450000)
> > > return 540000;
> > > - else if (max_pixclk > 337500)
> > > + else if (min_cdclk > 337500)
> > > return 450000;
> > > else
> > > return 337500;
> > > @@ -724,23 +721,23 @@ static void bdw_set_cdclk(struct drm_i915_private
> > > *dev_priv, cdclk, dev_priv->cdclk.hw.cdclk);
> > > }
> > >
> > > -static int skl_calc_cdclk(int max_pixclk, int vco)
> > > +static int skl_calc_cdclk(int min_cdclk, int vco)
> > > {
> > > if (vco == 8640000) {
> > > - if (max_pixclk > 540000)
> > > + if (min_cdclk > 540000)
> > > return 617143;
> > > - else if (max_pixclk > 432000)
> > > + else if (min_cdclk > 432000)
> > > return 540000;
> > > - else if (max_pixclk > 308571)
> > > + else if (min_cdclk > 308571)
> > > return 432000;
> > > else
> > > return 308571;
> > > } else {
> > > - if (max_pixclk > 540000)
> > > + if (min_cdclk > 540000)
> > > return 675000;
> > > - else if (max_pixclk > 450000)
> > > + else if (min_cdclk > 450000)
> > > return 540000;
> > > - else if (max_pixclk > 337500)
> > > + else if (min_cdclk > 337500)
> > > return 450000;
> > > else
> > > return 337500;
> > > @@ -1075,31 +1072,25 @@ void skl_uninit_cdclk(struct drm_i915_private
> > > *dev_priv) skl_set_cdclk(dev_priv, &cdclk_state);
> > > }
> > >
> > > -static int bxt_calc_cdclk(int max_pixclk)
> > > +static int bxt_calc_cdclk(int min_cdclk)
> > > {
> > > - if (max_pixclk > 576000)
> > > + if (min_cdclk > 576000)
> > > return 624000;
> > > - else if (max_pixclk > 384000)
> > > + else if (min_cdclk > 384000)
> > > return 576000;
> > > - else if (max_pixclk > 288000)
> > > + else if (min_cdclk > 288000)
> > > return 384000;
> > > - else if (max_pixclk > 144000)
> > > + else if (min_cdclk > 144000)
> > > return 288000;
> > > else
> > > return 144000;
> > > }
> > >
> > > -static int glk_calc_cdclk(int max_pixclk)
> > > +static int glk_calc_cdclk(int min_cdclk)
> > > {
> > > - /*
> > > - * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
> > > - * as a temporary workaround. Use a higher cdclk instead. (Note that
> > > - * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
> > > - * cdclk.)
> > > - */
> > > - if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100))
> > > + if (min_cdclk > 158400)
> > > return 316800;
> > > - else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100))
> > > + else if (min_cdclk > 79200)
> > > return 158400;
> > > else
> > > return 79200;
> > > @@ -1420,11 +1411,11 @@ void bxt_uninit_cdclk(struct drm_i915_private
> > > *dev_priv) bxt_set_cdclk(dev_priv, &cdclk_state);
> > > }
> > >
> > > -static int cnl_calc_cdclk(int max_pixclk)
> > > +static int cnl_calc_cdclk(int min_cdclk)
> > > {
> > > - if (max_pixclk > 336000)
> > > + if (min_cdclk > 336000)
> > > return 528000;
> > > - else if (max_pixclk > 168000)
> > > + else if (min_cdclk > 168000)
> > > return 336000;
> > > else
> > > return 168000;
> > > @@ -1732,98 +1723,106 @@ void intel_set_cdclk(struct drm_i915_private
> > > *dev_priv, dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> > > }
> > >
> > > -static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state
> > > *crtc_state, - int pixel_rate)
> > > +static int intel_min_cdclk(struct drm_i915_private *dev_priv,
> >
> > The names have started to sound similar.
> > intel_min_cdclk
> > intel_crtc_compute_min_cdclk
> > intel_compute_min_cdclk
> >
> > I can't think of anything better. How about something like
> > intel_pixel_rate_to_cdclk() ?
>
> Sure, I can respin with that.
>
> >
> >
> > > + int pixel_rate)
> > > +{
> > > + if (INTEL_GEN(dev_priv) >= 10)
> > > + /*
> > > + * FIXME: Switch to DIV_ROUND_UP(pixel_rate, 2)
> > > + * once DDI clock voltage requirements are
> > > + * handled correctly.
> > > + */
> > > + return pixel_rate;
> > > + else if (IS_GEMINILAKE(dev_priv))
> > > + /*
> > > + * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
> > > + * as a temporary workaround. Use a higher cdclk instead. (Note that
> > > + * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
> > > + * cdclk.)
> > > + */
> > > + return DIV_ROUND_UP(pixel_rate * 100, 2 * 99);
> > > + else if (IS_GEN9(dev_priv) ||
> > > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> >
> > I don't see why IS_HASWELL() is needed here. The callers seem to need a
> > .modeset_calc_cdclk() hook and Haswell does not have one.
>
> I still have the HSW code tucked away in a branch ;)
>
> >
> >
> > > + return pixel_rate;
> > > + else if (IS_CHERRYVIEW(dev_priv))
> > > + return DIV_ROUND_UP(pixel_rate * 100, 95);
> > > + else
> > > + return DIV_ROUND_UP(pixel_rate * 100, 90);
> >
> > Type out IS_VALLEYVIEW() explicitly for documentation?
>
> That 90% would apply to all remaining platforms. Not that we currently
> implement cdclk frequency scaling for those, but we could (at least for
> some gmch platforms).
>
> Even if we never add the cdclk code fo HSW/GMCH platforms I think keeping
> this in sync with the max_dotclock() thing makes it easier to cross
> check things.
>
> >
> > > +}
> > > +
> > > +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > > {
> > > struct drm_i915_private *dev_priv =
> > > to_i915(crtc_state->base.crtc->dev);
> > > + int min_cdclk;
> > > +
> > > + if (!crtc_state->base.enable)
> > > + return 0;
> > > +
> > > + min_cdclk = intel_min_cdclk(dev_priv, crtc_state->pixel_rate);
> > >
> > > /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > > if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> > > - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> > > + min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
> > >
> > > /* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
> > > * audio enabled, port width x4, and link rate HBR2 (5.4 GHz), or else
> > > * there may be audio corruption or screen corruption." This cdclk
> > > - * restriction for GLK is 316.8 MHz and since GLK can output two
> > > - * pixels per clock, the pixel rate becomes 2 * 316.8 MHz.
> > > + * restriction for GLK is 316.8 MHz.
> > > */
> > > if (intel_crtc_has_dp_encoder(crtc_state) &&
> > > crtc_state->has_audio &&
> > > crtc_state->port_clock >= 540000 &&
> > > crtc_state->lane_count == 4) {
> > > - if (IS_CANNONLAKE(dev_priv))
> > > - pixel_rate = max(316800, pixel_rate);
> > > - else if (IS_GEMINILAKE(dev_priv))
> > > - pixel_rate = max(2 * 316800, pixel_rate);
> > > - else
> > > - pixel_rate = max(432000, pixel_rate);
> > > + if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
> > > + /* Display WA #1145: glk,cnl */
> > > + min_cdclk = max(316800, min_cdclk);
> > > + } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
> > > + /* Display WA #1144: skl,bxt */
> > > + min_cdclk = max(432000, min_cdclk);
> > > + }
> > > }
> > >
> > > /* According to BSpec, "The CD clock frequency must be at least twice
> > > * the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
> > > - * The check for GLK has to be adjusted as the platform can output
> > > - * two pixels per clock.
> > > */
> > > - if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
> > > - if (IS_GEMINILAKE(dev_priv))
> > > - pixel_rate = max(2 * 2 * 96000, pixel_rate);
> > > - else
> > > - pixel_rate = max(2 * 96000, pixel_rate);
> > > - }
> > > + if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > > + min_cdclk = max(2 * 96000, min_cdclk);
> > >
> > > - return pixel_rate;
> > > + return min_cdclk;
> > > }
> > >
> > > -/* compute the max rate for new configuration */
> > > -static int intel_max_pixel_rate(struct drm_atomic_state *state)
> > > +static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > > {
> > > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > - struct drm_crtc *crtc;
> > > - struct drm_crtc_state *cstate;
> > > + struct intel_crtc *crtc;
> > > struct intel_crtc_state *crtc_state;
> > > - unsigned int max_pixel_rate = 0, i;
> > > + int min_cdclk = 0, i;
> > > enum pipe pipe;
> > >
> > > - memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
> > > - sizeof(intel_state->min_pixclk));
> > > -
> > > - for_each_new_crtc_in_state(state, crtc, cstate, i) {
> > > - int pixel_rate;
> > > -
> > > - crtc_state = to_intel_crtc_state(cstate);
> > > - if (!crtc_state->base.enable) {
> > > - intel_state->min_pixclk[i] = 0;
> > > - continue;
> > > - }
> > > + memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> > > + sizeof(intel_state->min_cdclk));
> > >
> > > - pixel_rate = crtc_state->pixel_rate;
> > > -
> > > - if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
> > > - pixel_rate =
> > > - bdw_adjust_min_pipe_pixel_rate(crtc_state,
> > > - pixel_rate);
> > > -
> > > - intel_state->min_pixclk[i] = pixel_rate;
> > > - }
> > > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> > > + intel_state->min_cdclk[i] =
> > > + intel_crtc_compute_min_cdclk(crtc_state);
> > >
> >
> > Like I wrote above, we could just reuse for_each_new_crtc_in_state()
>
> As mentioned that doesn't agree with the direction where we want to go.
>
> >
> > > for_each_pipe(dev_priv, pipe)
> > > - max_pixel_rate = max(intel_state->min_pixclk[pipe],
> > > - max_pixel_rate);
> > > + min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> > >
> > > - return max_pixel_rate;
> > > + return min_cdclk;
> > > }
> > >
> > > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > > {
> > > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > - int max_pixclk = intel_max_pixel_rate(state);
> > > + int min_cdclk = intel_compute_min_cdclk(state);
> > > struct intel_atomic_state *intel_state =
> > > to_intel_atomic_state(state);
> > > int cdclk;
> > >
> > > - cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> > > + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> > >
> > > if (cdclk > dev_priv->max_cdclk_freq) {
> > > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > @@ -1849,14 +1848,14 @@ static int bdw_modeset_calc_cdclk(struct
> > > drm_atomic_state *state) {
> > > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > > - int max_pixclk = intel_max_pixel_rate(state);
> > > + int min_cdclk = intel_compute_min_cdclk(state);
> > > int cdclk;
> > >
> > > /*
> > > * FIXME should also account for plane ratio
> > > * once 64bpp pixel formats are supported.
> > > */
> > > - cdclk = bdw_calc_cdclk(max_pixclk);
> > > + cdclk = bdw_calc_cdclk(min_cdclk);
> > >
> > > if (cdclk > dev_priv->max_cdclk_freq) {
> > > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > @@ -1882,7 +1881,7 @@ static int skl_modeset_calc_cdclk(struct
> > > drm_atomic_state *state) {
> > > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > - const int max_pixclk = intel_max_pixel_rate(state);
> > > + int min_cdclk = intel_compute_min_cdclk(state);
> > > int cdclk, vco;
> > >
> > > vco = intel_state->cdclk.logical.vco;
> > > @@ -1893,7 +1892,7 @@ static int skl_modeset_calc_cdclk(struct
> > > drm_atomic_state *state) * FIXME should also account for plane ratio
> > > * once 64bpp pixel formats are supported.
> > > */
> > > - cdclk = skl_calc_cdclk(max_pixclk, vco);
> > > + cdclk = skl_calc_cdclk(min_cdclk, vco);
> > >
> > > if (cdclk > dev_priv->max_cdclk_freq) {
> > > DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > > @@ -1920,16 +1919,16 @@ static int skl_modeset_calc_cdclk(struct
> > > drm_atomic_state *state) static int bxt_modeset_calc_cdclk(struct
> > > drm_atomic_state *state) {
> > > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > > - int max_pixclk = intel_max_pixel_rate(state);
> > > + int min_cdclk = intel_compute_min_cdclk(state);
> > > struct intel_atomic_state *intel_state =
> > > to_intel_atomic_state(state);
> > > int cdclk, vco;
> > >
> > > if (IS_GEMINILAKE(dev_priv)) {
> > > - cdclk = glk_calc_cdclk(max_pixclk);
> > > + cdclk = glk_calc_cdclk(min_cdclk);
> > > vco = glk_de_pll_vco(dev_priv, cdclk);
> > > } else {
> > > - cdclk = bxt_calc_cdclk(max_pixclk);
> > > + cdclk = bxt_calc_cdclk(min_cdclk);
> > > vco = bxt_de_pll_vco(dev_priv, cdclk);
> > > }
> > >
> > > @@ -1966,10 +1965,10 @@ static int cnl_modeset_calc_cdclk(struct
> > > drm_atomic_state *state) struct drm_i915_private *dev_priv =
> > > to_i915(state->dev);
> > > struct intel_atomic_state *intel_state =
> > > to_intel_atomic_state(state);
> > > - int max_pixclk = intel_max_pixel_rate(state);
> > > + int min_cdclk = intel_compute_min_cdclk(state);
> > > int cdclk, vco;
> > >
> > > - cdclk = cnl_calc_cdclk(max_pixclk);
> > > + cdclk = cnl_calc_cdclk(min_cdclk);
> > > vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> > >
> > > if (cdclk > dev_priv->max_cdclk_freq) {
> > > @@ -1999,14 +1998,21 @@ static int intel_compute_max_dotclk(struct
> > > drm_i915_private *dev_priv) {
> > > int max_cdclk_freq = dev_priv->max_cdclk_freq;
> > >
> > > - if (IS_GEMINILAKE(dev_priv))
> > > + if (INTEL_GEN(dev_priv) >= 10)
> > > + /*
> > > + * FIXME: Allow '2 * max_cdclk_freq'
> > > + * once DDI clock voltage requirements are
> > > + * handled correctly.
> > > + */
> > > + return max_cdclk_freq;
> > > + else if (IS_GEMINILAKE(dev_priv))
> > > /*
> > > * FIXME: Limiting to 99% as a temporary workaround. See
> > > - * glk_calc_cdclk() for details.
> > > + * intel_min_cdclk() for details.
> > > */
> > > return 2 * max_cdclk_freq * 99 / 100;
> > > - else if (INTEL_INFO(dev_priv)->gen >= 9 ||
> > > - IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> > > + else if (IS_GEN9(dev_priv) ||
> > > + IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
> > > return max_cdclk_freq;
> > > else if (IS_CHERRYVIEW(dev_priv))
> > > return max_cdclk_freq*95/100;
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c index 2144adc5b1d5..b47535f5d95d
> > > 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5920,7 +5920,7 @@ static void intel_crtc_disable_noatomic(struct
> > > drm_crtc *crtc, intel_crtc->enabled_power_domains = 0;
> > >
> > > dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
> > > - dev_priv->min_pixclk[intel_crtc->pipe] = 0;
> > > + dev_priv->min_cdclk[intel_crtc->pipe] = 0;
> > > }
> > >
> > > /*
> > > @@ -13292,8 +13292,8 @@ static int intel_atomic_commit(struct drm_device
> > > *dev, intel_atomic_track_fbs(state);
> > >
> > > if (intel_state->modeset) {
> > > - memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> > > - sizeof(intel_state->min_pixclk));
> > > + memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
> > > + sizeof(intel_state->min_cdclk));
> > > dev_priv->active_crtcs = intel_state->active_crtcs;
> > > dev_priv->cdclk.logical = intel_state->cdclk.logical;
> > > dev_priv->cdclk.actual = intel_state->cdclk.actual;
> > > @@ -15569,7 +15569,7 @@ static void intel_modeset_readout_hw_state(struct
> > > drm_device *dev) for_each_intel_crtc(dev, crtc) {
> > > struct intel_crtc_state *crtc_state =
> > > to_intel_crtc_state(crtc->base.state);
> > > - int pixclk = 0;
> > > + int min_cdclk = 0;
> > >
> > > memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
> > > if (crtc_state->base.active) {
> > > @@ -15590,22 +15590,15 @@ static void intel_modeset_readout_hw_state(struct
> > > drm_device *dev)
> > >
> > > intel_crtc_compute_pixel_rate(crtc_state);
> > >
> > > - if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv) ||
> > > - IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > - pixclk = crtc_state->pixel_rate;
> > > - else
> > > - WARN_ON(dev_priv->display.modeset_calc_cdclk);
> > > -
> > > - /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> > > - if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> > > - pixclk = DIV_ROUND_UP(pixclk * 100, 95);
> > > + if (dev_priv->display.modeset_calc_cdclk)
> > > + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> >
> > Hmm. So we were not applying the audio workarounds here. Wonder why it did not
> > cause any trouble.
>
> I guess usually there's a modeset happening at some point.
>
> >
> > I believe, the .modeset_calc_cdclk() functions are not called when this
> > hw_state is committed. Is that right?
>
> This is the state readout for init/resume. So not sure which commit
> you're referring to here?
>
I was referring to the commit that immediately follows the readout in
__intel_display_resume. But, I read the code again and it answered my
question :)
> >
> >
> > >
> > > drm_calc_timestamping_constants(&crtc->base,
> > > &crtc_state->base.adjusted_mode);
> > > update_scanline_offset(crtc);
> > > }
> > >
> > > - dev_priv->min_pixclk[crtc->pipe] = pixclk;
> > > + dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
> > >
> > > intel_pipe_config_sanity_check(dev_priv, crtc_state);
> > > }
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > b/drivers/gpu/drm/i915/intel_drv.h index d17a32437f07..8cc1b86b799a 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -383,7 +383,8 @@ struct intel_atomic_state {
> > > unsigned int active_pipe_changes;
> > >
> > > unsigned int active_crtcs;
> > > - unsigned int min_pixclk[I915_MAX_PIPES];
> > > + /* minimum acceptable cdclk for each pipe */
> > > + int min_cdclk[I915_MAX_PIPES];
> > >
> > > struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
> > >
> > > @@ -1308,6 +1309,7 @@ void intel_audio_init(struct drm_i915_private
> > > *dev_priv); void intel_audio_deinit(struct drm_i915_private *dev_priv);
> > >
> > > /* intel_cdclk.c */
> > > +int intel_crtc_compute_min_cdclk(const struct intel_crtc_state
> > > *crtc_state); void skl_init_cdclk(struct drm_i915_private *dev_priv);
> > > void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
> > > void cnl_init_cdclk(struct drm_i915_private *dev_priv);
> >
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()
2017-07-11 20:21 ` Pandiyan, Dhinakaran
@ 2017-07-12 10:18 ` Ville Syrjälä
0 siblings, 0 replies; 16+ messages in thread
From: Ville Syrjälä @ 2017-07-12 10:18 UTC (permalink / raw)
To: Pandiyan, Dhinakaran; +Cc: intel-gfx, Zanoni, Paulo R, Vivi, Rodrigo
On Tue, Jul 11, 2017 at 08:21:32PM +0000, Pandiyan, Dhinakaran wrote:
>
>
>
> On Mon, 2017-07-10 at 22:33 +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Currently the .modeset_calc_cdclk() hooks check the final cdclk value
> > against the max allowed. That's not really sufficient since the low
> > level calc_cdclk() functions effectively clamp the minimum required
> > cdclk to the max supported by the platform. Hence if the minimum
> > required exceeds the platforms capabilities we'd keep going anyway
> > using the max cdclk frequency.
> >
> > To fix that let's move the check earlier into
> > intel_crtc_compute_min_cdclk() and we'll check the minimum required
> > cdclk of the pipe against the maximum supported by the platform.
> >
>
> I suppose this should save some power in the case where one of the
> CRTC's pixel rate exceeds platform capabilities.
Rather it keeps the display working. If the cdclk can't supply enough
pixels to satisfy the port's demands then in the best case we get
constant underruns, in the worst case the box probably hard hangs.
> And failing the
> atomic_check instead of going with max_cdclk will help the userspace try
> a lower mode. Is that the idea?
Well, in theory you shouldn't even get this far as we should have
rejected the mode earlier. But I figured there's no harm in keeping the
checks since we do adjust the required cdcdlk here compared to earlier
estimates we might have made purely on the pixel rate.
>
> Moving the checks makes sense to me because that seems like the original
> intention anyway, but I think it's a good idea to get someone else to
> take a look too.
>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> > drivers/gpu/drm/i915/intel_cdclk.c | 96 +++++++++++++++++-------------------
> > drivers/gpu/drm/i915/intel_display.c | 5 +-
> > 2 files changed, 48 insertions(+), 53 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> > index 50f153dbea14..603950f1b87f 100644
> > --- a/drivers/gpu/drm/i915/intel_cdclk.c
> > +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> > @@ -1789,6 +1789,12 @@ int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
> > if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
> > min_cdclk = max(2 * 96000, min_cdclk);
> >
> > + if (min_cdclk > dev_priv->max_cdclk_freq) {
> > + DRM_DEBUG_KMS("required cdclk (%d kHz) exceeds max (%d kHz)\n",
> > + min_cdclk, dev_priv->max_cdclk_freq);
> > + return -EINVAL;
> > + }
> > +
> > return min_cdclk;
> > }
> >
> > @@ -1798,16 +1804,21 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > struct intel_crtc *crtc;
> > struct intel_crtc_state *crtc_state;
> > - int min_cdclk = 0, i;
> > + int min_cdclk, i;
> > enum pipe pipe;
> >
> > memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
> > sizeof(intel_state->min_cdclk));
> >
> > - for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
> > - intel_state->min_cdclk[i] =
> > - intel_crtc_compute_min_cdclk(crtc_state);
> > + for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i) {
> > + min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> > + if (min_cdclk < 0)
> > + return min_cdclk;
> > +
> > + intel_state->min_cdclk[i] = min_cdclk;
> > + }
> >
> > + min_cdclk = 0;
> > for_each_pipe(dev_priv, pipe)
> > min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
> >
> > @@ -1817,18 +1828,14 @@ static int intel_compute_min_cdclk(struct drm_atomic_state *state)
> > static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - int min_cdclk = intel_compute_min_cdclk(state);
> > - struct intel_atomic_state *intel_state =
> > - to_intel_atomic_state(state);
> > - int cdclk;
> > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > + int min_cdclk, cdclk;
> >
> > - cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> > + min_cdclk = intel_compute_min_cdclk(state);
> > + if (min_cdclk < 0)
> > + return min_cdclk;
> >
> > - if (cdclk > dev_priv->max_cdclk_freq) {
> > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > - cdclk, dev_priv->max_cdclk_freq);
> > - return -EINVAL;
> > - }
> > + cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
> >
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > @@ -1846,10 +1853,12 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> >
> > static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > - struct drm_i915_private *dev_priv = to_i915(state->dev);
> > struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > - int min_cdclk = intel_compute_min_cdclk(state);
> > - int cdclk;
> > + int min_cdclk, cdclk;
> > +
> > + min_cdclk = intel_compute_min_cdclk(state);
> > + if (min_cdclk < 0)
> > + return min_cdclk;
> >
> > /*
> > * FIXME should also account for plane ratio
> > @@ -1857,12 +1866,6 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> > */
> > cdclk = bdw_calc_cdclk(min_cdclk);
> >
> > - if (cdclk > dev_priv->max_cdclk_freq) {
> > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > - cdclk, dev_priv->max_cdclk_freq);
> > - return -EINVAL;
> > - }
> > -
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > if (!intel_state->active_crtcs) {
> > @@ -1879,10 +1882,13 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> >
> > static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - int min_cdclk = intel_compute_min_cdclk(state);
> > - int cdclk, vco;
> > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > + int min_cdclk, cdclk, vco;
> > +
> > + min_cdclk = intel_compute_min_cdclk(state);
> > + if (min_cdclk < 0)
> > + return min_cdclk;
> >
> > vco = intel_state->cdclk.logical.vco;
> > if (!vco)
> > @@ -1894,12 +1900,6 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > */
> > cdclk = skl_calc_cdclk(min_cdclk, vco);
> >
> > - if (cdclk > dev_priv->max_cdclk_freq) {
> > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > - cdclk, dev_priv->max_cdclk_freq);
> > - return -EINVAL;
> > - }
> > -
> > intel_state->cdclk.logical.vco = vco;
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > @@ -1919,10 +1919,12 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - int min_cdclk = intel_compute_min_cdclk(state);
> > - struct intel_atomic_state *intel_state =
> > - to_intel_atomic_state(state);
> > - int cdclk, vco;
> > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > + int min_cdclk, cdclk, vco;
> > +
> > + min_cdclk = intel_compute_min_cdclk(state);
> > + if (min_cdclk < 0)
> > + return min_cdclk;
> >
> > if (IS_GEMINILAKE(dev_priv)) {
> > cdclk = glk_calc_cdclk(min_cdclk);
> > @@ -1932,12 +1934,6 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > vco = bxt_de_pll_vco(dev_priv, cdclk);
> > }
> >
> > - if (cdclk > dev_priv->max_cdclk_freq) {
> > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > - cdclk, dev_priv->max_cdclk_freq);
> > - return -EINVAL;
> > - }
> > -
> > intel_state->cdclk.logical.vco = vco;
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > @@ -1963,20 +1959,16 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> > static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
> > {
> > struct drm_i915_private *dev_priv = to_i915(state->dev);
> > - struct intel_atomic_state *intel_state =
> > - to_intel_atomic_state(state);
> > - int min_cdclk = intel_compute_min_cdclk(state);
> > - int cdclk, vco;
> > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> > + int min_cdclk, cdclk, vco;
> > +
> > + min_cdclk = intel_compute_min_cdclk(state);
> > + if (min_cdclk < 0)
> > + return min_cdclk;
> >
> > cdclk = cnl_calc_cdclk(min_cdclk);
> > vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
> >
> > - if (cdclk > dev_priv->max_cdclk_freq) {
> > - DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> > - cdclk, dev_priv->max_cdclk_freq);
> > - return -EINVAL;
> > - }
> > -
> > intel_state->cdclk.logical.vco = vco;
> > intel_state->cdclk.logical.cdclk = cdclk;
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b47535f5d95d..1caf0ef82e36 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -15590,8 +15590,11 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> >
> > intel_crtc_compute_pixel_rate(crtc_state);
> >
> > - if (dev_priv->display.modeset_calc_cdclk)
> > + if (dev_priv->display.modeset_calc_cdclk) {
> > min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
> > + if (WARN_ON(min_cdclk < 0))
> > + min_cdclk = 0;
> > + }
> >
> > drm_calc_timestamping_constants(&crtc->base,
> > &crtc_state->base.adjusted_mode);
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
` (2 preceding siblings ...)
2017-07-11 9:47 ` [PATCH v3 1/2] " Dhinakaran Pandiyan
@ 2017-08-30 18:57 ` ville.syrjala
2017-08-31 18:48 ` Ville Syrjälä
2017-08-31 11:19 ` ✓ Fi.CI.BAT: success for series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2) Patchwork
2017-08-31 15:05 ` ✓ Fi.CI.IGT: " Patchwork
5 siblings, 1 reply; 16+ messages in thread
From: ville.syrjala @ 2017-08-30 18:57 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Paulo Zanoni, Rodrigo Vivi
From: Ville Syrjälä <ville.syrjala@linux.intel.com>
Make the min_pixclk thing less confusing by changing it to track
the minimum acceptable cdclk frequency instead. This means moving
the application of the guardbands to a slightly higher level from
the low level platform specific calc_cdclk() functions.
The immediate benefit is elimination of the confusing 2x factors
on GLK/CNL+ in the audio workarounds (which stems from the fact
that the pipes produce two pixels per clock).
v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
v3: Squash with the CNL cdclk limits patch (DK)
v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 12 ++-
drivers/gpu/drm/i915/intel_cdclk.c | 202 ++++++++++++++++++-----------------
drivers/gpu/drm/i915/intel_display.c | 21 ++--
drivers/gpu/drm/i915/intel_drv.h | 4 +-
4 files changed, 125 insertions(+), 114 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0383e879a315..7a20f58e711a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -569,6 +569,15 @@ struct i915_hotplug {
(__i)++) \
for_each_if (plane_state)
+#define for_each_new_intel_crtc_in_state(__state, crtc, new_crtc_state, __i) \
+ for ((__i) = 0; \
+ (__i) < (__state)->base.dev->mode_config.num_crtc && \
+ ((crtc) = to_intel_crtc((__state)->base.crtcs[__i].ptr), \
+ (new_crtc_state) = to_intel_crtc_state((__state)->base.crtcs[__i].new_state), 1); \
+ (__i)++) \
+ for_each_if (crtc)
+
+
struct drm_i915_private;
struct i915_mm_struct;
struct i915_mmu_object;
@@ -2335,7 +2344,8 @@ struct drm_i915_private {
struct mutex dpll_lock;
unsigned int active_crtcs;
- unsigned int min_pixclk[I915_MAX_PIPES];
+ /* minimum acceptable cdclk for each pipe */
+ int min_cdclk[I915_MAX_PIPES];
int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
index 1241e5891b29..fafffb04b447 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -417,24 +417,21 @@ static void hsw_get_cdclk(struct drm_i915_private *dev_priv,
cdclk_state->cdclk = 540000;
}
-static int vlv_calc_cdclk(struct drm_i915_private *dev_priv,
- int max_pixclk)
+static int vlv_calc_cdclk(struct drm_i915_private *dev_priv, int min_cdclk)
{
int freq_320 = (dev_priv->hpll_freq << 1) % 320000 != 0 ?
333333 : 320000;
- int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
/*
* We seem to get an unstable or solid color picture at 200MHz.
* Not sure what's wrong. For now use 200MHz only when all pipes
* are off.
*/
- if (!IS_CHERRYVIEW(dev_priv) &&
- max_pixclk > freq_320*limit/100)
+ if (IS_VALLEYVIEW(dev_priv) && min_cdclk > freq_320)
return 400000;
- else if (max_pixclk > 266667*limit/100)
+ else if (min_cdclk > 266667)
return freq_320;
- else if (max_pixclk > 0)
+ else if (min_cdclk > 0)
return 266667;
else
return 200000;
@@ -612,13 +609,13 @@ static void chv_set_cdclk(struct drm_i915_private *dev_priv,
intel_display_power_put(dev_priv, POWER_DOMAIN_PIPE_A);
}
-static int bdw_calc_cdclk(int max_pixclk)
+static int bdw_calc_cdclk(int min_cdclk)
{
- if (max_pixclk > 540000)
+ if (min_cdclk > 540000)
return 675000;
- else if (max_pixclk > 450000)
+ else if (min_cdclk > 450000)
return 540000;
- else if (max_pixclk > 337500)
+ else if (min_cdclk > 337500)
return 450000;
else
return 337500;
@@ -724,23 +721,23 @@ static void bdw_set_cdclk(struct drm_i915_private *dev_priv,
cdclk, dev_priv->cdclk.hw.cdclk);
}
-static int skl_calc_cdclk(int max_pixclk, int vco)
+static int skl_calc_cdclk(int min_cdclk, int vco)
{
if (vco == 8640000) {
- if (max_pixclk > 540000)
+ if (min_cdclk > 540000)
return 617143;
- else if (max_pixclk > 432000)
+ else if (min_cdclk > 432000)
return 540000;
- else if (max_pixclk > 308571)
+ else if (min_cdclk > 308571)
return 432000;
else
return 308571;
} else {
- if (max_pixclk > 540000)
+ if (min_cdclk > 540000)
return 675000;
- else if (max_pixclk > 450000)
+ else if (min_cdclk > 450000)
return 540000;
- else if (max_pixclk > 337500)
+ else if (min_cdclk > 337500)
return 450000;
else
return 337500;
@@ -1075,31 +1072,25 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)
skl_set_cdclk(dev_priv, &cdclk_state);
}
-static int bxt_calc_cdclk(int max_pixclk)
+static int bxt_calc_cdclk(int min_cdclk)
{
- if (max_pixclk > 576000)
+ if (min_cdclk > 576000)
return 624000;
- else if (max_pixclk > 384000)
+ else if (min_cdclk > 384000)
return 576000;
- else if (max_pixclk > 288000)
+ else if (min_cdclk > 288000)
return 384000;
- else if (max_pixclk > 144000)
+ else if (min_cdclk > 144000)
return 288000;
else
return 144000;
}
-static int glk_calc_cdclk(int max_pixclk)
+static int glk_calc_cdclk(int min_cdclk)
{
- /*
- * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
- * as a temporary workaround. Use a higher cdclk instead. (Note that
- * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
- * cdclk.)
- */
- if (max_pixclk > DIV_ROUND_UP(2 * 158400 * 99, 100))
+ if (min_cdclk > 158400)
return 316800;
- else if (max_pixclk > DIV_ROUND_UP(2 * 79200 * 99, 100))
+ else if (min_cdclk > 79200)
return 158400;
else
return 79200;
@@ -1420,11 +1411,11 @@ void bxt_uninit_cdclk(struct drm_i915_private *dev_priv)
bxt_set_cdclk(dev_priv, &cdclk_state);
}
-static int cnl_calc_cdclk(int max_pixclk)
+static int cnl_calc_cdclk(int min_cdclk)
{
- if (max_pixclk > 336000)
+ if (min_cdclk > 336000)
return 528000;
- else if (max_pixclk > 168000)
+ else if (min_cdclk > 168000)
return 336000;
else
return 168000;
@@ -1732,98 +1723,106 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
dev_priv->display.set_cdclk(dev_priv, cdclk_state);
}
-static int bdw_adjust_min_pipe_pixel_rate(struct intel_crtc_state *crtc_state,
- int pixel_rate)
+static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
+ int pixel_rate)
+{
+ if (INTEL_GEN(dev_priv) >= 10)
+ /*
+ * FIXME: Switch to DIV_ROUND_UP(pixel_rate, 2)
+ * once DDI clock voltage requirements are
+ * handled correctly.
+ */
+ return pixel_rate;
+ else if (IS_GEMINILAKE(dev_priv))
+ /*
+ * FIXME: Avoid using a pixel clock that is more than 99% of the cdclk
+ * as a temporary workaround. Use a higher cdclk instead. (Note that
+ * intel_compute_max_dotclk() limits the max pixel clock to 99% of max
+ * cdclk.)
+ */
+ return DIV_ROUND_UP(pixel_rate * 100, 2 * 99);
+ else if (IS_GEN9(dev_priv) ||
+ IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
+ return pixel_rate;
+ else if (IS_CHERRYVIEW(dev_priv))
+ return DIV_ROUND_UP(pixel_rate * 100, 95);
+ else
+ return DIV_ROUND_UP(pixel_rate * 100, 90);
+}
+
+int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state)
{
struct drm_i915_private *dev_priv =
to_i915(crtc_state->base.crtc->dev);
+ int min_cdclk;
+
+ if (!crtc_state->base.enable)
+ return 0;
+
+ min_cdclk = intel_pixel_rate_to_cdclk(dev_priv, crtc_state->pixel_rate);
/* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
- pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
+ min_cdclk = DIV_ROUND_UP(min_cdclk * 100, 95);
/* BSpec says "Do not use DisplayPort with CDCLK less than 432 MHz,
* audio enabled, port width x4, and link rate HBR2 (5.4 GHz), or else
* there may be audio corruption or screen corruption." This cdclk
- * restriction for GLK is 316.8 MHz and since GLK can output two
- * pixels per clock, the pixel rate becomes 2 * 316.8 MHz.
+ * restriction for GLK is 316.8 MHz.
*/
if (intel_crtc_has_dp_encoder(crtc_state) &&
crtc_state->has_audio &&
crtc_state->port_clock >= 540000 &&
crtc_state->lane_count == 4) {
- if (IS_CANNONLAKE(dev_priv))
- pixel_rate = max(316800, pixel_rate);
- else if (IS_GEMINILAKE(dev_priv))
- pixel_rate = max(2 * 316800, pixel_rate);
- else
- pixel_rate = max(432000, pixel_rate);
+ if (IS_CANNONLAKE(dev_priv) || IS_GEMINILAKE(dev_priv)) {
+ /* Display WA #1145: glk,cnl */
+ min_cdclk = max(316800, min_cdclk);
+ } else if (IS_GEN9(dev_priv) || IS_BROADWELL(dev_priv)) {
+ /* Display WA #1144: skl,bxt */
+ min_cdclk = max(432000, min_cdclk);
+ }
}
/* According to BSpec, "The CD clock frequency must be at least twice
* the frequency of the Azalia BCLK." and BCLK is 96 MHz by default.
- * The check for GLK has to be adjusted as the platform can output
- * two pixels per clock.
*/
- if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9) {
- if (IS_GEMINILAKE(dev_priv))
- pixel_rate = max(2 * 2 * 96000, pixel_rate);
- else
- pixel_rate = max(2 * 96000, pixel_rate);
- }
+ if (crtc_state->has_audio && INTEL_GEN(dev_priv) >= 9)
+ min_cdclk = max(2 * 96000, min_cdclk);
- return pixel_rate;
+ return min_cdclk;
}
-/* compute the max rate for new configuration */
-static int intel_max_pixel_rate(struct drm_atomic_state *state)
+static int intel_compute_min_cdclk(struct drm_atomic_state *state)
{
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(state->dev);
- struct drm_crtc *crtc;
- struct drm_crtc_state *cstate;
+ struct intel_crtc *crtc;
struct intel_crtc_state *crtc_state;
- unsigned int max_pixel_rate = 0, i;
+ int min_cdclk = 0, i;
enum pipe pipe;
- memcpy(intel_state->min_pixclk, dev_priv->min_pixclk,
- sizeof(intel_state->min_pixclk));
-
- for_each_new_crtc_in_state(state, crtc, cstate, i) {
- int pixel_rate;
-
- crtc_state = to_intel_crtc_state(cstate);
- if (!crtc_state->base.enable) {
- intel_state->min_pixclk[i] = 0;
- continue;
- }
+ memcpy(intel_state->min_cdclk, dev_priv->min_cdclk,
+ sizeof(intel_state->min_cdclk));
- pixel_rate = crtc_state->pixel_rate;
-
- if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9)
- pixel_rate =
- bdw_adjust_min_pipe_pixel_rate(crtc_state,
- pixel_rate);
-
- intel_state->min_pixclk[i] = pixel_rate;
- }
+ for_each_new_intel_crtc_in_state(intel_state, crtc, crtc_state, i)
+ intel_state->min_cdclk[i] =
+ intel_crtc_compute_min_cdclk(crtc_state);
for_each_pipe(dev_priv, pipe)
- max_pixel_rate = max(intel_state->min_pixclk[pipe],
- max_pixel_rate);
+ min_cdclk = max(intel_state->min_cdclk[pipe], min_cdclk);
- return max_pixel_rate;
+ return min_cdclk;
}
static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
int cdclk;
- cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
+ cdclk = vlv_calc_cdclk(dev_priv, min_cdclk);
if (cdclk > dev_priv->max_cdclk_freq) {
DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1849,14 +1848,14 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
int cdclk;
/*
* FIXME should also account for plane ratio
* once 64bpp pixel formats are supported.
*/
- cdclk = bdw_calc_cdclk(max_pixclk);
+ cdclk = bdw_calc_cdclk(min_cdclk);
if (cdclk > dev_priv->max_cdclk_freq) {
DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1882,7 +1881,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
struct drm_i915_private *dev_priv = to_i915(state->dev);
- const int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
int cdclk, vco;
vco = intel_state->cdclk.logical.vco;
@@ -1893,7 +1892,7 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
* FIXME should also account for plane ratio
* once 64bpp pixel formats are supported.
*/
- cdclk = skl_calc_cdclk(max_pixclk, vco);
+ cdclk = skl_calc_cdclk(min_cdclk, vco);
if (cdclk > dev_priv->max_cdclk_freq) {
DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
@@ -1920,16 +1919,16 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
{
struct drm_i915_private *dev_priv = to_i915(state->dev);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
int cdclk, vco;
if (IS_GEMINILAKE(dev_priv)) {
- cdclk = glk_calc_cdclk(max_pixclk);
+ cdclk = glk_calc_cdclk(min_cdclk);
vco = glk_de_pll_vco(dev_priv, cdclk);
} else {
- cdclk = bxt_calc_cdclk(max_pixclk);
+ cdclk = bxt_calc_cdclk(min_cdclk);
vco = bxt_de_pll_vco(dev_priv, cdclk);
}
@@ -1966,10 +1965,10 @@ static int cnl_modeset_calc_cdclk(struct drm_atomic_state *state)
struct drm_i915_private *dev_priv = to_i915(state->dev);
struct intel_atomic_state *intel_state =
to_intel_atomic_state(state);
- int max_pixclk = intel_max_pixel_rate(state);
+ int min_cdclk = intel_compute_min_cdclk(state);
int cdclk, vco;
- cdclk = cnl_calc_cdclk(max_pixclk);
+ cdclk = cnl_calc_cdclk(min_cdclk);
vco = cnl_cdclk_pll_vco(dev_priv, cdclk);
if (cdclk > dev_priv->max_cdclk_freq) {
@@ -1999,14 +1998,21 @@ static int intel_compute_max_dotclk(struct drm_i915_private *dev_priv)
{
int max_cdclk_freq = dev_priv->max_cdclk_freq;
- if (IS_GEMINILAKE(dev_priv))
+ if (INTEL_GEN(dev_priv) >= 10)
+ /*
+ * FIXME: Allow '2 * max_cdclk_freq'
+ * once DDI clock voltage requirements are
+ * handled correctly.
+ */
+ return max_cdclk_freq;
+ else if (IS_GEMINILAKE(dev_priv))
/*
* FIXME: Limiting to 99% as a temporary workaround. See
- * glk_calc_cdclk() for details.
+ * intel_min_cdclk() for details.
*/
return 2 * max_cdclk_freq * 99 / 100;
- else if (INTEL_INFO(dev_priv)->gen >= 9 ||
- IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
+ else if (IS_GEN9(dev_priv) ||
+ IS_BROADWELL(dev_priv) || IS_HASWELL(dev_priv))
return max_cdclk_freq;
else if (IS_CHERRYVIEW(dev_priv))
return max_cdclk_freq*95/100;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5683973bba58..d053751068a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6038,7 +6038,7 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc,
intel_crtc->enabled_power_domains = 0;
dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe);
- dev_priv->min_pixclk[intel_crtc->pipe] = 0;
+ dev_priv->min_cdclk[intel_crtc->pipe] = 0;
}
/*
@@ -12631,8 +12631,8 @@ static int intel_atomic_commit(struct drm_device *dev,
intel_atomic_track_fbs(state);
if (intel_state->modeset) {
- memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
- sizeof(intel_state->min_pixclk));
+ memcpy(dev_priv->min_cdclk, intel_state->min_cdclk,
+ sizeof(intel_state->min_cdclk));
dev_priv->active_crtcs = intel_state->active_crtcs;
dev_priv->cdclk.logical = intel_state->cdclk.logical;
dev_priv->cdclk.actual = intel_state->cdclk.actual;
@@ -15097,7 +15097,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
for_each_intel_crtc(dev, crtc) {
struct intel_crtc_state *crtc_state =
to_intel_crtc_state(crtc->base.state);
- int pixclk = 0;
+ int min_cdclk = 0;
memset(&crtc->base.mode, 0, sizeof(crtc->base.mode));
if (crtc_state->base.active) {
@@ -15118,22 +15118,15 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
intel_crtc_compute_pixel_rate(crtc_state);
- if (INTEL_GEN(dev_priv) >= 9 || IS_BROADWELL(dev_priv) ||
- IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
- pixclk = crtc_state->pixel_rate;
- else
- WARN_ON(dev_priv->display.modeset_calc_cdclk);
-
- /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
- if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
- pixclk = DIV_ROUND_UP(pixclk * 100, 95);
+ if (dev_priv->display.modeset_calc_cdclk)
+ min_cdclk = intel_crtc_compute_min_cdclk(crtc_state);
drm_calc_timestamping_constants(&crtc->base,
&crtc_state->base.adjusted_mode);
update_scanline_offset(crtc);
}
- dev_priv->min_pixclk[crtc->pipe] = pixclk;
+ dev_priv->min_cdclk[crtc->pipe] = min_cdclk;
intel_pipe_config_sanity_check(dev_priv, crtc_state);
}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 17649f13091c..62f783cd248b 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -384,7 +384,8 @@ struct intel_atomic_state {
unsigned int active_pipe_changes;
unsigned int active_crtcs;
- unsigned int min_pixclk[I915_MAX_PIPES];
+ /* minimum acceptable cdclk for each pipe */
+ int min_cdclk[I915_MAX_PIPES];
struct intel_shared_dpll_state shared_dpll[I915_NUM_PLLS];
@@ -1289,6 +1290,7 @@ void intel_audio_init(struct drm_i915_private *dev_priv);
void intel_audio_deinit(struct drm_i915_private *dev_priv);
/* intel_cdclk.c */
+int intel_crtc_compute_min_cdclk(const struct intel_crtc_state *crtc_state);
void skl_init_cdclk(struct drm_i915_private *dev_priv);
void skl_uninit_cdclk(struct drm_i915_private *dev_priv);
void cnl_init_cdclk(struct drm_i915_private *dev_priv);
--
2.13.5
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 16+ messages in thread
* ✓ Fi.CI.BAT: success for series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2)
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
` (3 preceding siblings ...)
2017-08-30 18:57 ` [PATCH v4 " ville.syrjala
@ 2017-08-31 11:19 ` Patchwork
2017-08-31 15:05 ` ✓ Fi.CI.IGT: " Patchwork
5 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-08-31 11:19 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2)
URL : https://patchwork.freedesktop.org/series/27078/
State : success
== Summary ==
Series 27078v2 series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
https://patchwork.freedesktop.org/api/1.0/series/27078/revisions/2/mbox/
Test kms_cursor_legacy:
Subgroup basic-flip-after-cursor-varying-size:
pass -> FAIL (fi-hsw-4770) fdo#102402 +1
fdo#102402 https://bugs.freedesktop.org/show_bug.cgi?id=102402
fi-bdw-5557u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:464s
fi-bdw-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:441s
fi-blb-e6850 total:288 pass:224 dwarn:1 dfail:0 fail:0 skip:63 time:360s
fi-bsw-n3050 total:288 pass:243 dwarn:0 dfail:0 fail:0 skip:45 time:564s
fi-bwr-2160 total:288 pass:184 dwarn:0 dfail:0 fail:0 skip:104 time:255s
fi-bxt-j4205 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:526s
fi-byt-j1900 total:288 pass:254 dwarn:1 dfail:0 fail:0 skip:33 time:533s
fi-byt-n2820 total:288 pass:250 dwarn:1 dfail:0 fail:0 skip:37 time:523s
fi-elk-e7500 total:288 pass:230 dwarn:0 dfail:0 fail:0 skip:58 time:438s
fi-glk-2a total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:612s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:2 skip:25 time:464s
fi-hsw-4770r total:288 pass:263 dwarn:0 dfail:0 fail:0 skip:25 time:433s
fi-ilk-650 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:426s
fi-ivb-3520m total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:505s
fi-ivb-3770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:478s
fi-kbl-7500u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:483s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:594s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:595s
fi-pnv-d510 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:532s
fi-skl-6260u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:470s
fi-skl-6770hq total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:492s
fi-skl-gvtdvm total:288 pass:266 dwarn:0 dfail:0 fail:0 skip:22 time:439s
fi-skl-x1585l total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:485s
fi-snb-2520m total:288 pass:251 dwarn:0 dfail:0 fail:0 skip:37 time:551s
fi-snb-2600 total:288 pass:249 dwarn:0 dfail:0 fail:1 skip:38 time:409s
fi-skl-6700k failed to connect after reboot
c399d43adc55a49d028d24ce7cdacc1823a4f159 drm-tip: 2017y-08m-31d-07h-25m-28s UTC integration manifest
a0e6c5492430 drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk()
0a10b9ef9742 drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5544/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* ✓ Fi.CI.IGT: success for series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2)
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
` (4 preceding siblings ...)
2017-08-31 11:19 ` ✓ Fi.CI.BAT: success for series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2) Patchwork
@ 2017-08-31 15:05 ` Patchwork
5 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2017-08-31 15:05 UTC (permalink / raw)
To: Ville Syrjälä; +Cc: intel-gfx
== Series Details ==
Series: series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2)
URL : https://patchwork.freedesktop.org/series/27078/
State : success
== Summary ==
Test kms_setmode:
Subgroup basic:
pass -> FAIL (shard-hsw) fdo#99912
Test kms_flip:
Subgroup plain-flip-fb-recreate:
fail -> PASS (shard-hsw)
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912
shard-hsw total:2265 pass:1231 dwarn:0 dfail:0 fail:18 skip:1016 time:9699s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5544/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-08-30 18:57 ` [PATCH v4 " ville.syrjala
@ 2017-08-31 18:48 ` Ville Syrjälä
2017-09-04 10:39 ` Maarten Lankhorst
0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2017-08-31 18:48 UTC (permalink / raw)
To: intel-gfx; +Cc: Dhinakaran Pandiyan, Paulo Zanoni, Rodrigo Vivi
On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Make the min_pixclk thing less confusing by changing it to track
> the minimum acceptable cdclk frequency instead. This means moving
> the application of the guardbands to a slightly higher level from
> the low level platform specific calc_cdclk() functions.
>
> The immediate benefit is elimination of the confusing 2x factors
> on GLK/CNL+ in the audio workarounds (which stems from the fact
> that the pipes produce two pixels per clock).
>
> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
> v3: Squash with the CNL cdclk limits patch (DK)
> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
I didn't get any objections from the CNL camp, so I went ahead and
pushed the series. Thanks for the reviews.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-08-31 18:48 ` Ville Syrjälä
@ 2017-09-04 10:39 ` Maarten Lankhorst
2017-09-04 15:58 ` Ville Syrjälä
0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 10:39 UTC (permalink / raw)
To: Ville Syrjälä, intel-gfx
Cc: Dhinakaran Pandiyan, Paulo Zanoni, Rodrigo Vivi
Op 31-08-17 om 20:48 schreef Ville Syrjälä:
> On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrjala@linux.intel.com wrote:
>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>
>> Make the min_pixclk thing less confusing by changing it to track
>> the minimum acceptable cdclk frequency instead. This means moving
>> the application of the guardbands to a slightly higher level from
>> the low level platform specific calc_cdclk() functions.
>>
>> The immediate benefit is elimination of the confusing 2x factors
>> on GLK/CNL+ in the audio workarounds (which stems from the fact
>> that the pipes produce two pixels per clock).
>>
>> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
>> v3: Squash with the CNL cdclk limits patch (DK)
>> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
>>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> I didn't get any objections from the CNL camp, so I went ahead and
> pushed the series. Thanks for the reviews.
>
I seem to have a WARN_ON during init now on my broadwell, likely related to this series?
[ 13.105310] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
[ 13.132264] systemd-journald[159]: Successfully sent stream file descriptor to service manager.
[ 13.161016] WARN_ON(min_cdclk < 0)
[ 13.161078] ------------[ cut here ]------------
[ 13.161336] WARNING: CPU: 1 PID: 209 at drivers/gpu/drm/i915/intel_display.c:15070 intel_modeset_setup_hw_state+0x15a4/0x3000 [i915]
[ 13.161517] Modules linked in: snd_seq_device snd_timer drbg i915(+) cfg80211 ecdh_generic(+) prime_numbers drm_kms_helper snd syscopyarea sysfillrect sysimgblt fb_sys_fops drm soundcore fan thermal i2c_designware_platform i2c_designware_core acpi_pad parport_pc ppdev parport autofs4
[ 13.161822] CPU: 1 PID: 209 Comm: systemd-udevd Tainted: G U 4.13.0-rc7-patser+ #5236
[ 13.161884] Hardware name: NUC5i7RYB, BIOS RYBDWi35.86A.0246.2015.0309.1355 03/09/2015
[ 13.161963] task: ffff8800cd054500 task.stack: ffff8800c6840000
[ 13.162083] RIP: 0010:intel_modeset_setup_hw_state+0x15a4/0x3000 [i915]
[ 13.162393] RSP: 0018:ffff8800c68472a0 EFLAGS: 00010282
[ 13.162434] RAX: 0000000000000016 RBX: ffff8800c65d4c80 RCX: ffff8800cd054cd8
[ 13.162478] RDX: 0000000000000000 RSI: ffff8800cd054d78 RDI: ffff8800cd054cd4
[ 13.162521] RBP: ffff8800c68473e0 R08: 0000000000000000 R09: 0000000000000000
[ 13.162565] R10: ffff8800c65d4e57 R11: 0000000000000000 R12: dffffc0000000000
[ 13.162611] R13: ffff8800c8190000 R14: ffff8800c65d6f88 R15: ffff8800c65d6e80
[ 13.162654] FS: 00007fc3599c08c0(0000) GS:ffff8800d4e80000(0000) knlGS:0000000000000000
[ 13.162710] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 13.162750] CR2: 00007ffd657bffb8 CR3: 00000000cb477000 CR4: 00000000003406e0
[ 13.162794] Call Trace:
[ 13.162911] ? intel_mode_from_pipe_config+0x560/0x560 [i915]
[ 13.162975] ? drm_modeset_lock+0x162/0x270 [drm]
[ 13.163036] ? drm_modeset_lock_all_ctx+0xf3/0x140 [drm]
[ 13.163192] intel_modeset_init+0x327b/0x4720 [i915]
[ 13.163316] ? intel_modeset_init_hw+0x160/0x160 [i915]
[ 13.163431] i915_driver_load+0x2c50/0x3300 [i915]
[ 13.163473] ? find_held_lock+0x36/0x1c0
[ 13.163579] ? __i915_printk+0x280/0x280 [i915]
[ 13.163722] ? wait_for_completion_killable_timeout+0x430/0x430
[ 13.163775] ? mutex_unlock+0xd/0x10
[ 13.163809] ? acpi_dev_found+0xa7/0xb0
[ 13.163910] i915_pci_probe+0x108/0x180 [i915]
[ 13.164011] ? i915_pci_remove+0x50/0x50 [i915]
[ 13.164080] local_pci_probe+0xe8/0x160
[ 13.164120] pci_device_probe+0x3fe/0x580
[ 13.164190] ? pci_device_remove+0x1b0/0x1b0
[ 13.164230] ? _raw_spin_unlock+0x2c/0x40
[ 13.164271] driver_probe_device+0x2fb/0x670
[ 13.164313] ? driver_probe_device+0x670/0x670
[ 13.164352] __driver_attach+0xff/0x140
[ 13.164388] bus_for_each_dev+0x11b/0x1b0
[ 13.164675] ? store_drivers_autoprobe+0x120/0x120
[ 13.164719] ? _raw_spin_unlock+0x2c/0x40
[ 13.164759] driver_attach+0x45/0x50
[ 13.164791] bus_add_driver+0x2a2/0x520
[ 13.164832] driver_register+0x256/0x310
[ 13.164865] ? __raw_spin_lock_init+0x2d/0xf0
[ 13.164905] __pci_register_driver+0x192/0x1a0
[ 13.165013] i915_init+0xc8/0xd5 [i915]
[ 13.165081] ? 0xffffffffc09e0000
[ 13.165114] do_one_initcall+0x121/0x204
[ 13.165206] ? initcall_blacklisted+0x160/0x160
[ 13.165245] ? kasan_unpoison_shadow+0x35/0x50
[ 13.165282] ? kasan_kmalloc+0xb6/0xd0
[ 13.165317] ? kasan_unpoison_shadow+0x35/0x50
[ 13.165355] ? __asan_register_globals+0x7c/0xa0
[ 13.165399] do_init_module+0x1b6/0x500
[ 13.165440] load_module+0x6f4b/0x85e0
[ 13.165501] ? module_frob_arch_sections+0x20/0x20
[ 13.165554] ? open_exec+0x40/0x40
[ 13.165601] SYSC_finit_module+0x110/0x180
[ 13.165635] ? SYSC_finit_module+0x110/0x180
[ 13.165672] ? SYSC_init_module+0x1e0/0x1e0
[ 13.165712] ? __secure_computing+0x204/0x220
[ 13.165751] ? syscall_trace_enter+0x531/0xcc0
[ 13.165800] ? do_syscall_64+0x47/0x350
[ 13.165832] ? SyS_init_module+0x10/0x10
[ 13.165864] SyS_finit_module+0x9/0x10
[ 13.165894] do_syscall_64+0x20a/0x350
[ 13.165930] entry_SYSCALL64_slow_path+0x25/0x25
[ 13.165964] RIP: 0033:0x7fc358845949
[ 13.165993] RSP: 002b:00007ffda5e44e08 EFLAGS: 00000246 ORIG_RAX: 0000000000000139
[ 13.166170] RAX: ffffffffffffffda RBX: 000055ab74c53830 RCX: 00007fc358845949
[ 13.166303] RDX: 0000000000000000 RSI: 000055ab74c53400 RDI: 0000000000000015
[ 13.166349] RBP: 000055ab74c53400 R08: 0000000000000000 R09: 000000000000002d
[ 13.166577] R10: 0000000000000015 R11: 0000000000000246 R12: 0000000000000000
[ 13.166781] R13: 000055ab74c888e0 R14: 0000000000020000 R15: 0000000000000000
[ 13.166841] Code: 31 c0 49 83 bd e8 54 00 00 00 74 23 48 89 df e8 d3 12 fb ff 85 c0 79 17 48 c7 c6 20 1a 8c c0 48 c7 c7 60 a9 8b c0 e8 b7 d9 b1 ef <0f> ff 31 c0 48 8b b5 08 ff ff ff 4c 89 ff 89 85 20 ff ff ff e8
[ 13.167257] ---[ end trace 8bc55e2833d4ddb0 ]---
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-09-04 10:39 ` Maarten Lankhorst
@ 2017-09-04 15:58 ` Ville Syrjälä
2017-09-04 18:51 ` Maarten Lankhorst
0 siblings, 1 reply; 16+ messages in thread
From: Ville Syrjälä @ 2017-09-04 15:58 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Dhinakaran Pandiyan, intel-gfx, Paulo Zanoni, Rodrigo Vivi
On Mon, Sep 04, 2017 at 12:39:25PM +0200, Maarten Lankhorst wrote:
> Op 31-08-17 om 20:48 schreef Ville Syrjälä:
> > On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrjala@linux.intel.com wrote:
> >> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>
> >> Make the min_pixclk thing less confusing by changing it to track
> >> the minimum acceptable cdclk frequency instead. This means moving
> >> the application of the guardbands to a slightly higher level from
> >> the low level platform specific calc_cdclk() functions.
> >>
> >> The immediate benefit is elimination of the confusing 2x factors
> >> on GLK/CNL+ in the audio workarounds (which stems from the fact
> >> that the pipes produce two pixels per clock).
> >>
> >> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
> >> v3: Squash with the CNL cdclk limits patch (DK)
> >> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
> >>
> >> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > I didn't get any objections from the CNL camp, so I went ahead and
> > pushed the series. Thanks for the reviews.
> >
> I seem to have a WARN_ON during init now on my broadwell, likely related to this series?
>
> [ 13.105310] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
> [ 13.132264] systemd-journald[159]: Successfully sent stream file descriptor to service manager.
> [ 13.161016] WARN_ON(min_cdclk < 0)
Hmm. I think I need to see the full dmesg to figure this one out.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-09-04 15:58 ` Ville Syrjälä
@ 2017-09-04 18:51 ` Maarten Lankhorst
2017-09-05 12:58 ` Ville Syrjälä
0 siblings, 1 reply; 16+ messages in thread
From: Maarten Lankhorst @ 2017-09-04 18:51 UTC (permalink / raw)
To: Ville Syrjälä
Cc: Dhinakaran Pandiyan, intel-gfx, Paulo Zanoni, Rodrigo Vivi
Op 04-09-17 om 17:58 schreef Ville Syrjälä:
> On Mon, Sep 04, 2017 at 12:39:25PM +0200, Maarten Lankhorst wrote:
>> Op 31-08-17 om 20:48 schreef Ville Syrjälä:
>>> On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrjala@linux.intel.com wrote:
>>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>>
>>>> Make the min_pixclk thing less confusing by changing it to track
>>>> the minimum acceptable cdclk frequency instead. This means moving
>>>> the application of the guardbands to a slightly higher level from
>>>> the low level platform specific calc_cdclk() functions.
>>>>
>>>> The immediate benefit is elimination of the confusing 2x factors
>>>> on GLK/CNL+ in the audio workarounds (which stems from the fact
>>>> that the pipes produce two pixels per clock).
>>>>
>>>> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
>>>> v3: Squash with the CNL cdclk limits patch (DK)
>>>> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
>>>>
>>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> I didn't get any objections from the CNL camp, so I went ahead and
>>> pushed the series. Thanks for the reviews.
>>>
>> I seem to have a WARN_ON during init now on my broadwell, likely related to this series?
>>
>> [ 13.105310] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
>> [ 13.132264] systemd-journald[159]: Successfully sent stream file descriptor to service manager.
>> [ 13.161016] WARN_ON(min_cdclk < 0)
> Hmm. I think I need to see the full dmesg to figure this one out.
>
[ 188.354997] [drm:intel_modeset_init [i915]] 3 display pipes available.
[ 188.356575] [drm:intel_update_cdclk [i915]] Current CD clock rate: 540000 kHz, VCO: 0 kHz, ref: 0 kHz
[ 188.357067] [drm:intel_update_max_cdclk [i915]] Max CD clock rate: 540000 kHz
[ 188.357220] [drm:intel_update_max_cdclk [i915]] Max dotclock rate: 540000 kHz
... <snip some hw radout spam>
[ 188.363648] [drm:drm_atomic_set_mode_for_crtc [drm]] Set [MODE:640x480] for CRTC state ffff8800c79e2200
[ 188.363784] [drm:intel_crtc_compute_min_cdclk [i915]] required cdclk (607500 kHz) exceeds max (540000 kHz)
[ 188.363848] WARN_ON(min_cdclk < 0)
...
[ 188.368005] [drm:intel_dump_pipe_config [i915]] [CRTC:36:pipe A][setup_hw_state]
[ 188.368155] [drm:intel_dump_pipe_config [i915]] cpu_transcoder: A, pipe bpp: 24, dithering: 0
[ 188.368276] [drm:intel_dump_pipe_config [i915]] audio: 0, infoframes: 0
[ 188.368390] [drm:intel_dump_pipe_config [i915]] requested mode:
[ 188.368692] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"640x480" 1286 540000 640 656 752 800 480 490 492 525 0x40 0xa
[ 188.368833] [drm:intel_dump_pipe_config [i915]] adjusted mode:
[ 188.368897] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"640x480" 1286 540000 640 656 752 800 480 490 492 525 0x40 0xa
[ 188.369041] [drm:intel_dump_pipe_config [i915]] crtc timings: 540000 640 656 752 800 480 490 492 525, type: 0x40 flags: 0xa
[ 188.369235] [drm:intel_dump_pipe_config [i915]] port clock: 540000, pipe src size: 720x400, pixel rate 607500
[ 188.369366] [drm:intel_dump_pipe_config [i915]] pch pfit: pos: 0x00000000, size: 0x028001e0, enabled
[ 188.369490] [drm:intel_dump_pipe_config [i915]] ips: 0, double wide: 0
[ 188.369600] [drm:hsw_dump_hw_state [i915]] dpll_hw_state: wrpll: 0x0 spll: 0x0
[ 188.369719] [drm:intel_dump_pipe_config [i915]] planes on this crtc
Is this enough info?
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v4 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock"
2017-09-04 18:51 ` Maarten Lankhorst
@ 2017-09-05 12:58 ` Ville Syrjälä
0 siblings, 0 replies; 16+ messages in thread
From: Ville Syrjälä @ 2017-09-05 12:58 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: Dhinakaran Pandiyan, intel-gfx, Paulo Zanoni, Rodrigo Vivi
On Mon, Sep 04, 2017 at 08:51:16PM +0200, Maarten Lankhorst wrote:
> Op 04-09-17 om 17:58 schreef Ville Syrjälä:
> > On Mon, Sep 04, 2017 at 12:39:25PM +0200, Maarten Lankhorst wrote:
> >> Op 31-08-17 om 20:48 schreef Ville Syrjälä:
> >>> On Wed, Aug 30, 2017 at 09:57:03PM +0300, ville.syrjala@linux.intel.com wrote:
> >>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>>>
> >>>> Make the min_pixclk thing less confusing by changing it to track
> >>>> the minimum acceptable cdclk frequency instead. This means moving
> >>>> the application of the guardbands to a slightly higher level from
> >>>> the low level platform specific calc_cdclk() functions.
> >>>>
> >>>> The immediate benefit is elimination of the confusing 2x factors
> >>>> on GLK/CNL+ in the audio workarounds (which stems from the fact
> >>>> that the pipes produce two pixels per clock).
> >>>>
> >>>> v2: Keep cdclk higher on CNL to workaround missing DDI clock voltage handling
> >>>> v3: Squash with the CNL cdclk limits patch (DK)
> >>>> v4: s/intel_min_cdclk/intel_pixel_rate_to_cdclk/ (DK)
> >>>>
> >>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>>> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> I didn't get any objections from the CNL camp, so I went ahead and
> >>> pushed the series. Thanks for the reviews.
> >>>
> >> I seem to have a WARN_ON during init now on my broadwell, likely related to this series?
> >>
> >> [ 13.105310] i915 0000:00:02.0: vgaarb: changed VGA decodes: olddecodes=io+mem,decodes=io+mem:owns=io+mem
> >> [ 13.132264] systemd-journald[159]: Successfully sent stream file descriptor to service manager.
> >> [ 13.161016] WARN_ON(min_cdclk < 0)
> > Hmm. I think I need to see the full dmesg to figure this one out.
> >
> [ 188.354997] [drm:intel_modeset_init [i915]] 3 display pipes available.
> [ 188.356575] [drm:intel_update_cdclk [i915]] Current CD clock rate: 540000 kHz, VCO: 0 kHz, ref: 0 kHz
> [ 188.357067] [drm:intel_update_max_cdclk [i915]] Max CD clock rate: 540000 kHz
> [ 188.357220] [drm:intel_update_max_cdclk [i915]] Max dotclock rate: 540000 kHz
> ... <snip some hw radout spam>
> [ 188.363648] [drm:drm_atomic_set_mode_for_crtc [drm]] Set [MODE:640x480] for CRTC state ffff8800c79e2200
> [ 188.363784] [drm:intel_crtc_compute_min_cdclk [i915]] required cdclk (607500 kHz) exceeds max (540000 kHz)
> [ 188.363848] WARN_ON(min_cdclk < 0)
> ...
> [ 188.368005] [drm:intel_dump_pipe_config [i915]] [CRTC:36:pipe A][setup_hw_state]
> [ 188.368155] [drm:intel_dump_pipe_config [i915]] cpu_transcoder: A, pipe bpp: 24, dithering: 0
> [ 188.368276] [drm:intel_dump_pipe_config [i915]] audio: 0, infoframes: 0
> [ 188.368390] [drm:intel_dump_pipe_config [i915]] requested mode:
> [ 188.368692] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"640x480" 1286 540000 640 656 752 800 480 490 492 525 0x40 0xa
> [ 188.368833] [drm:intel_dump_pipe_config [i915]] adjusted mode:
> [ 188.368897] [drm:drm_mode_debug_printmodeline [drm]] Modeline 0:"640x480" 1286 540000 640 656 752 800 480 490 492 525 0x40 0xa
> [ 188.369041] [drm:intel_dump_pipe_config [i915]] crtc timings: 540000 640 656 752 800 480 490 492 525, type: 0x40 flags: 0xa
Cool. 640x480 with a refresh rate of ~1286 Hz. I want one of those displays :P
So now the question becomes if we made a mistake in the clock/timings readout or
if the hardware is really programmed to use those values.
> [ 188.369235] [drm:intel_dump_pipe_config [i915]] port clock: 540000, pipe src size: 720x400, pixel rate 607500
> [ 188.369366] [drm:intel_dump_pipe_config [i915]] pch pfit: pos: 0x00000000, size: 0x028001e0, enabled
> [ 188.369490] [drm:intel_dump_pipe_config [i915]] ips: 0, double wide: 0
> [ 188.369600] [drm:hsw_dump_hw_state [i915]] dpll_hw_state: wrpll: 0x0 spll: 0x0
> [ 188.369719] [drm:intel_dump_pipe_config [i915]] planes on this crtc
>
>
> Is this enough info?
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-09-05 12:58 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-10 19:33 [PATCH v3 1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" ville.syrjala
2017-07-10 19:33 ` [PATCH 2/2] drm/i915: Consolidate max_cdclk_freq check in intel_crtc_compute_min_cdclk() ville.syrjala
2017-07-11 20:21 ` Pandiyan, Dhinakaran
2017-07-12 10:18 ` Ville Syrjälä
2017-07-10 19:58 ` ✗ Fi.CI.BAT: warning for series starting with [v3,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" Patchwork
2017-07-11 9:47 ` [PATCH v3 1/2] " Dhinakaran Pandiyan
2017-07-11 13:00 ` Ville Syrjälä
2017-07-11 20:35 ` Pandiyan, Dhinakaran
2017-08-30 18:57 ` [PATCH v4 " ville.syrjala
2017-08-31 18:48 ` Ville Syrjälä
2017-09-04 10:39 ` Maarten Lankhorst
2017-09-04 15:58 ` Ville Syrjälä
2017-09-04 18:51 ` Maarten Lankhorst
2017-09-05 12:58 ` Ville Syrjälä
2017-08-31 11:19 ` ✓ Fi.CI.BAT: success for series starting with [v4,1/2] drm/i915: Track minimum acceptable cdclk instead of "minimum dotclock" (rev2) Patchwork
2017-08-31 15:05 ` ✓ Fi.CI.IGT: " Patchwork
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.