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

[PATCH v2 1/2] 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>
Signed-off-by: Mengdong Lin <mengdong.lin@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 v2 1/2] drm/i915: provide interface for audio driver to query cdclk

[Jani Nikula]

On Thu, 03 Jul 2014, mengdong.lin@intel.com wrote:
> From: Jani Nikula <jani.nikula@intel.com>

I wrote this as a quick hack patch to try as an alternative to [1] which
ended up not working on Haswell. Please reassure me that this is going
to be a temporary solution until we get a more generic interface between
the audio and display drivers. I don't much like this, but at least it's
isolated and small.

I'd like the commit message amended with something like:

"""
If the display power well has been disabled, the display audio
controller divider values EM4 MVALUE and EM5 NVALUE will have been
lost. The CDCLK frequency is required for reprogramming them. Provide a
private interface for the audio driver to query CDCLK.

This is a stopgap solution until a more generic interface between audio
and display drivers has been implemented.
"""

I'd also like to have an additional Reviewed-by from the i915
side. After that, I'm fine with merging this through alsa.

BR,
Jani.


[1] http://mid.gmane.org/cover.1404222047.git.jani.nikula@intel.com


>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Mengdong Lin <mengdong.lin@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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Lin, Mengdong]

Hi Jani,

> -----Original Message-----
> From: Nikula, Jani
> Sent: Thursday, July 03, 2014 3:33 PM
 
> I wrote this as a quick hack patch to try as an alternative to [1] which
> ended up not working on Haswell. Please reassure me that this is going to
> be a temporary solution until we get a more generic interface between the
> audio and display drivers. I don't much like this, but at least it's isolated
> and small.

Sure. We'll clean up code when the generic interface is ready. 
This is only a temporary solution, safer than the BIOS notifications.

And would you like to further revise this patch? 
But please keep this API not changed at the moment since the audio driver already queries the symbol name.

Thanks
Mengdong

> I'd like the commit message amended with something like:
> 
> """
> If the display power well has been disabled, the display audio controller
> divider values EM4 MVALUE and EM5 NVALUE will have been lost. The
> CDCLK frequency is required for reprogramming them. Provide a private
> interface for the audio driver to query CDCLK.
> 
> This is a stopgap solution until a more generic interface between audio
> and display drivers has been implemented.
> """
> 
> I'd also like to have an additional Reviewed-by from the i915 side. After
> that, I'm fine with merging this through alsa.

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

[Jani Nikula]

On Thu, 03 Jul 2014, "Lin, Mengdong" <mengdong.lin@intel.com> wrote:
> Hi Jani,
>
>> -----Original Message-----
>> From: Nikula, Jani
>> Sent: Thursday, July 03, 2014 3:33 PM
>  
>> I wrote this as a quick hack patch to try as an alternative to [1] which
>> ended up not working on Haswell. Please reassure me that this is going to
>> be a temporary solution until we get a more generic interface between the
>> audio and display drivers. I don't much like this, but at least it's isolated
>> and small.
>
> Sure. We'll clean up code when the generic interface is ready. 
> This is only a temporary solution, safer than the BIOS notifications.

Thanks.

> And would you like to further revise this patch? 
> But please keep this API not changed at the moment since the audio
> driver already queries the symbol name.

Please just take over the patch, and amend the commit message along the
lines of what I proposed. I think it's easiest if it gets merged through
the alsa tree - you have a dependency on it and your changes are much
bigger. The i915 changes are isolated and not expected to conflict.

But as I said, I'd like a Reviewed-by from another i915 developer
first. I don't like to push/ack my own patches without review from
others, no matter how small the patches.

Thanks,
Jani.



>
> Thanks
> Mengdong
>
>> I'd like the commit message amended with something like:
>> 
>> """
>> If the display power well has been disabled, the display audio controller
>> divider values EM4 MVALUE and EM5 NVALUE will have been lost. The
>> CDCLK frequency is required for reprogramming them. Provide a private
>> interface for the audio driver to query CDCLK.
>> 
>> This is a stopgap solution until a more generic interface between audio
>> and display drivers has been implemented.
>> """
>> 
>> I'd also like to have an additional Reviewed-by from the i915 side. After
>> that, I'm fine with merging this through alsa.

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Damien Lespiau]

