[PATCH 1/2] drm/i915: Get CZ clock for VLV

[PATCH 1/2] drm/i915: Get CZ clock for VLV

[Vandana Kannan]

CZ clock is related to data flow from memory to display plane. This is
required for comparison with CD clock before programming PFI credits.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 174a294..baeb56f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2764,6 +2764,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
 int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
 int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
 
+int valleyview_get_cz_clock_speed(struct drm_device *dev);
+
 #define FORCEWAKE_RENDER	(1 << 0)
 #define FORCEWAKE_MEDIA		(1 << 1)
 #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fe5c276..1b8f095 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -601,6 +601,7 @@ enum punit_power_well {
 #define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
 #define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
 #define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
+#define CCK_CZ_CONTROL				0x62
 
 /**
  * DOC: DPIO
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 99eb7ca..56a8090 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5278,6 +5278,26 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
 	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
 }
 
+int valleyview_get_cz_clock_speed(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	int vco = valleyview_get_vco(dev_priv);
+	u32 val;
+	int divider;
+
+	mutex_lock(&dev_priv->dpio_lock);
+	val = vlv_cck_read(dev_priv, CCK_CZ_CONTROL);
+	mutex_unlock(&dev_priv->dpio_lock);
+
+	divider = val & DISPLAY_FREQUENCY_VALUES;
+
+	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
+	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
+	     "czclk change in progress\n");
+
+	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
+}
+
 static int i945_get_display_clock_speed(struct drm_device *dev)
 {
 	return 400000;
-- 
2.0.1

[PATCH 2/2] drm/i915: Program PFI credits for VLV

[Vandana Kannan]

From: Vidya Srinivas <vidya.srinivas@intel.com>

PFI credit programming is required when CD clock (related to data flow from
display pipeline to end display) is greater than CZ clock (related to data
flow from memory to display plane). This programming should be done when all
planes are OFF to avoid intermittent hangs while accessing memory even from
different Gfx units (not just display).

If cdclk/czclk >=1, PFI credits could be set as any number. To get better
performance, larger PFI credit can be assigned to PND. Otherwise if
cdclk/czclk<1, the default PFI credit of 8 should be set.

v2:
    - Change log to lower log level instead of DRM_ERROR
    - Change function name to valleyview_program_pfi_credits
    - Move program PFI credits to modeset_init instead of intel_set_mode
    - Change magic numbers to logical constants

Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  6 ++++++
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_reg.h      |  5 +++++
 drivers/gpu/drm/i915/intel_display.c |  4 +++-
 drivers/gpu/drm/i915/intel_pm.c      | 22 ++++++++++++++++++++++
 5 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b2b649c..73617b5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
 	console_unlock();
 
+	if (IS_VALLEYVIEW(dev))
+		valleyview_program_pfi_credits(dev_priv, false);
+
 	dev_priv->suspend_count++;
 
 	intel_display_set_init_power(dev_priv, false);
@@ -693,6 +696,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	if (IS_VALLEYVIEW(dev))
+		valleyview_program_pfi_credits(dev_priv, true);
+
 	intel_opregion_notify_adapter(dev, PCI_D0);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index baeb56f..38907ef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2152,6 +2152,8 @@ extern struct i915_params i915 __read_mostly;
 
 				/* i915_dma.c */
 void i915_update_dri1_breadcrumb(struct drm_device *dev);
