[alsa-devel] [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback

[Yang, Libin]

Hi Daniel,

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Wednesday, August 12, 2015 9:06 PM
> To: Jani Nikula
> Cc: Yang, Libin; alsa-devel at alsa-project.org; tiwai at suse.de; intel-
> gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch
> Subject: Re: [Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts
> callback
> 
> On Mon, Aug 10, 2015 at 03:16:46PM +0300, Jani Nikula wrote:
> > On Mon, 10 Aug 2015, libin.yang at intel.com wrote:
> > > From: Libin Yang <libin.yang at intel.com>
> > >
> > > Display audio may not work at some frequencies
> > > with the HW provided N/CTS.
> > >
> > > This patch sets the proper N value for the
> > > given audio sample rate at the impacted frequencies.
> > > At other frequencies, it will use the N/CTS value
> > > which HW provides.
> > >
> > > Signed-off-by: Libin Yang <libin.yang at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_reg.h    |  2 +
> > >  drivers/gpu/drm/i915/intel_audio.c | 95
> ++++++++++++++++++++++++++++++++++++++
> > >  2 files changed, 97 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > > index ea46d68..da2d128 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7014,6 +7014,8 @@ enum skl_disp_power_wells {
> > >  					_HSW_AUD_MISC_CTRL_A, \
> > >  					_HSW_AUD_MISC_CTRL_B)
> > >
> > > +#define HSW_AUD_PIPE_CONN_SEL_CTRL  0x650ac
> > > +
> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_A	0x650b4
> > >  #define _HSW_AUD_DIP_ELD_CTRL_ST_B	0x651b4
> > >  #define HSW_AUD_DIP_ELD_CTRL(pipe) _PIPE(pipe, \
> > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> b/drivers/gpu/drm/i915/intel_audio.c
> > > index dc32cf4..eddf37f 100644
> > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > @@ -68,6 +68,30 @@ static const struct {
> > >  	{ 148500, AUD_CONFIG_PIXEL_CLOCK_HDMI_148500 },
> > >  };
> > >
> > > +#define TMDS_297M 297000
> > > +#define TMDS_296M DIV_ROUND_UP(297000 * 1000, 1001)
> > > +static const struct {
> > > +	int sample_rate;
> > > +	int clock;
> > > +	int n;
> > > +	int cts;
> > > +} aud_ncts[] = {
> > > +	{ 44100, TMDS_296M, 4459, 234375 },
> > > +	{ 44100, TMDS_297M, 4704, 247500 },
> > > +	{ 48000, TMDS_296M, 5824, 281250 },
> > > +	{ 48000, TMDS_297M, 5120, 247500 },
> > > +	{ 32000, TMDS_296M, 5824, 421875 },
> > > +	{ 32000, TMDS_297M, 3072, 222750 },
> > > +	{ 88200, TMDS_296M, 8918, 234375 },
> > > +	{ 88200, TMDS_297M, 9408, 247500 },
> > > +	{ 96000, TMDS_296M, 11648, 281250 },
> > > +	{ 96000, TMDS_297M, 10240, 247500 },
> > > +	{ 176400, TMDS_296M, 17836, 234375 },
> > > +	{ 176400, TMDS_297M, 18816, 247500 },
> > > +	{ 44100, TMDS_296M, 23296, 281250 },
> > > +	{ 44100, TMDS_297M, 20480, 247500 },
> > > +};
> > > +
> > >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
> > >  static u32 audio_config_hdmi_pixel_clock(struct
> drm_display_mode *mode)
> > >  {
> > > @@ -514,12 +538,83 @@ static int
> i915_audio_component_get_cdclk_freq(struct device *dev)
> > >  	return ret;
> > >  }
> > >
> > > +static int i915_audio_component_set_ncts(struct device *dev, int
> port,
> > > +			int dev_entry, int rate)
> > > +{
> > > +	struct drm_i915_private *dev_priv = dev_to_i915(dev);
> > > +	struct drm_device *drm_dev = dev_priv->dev;
> > > +	struct intel_encoder *intel_encoder;
> > > +	struct intel_digital_port *intel_dig_port;
> > > +	struct intel_crtc *crtc;
> > > +	struct drm_display_mode *mode;
> > > +	enum pipe pipe = -1;
> > > +	u32 tmp;
> > > +	int i;
> > > +	int n_low, n_up, n = 0;
> >
> > I think you'll need the power well get here, and put at the end. Or
> > check that we it.
> 
> If we call this and end up actually dropping the power well then the
> register writes will go exactly nowhere at all. Which indicates a bug in
> how this is orchestrated. There is probably one ...

Sorry, I'm not understanding your meaning clearly. 
Do you mean if we put the power well, the register's value will
be invalid? Actually, we found another issue for audio power
management (this is another bug, not related to this feature),
and I'm thinking to add get_power/put_power as Jani said
in another patch.

Regards,
Libin

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch