[PATCH 0/3] ALSA: hda - restore BCLK from CDCLK for Haswell/Broadwell display audio

[PATCH 0/3] ALSA: hda - restore BCLK from CDCLK for Haswell/Broadwell display audio

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

The 3 patches are for Haswell/Broadwell display HD-A controller.

The 1st patch is for i915 display driver, providing a private interface for
the audio driver to query CDCLK.

The other 2 patches are for HD-A driver, will restore BCLK by setting M/N 
values as per CDCLK.

Mengdong Lin (3):
  drm/i915: provide interface for audio driver to query cdclk
  ALSA: hda - define hda_get_display_clk to query CDCLK for
    Haswell/Broadwell
  ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display
    HDA controller

 drivers/gpu/drm/i915/intel_pm.c | 21 +++++++++++++++
 include/drm/i915_powerwell.h    |  1 +
 sound/pci/hda/hda_i915.c        | 22 +++++++++++++++
 sound/pci/hda/hda_i915.h        |  1 +
 sound/pci/hda/hda_intel.c       | 60 +++++++++++++++++++++++++----------------
 5 files changed, 82 insertions(+), 23 deletions(-)

-- 
1.8.1.2

[PATCH 1/3] drm/i915: provide interface for audio driver to query cdclk

[mengdong.lin]

From: Jani Nikula <jani.nikula@intel.com>

Signed-off-by: Jani Nikula <jani.nikula@intel.com>

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a90fdbd..21170e5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6256,6 +6256,27 @@ int i915_release_power_well(void)
 }
 EXPORT_SYMBOL_GPL(i915_release_power_well);
 
+/*
+ * Private interface for the audio driver to get CDCLK in kHz.
+ *
+ * Caller must request power well using i915_request_power_well() prior to
+ * making the call.
+ */
+int i915_get_cdclk_freq(void)
+{
+	struct drm_i915_private *dev_priv;
+
+	if (!hsw_pwr)
+		return -ENODEV;
+
+	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
+				power_domains);
+
+	return intel_ddi_get_cdclk_freq(dev_priv);
+}
+EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
+
+
 #define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
 
 #define HSW_ALWAYS_ON_POWER_DOMAINS (			\
diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h
index 2baba99..baa6f11 100644
--- a/include/drm/i915_powerwell.h
+++ b/include/drm/i915_powerwell.h
@@ -32,5 +32,6 @@
 /* For use by hda_i915 driver */
 extern int i915_request_power_well(void);
 extern int i915_release_power_well(void);
+extern int i915_get_cdclk_freq(void);
 
 #endif				/* _I915_POWERWELL_H_ */
-- 
1.8.1.2

Re: [PATCH 1/3] drm/i915: provide interface for audio driver to query cdclk

[Takashi Iwai]

At Wed,  2 Jul 2014 22:43:46 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Jani Nikula <jani.nikula@intel.com>
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

If you submit a patch, you need to add your own sign-off.
Just your signed-off-by line after Jani's line.

The code change itself looks OK.


thanks,

Takashi

> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a90fdbd..21170e5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6256,6 +6256,27 @@ int i915_release_power_well(void)
>  }
>  EXPORT_SYMBOL_GPL(i915_release_power_well);
>  
> +/*
> + * Private interface for the audio driver to get CDCLK in kHz.
> + *
> + * Caller must request power well using i915_request_power_well() prior to
> + * making the call.
> + */
> +int i915_get_cdclk_freq(void)
> +{
> +	struct drm_i915_private *dev_priv;
> +
> +	if (!hsw_pwr)
> +		return -ENODEV;
> +
> +	dev_priv = container_of(hsw_pwr, struct drm_i915_private,
> +				power_domains);
> +
> +	return intel_ddi_get_cdclk_freq(dev_priv);
> +}
> +EXPORT_SYMBOL_GPL(i915_get_cdclk_freq);
> +
> +
>  #define POWER_DOMAIN_MASK (BIT(POWER_DOMAIN_NUM) - 1)
>  
>  #define HSW_ALWAYS_ON_POWER_DOMAINS (			\
> diff --git a/include/drm/i915_powerwell.h b/include/drm/i915_powerwell.h
> index 2baba99..baa6f11 100644
> --- a/include/drm/i915_powerwell.h
> +++ b/include/drm/i915_powerwell.h
> @@ -32,5 +32,6 @@
>  /* For use by hda_i915 driver */
>  extern int i915_request_power_well(void);
>  extern int i915_release_power_well(void);
> +extern int i915_get_cdclk_freq(void);
>  
>  #endif				/* _I915_POWERWELL_H_ */
> -- 
> 1.8.1.2
> 

