[Intel-gfx] [PATCH] drm/i915: set proper N/M in modeset

[Yang, Libin]

Hi Ville,

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Friday, July 29, 2016 5:47 PM
> To: Yang, Libin <libin.yang at intel.com>
> Cc: libin.yang at linux.intel.com; intel-gfx at lists.freedesktop.org;
> jani.nikula at linux.intel.com; Vetter, Daniel <daniel.vetter at intel.com>;
> tiwai at suse.de
> Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> 
> On Fri, Jul 29, 2016 at 05:54:23AM +0000, Yang, Libin wrote:
> > Hi Ville,
> >
> > > -----Original Message-----
> > > From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> > > Sent: Thursday, July 28, 2016 3:42 PM
> > > To: libin.yang at linux.intel.com
> > > Cc: intel-gfx at lists.freedesktop.org; jani.nikula at linux.intel.com;
> > > Vetter, Daniel <daniel.vetter at intel.com>; tiwai at suse.de; Yang, Libin
> > > <libin.yang at intel.com>
> > > Subject: Re: [PATCH] drm/i915: set proper N/M in modeset
> > >
> > > On Thu, Jul 14, 2016 at 03:06:21PM +0800, libin.yang at linux.intel.com
> wrote:
> > > > From: Libin Yang <libin.yang at linux.intel.com>
> > > >
> > > > When modeset occurs and the LS_CLK is set to some special values
> > > > in DP mode, the N/M need to be set manually if audio is playing.
> > > >
> > > > Also, the patch applies
> > > > commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset") to
> > > > APL platform.
> > > >
> > > > Signed-off-by: Libin Yang <libin.yang at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_reg.h    |   6 ++
> > > >  drivers/gpu/drm/i915/intel_audio.c | 116
> > > > ++++++++++++++++++++++++++++++++-----
> > > >  2 files changed, 108 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > > > b/drivers/gpu/drm/i915/i915_reg.h index 8bfde75..2f9d00e 100644
> > > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > > @@ -7351,6 +7351,12 @@ enum {
> > > >  #define _HSW_AUD_CONFIG_B		0x65100
> > > >  #define HSW_AUD_CFG(pipe)		_MMIO_PIPE(pipe,
> > > _HSW_AUD_CONFIG_A, _HSW_AUD_CONFIG_B)
> > > >
> > > > +#define _HSW_AUD_M_CTS_ENABLE_A		0x65028
> > > > +#define _HSW_AUD_M_CTS_ENABLE_B		0x65128
> > > > +#define HSW_AUD_M_CTS_ENABLE(pipe)
> 	_MMIO_PIPE(pipe,
> > > _HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
> > > > +#define   AUD_M_CTS_M_VALUE_INDEX	(1 << 21)
> > > > +#define   AUD_M_CTS_M_PROG_ENABLE	(1 << 20)
> > > > +
> > > >  #define _HSW_AUD_MISC_CTRL_A		0x65010
> > > >  #define _HSW_AUD_MISC_CTRL_B		0x65110
> > > >  #define HSW_AUD_MISC_CTRL(pipe)		_MMIO_PIPE(pipe,
> > > _HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
> > > > diff --git a/drivers/gpu/drm/i915/intel_audio.c
> > > > b/drivers/gpu/drm/i915/intel_audio.c
> > > > index 6700a7b..e2e4d4b 100644
> > > > --- a/drivers/gpu/drm/i915/intel_audio.c
> > > > +++ b/drivers/gpu/drm/i915/intel_audio.c
> > > > @@ -98,6 +98,22 @@ static const struct {
> > > >  	{ 192000, TMDS_297M, 20480, 247500 },  };
> > > >
> > > > +#define LC_533M 533250
> > > > +#define LC_148M 148500
> > > > +static const struct {
> > > > +	int sample_rate;
> > > > +	int clock;
> > > > +	int n;
> > > > +	int m;
> > > > +} aud_nm[] = {
> > > > +	{48000, LC_533M, 88875, 4096},
> > > > +	{44100, LC_533M, 148125, 6272},
> > > > +	{32000, LC_533M, 266625, 8192},
> > > > +	{48000, LC_148M, 12375, 2048},
> > > > +	{44100, LC_148M, 20625, 3136},
> > > > +	{32000, LC_148M, 37125, 4096},
> > > > +};
> > > > +
> > > >  /* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */  static
> > > > u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode
> > > > *adjusted_mode)  { @@ -121,20 +137,50 @@ static u32
> > > > audio_config_hdmi_pixel_clock(const struct drm_display_mode
> *adjusted
> > > >  	return hdmi_audio_clock[i].config;  }
> > > >
> > > > -static int audio_config_get_n(const struct drm_display_mode
> > > > *mode, int rate)
> > > > +static int audio_config_get_n(struct intel_crtc *crtc,
> > > > +			      const struct drm_display_mode *mode, int rate) {
> > > > +	int i;
> > > > +
> > > > +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
> > > > +		for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > > > +			if ((rate == aud_ncts[i].sample_rate) &&
> > > > +				(mode->clock == aud_ncts[i].clock)) {
> > > > +				return aud_ncts[i].n;
> > > > +			}
> > > > +		}
> > > > +	}
> > > > +
> > > > +	if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
> > > > +		for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
> > > > +			if ((rate == aud_nm[i].sample_rate) &&
> > > > +				(mode->clock == aud_nm[i].clock)) {
> > >
> > > So the spec says we should have "Maud / Naud = 512 * fs / f_LS_Clk",
> > > where fs is the audio sample rate, and f_LS_CLK is the link symbol clock.
> > > With that in mind this should not be looking at mode->clock but
> > > crtc->config->port_clock.
> > >
> > > So I don't actually understand why you say LS_CLK has "special" values.
> > > It shouldn't. It's always either 162, 270, or 540 MHz.
> >
> > Thanks for the correction. I will use crtc->config->port_clock.
> 
> But why do you need to do it at all? The hardware can't deal with one of the
> standard link rates on its own?

Yes, we found that the HW can't set the N/M automatically. This will cause
there is several seconds of silence at the beginning of audio playback.

> 
> >
> > >
> > > Actually even the HDMI case is wrong in the code, it should be
> > > looking at
> > > mode->crtc_clock instead of mode->clock. Or perhaps even port_clock,
> > > mode->if my
> > > reading of the HDMI spec is correct, but I never got any sane answer
> > > from any hw folks to my questions about this :(
> >
> > I will try mode->crtc_clock later for HDMI. Thanks.
> 
> While you're at it, can you pls do s/mode/adjusted_mode/ ? A while back I
> unified our naming convention for these things, but looks like the audio code
> has since gone back to the inconsisten old way.

Yes, I will do it, replace mode with adjusted_mode.

Regards,
Libin

> 
> >
> > Regards,
> > Libin
> >
> > >