+extern void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
+					   bool flag);
 extern void i915_kernel_lost_context(struct drm_device * dev);
 extern int i915_driver_load(struct drm_device *, unsigned long flags);
 extern int i915_driver_unload(struct drm_device *);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1b8f095..92b8afc 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1914,6 +1914,11 @@ enum punit_power_well {
 #define   CZCLK_FREQ_MASK	0xf
 #define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
 
+#define GCI_CONTROL		(VLV_DISPLAY_BASE + 0x650C)
+#define   PFI_CREDIT		(7 << 28)
+#define   PFI_CREDIT_RESEND	(1 << 27)
+#define   VGA_FAST_MODE_DISABLE (1 << 14)
+
 /*
  * Palette regs
  */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56a8090..521943a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12555,8 +12555,10 @@ void intel_modeset_init_hw(struct drm_device *dev)
 {
 	intel_prepare_ddi(dev);
 
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
 		vlv_update_cdclk(dev);
+		valleyview_program_pfi_credits(dev->dev_private, true);
+	}
 
 	intel_init_clock_gating(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 3f88f29..fe55c54 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6780,6 +6780,28 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
 	pm_runtime_disable(device);
 }
 
+void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
+				    bool flag)
+{
+	int cd_clk, cz_clk;
+
+	if (!flag) {
+		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
+		return;
+	}
+
+	cd_clk = dev_priv->display.get_display_clock_speed(dev_priv->dev);
+	cz_clk = valleyview_get_cz_clock_speed(dev_priv->dev);
+
+	if (cd_clk >= cz_clk) {
+		/* WA - write default credits before re-programming */
+		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
+		I915_WRITE(GCI_CONTROL, (PFI_CREDIT | PFI_CREDIT_RESEND |
+					VGA_FAST_MODE_DISABLE));
+	} else
+		DRM_DEBUG_KMS("cd clk < cz clk");
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
-- 
2.0.1

Re: [PATCH 1/2] drm/i915: Get CZ clock for VLV

[Ville Syrjälä]

On Mon, Aug 04, 2014 at 10:44:04PM +0530, Vandana Kannan wrote:
> CZ clock is related to data flow from memory to display plane. This is
> required for comparison with CD clock before programming PFI credits.
> 
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++++
>  3 files changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 174a294..baeb56f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2764,6 +2764,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>  int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
>  int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
>  
> +int valleyview_get_cz_clock_speed(struct drm_device *dev);
> +
>  #define FORCEWAKE_RENDER	(1 << 0)
>  #define FORCEWAKE_MEDIA		(1 << 1)
>  #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fe5c276..1b8f095 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -601,6 +601,7 @@ enum punit_power_well {
>  #define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
>  #define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
>  #define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
> +#define CCK_CZ_CONTROL				0x62

Please move the reg define to just above DISPLAY_CLOCK_CONTROL to keep
things in neat order.

Also it might be a good idea to rename the DISPLAY_TRUNK_* and
DISPLAY_FREQUENCY_* bits to CCK_... instead of DISPLAY_... to make
it clear they apply to all CCK clock control registers.

>  
>  /**
>   * DOC: DPIO
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 99eb7ca..56a8090 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5278,6 +5278,26 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
>  	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
>  }
>  
> +int valleyview_get_cz_clock_speed(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int vco = valleyview_get_vco(dev_priv);
> +	u32 val;
> +	int divider;
> +
> +	mutex_lock(&dev_priv->dpio_lock);
> +	val = vlv_cck_read(dev_priv, CCK_CZ_CONTROL);
> +	mutex_unlock(&dev_priv->dpio_lock);
> +
> +	divider = val & DISPLAY_FREQUENCY_VALUES;
> +
> +	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
> +	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
> +	     "czclk change in progress\n");
> +
> +	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
> +}

We might want to refactor this a bit. Eg.:

static int valleyview_get_cck_clock_speed(u32 reg, const char *name)
{
  ... do the cck read etc. from CCK clock control register 'reg'
}

static int valleyview_get_cz_clock_speed()
{
	return valleyview_get_cck_clock_speed(CCK_CZ_CLOCK_CONTROL, "czclk");
}

static int valleyview_get_display_clock_speed()
{
	...
	return valleyview_get_cck_clock_speed(CCK_DISPLAY_CLOCK_CONTROL, "cdclk");
}

> +
>  static int i945_get_display_clock_speed(struct drm_device *dev)
>  {
>  	return 400000;
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 1/2] drm/i915: Get CZ clock for VLV

[Vandana Kannan]

On Aug-05-2014 6:39 PM, Ville Syrjälä wrote:
> On Mon, Aug 04, 2014 at 10:44:04PM +0530, Vandana Kannan wrote:
>> CZ clock is related to data flow from memory to display plane. This is
>> required for comparison with CD clock before programming PFI credits.
>>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
>>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++++
>>  3 files changed, 23 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 174a294..baeb56f 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2764,6 +2764,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
>>  int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
>>  int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
>>  
>> +int valleyview_get_cz_clock_speed(struct drm_device *dev);
>> +
>>  #define FORCEWAKE_RENDER	(1 << 0)
>>  #define FORCEWAKE_MEDIA		(1 << 1)
>>  #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index fe5c276..1b8f095 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -601,6 +601,7 @@ enum punit_power_well {
>>  #define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
>>  #define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
>>  #define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
>> +#define CCK_CZ_CONTROL				0x62
> 
> Please move the reg define to just above DISPLAY_CLOCK_CONTROL to keep
> things in neat order.
> 
Ok, will make this change..

> Also it might be a good idea to rename the DISPLAY_TRUNK_* and
> DISPLAY_FREQUENCY_* bits to CCK_... instead of DISPLAY_... to make
> it clear they apply to all CCK clock control registers.
> 
Will submit this renaming part as a separate patch..
>>  
>>  /**
>>   * DOC: DPIO
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 99eb7ca..56a8090 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5278,6 +5278,26 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
>>  	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
>>  }
>>  
>> +int valleyview_get_cz_clock_speed(struct drm_device *dev)
>> +{
>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>> +	int vco = valleyview_get_vco(dev_priv);
>> +	u32 val;
>> +	int divider;
>> +
>> +	mutex_lock(&dev_priv->dpio_lock);
>> +	val = vlv_cck_read(dev_priv, CCK_CZ_CONTROL);
>> +	mutex_unlock(&dev_priv->dpio_lock);
>> +
>> +	divider = val & DISPLAY_FREQUENCY_VALUES;
>> +
>> +	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
>> +	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
>> +	     "czclk change in progress\n");
>> +
>> +	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
>> +}
> 
> We might want to refactor this a bit. Eg.:
> 
> static int valleyview_get_cck_clock_speed(u32 reg, const char *name)
> {
>   ... do the cck read etc. from CCK clock control register 'reg'
> }
> 
> static int valleyview_get_cz_clock_speed()
> {
> 	return valleyview_get_cck_clock_speed(CCK_CZ_CLOCK_CONTROL, "czclk");
> }
> 
> static int valleyview_get_display_clock_speed()
> {
> 	...
> 	return valleyview_get_cck_clock_speed(CCK_DISPLAY_CLOCK_CONTROL, "cdclk");
> }
> 
As part of the above refactoring, how about having an enum to specify
CDCLK or CZCLK instead of char *?

- Vandana
>> +
>>  static int i945_get_display_clock_speed(struct drm_device *dev)
>>  {
>>  	return 400000;
>> -- 
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 1/2] drm/i915: Get CZ clock for VLV

[Ville Syrjälä]

On Tue, Aug 05, 2014 at 08:57:07PM +0530, Vandana Kannan wrote:
> On Aug-05-2014 6:39 PM, Ville Syrjälä wrote:
> > On Mon, Aug 04, 2014 at 10:44:04PM +0530, Vandana Kannan wrote:
> >> CZ clock is related to data flow from memory to display plane. This is
> >> required for comparison with CD clock before programming PFI credits.
> >>
> >> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
> >>  drivers/gpu/drm/i915/i915_reg.h      |  1 +
> >>  drivers/gpu/drm/i915/intel_display.c | 20 ++++++++++++++++++++
> >>  3 files changed, 23 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 174a294..baeb56f 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -2764,6 +2764,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
> >>  int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
> >>  int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
> >>  
> >> +int valleyview_get_cz_clock_speed(struct drm_device *dev);
> >> +
> >>  #define FORCEWAKE_RENDER	(1 << 0)
> >>  #define FORCEWAKE_MEDIA		(1 << 1)
> >>  #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index fe5c276..1b8f095 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -601,6 +601,7 @@ enum punit_power_well {
> >>  #define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
> >>  #define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
> >>  #define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
> >> +#define CCK_CZ_CONTROL				0x62
> > 
> > Please move the reg define to just above DISPLAY_CLOCK_CONTROL to keep
> > things in neat order.
> > 
> Ok, will make this change..
> 
> > Also it might be a good idea to rename the DISPLAY_TRUNK_* and
> > DISPLAY_FREQUENCY_* bits to CCK_... instead of DISPLAY_... to make
> > it clear they apply to all CCK clock control registers.
> > 
> Will submit this renaming part as a separate patch..
> >>  
> >>  /**
> >>   * DOC: DPIO
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index 99eb7ca..56a8090 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -5278,6 +5278,26 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
> >>  	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
> >>  }
> >>  
> >> +int valleyview_get_cz_clock_speed(struct drm_device *dev)
> >> +{
> >> +	struct drm_i915_private *dev_priv = dev->dev_private;
> >> +	int vco = valleyview_get_vco(dev_priv);
> >> +	u32 val;
> >> +	int divider;
> >> +
> >> +	mutex_lock(&dev_priv->dpio_lock);
> >> +	val = vlv_cck_read(dev_priv, CCK_CZ_CONTROL);
> >> +	mutex_unlock(&dev_priv->dpio_lock);
> >> +
> >> +	divider = val & DISPLAY_FREQUENCY_VALUES;
> >> +
> >> +	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
> >> +	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
> >> +	     "czclk change in progress\n");
> >> +
> >> +	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
> >> +}
> > 
> > We might want to refactor this a bit. Eg.:
> > 
> > static int valleyview_get_cck_clock_speed(u32 reg, const char *name)
> > {
> >   ... do the cck read etc. from CCK clock control register 'reg'
> > }
> > 
> > static int valleyview_get_cz_clock_speed()
> > {
> > 	return valleyview_get_cck_clock_speed(CCK_CZ_CLOCK_CONTROL, "czclk");
> > }
> > 
> > static int valleyview_get_display_clock_speed()
> > {
> > 	...
> > 	return valleyview_get_cck_clock_speed(CCK_DISPLAY_CLOCK_CONTROL, "cdclk");
> > }
> > 
> As part of the above refactoring, how about having an enum to specify
> CDCLK or CZCLK instead of char *?

We had such a thing in intel_i2c.c until I killed it here:
drm/i915: Kill duplicated cdclk readout code from i2c

Personally I don't see much point in having it since we'd just have
these few users. But if you want to add it I don't mind too much either.

> 
> - Vandana
> >> +
> >>  static int i945_get_display_clock_speed(struct drm_device *dev)
> >>  {
> >>  	return 400000;
> >> -- 
> >> 2.0.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Ville Syrjälä
Intel OTC

[PATCH 1/3] drm/i915: Renaming CCK related reg definitions

[Vandana Kannan]

Rename the DISPLAY_TRUNK_* and DISPLAY_FREQUENCY_* bits to CCK_... instead
of DISPLAY_... to make it clear they apply to all CCK clock control registers.
Suggested by Ville.

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
Cc: Ville Syrjä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      | 10 +++++-----
 drivers/gpu/drm/i915/intel_display.c | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 187f862..a8275b7 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -617,11 +617,11 @@ enum punit_power_well {
 #define  DSI_PLL_M1_DIV_SHIFT			0
 #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
 #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
-#define  DISPLAY_TRUNK_FORCE_ON			(1 << 17)
-#define  DISPLAY_TRUNK_FORCE_OFF		(1 << 16)
-#define  DISPLAY_FREQUENCY_STATUS		(0x1f << 8)
-#define  DISPLAY_FREQUENCY_STATUS_SHIFT		8
-#define  DISPLAY_FREQUENCY_VALUES		(0x1f << 0)
+#define  CCK_TRUNK_FORCE_ON			(1 << 17)
+#define  CCK_TRUNK_FORCE_OFF			(1 << 16)
+#define  CCK_FREQUENCY_STATUS			(0x1f << 8)
+#define  CCK_FREQUENCY_STATUS_SHIFT		8
+#define  CCK_FREQUENCY_VALUES			(0x1f << 0)
 
 /**
  * DOC: DPIO
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d828e40..f1f1b54 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4503,12 +4503,12 @@ static void valleyview_set_cdclk(struct drm_device *dev, int cdclk)
 		mutex_lock(&dev_priv->dpio_lock);
 		/* adjust cdclk divider */
 		val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
-		val &= ~DISPLAY_FREQUENCY_VALUES;
+		val &= ~CCK_FREQUENCY_VALUES;
 		val |= divider;
 		vlv_cck_write(dev_priv, CCK_DISPLAY_CLOCK_CONTROL, val);
 
 		if (wait_for((vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL) &
-			      DISPLAY_FREQUENCY_STATUS) == (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
+			      CCK_FREQUENCY_STATUS) == (divider << CCK_FREQUENCY_STATUS_SHIFT),
 			     50))
 			DRM_ERROR("timed out waiting for CDclk change\n");
 		mutex_unlock(&dev_priv->dpio_lock);
