Re: [Intel-gfx] [PATCH V7] drm/i915/skl: SKL CDCLK change on modeset tracking VCO

[Clint Taylor]

On 02/25/2016 05:49 AM, Ville Syrjälä wrote:

On Tue, Feb 16, 2016 at 09:44:55AM -0800, clinton.a.tay...@intel.com wrote:

From: Clint Taylor 

Set cdclk based on the max required pixel clock based on VCO
selected. Track boot vco instead of boot cdclk.

The vco is now tracked at the atomic level and all CRTCs updated if
the required vco is changed. Not tested with eDP v1.4 panels that
require 8640 vco due to availability.

V1: initial version
V2: add vco tracking in intel_dp_compute_config(), rename
skl_boot_cdclk.
V3: rebase, V2 feedback not possible as encoders are not aware of
atomic.
V4: track target vco is atomic state. modeset all CRTCs if vco changes
V5: rename atomic variable, cleaner if/else logic, use existing vco if
 encoder does not return a new vco value. check_patch.pl cleanup
V6: simplify logic in intel_modeset_checks.
V7: reorder an IF for readability and whitespace fix.

Signed-off-by: Clint Taylor 
Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 


I finally got around to actually trying this out myself, and I
noticed a few remaining problems:

- skl_modeset_calc_cdclk() should calculate dev_cdclk as well
   dev_cdclk will be the same as cdclk, except when all pipes are
   inactive, at which point dev_cdclk should be the minimum cdclk


That explains the relationship between dev_cdclk and cdclk.


- skl_modeset_commit_cdclk() should commit dev_cdclk, not cdclk


correct, based on the above statement. New version on its way.


- modeset_update_crtc_power_domains() should check of the current
   vco is different from the requested vco in addition to checking
   the dev_cdclk vs. current cdclk, just like intel_modeset_checks()
   does
modeset_update_crtc_power_domains() no longer exists. However it appears 
the functionality has been moved to intel_atomic_commit(). I will add 
the vco check into intel_atomic_commit()




So the current thing works, but it fails to drop the cdclk to minimum
when all pipes are inactive, and it also reprograms the cdclk every
time, I assume since it forgot to compute dev_cdclk and so that one
is probably left at 0 and so it never matches the current cdclk.


---
  drivers/gpu/drm/i915/i915_drv.h  |2 +-
  drivers/gpu/drm/i915/intel_ddi.c |2 +-
  drivers/gpu/drm/i915/intel_display.c |  102 +-
  drivers/gpu/drm/i915/intel_dp.c  |6 +-
  drivers/gpu/drm/i915/intel_drv.h |4 ++
  5 files changed, 99 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8216665..f65dd1a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1822,7 +1822,7 @@ struct drm_i915_private {
int num_fence_regs; /* 8 on pre-965, 16 otherwise */

unsigned int fsb_freq, mem_freq, is_ddr3;
-   unsigned int skl_boot_cdclk;
+   unsigned int skl_vco_freq;
unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
unsigned int max_dotclk_freq;
unsigned int hpll_freq;
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 6d5b09f..285adab 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev)
int cdclk_freq;

cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
-   dev_priv->skl_boot_cdclk = cdclk_freq;
+   dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq);
if (skl_sanitize_cdclk(dev_priv))
DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 9e2273b..e118ce0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq)
return (freq - 1000) / 500;
  }

