[Intel-gfx] [PATCH v2 2/4] drm/i915: implement set_ncts callback
Yang, Libin
libin.yang at intel.com
Mon Aug 10 19:49:50 PDT 2015
Hi Jani,
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula at linux.intel.com]
> Sent: Monday, August 10, 2015 8:00 PM
> To: 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, 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
>
> You're not using this register in this patch.
Oh, yes. In my first inside version, I use it. But remove using it
later. I will remove it. Thanks.
>
> > +
> > #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;
> > +
> > + /* 1. get the pipe */
> > + for_each_intel_encoder(drm_dev, intel_encoder) {
> > + intel_dig_port = enc_to_dig_port(&intel_encoder-
> >base);
> > + if (port == intel_dig_port->port) {
> > + crtc = to_intel_crtc(intel_encoder->base.crtc);
> > + if (!crtc) {
> > + DRM_DEBUG_KMS("%s: crtc is NULL\n",
> __func__);
> > + continue;
> > + }
> > + pipe = crtc->pipe;
> > + break;
> > + }
> > + }
> > +
> > + if (pipe == -1) {
>
> INVALID_PIPE
Got it. Thanks.
>
> > + DRM_DEBUG_KMS("no pipe for the port %c\n",
> port_name(port));
> > + return -ENODEV;
> > + }
> > + DRM_DEBUG_KMS("pipe %c connects port %c\n",
> > + pipe_name(pipe), port_name(port));
> > + mode = &crtc->config->base.adjusted_mode;
> > +
> > + /* 2. Need set the N/CTS manually at some frequencies */
> > + if ((mode->clock != TMDS_297M) &&
> > + (mode->clock != TMDS_296M)) {
> > + tmp = I915_READ(HSW_AUD_CFG(pipe));
> > + tmp &= ~AUD_CONFIG_N_PROG_ENABLE;
> > + I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > + return 0;
> > + }
>
> I'm thinking it would be better to always use the manual setting. There
> may be obscure bugs due to *sometimes* using automatic mode and
> some
> other times not. Or maybe we could use automatic mode in error
> scenarios?
We have done a lot test using automatic mode in other
TMDS (resolution). If we always use the manual setting,
we may need too much testing on other TMDS on different platforms.
It seems using automatic mode in error scenarios will cause no sound.
>
> > +
> > + /* 3. calculate the N/CTS */
> > + for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
> > + if ((rate == aud_ncts[i].sample_rate) &&
> > + (mode->clock == aud_ncts[i].clock)) {
> > + n = aud_ncts[i].n;
> > + break;
> > + }
> > + }
>
> This part looks like a function to be abstracted (see
> e.g. audio_config_hdmi_pixel_clock).
OK. I will use a function for it.
>
> > + if (n == 0)
> > + return 0;
>
> This needs at least a debug warning and perhaps reverting to the
> automatic mode.
OK. I see.
>
> > + n_low = n & 0xfff;
> > + n_up = (n >> 12) & 0xff;
> > +
> > + /* 4. set the N/CTS */
> > + tmp = I915_READ(HSW_AUD_CFG(pipe));
> > + tmp |= AUD_CONFIG_N_PROG_ENABLE;
> > + tmp &= ~AUD_CONFIG_UPPER_N_MASK;
> > + tmp |= (n_up << AUD_CONFIG_UPPER_N_SHIFT);
> > + tmp &= ~AUD_CONFIG_LOWER_N_MASK;
> > + tmp |= (n_low << AUD_CONFIG_LOWER_N_SHIFT);
>
> You could squash the ors and ands together.
OK.
Regards,
Libin
>
> > + I915_WRITE(HSW_AUD_CFG(pipe), tmp);
> > +
> > + return 0;
> > +}
> > +
> > static const struct i915_audio_component_ops
> i915_audio_component_ops = {
> > .owner = THIS_MODULE,
> > .get_power = i915_audio_component_get_power,
> > .put_power = i915_audio_component_put_power,
> > .codec_wake_override =
> i915_audio_component_codec_wake_override,
> > .get_cdclk_freq = i915_audio_component_get_cdclk_freq,
> > + .set_ncts = i915_audio_component_set_ncts,
> > };
> >
> > static int i915_audio_component_bind(struct device *i915_dev,
> > --
> > 1.9.1
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Jani Nikula, Intel Open Source Technology Center
More information about the Intel-gfx
mailing list