@@ -5331,10 +5331,10 @@ static int valleyview_get_display_clock_speed(struct drm_device *dev)
 	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
 	mutex_unlock(&dev_priv->dpio_lock);
 
-	divider = val & DISPLAY_FREQUENCY_VALUES;
+	divider = val & CCK_FREQUENCY_VALUES;
 
-	WARN((val & DISPLAY_FREQUENCY_STATUS) !=
-	     (divider << DISPLAY_FREQUENCY_STATUS_SHIFT),
+	WARN((val & CCK_FREQUENCY_STATUS) !=
+	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
 	     "cdclk change in progress\n");
 
 	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
-- 
2.0.1

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

[PATCH v2 2/3] drm/i915: Get CZ clock for VLV

[Vandana Kannan]

CZ clock is related to data flow from memory to display plane. This is
required for comparison with CD clock before programming PFI credits.

v2: Ville's review comments
	- Re-ordered CCK_CZ_CONTROL
	- Refactored get_clock_speed

Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_reg.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 43 ++++++++++++++++++++++++++++++------
 3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dccd0a2..881e0a6 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2784,6 +2784,8 @@ void vlv_flisdsi_write(struct drm_i915_private *dev_priv, u32 reg, u32 val);
 int vlv_gpu_freq(struct drm_i915_private *dev_priv, int val);
 int vlv_freq_opcode(struct drm_i915_private *dev_priv, int val);
 
+int valleyview_get_cz_clock_speed(struct drm_device *dev);
+
 #define FORCEWAKE_RENDER	(1 << 0)
 #define FORCEWAKE_MEDIA		(1 << 1)
 #define FORCEWAKE_ALL		(FORCEWAKE_RENDER | FORCEWAKE_MEDIA)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index a8275b7..fb111cd 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -616,6 +616,7 @@ enum punit_power_well {
 #define  DSI_PLL_N1_DIV_MASK			(3 << 16)
 #define  DSI_PLL_M1_DIV_SHIFT			0
 #define  DSI_PLL_M1_DIV_MASK			(0x1ff << 0)
+#define CCK_CZ_CONTROL				0x62
 #define CCK_DISPLAY_CLOCK_CONTROL		0x6b
 #define  CCK_TRUNK_FORCE_ON			(1 << 17)
 #define  CCK_TRUNK_FORCE_OFF			(1 << 16)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f1f1b54..2089319 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5316,30 +5316,59 @@ static int intel_crtc_compute_config(struct intel_crtc *crtc,
 	return 0;
 }
 
-static int valleyview_get_display_clock_speed(struct drm_device *dev)
+enum disp_clk {
+	CDCLK,
+	CZCLK
+};
+
+static int valleyview_get_cck_clock_speed(struct drm_device *dev,
+					  enum disp_clk clk)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int vco = valleyview_get_vco(dev_priv);
-	u32 val;
+	u32 val, reg;
 	int divider;
 
-	/* FIXME: Punit isn't quite ready yet */
-	if (IS_CHERRYVIEW(dev))
-		return 400000;
+	switch(clk) {
+	case CDCLK:
+	default:
+		reg = CCK_DISPLAY_CLOCK_CONTROL;
+		break;
+	case CZCLK:
+		reg = CCK_CZ_CONTROL;
+		break;
+	}
+
 	mutex_lock(&dev_priv->dpio_lock);
-	val = vlv_cck_read(dev_priv, CCK_DISPLAY_CLOCK_CONTROL);
+	val = vlv_cck_read(dev_priv, reg);
 	mutex_unlock(&dev_priv->dpio_lock);
 
 	divider = val & CCK_FREQUENCY_VALUES;
 
 	WARN((val & CCK_FREQUENCY_STATUS) !=
 	     (divider << CCK_FREQUENCY_STATUS_SHIFT),
-	     "cdclk change in progress\n");
+	     "%sclk change in progress\n", (clk == CDCLK) ? "cd" : "cz");
 
 	return DIV_ROUND_CLOSEST(vco << 1, divider + 1);
 }
 