On Thu, Jul 03, 2014 at 10:32:38AM +0300, Jani Nikula wrote:
> On Thu, 03 Jul 2014, mengdong.lin@intel.com wrote:
> > From: Jani Nikula <jani.nikula@intel.com>
> 
> I wrote this as a quick hack patch to try as an alternative to [1] which
> ended up not working on Haswell. Please reassure me that this is going
> to be a temporary solution until we get a more generic interface between
> the audio and display drivers. I don't much like this, but at least it's
> isolated and small.
> 
> I'd like the commit message amended with something like:
> 
> """
> If the display power well has been disabled, the display audio
> controller divider values EM4 MVALUE and EM5 NVALUE will have been
> lost. The CDCLK frequency is required for reprogramming them. Provide a
> private interface for the audio driver to query CDCLK.
> 
> This is a stopgap solution until a more generic interface between audio
> and display drivers has been implemented.
> """
> 
> I'd also like to have an additional Reviewed-by from the i915
> side. After that, I'm fine with merging this through alsa.

Sure.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

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

[Lin, Mengdong]

> -----Original Message-----
> From: Lespiau, Damien
> Sent: Thursday, July 03, 2014 7:19 PM
> To: Nikula, Jani
> Cc: Lin, Mengdong; alsa-devel@alsa-project.org; tiwai@suse.de;
> intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2 1/2] drm/i915: provide interface for audio
> driver to query cdclk
> 
> On Thu, Jul 03, 2014 at 10:32:38AM +0300, Jani Nikula wrote:
> > On Thu, 03 Jul 2014, mengdong.lin@intel.com wrote:
> > > From: Jani Nikula <jani.nikula@intel.com>
> >
> > I wrote this as a quick hack patch to try as an alternative to [1]
> > which ended up not working on Haswell. Please reassure me that this is
> > going to be a temporary solution until we get a more generic interface
> > between the audio and display drivers. I don't much like this, but at
> > least it's isolated and small.
> >
> > I'd like the commit message amended with something like:
> >
> > """
> > If the display power well has been disabled, the display audio
> > controller divider values EM4 MVALUE and EM5 NVALUE will have been
> > lost. The CDCLK frequency is required for reprogramming them. Provide
> > a private interface for the audio driver to query CDCLK.
> >
> > This is a stopgap solution until a more generic interface between
> > audio and display drivers has been implemented.
> > """
> >
> > I'd also like to have an additional Reviewed-by from the i915 side.
> > After that, I'm fine with merging this through alsa.
> 
> Sure.
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> 
> --
> Damien

Thank you, Jani and Damien!

I'll add the comment message and submit the revised patch tomorrow.

Regards
Mengdong

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

[mengdong.lin]

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

For Haswell and Broadwell, if the display power well has been disabled,
the display audio controller divider values EM4 M VALUE and EM5 N VALUE
will have been lost. The CDCLK frequency is required for reprogramming them
to generate 24MHz HD-A link BCLK. So provide a private interface for the
audio driver to query CDCLK.

This is a stopgap solution until a more generic interface between audio
and display drivers has been implemented.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@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 v3 1/2] drm/i915: provide interface for audio driver to query cdclk

[Takashi Iwai]

At Fri,  4 Jul 2014 10:00:37 +0800,
mengdong.lin@intel.com wrote:
> 
> From: Jani Nikula <jani.nikula@intel.com>
> 
> For Haswell and Broadwell, if the display power well has been disabled,
> the display audio controller divider values EM4 M VALUE and EM5 N VALUE
> will have been lost. The CDCLK frequency is required for reprogramming them
> to generate 24MHz HD-A link BCLK. So provide a private interface for the
> audio driver to query CDCLK.
> 
> This is a stopgap solution until a more generic interface between audio
> and display drivers has been implemented.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

Thanks, applied (with Cc to stable).


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
>