-static unsigned int skl_cdclk_get_vco(unsigned int freq)
+unsigned int skl_cdclk_get_vco(unsigned int freq)
  {
unsigned int i;

@@ -5821,17 +5821,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv)

  void skl_init_cdclk(struct drm_i915_private *dev_priv)
  {
-   unsigned int required_vco;
+   unsigned int cdclk;

/* DPLL0 not enabled (happens on early BIOS versions) */
if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
/* enable DPLL0 */
-   required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
-   skl_dpll0_enable(dev_priv, required_vco);
+   if (dev_priv->skl_vco_freq != 8640)
+   dev_priv->skl_vco_freq = 8100;
+   skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq);
+   cdclk = ((dev_priv->skl_vco_freq == 

Re: [Intel-gfx] [PATCH V7] drm/i915/skl: SKL CDCLK change on modeset tracking VCO

[Ville Syrjälä]

On Tue, Feb 16, 2016 at 09:44:55AM -0800, clinton.a.tay...@intel.com wrote:
> From: Clint Taylor 
> 
> Set cdclk based on the max required pixel clock based on VCO
> selected. Track boot vco instead of boot cdclk.
> 
> The vco is now tracked at the atomic level and all CRTCs updated if
> the required vco is changed. Not tested with eDP v1.4 panels that
> require 8640 vco due to availability.
> 
> V1: initial version
> V2: add vco tracking in intel_dp_compute_config(), rename
> skl_boot_cdclk.
> V3: rebase, V2 feedback not possible as encoders are not aware of
> atomic.
> V4: track target vco is atomic state. modeset all CRTCs if vco changes
> V5: rename atomic variable, cleaner if/else logic, use existing vco if
> encoder does not return a new vco value. check_patch.pl cleanup
> V6: simplify logic in intel_modeset_checks.
> V7: reorder an IF for readability and whitespace fix.
> 
> Signed-off-by: Clint Taylor 
> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 

I finally got around to actually trying this out myself, and I
noticed a few remaining problems:

- skl_modeset_calc_cdclk() should calculate dev_cdclk as well
  dev_cdclk will be the same as cdclk, except when all pipes are
  inactive, at which point dev_cdclk should be the minimum cdclk
- skl_modeset_commit_cdclk() should commit dev_cdclk, not cdclk
- modeset_update_crtc_power_domains() should check of the current
  vco is different from the requested vco in addition to checking
  the dev_cdclk vs. current cdclk, just like intel_modeset_checks()
  does

So the current thing works, but it fails to drop the cdclk to minimum
when all pipes are inactive, and it also reprograms the cdclk every
time, I assume since it forgot to compute dev_cdclk and so that one
is probably left at 0 and so it never matches the current cdclk.

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |2 +-
>  drivers/gpu/drm/i915/intel_ddi.c |2 +-
>  drivers/gpu/drm/i915/intel_display.c |  102 
> +-
>  drivers/gpu/drm/i915/intel_dp.c  |6 +-
>  drivers/gpu/drm/i915/intel_drv.h |4 ++
>  5 files changed, 99 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8216665..f65dd1a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1822,7 +1822,7 @@ struct drm_i915_private {
>   int num_fence_regs; /* 8 on pre-965, 16 otherwise */
>  
>   unsigned int fsb_freq, mem_freq, is_ddr3;
> - unsigned int skl_boot_cdclk;
> + unsigned int skl_vco_freq;
>   unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
>   unsigned int max_dotclk_freq;
>   unsigned int hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 6d5b09f..285adab 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev)
>   int cdclk_freq;
>  
>   cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> - dev_priv->skl_boot_cdclk = cdclk_freq;
> + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq);
>   if (skl_sanitize_cdclk(dev_priv))
>   DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>   if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 9e2273b..e118ce0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq)
>   return (freq - 1000) / 500;
>  }
>  
> -static unsigned int skl_cdclk_get_vco(unsigned int freq)
> +unsigned int skl_cdclk_get_vco(unsigned int freq)
>  {
>   unsigned int i;
>  
> @@ -5821,17 +5821,21 @@ void skl_uninit_cdclk(struct drm_i915_private 
> *dev_priv)
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  {
> - unsigned int required_vco;
> + unsigned int cdclk;
>  
>   /* DPLL0 not enabled (happens on early BIOS versions) */
>   if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
>   /* enable DPLL0 */
> - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
> - skl_dpll0_enable(dev_priv, required_vco);
> + if (dev_priv->skl_vco_freq != 8640)
> + dev_priv->skl_vco_freq = 8100;
> + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq);
> + cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500 : 308570);
> + } else {
> + cdclk = dev_priv->cdclk_freq;
>   }
>  
> - /* set CDCLK to the frequency the BIOS chose */
> - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> + /* set CDCLK to the lowest 

Re: [Intel-gfx] [PATCH V7] drm/i915/skl: SKL CDCLK change on modeset tracking VCO

[Ville Syrjälä]

On Tue, Feb 16, 2016 at 09:44:55AM -0800, clinton.a.tay...@intel.com wrote:
> From: Clint Taylor 
> 
> Set cdclk based on the max required pixel clock based on VCO
> selected. Track boot vco instead of boot cdclk.
> 
> The vco is now tracked at the atomic level and all CRTCs updated if
> the required vco is changed. Not tested with eDP v1.4 panels that
> require 8640 vco due to availability.
> 
> V1: initial version
> V2: add vco tracking in intel_dp_compute_config(), rename
> skl_boot_cdclk.
> V3: rebase, V2 feedback not possible as encoders are not aware of
> atomic.
> V4: track target vco is atomic state. modeset all CRTCs if vco changes
> V5: rename atomic variable, cleaner if/else logic, use existing vco if
> encoder does not return a new vco value. check_patch.pl cleanup
> V6: simplify logic in intel_modeset_checks.
> V7: reorder an IF for readability and whitespace fix.
> 
> Signed-off-by: Clint Taylor 
> Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= 

Looks pretty good to me.
Reviewed-by: Ville Syrjälä 

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |2 +-
>  drivers/gpu/drm/i915/intel_ddi.c |2 +-
>  drivers/gpu/drm/i915/intel_display.c |  102 
> +-
>  drivers/gpu/drm/i915/intel_dp.c  |6 +-
>  drivers/gpu/drm/i915/intel_drv.h |4 ++
>  5 files changed, 99 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 8216665..f65dd1a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1822,7 +1822,7 @@ struct drm_i915_private {
>   int num_fence_regs; /* 8 on pre-965, 16 otherwise */
>  
>   unsigned int fsb_freq, mem_freq, is_ddr3;
> - unsigned int skl_boot_cdclk;
> + unsigned int skl_vco_freq;
>   unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
>   unsigned int max_dotclk_freq;
>   unsigned int hpll_freq;
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index 6d5b09f..285adab 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev)
>   int cdclk_freq;
>  
>   cdclk_freq = dev_priv->display.get_display_clock_speed(dev);
> - dev_priv->skl_boot_cdclk = cdclk_freq;
> + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq);
>   if (skl_sanitize_cdclk(dev_priv))
>   DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n");
>   if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE))
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 9e2273b..e118ce0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq)
>   return (freq - 1000) / 500;
>  }
>  
> -static unsigned int skl_cdclk_get_vco(unsigned int freq)
> +unsigned int skl_cdclk_get_vco(unsigned int freq)
>  {
>   unsigned int i;
>  
> @@ -5821,17 +5821,21 @@ void skl_uninit_cdclk(struct drm_i915_private 
> *dev_priv)
>  
>  void skl_init_cdclk(struct drm_i915_private *dev_priv)
>  {
> - unsigned int required_vco;
> + unsigned int cdclk;
>  
>   /* DPLL0 not enabled (happens on early BIOS versions) */
>   if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) {
>   /* enable DPLL0 */
> - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk);
> - skl_dpll0_enable(dev_priv, required_vco);
> + if (dev_priv->skl_vco_freq != 8640)
> + dev_priv->skl_vco_freq = 8100;
> + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq);
> + cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500 : 308570);
> + } else {
> + cdclk = dev_priv->cdclk_freq;
>   }
>  
> - /* set CDCLK to the frequency the BIOS chose */
> - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk);
> + /* set CDCLK to the lowest frequency, Modeset follows */
> + skl_set_cdclk(dev_priv, cdclk);
>  
>   /* enable DBUF power */
>   I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST);
> @@ -5847,7 +5851,7 @@ int skl_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>  {
>   uint32_t lcpll1 = I915_READ(LCPLL1_CTL);
>   uint32_t cdctl = I915_READ(CDCLK_CTL);
> - int freq = dev_priv->skl_boot_cdclk;
> + int freq = dev_priv->cdclk_freq;
>  
>   /*
>* check if the pre-os intialized the display
> @@ -5871,11 +5875,7 @@ int skl_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>   /* All well; nothing to sanitize */
>   return false;
>  sanitize:
> - /*
> -  * As of now initialize with