[Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
into one function 'intel_modeset_global_pipes()'
Signed-off-by: Mika Kahola
---
drivers/gpu/drm/i915/intel_display.c | 89 +---
1 file changed, 41 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 5ed40df..7180d2b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5209,38 +5209,6 @@ static int valleyview_calc_cdclk(struct drm_i915_private
*dev_priv,
return 20;
}
-/* compute the max pixel clock for new configuration */
-static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
-{
- struct drm_device *dev = dev_priv->dev;
- struct intel_crtc *intel_crtc;
- int max_pixclk = 0;
-
- for_each_intel_crtc(dev, intel_crtc) {
- if (intel_crtc->new_enabled)
- max_pixclk = max(max_pixclk,
-
intel_crtc->new_config->base.adjusted_mode.crtc_clock);
- }
-
- return max_pixclk;
-}
-
-static void valleyview_modeset_global_pipes(struct drm_device *dev,
- unsigned *prepare_pipes)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_crtc *intel_crtc;
- int max_pixclk = intel_mode_max_pixclk(dev_priv);
-
- if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq)
- return;
-
- /* disable/enable all currently active pipes while we change cdclk */
- for_each_intel_crtc(dev, intel_crtc)
- if (intel_crtc->base.state->enable)
- *prepare_pipes |= (1 << intel_crtc->pipe);
-}
-
static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
{
unsigned int credits, default_credits;
@@ -5277,6 +5245,22 @@ static void vlv_program_pfi_credits(struct
drm_i915_private *dev_priv)
WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
}
+/* compute the max pixel clock for new configuration */
+static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
+{
+ struct drm_device *dev = dev_priv->dev;
+ struct intel_crtc *intel_crtc;
+ int max_pixclk = 0;
+
+ for_each_intel_crtc(dev, intel_crtc) {
+ if (intel_crtc->new_enabled)
+ max_pixclk = max(max_pixclk,
+
intel_crtc->new_config->base.adjusted_mode.crtc_clock);
+ }
+
+ return max_pixclk;
+}
+
static void valleyview_modeset_global_resources(struct drm_atomic_state *state)
{
struct drm_device *dev = state->dev;
@@ -8973,21 +8957,38 @@ static void broadwell_set_cdclk(struct drm_device *dev,
int cdclk)
cdclk, dev_priv->cdclk_freq);
}
-static void haswell_modeset_global_pipes(struct drm_device *dev,
-unsigned *prepare_pipes)
+static void intel_modeset_global_pipes(struct drm_device *dev,
+ unsigned *prepare_pipes,
+ unsigned *disable_pipes)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *crtc;
- int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
+ int max_pixclk;
- if (haswell_calc_cdclk(dev_priv, max_pixel_rate) ==
- dev_priv->cdclk_freq)
+ /* this modeset is valid only for VLV, HSW, and BDW */
+ if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev))
return;
+ if (IS_VALLEYVIEW(dev)) {
+ max_pixclk = intel_mode_max_pixclk(dev_priv);
+ if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
+ dev_priv->cdclk_freq)
+ return;
+ } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+ max_pixclk = ilk_max_pixel_rate(dev_priv);
+ if (haswell_calc_cdclk(dev_priv, max_pixclk) ==
+ dev_priv->cdclk_freq)
+ return;
+
+ }
+
/* disable/enable all currently active pipes while we change cdclk */
for_each_intel_crtc(dev, crtc)
- if (crtc->base.enabled)
- *prepare_pipes |= 1 << crtc->pipe;
+ if (crtc->base.state->enable)
+ *prepare_pipes |= (1 << crtc->pipe);
+
+ /* may have added more to prepare_pipes than we should */
+ *prepare_pipes &= ~*disable_pipes;
}
static void haswell_modeset_global_resources(struct drm_atomic_state *state)
@@ -12120,15 +12121,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
* mode set on this crtc. For other crtcs we need to use the
* adjusted_mode bits in the crtc directly.
*/
- if (IS_VALLEYVIEW(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
- if (IS_VALLEYVIEW(
[Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
into one function 'intel_modeset_global_pipes()'
v2:
- we don't modify 'disable_pipes', so passing this as a pointer
is removed (based on Ville's comment)
- introduced a new function 'intel_calc_cdclk()' that combines
routines from 'valleyview_calc_cdclk()' and 'haswell_calc_cdclk()'
Signed-off-by: Mika Kahola
---
drivers/gpu/drm/i915/intel_display.c | 191 ---
1 file changed, 88 insertions(+), 103 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7b97907..18a1262 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5179,66 +5179,62 @@ static void cherryview_set_cdclk(struct drm_device
*dev, int cdclk)
intel_update_cdclk(dev);
}
-static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
+static int intel_calc_cdclk(struct drm_i915_private *dev_priv,
int max_pixclk)
{
- int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ? 33 :
32;
- int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
+ int cdclk = 20;
- /*
-* Really only a few cases to deal with, as only 4 CDclks are supported:
-* 200MHz
-* 267MHz
-* 320/333MHz (depends on HPLL freq)
-* 400MHz (VLV only)
-* So we check to see whether we're above 90% (VLV) or 95% (CHV)
-* of the lower bin and adjust if needed.
-*
-* 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)
- return 40;
- else if (max_pixclk > 27*limit/100)
- return freq_320;
- else if (max_pixclk > 0)
- return 27;
- else
- return 20;
-}
-
-/* compute the max pixel clock for new configuration */
-static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
-{
- struct drm_device *dev = dev_priv->dev;
- struct intel_crtc *intel_crtc;
- int max_pixclk = 0;
+ if (IS_VALLEYVIEW(dev_priv)) {
+ int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ?
33 : 32;
+ int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
+ /*
+* Really only a few cases to deal with, as only 4 CDclks are
supported:
+* 200MHz
+* 267MHz
+* 320/333MHz (depends on HPLL freq)
+* 400MHz (VLV only)
+* So we check to see whether we're above 90% (VLV) or 95% (CHV)
+* of the lower bin and adjust if needed.
+*
+* 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)
+ cdclk = 40;
+ else if (max_pixclk > 27*limit/100)
+ cdclk = freq_320;
+ else if (max_pixclk > 0)
+ cdclk = 27;
+ else
+ cdclk = 20;
+ } else if (IS_HASWELL(dev_priv)) {
+ /*
+* FIXME should also account for plane ratio
+* once 64bpp pixel formats are supported.
+*/
+ if (max_pixclk > 54)
+ cdclk = 675000;
+ else if (max_pixclk > 45)
+ cdclk = 54;
+ else if (max_pixclk > 337500 || !IS_HSW_ULX(dev_priv))
+ cdclk = 45;
+ else
+ cdclk = 337500;
- for_each_intel_crtc(dev, intel_crtc) {
- if (intel_crtc->new_enabled)
- max_pixclk = max(max_pixclk,
-
intel_crtc->new_config->base.adjusted_mode.crtc_clock);
+ /*
+* FIXME move the cdclk caclulation to
+* compute_config() so we can fail gracegully.
+*/
+ if (cdclk > dev_priv->max_cdclk_freq) {
+ DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d
kHz)\n",
+ cdclk, dev_priv->max_cdclk_freq);
+ cdclk = dev_priv->max_cdclk_freq;
+ }
}
- return max_pixclk;
-}
-
-static void valleyview_modeset_global_pipes(struct drm_device *dev,
- unsigned *prepare_pipes)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_crtc *i
[Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
into one function 'intel_modeset_global_pipes()'
v2:
- we don't modify 'disable_pipes', so passing this as a pointer
is removed (based on Ville's comment)
- introduced a new function 'intel_calc_cdclk()' that combines
routines from 'valleyview_calc_cdclk()' and 'haswell_calc_cdclk()'
v3:
- Let's take a step back and not remove the routines 'valleyview_calc_cdclk()'
and 'haswell_calc_cdclk()' from newly introduced routine
'intel_calc_cdclk()' (based on Ville's comment)
Signed-off-by: Mika Kahola
---
drivers/gpu/drm/i915/intel_display.c | 63 ++--
1 file changed, 32 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 7b97907..f05bd12 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5225,22 +5225,6 @@ static int intel_mode_max_pixclk(struct drm_i915_private
*dev_priv)
return max_pixclk;
}
-static void valleyview_modeset_global_pipes(struct drm_device *dev,
- unsigned *prepare_pipes)
-{
- struct drm_i915_private *dev_priv = dev->dev_private;
- struct intel_crtc *intel_crtc;
- int max_pixclk = intel_mode_max_pixclk(dev_priv);
-
- if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq)
- return;
-
- /* disable/enable all currently active pipes while we change cdclk */
- for_each_intel_crtc(dev, intel_crtc)
- if (intel_crtc->base.state->enable)
- *prepare_pipes |= (1 << intel_crtc->pipe);
-}
-
static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
{
unsigned int credits, default_credits;
@@ -8787,21 +8771,46 @@ static void broadwell_set_cdclk(struct drm_device *dev,
int cdclk)
cdclk, dev_priv->cdclk_freq);
}
-static void haswell_modeset_global_pipes(struct drm_device *dev,
-unsigned *prepare_pipes)
+static int intel_calc_cdclk(struct drm_device *dev, int max_pixclk)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ int cdclk;
+
+ if (IS_VALLEYVIEW(dev))
+ cdclk = valleyview_calc_cdclk(dev_priv, max_pixclk);
+ else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+ cdclk = haswell_calc_cdclk(dev_priv, max_pixclk);
+
+ return cdclk;
+}
+
+static void intel_modeset_global_pipes(struct drm_device *dev,
+ unsigned *prepare_pipes,
+ unsigned disable_pipes)
{
struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *crtc;
- int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
+ int max_pixclk;
- if (haswell_calc_cdclk(dev_priv, max_pixel_rate) ==
- dev_priv->cdclk_freq)
+ /* this modeset is valid only for VLV, HSW, and BDW */
+ if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev))
+ return;
+
+ if (IS_VALLEYVIEW(dev))
+ max_pixclk = intel_mode_max_pixclk(dev_priv);
+ else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+ max_pixclk = ilk_max_pixel_rate(dev_priv);
+
+ if (intel_calc_cdclk(dev, max_pixclk) == dev_priv->cdclk_freq)
return;
/* disable/enable all currently active pipes while we change cdclk */
for_each_intel_crtc(dev, crtc)
- if (crtc->base.enabled)
+ if (crtc->base.state->enable)
*prepare_pipes |= 1 << crtc->pipe;
+
+ /* may have added more to prepare_pipes than we should */
+ *prepare_pipes &= ~disable_pipes;
}
static void haswell_modeset_global_resources(struct drm_atomic_state *state)
@@ -11933,15 +11942,7 @@ static int __intel_set_mode(struct drm_crtc *crtc,
* mode set on this crtc. For other crtcs we need to use the
* adjusted_mode bits in the crtc directly.
*/
- if (IS_VALLEYVIEW(dev) || IS_HASWELL(dev) || IS_BROADWELL(dev)) {
- if (IS_VALLEYVIEW(dev))
- valleyview_modeset_global_pipes(dev, &prepare_pipes);
- else
- haswell_modeset_global_pipes(dev, &prepare_pipes);
-
- /* may have added more to prepare_pipes than we should */
- prepare_pipes &= ~disable_pipes;
- }
+ intel_modeset_global_pipes(dev, &prepare_pipes, disable_pipes);
ret = __intel_set_mode_setup_plls(dev, modeset_pipes, disable_pipes);
if (ret)
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
On Tue, Mar 31, 2015 at 02:14:23PM +0300, Mika Kahola wrote:
> Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
> into one function 'intel_modeset_global_pipes()'
>
> Signed-off-by: Mika Kahola
> ---
> drivers/gpu/drm/i915/intel_display.c | 89
> +---
> 1 file changed, 41 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 5ed40df..7180d2b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5209,38 +5209,6 @@ static int valleyview_calc_cdclk(struct
> drm_i915_private *dev_priv,
> return 20;
> }
>
> -/* compute the max pixel clock for new configuration */
> -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> -{
> - struct drm_device *dev = dev_priv->dev;
> - struct intel_crtc *intel_crtc;
> - int max_pixclk = 0;
> -
> - for_each_intel_crtc(dev, intel_crtc) {
> - if (intel_crtc->new_enabled)
> - max_pixclk = max(max_pixclk,
> -
> intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> - }
> -
> - return max_pixclk;
> -}
> -
> -static void valleyview_modeset_global_pipes(struct drm_device *dev,
> - unsigned *prepare_pipes)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - struct intel_crtc *intel_crtc;
> - int max_pixclk = intel_mode_max_pixclk(dev_priv);
> -
> - if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq)
> - return;
> -
> - /* disable/enable all currently active pipes while we change cdclk */
> - for_each_intel_crtc(dev, intel_crtc)
> - if (intel_crtc->base.state->enable)
> - *prepare_pipes |= (1 << intel_crtc->pipe);
> -}
> -
> static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> {
> unsigned int credits, default_credits;
> @@ -5277,6 +5245,22 @@ static void vlv_program_pfi_credits(struct
> drm_i915_private *dev_priv)
> WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
> }
>
> +/* compute the max pixel clock for new configuration */
> +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> +{
> + struct drm_device *dev = dev_priv->dev;
> + struct intel_crtc *intel_crtc;
> + int max_pixclk = 0;
> +
> + for_each_intel_crtc(dev, intel_crtc) {
> + if (intel_crtc->new_enabled)
> + max_pixclk = max(max_pixclk,
> +
> intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> + }
> +
> + return max_pixclk;
> +}
> +
> static void valleyview_modeset_global_resources(struct drm_atomic_state
> *state)
> {
> struct drm_device *dev = state->dev;
> @@ -8973,21 +8957,38 @@ static void broadwell_set_cdclk(struct drm_device
> *dev, int cdclk)
>cdclk, dev_priv->cdclk_freq);
> }
>
> -static void haswell_modeset_global_pipes(struct drm_device *dev,
> - unsigned *prepare_pipes)
> +static void intel_modeset_global_pipes(struct drm_device *dev,
> +unsigned *prepare_pipes,
> +unsigned *disable_pipes)
You don't modify disable_pipes, so no need to pass as pointer.
I do think passing disable_pipes into intel_modeset_global_pipes() does
make sense however, as it makes it clearer why we need to clear out the
disable_pipes when the code is all in one place like this.
> {
> struct drm_i915_private *dev_priv = dev->dev_private;
> struct intel_crtc *crtc;
> - int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> + int max_pixclk;
>
> - if (haswell_calc_cdclk(dev_priv, max_pixel_rate) ==
> - dev_priv->cdclk_freq)
> + /* this modeset is valid only for VLV, HSW, and BDW */
> + if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev))
> return;
>
> + if (IS_VALLEYVIEW(dev)) {
> + max_pixclk = intel_mode_max_pixclk(dev_priv);
> + if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
> + dev_priv->cdclk_freq)
> + return;
> + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> + max_pixclk = ilk_max_pixel_rate(dev_priv);
> + if (haswell_calc_cdclk(dev_priv, max_pixclk) ==
> + dev_priv->cdclk_freq)
> + return;
> +
> + }
Maybe move the current vs. newly computed pixclk comparison out of
the if ladder, so we don't have to duplicate it for each platform?
It should also get rid of the ugly line length issue we have here.
Eventually we should get rid of the platform specifics here, but
that requires that we sort out the pfit pixel rate mess for all
platforms in the same way. Currently we just ignore the
Re: [Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
On Tue, Mar 31, 2015 at 05:45:56PM +0300, Ville Syrjälä wrote:
> On Tue, Mar 31, 2015 at 02:14:23PM +0300, Mika Kahola wrote:
> > Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
> > into one function 'intel_modeset_global_pipes()'
> >
> > Signed-off-by: Mika Kahola
> > ---
> > drivers/gpu/drm/i915/intel_display.c | 89
> > +---
> > 1 file changed, 41 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 5ed40df..7180d2b 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5209,38 +5209,6 @@ static int valleyview_calc_cdclk(struct
> > drm_i915_private *dev_priv,
> > return 20;
> > }
> >
> > -/* compute the max pixel clock for new configuration */
> > -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> > -{
> > - struct drm_device *dev = dev_priv->dev;
> > - struct intel_crtc *intel_crtc;
> > - int max_pixclk = 0;
> > -
> > - for_each_intel_crtc(dev, intel_crtc) {
> > - if (intel_crtc->new_enabled)
> > - max_pixclk = max(max_pixclk,
> > -
> > intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> > - }
> > -
> > - return max_pixclk;
> > -}
> > -
> > -static void valleyview_modeset_global_pipes(struct drm_device *dev,
> > - unsigned *prepare_pipes)
> > -{
> > - struct drm_i915_private *dev_priv = dev->dev_private;
> > - struct intel_crtc *intel_crtc;
> > - int max_pixclk = intel_mode_max_pixclk(dev_priv);
> > -
> > - if (valleyview_calc_cdclk(dev_priv, max_pixclk) == dev_priv->cdclk_freq)
> > - return;
> > -
> > - /* disable/enable all currently active pipes while we change cdclk */
> > - for_each_intel_crtc(dev, intel_crtc)
> > - if (intel_crtc->base.state->enable)
> > - *prepare_pipes |= (1 << intel_crtc->pipe);
> > -}
> > -
> > static void vlv_program_pfi_credits(struct drm_i915_private *dev_priv)
> > {
> > unsigned int credits, default_credits;
> > @@ -5277,6 +5245,22 @@ static void vlv_program_pfi_credits(struct
> > drm_i915_private *dev_priv)
> > WARN_ON(I915_READ(GCI_CONTROL) & PFI_CREDIT_RESEND);
> > }
> >
> > +/* compute the max pixel clock for new configuration */
> > +static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> > +{
> > + struct drm_device *dev = dev_priv->dev;
> > + struct intel_crtc *intel_crtc;
> > + int max_pixclk = 0;
> > +
> > + for_each_intel_crtc(dev, intel_crtc) {
> > + if (intel_crtc->new_enabled)
> > + max_pixclk = max(max_pixclk,
> > +
> > intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> > + }
> > +
> > + return max_pixclk;
> > +}
> > +
> > static void valleyview_modeset_global_resources(struct drm_atomic_state
> > *state)
> > {
> > struct drm_device *dev = state->dev;
> > @@ -8973,21 +8957,38 @@ static void broadwell_set_cdclk(struct drm_device
> > *dev, int cdclk)
> > cdclk, dev_priv->cdclk_freq);
> > }
> >
> > -static void haswell_modeset_global_pipes(struct drm_device *dev,
> > -unsigned *prepare_pipes)
> > +static void intel_modeset_global_pipes(struct drm_device *dev,
> > + unsigned *prepare_pipes,
> > + unsigned *disable_pipes)
>
> You don't modify disable_pipes, so no need to pass as pointer.
>
> I do think passing disable_pipes into intel_modeset_global_pipes() does
> make sense however, as it makes it clearer why we need to clear out the
> disable_pipes when the code is all in one place like this.
That's a good point.
>
> > {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > struct intel_crtc *crtc;
> > - int max_pixel_rate = ilk_max_pixel_rate(dev_priv);
> > + int max_pixclk;
> >
> > - if (haswell_calc_cdclk(dev_priv, max_pixel_rate) ==
> > - dev_priv->cdclk_freq)
> > + /* this modeset is valid only for VLV, HSW, and BDW */
> > + if (!IS_VALLEYVIEW(dev) && !IS_HASWELL(dev) && !IS_BROADWELL(dev))
> > return;
> >
> > + if (IS_VALLEYVIEW(dev)) {
> > + max_pixclk = intel_mode_max_pixclk(dev_priv);
> > + if (valleyview_calc_cdclk(dev_priv, max_pixclk) ==
> > + dev_priv->cdclk_freq)
> > + return;
> > + } else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
> > + max_pixclk = ilk_max_pixel_rate(dev_priv);
> > + if (haswell_calc_cdclk(dev_priv, max_pixclk) ==
> > + dev_priv->cdclk_freq)
> > + return;
> > +
> > + }
>
> Maybe move the current vs. newly computed pixclk comparison out of
> the if ladder, so we don't have to duplicate it for each platform?
> It should also get rid of
Re: [Intel-gfx] [PATCH 19/19] drm/i915: Modeset global_pipes() update
On Thu, Apr 02, 2015 at 01:05:31PM +0300, Mika Kahola wrote:
> Combined Valleyview, Haswell and Broadwell '*_modeset_global_pipes()'
> into one function 'intel_modeset_global_pipes()'
>
> v2:
> - we don't modify 'disable_pipes', so passing this as a pointer
> is removed (based on Ville's comment)
> - introduced a new function 'intel_calc_cdclk()' that combines
> routines from 'valleyview_calc_cdclk()' and 'haswell_calc_cdclk()'
>
> Signed-off-by: Mika Kahola
> ---
> drivers/gpu/drm/i915/intel_display.c | 191
> ---
> 1 file changed, 88 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7b97907..18a1262 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5179,66 +5179,62 @@ static void cherryview_set_cdclk(struct drm_device
> *dev, int cdclk)
> intel_update_cdclk(dev);
> }
>
> -static int valleyview_calc_cdclk(struct drm_i915_private *dev_priv,
> +static int intel_calc_cdclk(struct drm_i915_private *dev_priv,
>int max_pixclk)
> {
> - int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ? 33 :
> 32;
> - int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
> + int cdclk = 20;
>
> - /*
> - * Really only a few cases to deal with, as only 4 CDclks are supported:
> - * 200MHz
> - * 267MHz
> - * 320/333MHz (depends on HPLL freq)
> - * 400MHz (VLV only)
> - * So we check to see whether we're above 90% (VLV) or 95% (CHV)
> - * of the lower bin and adjust if needed.
> - *
> - * 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)
> - return 40;
> - else if (max_pixclk > 27*limit/100)
> - return freq_320;
> - else if (max_pixclk > 0)
> - return 27;
> - else
> - return 20;
> -}
I'd leave the valleyview_calc_cdclk() and haswell_calc_cdclk() alone.
Stuffing them into a single function makes the patch quite ugly. Also
I suppose we might want to turn it into a vfunc in the future and
having separate funcs for different platforms would make that easier.
> -
> -/* compute the max pixel clock for new configuration */
> -static int intel_mode_max_pixclk(struct drm_i915_private *dev_priv)
> -{
> - struct drm_device *dev = dev_priv->dev;
> - struct intel_crtc *intel_crtc;
> - int max_pixclk = 0;
> + if (IS_VALLEYVIEW(dev_priv)) {
> + int freq_320 = (dev_priv->hpll_freq << 1) % 32 != 0 ?
> 33 : 32;
> + int limit = IS_CHERRYVIEW(dev_priv) ? 95 : 90;
> + /*
> + * Really only a few cases to deal with, as only 4 CDclks are
> supported:
> + * 200MHz
> + * 267MHz
> + * 320/333MHz (depends on HPLL freq)
> + * 400MHz (VLV only)
> + * So we check to see whether we're above 90% (VLV) or 95% (CHV)
> + * of the lower bin and adjust if needed.
> + *
> + * 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)
> + cdclk = 40;
> + else if (max_pixclk > 27*limit/100)
> + cdclk = freq_320;
> + else if (max_pixclk > 0)
> + cdclk = 27;
> + else
> + cdclk = 20;
> + } else if (IS_HASWELL(dev_priv)) {
> + /*
> + * FIXME should also account for plane ratio
> + * once 64bpp pixel formats are supported.
> + */
> + if (max_pixclk > 54)
> + cdclk = 675000;
> + else if (max_pixclk > 45)
> + cdclk = 54;
> + else if (max_pixclk > 337500 || !IS_HSW_ULX(dev_priv))
> + cdclk = 45;
> + else
> + cdclk = 337500;
>
> - for_each_intel_crtc(dev, intel_crtc) {
> - if (intel_crtc->new_enabled)
> - max_pixclk = max(max_pixclk,
> -
> intel_crtc->new_config->base.adjusted_mode.crtc_clock);
> + /*
> + * FIXME move the cdclk caclulation to
> + * compute_config() so we can fail gracegully.
> + */
> + if (cdclk > dev_priv->max_cdclk_freq) {
> + DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d
> kHz)\n",
>