+static int valleyview_get_display_clock_speed(struct drm_device *dev)
+{
+	/* FIXME: Punit isn't quite ready yet */
+	if (IS_CHERRYVIEW(dev))
+		return 400000;
+	else
+		return valleyview_get_cck_clock_speed(dev, CDCLK);
+}
+
+int valleyview_get_cz_clock_speed(struct drm_device *dev)
+{
+	return valleyview_get_cck_clock_speed(dev, CZCLK);
+}
+
 static int i945_get_display_clock_speed(struct drm_device *dev)
 {
 	return 400000;
-- 
2.0.1

[PATCH 3/3] drm/i915: Program PFI credits for VLV

[Vandana Kannan]

From: Vidya Srinivas <vidya.srinivas@intel.com>

PFI credit programming is required when CD clock (related to data flow from
display pipeline to end display) is greater than CZ clock (related to data
flow from memory to display plane). This programming should be done when all
planes are OFF to avoid intermittent hangs while accessing memory even from
different Gfx units (not just display).

If cdclk/czclk >=1, PFI credits could be set as any number. To get better
performance, larger PFI credit can be assigned to PND. Otherwise if
cdclk/czclk<1, the default PFI credit of 8 should be set.

v2:
    - Change log to lower log level instead of DRM_ERROR
    - Change function name to valleyview_program_pfi_credits
    - Move program PFI credits to modeset_init instead of intel_set_mode
    - Change magic numbers to logical constants

Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |  6 ++++++
 drivers/gpu/drm/i915/i915_drv.h      |  2 ++
 drivers/gpu/drm/i915/i915_reg.h      |  5 +++++
 drivers/gpu/drm/i915/intel_display.c |  4 +++-
 drivers/gpu/drm/i915/intel_pm.c      | 22 ++++++++++++++++++++++
 5 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c4b25c..00e0b4a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
 	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
 	console_unlock();
 
+	if (IS_VALLEYVIEW(dev))
+		valleyview_program_pfi_credits(dev_priv, false);
+
 	dev_priv->suspend_count++;
 
 	intel_display_set_init_power(dev_priv, false);
@@ -693,6 +696,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
 	dev_priv->modeset_restore = MODESET_DONE;
 	mutex_unlock(&dev_priv->modeset_restore_lock);
 
+	if (IS_VALLEYVIEW(dev))
+		valleyview_program_pfi_credits(dev_priv, true);
+
 	intel_opregion_notify_adapter(dev, PCI_D0);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 881e0a6..88fd4c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2172,6 +2172,8 @@ extern struct i915_params i915 __read_mostly;
 
 				/* i915_dma.c */
 void i915_update_dri1_breadcrumb(struct drm_device *dev);
+extern void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
+					   bool flag);
 extern void i915_kernel_lost_context(struct drm_device * dev);
 extern int i915_driver_load(struct drm_device *, unsigned long flags);
 extern int i915_driver_unload(struct drm_device *);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index fb111cd..7f4c2f5 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1937,6 +1937,11 @@ enum punit_power_well {
 #define   CZCLK_FREQ_MASK	0xf
 #define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
 
+#define GCI_CONTROL		(VLV_DISPLAY_BASE + 0x650C)
+#define   PFI_CREDIT		(7 << 28)
+#define   PFI_CREDIT_RESEND	(1 << 27)
+#define   VGA_FAST_MODE_DISABLE (1 << 14)
+
 /*
  * Palette regs
  */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2089319..2af2e60 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12691,8 +12691,10 @@ void intel_modeset_init_hw(struct drm_device *dev)
 {
 	intel_prepare_ddi(dev);
 
-	if (IS_VALLEYVIEW(dev))
+	if (IS_VALLEYVIEW(dev)) {
 		vlv_update_cdclk(dev);
+		valleyview_program_pfi_credits(dev->dev_private, true);
+	}
 
 	intel_init_clock_gating(dev);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index ba8dfeb..ad8e190 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7154,6 +7154,28 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
 	pm_runtime_disable(device);
 }
 
+void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
+				    bool flag)
+{
+	int cd_clk, cz_clk;
+
+	if (!flag) {
+		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
+		return;
+	}
+
+	cd_clk = dev_priv->display.get_display_clock_speed(dev_priv->dev);
+	cz_clk = valleyview_get_cz_clock_speed(dev_priv->dev);
+
+	if (cd_clk >= cz_clk) {
+		/* WA - write default credits before re-programming */
+		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
+		I915_WRITE(GCI_CONTROL, (PFI_CREDIT | PFI_CREDIT_RESEND |
+					VGA_FAST_MODE_DISABLE));
+	} else
+		DRM_DEBUG_KMS("cd clk < cz clk");
+}
+
 /* Set up chip specific power management-related functions */
 void intel_init_pm(struct drm_device *dev)
 {
-- 
2.0.1

Re: [PATCH 3/3] drm/i915: Program PFI credits for VLV

[Ville Syrjälä]

On Thu, Aug 07, 2014 at 06:40:03PM +0530, Vandana Kannan wrote:
> From: Vidya Srinivas <vidya.srinivas@intel.com>
> 
> PFI credit programming is required when CD clock (related to data flow from
> display pipeline to end display) is greater than CZ clock (related to data
> flow from memory to display plane). This programming should be done when all
> planes are OFF to avoid intermittent hangs while accessing memory even from
> different Gfx units (not just display).
> 
> If cdclk/czclk >=1, PFI credits could be set as any number. To get better
> performance, larger PFI credit can be assigned to PND. Otherwise if
> cdclk/czclk<1, the default PFI credit of 8 should be set.

Do we have any docs for this? I see the register in the docs but nothing
about the correct programming. Some kind of refrence to the used
documentation would be nice.

> 
> v2:
>     - Change log to lower log level instead of DRM_ERROR
>     - Change function name to valleyview_program_pfi_credits
>     - Move program PFI credits to modeset_init instead of intel_set_mode
>     - Change magic numbers to logical constants
> 
> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
> Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  6 ++++++
>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>  drivers/gpu/drm/i915/i915_reg.h      |  5 +++++
>  drivers/gpu/drm/i915/intel_display.c |  4 +++-
>  drivers/gpu/drm/i915/intel_pm.c      | 22 ++++++++++++++++++++++
>  5 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 6c4b25c..00e0b4a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
>  	console_unlock();
>  
> +	if (IS_VALLEYVIEW(dev))
> +		valleyview_program_pfi_credits(dev_priv, false);

If we want to do this for system suspend I think we should stick it
into i915_drm_freeze() (just after turning off the pipes). Not sure if
we should also force cdclk to minimum there...

Hmm. Is the GCI_CONTROL register is part of the disp2d power well? If it
is we probably need to enable the power well around the register write.

> +
>  	dev_priv->suspend_count++;
>  
>  	intel_display_set_init_power(dev_priv, false);
> @@ -693,6 +696,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>  	dev_priv->modeset_restore = MODESET_DONE;
>  	mutex_unlock(&dev_priv->modeset_restore_lock);
>  
> +	if (IS_VALLEYVIEW(dev))
> +		valleyview_program_pfi_credits(dev_priv, true);

I think this thing can be dropped. We need to do the reprogramming as
part of the modeset global resources handling.

> +
>  	intel_opregion_notify_adapter(dev, PCI_D0);
>  
>  	return 0;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 881e0a6..88fd4c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2172,6 +2172,8 @@ extern struct i915_params i915 __read_mostly;
>  
>  				/* i915_dma.c */
>  void i915_update_dri1_breadcrumb(struct drm_device *dev);
> +extern void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
> +					   bool flag);
>  extern void i915_kernel_lost_context(struct drm_device * dev);
>  extern int i915_driver_load(struct drm_device *, unsigned long flags);
>  extern int i915_driver_unload(struct drm_device *);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index fb111cd..7f4c2f5 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1937,6 +1937,11 @@ enum punit_power_well {
>  #define   CZCLK_FREQ_MASK	0xf
>  #define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
>  
> +#define GCI_CONTROL		(VLV_DISPLAY_BASE + 0x650C)
> +#define   PFI_CREDIT		(7 << 28)

Maybe something like this for a bit more self documenting code.

#define PFI_CREDIT(x) (((x)-8) << 28) /* 8-15 */

> +#define   PFI_CREDIT_RESEND	(1 << 27)
> +#define   VGA_FAST_MODE_DISABLE (1 << 14)
> +
>  /*
>   * Palette regs
>   */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2089319..2af2e60 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12691,8 +12691,10 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  {
>  	intel_prepare_ddi(dev);
>  
> -	if (IS_VALLEYVIEW(dev))
> +	if (IS_VALLEYVIEW(dev)) {
>  		vlv_update_cdclk(dev);
> +		valleyview_program_pfi_credits(dev->dev_private, true);

The planes may be be active here. So for the initial state we just need
to trust that the BIOS got it right.

For runtime cdclk changes this needs to be handled in
valleyview_modeset_global_resources().

> +	}
>  
>  	intel_init_clock_gating(dev);
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ba8dfeb..ad8e190 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -7154,6 +7154,28 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
>  	pm_runtime_disable(device);
>  }
>  
> +void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
> +				    bool flag)
> +{
> +	int cd_clk, cz_clk;
> +
> +	if (!flag) {
> +		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
> +		return;
> +	}
> +
> +	cd_clk = dev_priv->display.get_display_clock_speed(dev_priv->dev);
> +	cz_clk = valleyview_get_cz_clock_speed(dev_priv->dev);
> +
> +	if (cd_clk >= cz_clk) {
> +		/* WA - write default credits before re-programming */
> +		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);

Why do you write just the vga fast mode disable bit first?

> +		I915_WRITE(GCI_CONTROL, (PFI_CREDIT | PFI_CREDIT_RESEND |
> +					VGA_FAST_MODE_DISABLE));
> +	} else
> +		DRM_DEBUG_KMS("cd clk < cz clk");