[PATCH 2/3] ALSA: hda - define hda_get_display_clk to query CDCLK for Haswell/Broadwell

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

This patch defines hda_get_display_clk() to query Core Display Clock (CDCLK)
from the i915 display driver, via a private API i915_get_cdclk_freq().

The audio driver will set M/N values as per the CDCLK for restoring BCLK of
the display HD-A controller.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_i915.c b/sound/pci/hda/hda_i915.c
index e9e8a4a..76db293 100644
--- a/sound/pci/hda/hda_i915.c
+++ b/sound/pci/hda/hda_i915.c
@@ -24,6 +24,7 @@
 
 static int (*get_power)(void);
 static int (*put_power)(void);
+static int (*get_cdclk)(void);
 
 int hda_display_power(bool enable)
 {
@@ -38,6 +39,14 @@ int hda_display_power(bool enable)
 		return put_power();
 }
 
+int hda_get_display_clk(void)
+{
+	if (!get_cdclk)
+		return -ENODEV;
+
+	return get_cdclk();
+}
+
 int hda_i915_init(void)
 {
 	int err = 0;
@@ -55,6 +64,15 @@ int hda_i915_init(void)
 		return -ENODEV;
 	}
 
+	get_cdclk = symbol_request(i915_get_cdclk_freq);
+	if (!get_cdclk) {
+		symbol_put(i915_request_power_well);
+		get_power = NULL;
+		symbol_put(i915_release_power_well);
+		put_power = NULL;
+		return -ENODEV;
+	}
+
 	pr_debug("HDA driver get symbol successfully from i915 module\n");
 
 	return err;
@@ -70,6 +88,10 @@ int hda_i915_exit(void)
 		symbol_put(i915_release_power_well);
 		put_power = NULL;
 	}
+	if (get_cdclk) {
+		symbol_put(i915_get_cdclk_freq);
+		get_cdclk = NULL;
+	}
 
 	return 0;
 }
diff --git a/sound/pci/hda/hda_i915.h b/sound/pci/hda/hda_i915.h
index bfd835f..b4420e1 100644
--- a/sound/pci/hda/hda_i915.h
+++ b/sound/pci/hda/hda_i915.h
@@ -18,6 +18,7 @@
 
 #ifdef CONFIG_SND_HDA_I915
 int hda_display_power(bool enable);
+int hda_get_display_clk(void);
 int hda_i915_init(void);
 int hda_i915_exit(void);
 #else
-- 
1.8.1.2

[PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller

[mengdong.lin]

From: Mengdong Lin <mengdong.lin@intel.com>

For HSW/BDW display HD-A controller, this patch restores BCLK by setting the
M/N values as per the core display clock (CDCLK) queried from i915 display
driver.

And the audio driver will also restore BCLK in azx_first_init() since the
display driver can turn off the shared power in boot phase if only eDP is
connected and M/N values will be lost and must be reprogrammed.

Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
index 25753db..c7dfd1d 100644
--- a/sound/pci/hda/hda_intel.c
+++ b/sound/pci/hda/hda_intel.c
@@ -299,10 +299,6 @@ static char *driver_short_names[] = {
 
 struct hda_intel {
 	struct azx chip;
-
-	/* HSW/BDW display HDA controller to restore BCLK from CDCLK */
-	unsigned int bclk_m;
-	unsigned int bclk_n;
 };
 
 
@@ -598,20 +594,41 @@ static int param_set_xint(const char *val, const struct kernel_param *kp)
 #define azx_del_card_list(chip) /* NOP */
 #endif /* CONFIG_PM */
 
-static void haswell_save_bclk(struct azx *chip)
-{
-	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
-
-	hda->bclk_m = azx_readw(chip, EM4);
-	hda->bclk_n = azx_readw(chip, EM5);
-}
 
 static void haswell_restore_bclk(struct azx *chip)
 {
-	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
+	int cdclk_freq;
+	unsigned int bclk_m, bclk_n;
+
+	cdclk_freq = hda_get_display_clk();
+	switch (cdclk_freq) {
+	case 337500:
+		bclk_m = 16;
+		bclk_n = 225;
+		break;
+
+	case 450000: /* default CDCLK 450MHz */
+		bclk_m = 4;
+		bclk_n = 75;
+		break;
+
+	case 540000:
+		bclk_m = 4;
+		bclk_n = 90;
+		break;
+
+	case 675000:
+		bclk_m = 8;
+		bclk_n = 225;
+		break;
 
-	azx_writew(chip, EM4, hda->bclk_m);
-	azx_writew(chip, EM5, hda->bclk_n);
+	default:
+		bclk_m = 4;
+		bclk_n = 75;
+	}
+
+	azx_writew(chip, EM4, bclk_m);
+	azx_writew(chip, EM5, bclk_n);
 }
 
 #if defined(CONFIG_PM_SLEEP) || defined(SUPPORT_VGA_SWITCHEROO)
@@ -641,12 +658,6 @@ static int azx_suspend(struct device *dev)
 		chip->irq = -1;
 	}
 
-	/* Save BCLK M/N values before they become invalid in D3.
-	 * Will test if display power well can be released now.
-	 */
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
-		haswell_save_bclk(chip);
-
 	if (chip->msi)
 		pci_disable_msi(chip->pci);
 	pci_disable_device(pci);
@@ -713,10 +724,9 @@ static int azx_runtime_suspend(struct device *dev)
 	azx_stop_chip(chip);
 	azx_enter_link_reset(chip);
 	azx_clear_irq_pending(chip);
-	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL) {
-		haswell_save_bclk(chip);
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
 		hda_display_power(false);
-	}
+
 	return 0;
 }
 
@@ -1426,6 +1436,10 @@ static int azx_first_init(struct azx *chip)
 
 	/* initialize chip */
 	azx_init_pci(chip);
+
+	if (chip->driver_caps & AZX_DCAPS_I915_POWERWELL)
+		haswell_restore_bclk(chip);
+
 	azx_init_chip(chip, (probe_only[dev] & 2) == 0);
 
 	/* codec detection */
-- 
1.8.1.2

Re: [PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller

[Takashi Iwai]

At Wed,  2 Jul 2014 22:44:07 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Mengdong Lin <mengdong.lin@intel.com>
> 
> For HSW/BDW display HD-A controller, this patch restores BCLK by setting the
> M/N values as per the core display clock (CDCLK) queried from i915 display
> driver.
> 
> And the audio driver will also restore BCLK in azx_first_init() since the
> display driver can turn off the shared power in boot phase if only eDP is
> connected and M/N values will be lost and must be reprogrammed.
> 
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> 
> diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> index 25753db..c7dfd1d 100644
> --- a/sound/pci/hda/hda_intel.c
> +++ b/sound/pci/hda/hda_intel.c
> @@ -299,10 +299,6 @@ static char *driver_short_names[] = {
>  
>  struct hda_intel {
>  	struct azx chip;
> -
> -	/* HSW/BDW display HDA controller to restore BCLK from CDCLK */
> -	unsigned int bclk_m;
> -	unsigned int bclk_n;
>  };
>  
>  
> @@ -598,20 +594,41 @@ static int param_set_xint(const char *val, const struct kernel_param *kp)
>  #define azx_del_card_list(chip) /* NOP */
>  #endif /* CONFIG_PM */
>  
> -static void haswell_save_bclk(struct azx *chip)
> -{
> -	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> -
> -	hda->bclk_m = azx_readw(chip, EM4);
> -	hda->bclk_n = azx_readw(chip, EM5);
> -}
>  
>  static void haswell_restore_bclk(struct azx *chip)
>  {
> -	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> +	int cdclk_freq;
> +	unsigned int bclk_m, bclk_n;
> +
> +	cdclk_freq = hda_get_display_clk();
> +	switch (cdclk_freq) {
> +	case 337500:
> +		bclk_m = 16;
> +		bclk_n = 225;
> +		break;
> +
> +	case 450000: /* default CDCLK 450MHz */
> +		bclk_m = 4;
> +		bclk_n = 75;
> +		break;
> +
> +	case 540000:
> +		bclk_m = 4;
> +		bclk_n = 90;
> +		break;
> +
> +	case 675000:
> +		bclk_m = 8;
> +		bclk_n = 225;
> +		break;
>  
> -	azx_writew(chip, EM4, hda->bclk_m);
> -	azx_writew(chip, EM5, hda->bclk_n);
> +	default:
> +		bclk_m = 4;
> +		bclk_n = 75;

You can put "default:" around "case 45000:", and move the comment
about default CDCLK there.

> +	}
> +
> +	azx_writew(chip, EM4, bclk_m);
> +	azx_writew(chip, EM5, bclk_n);
>  }

IMO, the whole function can be moved to hda_i915.c and provided as a
function such as hda_set_display_clk().  Then you can drop
hda_get_display_clk() but call get_cdclk() internally.

Also, all these patches may have Cc to stable, right?


thanks,

Takashi

Re: [PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller

[Lin, Mengdong]

Hi Takashi,

Thanks for your review! The revised patches are submitted: 
[PATCH v2 1/2] drm/i915: provide interface for audio driver to query cdclk
[PATCH v2 2/2] ALSA: hda - restore BCLK M/N value as per CDCLK for HSW/BDW display HDA controller

And we think these patches may have Cc to stable, since Haswell need them.

Hi Jani,

Your first patch was also submitted to Intel gfx mailing list. Thanks for your help on this issue!

Regards
Mengdong

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai@suse.de]
> Sent: Wednesday, July 02, 2014 11:01 PM
> To: Lin, Mengdong
> Cc: alsa-devel@alsa-project.org; Nikula, Jani
> Subject: Re: [PATCH 3/3] ALSA: hda - restore BCLK M/N value as per CDCLK
> for HSW/BDW display HDA controller
> 
> At Wed,  2 Jul 2014 22:44:07 +0800,
> mengdong.lin@intel.com wrote:
> >
> > From: Mengdong Lin <mengdong.lin@intel.com>
> >
> > For HSW/BDW display HD-A controller, this patch restores BCLK by
> > setting the M/N values as per the core display clock (CDCLK) queried
> > from i915 display driver.
> >
> > And the audio driver will also restore BCLK in azx_first_init() since
> > the display driver can turn off the shared power in boot phase if only
> > eDP is connected and M/N values will be lost and must be
> reprogrammed.
> >
> > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> >
> > diff --git a/sound/pci/hda/hda_intel.c b/sound/pci/hda/hda_intel.c
> > index 25753db..c7dfd1d 100644
> > --- a/sound/pci/hda/hda_intel.c
> > +++ b/sound/pci/hda/hda_intel.c
> > @@ -299,10 +299,6 @@ static char *driver_short_names[] = {
> >
> >  struct hda_intel {
> >  	struct azx chip;
> > -
> > -	/* HSW/BDW display HDA controller to restore BCLK from CDCLK */
> > -	unsigned int bclk_m;
> > -	unsigned int bclk_n;
> >  };
> >
> >
> > @@ -598,20 +594,41 @@ static int param_set_xint(const char *val,
> const
> > struct kernel_param *kp)  #define azx_del_card_list(chip) /* NOP */
> > #endif /* CONFIG_PM */
> >
> > -static void haswell_save_bclk(struct azx *chip) -{
> > -	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> > -
> > -	hda->bclk_m = azx_readw(chip, EM4);
> > -	hda->bclk_n = azx_readw(chip, EM5);
> > -}
> >
> >  static void haswell_restore_bclk(struct azx *chip)  {
> > -	struct hda_intel *hda = container_of(chip, struct hda_intel, chip);
> > +	int cdclk_freq;
> > +	unsigned int bclk_m, bclk_n;
> > +
> > +	cdclk_freq = hda_get_display_clk();
> > +	switch (cdclk_freq) {
> > +	case 337500:
> > +		bclk_m = 16;
> > +		bclk_n = 225;
> > +		break;
> > +
> > +	case 450000: /* default CDCLK 450MHz */
> > +		bclk_m = 4;
> > +		bclk_n = 75;
> > +		break;
> > +
> > +	case 540000:
> > +		bclk_m = 4;
> > +		bclk_n = 90;
> > +		break;
> > +
> > +	case 675000:
> > +		bclk_m = 8;
> > +		bclk_n = 225;
> > +		break;
> >
> > -	azx_writew(chip, EM4, hda->bclk_m);
> > -	azx_writew(chip, EM5, hda->bclk_n);
> > +	default:
> > +		bclk_m = 4;
> > +		bclk_n = 75;
> 
> You can put "default:" around "case 45000:", and move the comment
> about default CDCLK there.
> 
> > +	}
> > +
> > +	azx_writew(chip, EM4, bclk_m);
> > +	azx_writew(chip, EM5, bclk_n);
> >  }
> 
> IMO, the whole function can be moved to hda_i915.c and provided as a
> function such as hda_set_display_clk().  Then you can drop
> hda_get_display_clk() but call get_cdclk() internally.
> 
> Also, all these patches may have Cc to stable, right?
> 
> 
> thanks,
> 
> Takashi