So we never reprogram the credits for the cd<cz case? Shouldn't we have
something like this here?
I915_WRITE(GCI_CONTROL, PFI_CREDIT(8) | PFI_CREDIT_RESEND | VGA_FAST_MODE_DISABLE);

Also does the PFI_CREDIT_RESEND bit get cleared immediately by the
hardware or do we have to poll for it to clear?

> +}
> +
>  /* Set up chip specific power management-related functions */
>  void intel_init_pm(struct drm_device *dev)
>  {
> -- 
> 2.0.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/3] drm/i915: Program PFI credits for VLV

[Vandana Kannan]

Hi Ville,

Apologize for the delay in reply.
For your inputs on the programming side (modeset global resources
handling and self documenting code parts), I agree with you and will
make the changes.
For opens related to info on the feature, internal discussions are
ongoing. I will get back to you on them next week..

Thanks,
Vandana

On Aug-08-2014 6:33 PM, Ville Syrjälä wrote:
> On Thu, Aug 07, 2014 at 06:40:03PM +0530, Vandana Kannan wrote:
>> From: Vidya Srinivas <vidya.srinivas@intel.com>
>>
>> PFI credit programming is required when CD clock (related to data flow from
>> display pipeline to end display) is greater than CZ clock (related to data
>> flow from memory to display plane). This programming should be done when all
>> planes are OFF to avoid intermittent hangs while accessing memory even from
>> different Gfx units (not just display).
>>
>> If cdclk/czclk >=1, PFI credits could be set as any number. To get better
>> performance, larger PFI credit can be assigned to PND. Otherwise if
>> cdclk/czclk<1, the default PFI credit of 8 should be set.
> 
> Do we have any docs for this? I see the register in the docs but nothing
> about the correct programming. Some kind of refrence to the used
> documentation would be nice.
> 
>>
>> v2:
>>     - Change log to lower log level instead of DRM_ERROR
>>     - Change function name to valleyview_program_pfi_credits
>>     - Move program PFI credits to modeset_init instead of intel_set_mode
>>     - Change magic numbers to logical constants
>>
>> Signed-off-by: Vidya Srinivas <vidya.srinivas@intel.com>
>> Signed-off-by: Gajanan Bhat <gajanan.bhat@intel.com>
>> Signed-off-by: Vandana Kannan <vandana.kannan@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c      |  6 ++++++
>>  drivers/gpu/drm/i915/i915_drv.h      |  2 ++
>>  drivers/gpu/drm/i915/i915_reg.h      |  5 +++++
>>  drivers/gpu/drm/i915/intel_display.c |  4 +++-
>>  drivers/gpu/drm/i915/intel_pm.c      | 22 ++++++++++++++++++++++
>>  5 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 6c4b25c..00e0b4a 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -558,6 +558,9 @@ static int i915_drm_freeze(struct drm_device *dev)
>>  	intel_fbdev_set_suspend(dev, FBINFO_STATE_SUSPENDED);
>>  	console_unlock();
>>  
>> +	if (IS_VALLEYVIEW(dev))
>> +		valleyview_program_pfi_credits(dev_priv, false);
> 
> If we want to do this for system suspend I think we should stick it
> into i915_drm_freeze() (just after turning off the pipes). Not sure if
> we should also force cdclk to minimum there...
> 
> Hmm. Is the GCI_CONTROL register is part of the disp2d power well? If it
> is we probably need to enable the power well around the register write.
> 
>> +
>>  	dev_priv->suspend_count++;
>>  
>>  	intel_display_set_init_power(dev_priv, false);
>> @@ -693,6 +696,9 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
>>  	dev_priv->modeset_restore = MODESET_DONE;
>>  	mutex_unlock(&dev_priv->modeset_restore_lock);
>>  
>> +	if (IS_VALLEYVIEW(dev))
>> +		valleyview_program_pfi_credits(dev_priv, true);
> 
> I think this thing can be dropped. We need to do the reprogramming as
> part of the modeset global resources handling.
> 
>> +
>>  	intel_opregion_notify_adapter(dev, PCI_D0);
>>  
>>  	return 0;
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 881e0a6..88fd4c7 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2172,6 +2172,8 @@ extern struct i915_params i915 __read_mostly;
>>  
>>  				/* i915_dma.c */
>>  void i915_update_dri1_breadcrumb(struct drm_device *dev);
>> +extern void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
>> +					   bool flag);
>>  extern void i915_kernel_lost_context(struct drm_device * dev);
>>  extern int i915_driver_load(struct drm_device *, unsigned long flags);
>>  extern int i915_driver_unload(struct drm_device *);
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index fb111cd..7f4c2f5 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1937,6 +1937,11 @@ enum punit_power_well {
>>  #define   CZCLK_FREQ_MASK	0xf
>>  #define GMBUSFREQ_VLV		(VLV_DISPLAY_BASE + 0x6510)
>>  
>> +#define GCI_CONTROL		(VLV_DISPLAY_BASE + 0x650C)
>> +#define   PFI_CREDIT		(7 << 28)
> 
> Maybe something like this for a bit more self documenting code.
> 
> #define PFI_CREDIT(x) (((x)-8) << 28) /* 8-15 */
> 
>> +#define   PFI_CREDIT_RESEND	(1 << 27)
>> +#define   VGA_FAST_MODE_DISABLE (1 << 14)
>> +
>>  /*
>>   * Palette regs
>>   */
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 2089319..2af2e60 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -12691,8 +12691,10 @@ void intel_modeset_init_hw(struct drm_device *dev)
>>  {
>>  	intel_prepare_ddi(dev);
>>  
>> -	if (IS_VALLEYVIEW(dev))
>> +	if (IS_VALLEYVIEW(dev)) {
>>  		vlv_update_cdclk(dev);
>> +		valleyview_program_pfi_credits(dev->dev_private, true);
> 
> The planes may be be active here. So for the initial state we just need
> to trust that the BIOS got it right.
> 
> For runtime cdclk changes this needs to be handled in
> valleyview_modeset_global_resources().
> 
>> +	}
>>  
>>  	intel_init_clock_gating(dev);
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index ba8dfeb..ad8e190 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -7154,6 +7154,28 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
>>  	pm_runtime_disable(device);
>>  }
>>  
>> +void valleyview_program_pfi_credits(struct drm_i915_private *dev_priv,
>> +				    bool flag)
>> +{
>> +	int cd_clk, cz_clk;
>> +
>> +	if (!flag) {
>> +		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
>> +		return;
>> +	}
>> +
>> +	cd_clk = dev_priv->display.get_display_clock_speed(dev_priv->dev);
>> +	cz_clk = valleyview_get_cz_clock_speed(dev_priv->dev);
>> +
>> +	if (cd_clk >= cz_clk) {
>> +		/* WA - write default credits before re-programming */
>> +		I915_WRITE(GCI_CONTROL, VGA_FAST_MODE_DISABLE);
> 
> Why do you write just the vga fast mode disable bit first?
> 
>> +		I915_WRITE(GCI_CONTROL, (PFI_CREDIT | PFI_CREDIT_RESEND |
>> +					VGA_FAST_MODE_DISABLE));
>> +	} else
>> +		DRM_DEBUG_KMS("cd clk < cz clk");
> 
> So we never reprogram the credits for the cd<cz case? Shouldn't we have
> something like this here?
> I915_WRITE(GCI_CONTROL, PFI_CREDIT(8) | PFI_CREDIT_RESEND | VGA_FAST_MODE_DISABLE);
> 
> Also does the PFI_CREDIT_RESEND bit get cleared immediately by the
> hardware or do we have to poll for it to clear?
> 
>> +}
>> +
>>  /* Set up chip specific power management-related functions */
>>  void intel_init_pm(struct drm_device *dev)
>>  {
>> -- 
>> 2.0.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>