| Message ID | 1485461141-14023-5-git-send-email-pierre-louis.bossart@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > Enable chicken bit on LPE mode setup and unmute amp on > notification > > FIXME: should these two phases done somewhere else? > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_reg.h | 12 ++++++++++++ > drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++ > 2 files changed, 39 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index a9ffc8d..ee90f64 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells { > #define I915_HDMI_LPE_AUDIO_BASE (VLV_DISPLAY_BASE + 0x65000) > #define I915_HDMI_LPE_AUDIO_SIZE 0x1000 > > +/* DisplayPort Audio w/ LPE */ > +#define CHICKEN_BIT_DBG_ENABLE (1 << 0) > +#define AMP_UNMUTE (1 << 1) The convention is to define registers first and the contents/bits for each register immedialy below. For groups of registers (like PORT_EN_B/C/D below) define all registers first and bits immediately below. (But note that the chicken register is not part of the group.) > +#define AUD_CHICKEN_BIT_REG 0x62F38 > +#define AUD_PORT_EN_B_DBG 0x62F20 > +#define AUD_PORT_EN_C_DBG 0x62F28 > +#define AUD_PORT_EN_D_DBG 0x62F2C Please don't define these separately without the display base, use (VLV_DISPLAY_BASE + 0x62f38) style instead. All the other uses of VLV_DISPLAY_BASE in the file follow the same convention. > +#define VLV_AUD_CHICKEN_BIT_REG _MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG) > +#define VLV_AUD_PORT_EN_B_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG) > +#define VLV_AUD_PORT_EN_C_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG) > +#define VLV_AUD_PORT_EN_D_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG) > + Would be nice to have a macro VLV_AUD_PORT_EN_DBG(port), but damn those reg offsets don't make it easy... > #define GEN6_BSD_RNCID _MMIO(0x12198) > > #define GEN7_FF_THREAD_MODE _MMIO(0x20a0) > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c > index 245523e..b3134ef 100644 > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c > @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv) > goto err_free_irq; > } > > + /* Enable DPAudio debug bits by default */ > + if (IS_CHERRYVIEW(dev_priv)) { > + u32 chicken_bit; > + > + chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG); > + I915_WRITE(VLV_AUD_CHICKEN_BIT_REG, > + chicken_bit | CHICKEN_BIT_DBG_ENABLE); > + } > + > return 0; > err_free_irq: > irq_free_desc(dev_priv->lpe_audio.irq); > @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > pdata->tmds_clock_speed = tmds_clk_speed; > if (link_rate) > pdata->link_rate = link_rate; > + > + if (dp_output) { /* unmute the amp */ > + u32 audio_enable; > + > + if (port == PORT_B) { > + audio_enable = I915_READ(VLV_AUD_PORT_EN_B_DBG); > + I915_WRITE(VLV_AUD_PORT_EN_B_DBG, > + audio_enable & ~AMP_UNMUTE); > + } else if (port == PORT_C) { > + audio_enable = I915_READ(VLV_AUD_PORT_EN_C_DBG); > + I915_WRITE(VLV_AUD_PORT_EN_C_DBG, > + audio_enable & ~AMP_UNMUTE); > + } else if (port == PORT_D) { > + audio_enable = I915_READ(VLV_AUD_PORT_EN_D_DBG); > + I915_WRITE(VLV_AUD_PORT_EN_D_DBG, > + audio_enable & ~AMP_UNMUTE); > + } > + } > } else { > memset(pdata->eld.eld_data, 0, > HDMI_MAX_ELD_BYTES);
On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote: > On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > > Enable chicken bit on LPE mode setup and unmute amp on > > notification > > > > FIXME: should these two phases done somewhere else? > > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > --- > > drivers/gpu/drm/i915/i915_reg.h | 12 ++++++++++++ > > drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++ > > 2 files changed, 39 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > index a9ffc8d..ee90f64 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells { > > #define I915_HDMI_LPE_AUDIO_BASE (VLV_DISPLAY_BASE + 0x65000) > > #define I915_HDMI_LPE_AUDIO_SIZE 0x1000 > > > > +/* DisplayPort Audio w/ LPE */ > > +#define CHICKEN_BIT_DBG_ENABLE (1 << 0) > > +#define AMP_UNMUTE (1 << 1) That should be called AMP_MUTE I think, > > The convention is to define registers first and the contents/bits for > each register immedialy below. For groups of registers (like > PORT_EN_B/C/D below) define all registers first and bits immediately > below. (But note that the chicken register is not part of the group.) > > > +#define AUD_CHICKEN_BIT_REG 0x62F38 Spec calls this AUD_CHICKENBIT_REG. Might as well follow it to the letter. > > +#define AUD_PORT_EN_B_DBG 0x62F20 > > +#define AUD_PORT_EN_C_DBG 0x62F28 > > +#define AUD_PORT_EN_D_DBG 0x62F2C These match the spec. But to match the standard i915 convention they should be called _AUD_PORT_EN_B_DBG etc. Same forthe chicken bit register. > > Please don't define these separately without the display base, use > (VLV_DISPLAY_BASE + 0x62f38) style instead. All the other uses of > VLV_DISPLAY_BASE in the file follow the same convention. > > > +#define VLV_AUD_CHICKEN_BIT_REG _MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG) > > +#define VLV_AUD_PORT_EN_B_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG) > > +#define VLV_AUD_PORT_EN_C_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG) > > +#define VLV_AUD_PORT_EN_D_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG) > > + > > Would be nice to have a macro VLV_AUD_PORT_EN_DBG(port), but damn those > reg offsets don't make it easy... _MMIO_PORT3(). > > > > #define GEN6_BSD_RNCID _MMIO(0x12198) > > > > #define GEN7_FF_THREAD_MODE _MMIO(0x20a0) > > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c > > index 245523e..b3134ef 100644 > > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c > > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c > > @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv) > > goto err_free_irq; > > } > > > > + /* Enable DPAudio debug bits by default */ > > + if (IS_CHERRYVIEW(dev_priv)) { VLV too. And like I said we might need this in the powerwell code as well. You should make a test to see if the register value is retained across the display power well being turned off. Eg. simply disable all displays, check the log to make sure it really did turn off the display power well, then re-enable some displays, and finally check if the register value was retained or not). > > + u32 chicken_bit; > > + > > + chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG); > > + I915_WRITE(VLV_AUD_CHICKEN_BIT_REG, > > + chicken_bit | CHICKEN_BIT_DBG_ENABLE); > > + } > > + > > return 0; > > err_free_irq: > > irq_free_desc(dev_priv->lpe_audio.irq); > > @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > > pdata->tmds_clock_speed = tmds_clk_speed; > > if (link_rate) > > pdata->link_rate = link_rate; > > + > > + if (dp_output) { /* unmute the amp */ The spec doesn't distinquish DP vs. HDMI here. So I presume we should be able to do this always. And I think we might want to mute things again when disabling audio. > > + u32 audio_enable; > > + > > + if (port == PORT_B) { > > + audio_enable = I915_READ(VLV_AUD_PORT_EN_B_DBG); > > + I915_WRITE(VLV_AUD_PORT_EN_B_DBG, > > + audio_enable & ~AMP_UNMUTE); > > + } else if (port == PORT_C) { > > + audio_enable = I915_READ(VLV_AUD_PORT_EN_C_DBG); > > + I915_WRITE(VLV_AUD_PORT_EN_C_DBG, > > + audio_enable & ~AMP_UNMUTE); > > + } else if (port == PORT_D) { > > + audio_enable = I915_READ(VLV_AUD_PORT_EN_D_DBG); > > + I915_WRITE(VLV_AUD_PORT_EN_D_DBG, > > + audio_enable & ~AMP_UNMUTE); > > + } > > + } > > } else { > > memset(pdata->eld.eld_data, 0, > > HDMI_MAX_ELD_BYTES); > > -- > Jani Nikula, Intel Open Source Technology Center
On Fri, 27 Jan 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote: >> On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: >> > Enable chicken bit on LPE mode setup and unmute amp on >> > notification >> > >> > FIXME: should these two phases done somewhere else? >> > >> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> >> > --- >> > drivers/gpu/drm/i915/i915_reg.h | 12 ++++++++++++ >> > drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++ >> > 2 files changed, 39 insertions(+) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h >> > index a9ffc8d..ee90f64 100644 >> > --- a/drivers/gpu/drm/i915/i915_reg.h >> > +++ b/drivers/gpu/drm/i915/i915_reg.h >> > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells { >> > #define I915_HDMI_LPE_AUDIO_BASE (VLV_DISPLAY_BASE + 0x65000) >> > #define I915_HDMI_LPE_AUDIO_SIZE 0x1000 >> > >> > +/* DisplayPort Audio w/ LPE */ >> > +#define CHICKEN_BIT_DBG_ENABLE (1 << 0) >> > +#define AMP_UNMUTE (1 << 1) > > That should be called AMP_MUTE I think, > >> >> The convention is to define registers first and the contents/bits for >> each register immedialy below. For groups of registers (like >> PORT_EN_B/C/D below) define all registers first and bits immediately >> below. (But note that the chicken register is not part of the group.) >> >> > +#define AUD_CHICKEN_BIT_REG 0x62F38 > > Spec calls this AUD_CHICKENBIT_REG. Might as well follow it to the > letter. > >> > +#define AUD_PORT_EN_B_DBG 0x62F20 >> > +#define AUD_PORT_EN_C_DBG 0x62F28 >> > +#define AUD_PORT_EN_D_DBG 0x62F2C > > These match the spec. But to match the standard i915 convention they > should be called _AUD_PORT_EN_B_DBG etc. Same forthe chicken bit > register. > >> >> Please don't define these separately without the display base, use >> (VLV_DISPLAY_BASE + 0x62f38) style instead. All the other uses of >> VLV_DISPLAY_BASE in the file follow the same convention. >> >> > +#define VLV_AUD_CHICKEN_BIT_REG _MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG) >> > +#define VLV_AUD_PORT_EN_B_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG) >> > +#define VLV_AUD_PORT_EN_C_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG) >> > +#define VLV_AUD_PORT_EN_D_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG) >> > + >> >> Would be nice to have a macro VLV_AUD_PORT_EN_DBG(port), but damn those >> reg offsets don't make it easy... > > _MMIO_PORT3(). Works for ports A, B, C, but here we have ports B, C, D. BR, Jani. > >> >> >> > #define GEN6_BSD_RNCID _MMIO(0x12198) >> > >> > #define GEN7_FF_THREAD_MODE _MMIO(0x20a0) >> > diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c >> > index 245523e..b3134ef 100644 >> > --- a/drivers/gpu/drm/i915/intel_lpe_audio.c >> > +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c >> > @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv) >> > goto err_free_irq; >> > } >> > >> > + /* Enable DPAudio debug bits by default */ >> > + if (IS_CHERRYVIEW(dev_priv)) { > > VLV too. And like I said we might need this in the powerwell code as > well. You should make a test to see if the register value is retained > across the display power well being turned off. Eg. simply disable all > displays, check the log to make sure it really did turn off the display > power well, then re-enable some displays, and finally check if the > register value was retained or not). > >> > + u32 chicken_bit; >> > + >> > + chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG); >> > + I915_WRITE(VLV_AUD_CHICKEN_BIT_REG, >> > + chicken_bit | CHICKEN_BIT_DBG_ENABLE); >> > + } >> > + >> > return 0; >> > err_free_irq: >> > irq_free_desc(dev_priv->lpe_audio.irq); >> > @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, >> > pdata->tmds_clock_speed = tmds_clk_speed; >> > if (link_rate) >> > pdata->link_rate = link_rate; >> > + >> > + if (dp_output) { /* unmute the amp */ > > The spec doesn't distinquish DP vs. HDMI here. So I presume we should be > able to do this always. > > And I think we might want to mute things again when disabling audio. > >> > + u32 audio_enable; >> > + >> > + if (port == PORT_B) { >> > + audio_enable = I915_READ(VLV_AUD_PORT_EN_B_DBG); >> > + I915_WRITE(VLV_AUD_PORT_EN_B_DBG, >> > + audio_enable & ~AMP_UNMUTE); >> > + } else if (port == PORT_C) { >> > + audio_enable = I915_READ(VLV_AUD_PORT_EN_C_DBG); >> > + I915_WRITE(VLV_AUD_PORT_EN_C_DBG, >> > + audio_enable & ~AMP_UNMUTE); >> > + } else if (port == PORT_D) { >> > + audio_enable = I915_READ(VLV_AUD_PORT_EN_D_DBG); >> > + I915_WRITE(VLV_AUD_PORT_EN_D_DBG, >> > + audio_enable & ~AMP_UNMUTE); >> > + } >> > + } >> > } else { >> > memset(pdata->eld.eld_data, 0, >> > HDMI_MAX_ELD_BYTES); >> >> -- >> Jani Nikula, Intel Open Source Technology Center
On Fri, Jan 27, 2017 at 03:51:34PM +0200, Jani Nikula wrote: > On Fri, 27 Jan 2017, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote: > >> On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > >> > Enable chicken bit on LPE mode setup and unmute amp on > >> > notification > >> > > >> > FIXME: should these two phases done somewhere else? > >> > > >> > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > >> > --- > >> > drivers/gpu/drm/i915/i915_reg.h | 12 ++++++++++++ > >> > drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++ > >> > 2 files changed, 39 insertions(+) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > >> > index a9ffc8d..ee90f64 100644 > >> > --- a/drivers/gpu/drm/i915/i915_reg.h > >> > +++ b/drivers/gpu/drm/i915/i915_reg.h > >> > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells { > >> > #define I915_HDMI_LPE_AUDIO_BASE (VLV_DISPLAY_BASE + 0x65000) > >> > #define I915_HDMI_LPE_AUDIO_SIZE 0x1000 > >> > > >> > +/* DisplayPort Audio w/ LPE */ > >> > +#define CHICKEN_BIT_DBG_ENABLE (1 << 0) > >> > +#define AMP_UNMUTE (1 << 1) > > > > That should be called AMP_MUTE I think, > > > >> > >> The convention is to define registers first and the contents/bits for > >> each register immedialy below. For groups of registers (like > >> PORT_EN_B/C/D below) define all registers first and bits immediately > >> below. (But note that the chicken register is not part of the group.) > >> > >> > +#define AUD_CHICKEN_BIT_REG 0x62F38 > > > > Spec calls this AUD_CHICKENBIT_REG. Might as well follow it to the > > letter. > > > >> > +#define AUD_PORT_EN_B_DBG 0x62F20 > >> > +#define AUD_PORT_EN_C_DBG 0x62F28 > >> > +#define AUD_PORT_EN_D_DBG 0x62F2C > > > > These match the spec. But to match the standard i915 convention they > > should be called _AUD_PORT_EN_B_DBG etc. Same forthe chicken bit > > register. > > > >> > >> Please don't define these separately without the display base, use > >> (VLV_DISPLAY_BASE + 0x62f38) style instead. All the other uses of > >> VLV_DISPLAY_BASE in the file follow the same convention. > >> > >> > +#define VLV_AUD_CHICKEN_BIT_REG _MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG) > >> > +#define VLV_AUD_PORT_EN_B_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG) > >> > +#define VLV_AUD_PORT_EN_C_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG) > >> > +#define VLV_AUD_PORT_EN_D_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG) > >> > + > >> > >> Would be nice to have a macro VLV_AUD_PORT_EN_DBG(port), but damn those > >> reg offsets don't make it easy... > > > > _MMIO_PORT3(). > > Works for ports A, B, C, but here we have ports B, C, D. (port)-PORT_B is easy enough to stick in there ;)
On Fri, Jan 27, 2017 at 03:17:34PM +0200, Ville Syrjälä wrote: > On Fri, Jan 27, 2017 at 12:08:58PM +0200, Jani Nikula wrote: > > On Thu, 26 Jan 2017, Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> wrote: > > > Enable chicken bit on LPE mode setup and unmute amp on > > > notification > > > > > > FIXME: should these two phases done somewhere else? > > > > > > Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 12 ++++++++++++ > > > drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++ > > > 2 files changed, 39 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > > > index a9ffc8d..ee90f64 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells { > > > #define I915_HDMI_LPE_AUDIO_BASE (VLV_DISPLAY_BASE + 0x65000) > > > #define I915_HDMI_LPE_AUDIO_SIZE 0x1000 > > > > > > +/* DisplayPort Audio w/ LPE */ > > > +#define CHICKEN_BIT_DBG_ENABLE (1 << 0) > > > +#define AMP_UNMUTE (1 << 1) > > That should be called AMP_MUTE I think, > > > > > The convention is to define registers first and the contents/bits for > > each register immedialy below. For groups of registers (like > > PORT_EN_B/C/D below) define all registers first and bits immediately > > below. (But note that the chicken register is not part of the group.) > > > > > +#define AUD_CHICKEN_BIT_REG 0x62F38 > > Spec calls this AUD_CHICKENBIT_REG. Might as well follow it to the > letter. > > > > +#define AUD_PORT_EN_B_DBG 0x62F20 > > > +#define AUD_PORT_EN_C_DBG 0x62F28 > > > +#define AUD_PORT_EN_D_DBG 0x62F2C > > These match the spec. But to match the standard i915 convention they > should be called _AUD_PORT_EN_B_DBG etc. Same forthe chicken bit > register. Actually they just match one version of the spec I had lying around. Another versions says: AUD_PORT_EN_B_DBG 0x62F20 AUD_PORT_EN_C_DBG 0x62F30 AUD_PORT_EN_D_DBG 0x62F34
Thanks Jani and Ville for the comments. Couple of precisions needed below: >> >>> #define GEN6_BSD_RNCID _MMIO(0x12198) >>> >>> #define GEN7_FF_THREAD_MODE _MMIO(0x20a0) >>> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c >>> index 245523e..b3134ef 100644 >>> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c >>> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c >>> @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv) >>> goto err_free_irq; >>> } >>> >>> + /* Enable DPAudio debug bits by default */ >>> + if (IS_CHERRYVIEW(dev_priv)) { > VLV too. And like I said we might need this in the powerwell code as > well. You should make a test to see if the register value is retained > across the display power well being turned off. Eg. simply disable all > displays, check the log to make sure it really did turn off the display > power well, then re-enable some displays, and finally check if the > register value was retained or not). VLV has DisplayPort? I thought this was an addition on CHT, and I really don't know of any devices with this combination of HDaudio disabled+DP. I'd rather keep this CHT-only until we find a device where this can be tested. On the powerwell, I could use more guidance. i tried this first solution to see if streaming worked (and it did :-)). I don't mind moving the code somewhere else but I have no idea where. > >>> + u32 chicken_bit; >>> + >>> + chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG); >>> + I915_WRITE(VLV_AUD_CHICKEN_BIT_REG, >>> + chicken_bit | CHICKEN_BIT_DBG_ENABLE); >>> + } >>> + >>> return 0; >>> err_free_irq: >>> irq_free_desc(dev_priv->lpe_audio.irq); >>> @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, >>> pdata->tmds_clock_speed = tmds_clk_speed; >>> if (link_rate) >>> pdata->link_rate = link_rate; >>> + >>> + if (dp_output) { /* unmute the amp */ > The spec doesn't distinquish DP vs. HDMI here. So I presume we should be > able to do this always. I'll try to see if HDMI still works with this. We could tentatively add unmute in all cases but I'll need to add a test for Baytrail (no PORT_D) so in the end it's the same number of tests. > > And I think we might want to mute things again when disabling audio. I was wondering if there would be side effects of writing to a register controlling a port if that port is not connected any longer. I'll give it a try.
On Fri, Jan 27, 2017 at 08:47:50AM -0600, Pierre-Louis Bossart wrote: > Thanks Jani and Ville for the comments. Couple of precisions needed below: > >> > >>> #define GEN6_BSD_RNCID _MMIO(0x12198) > >>> > >>> #define GEN7_FF_THREAD_MODE _MMIO(0x20a0) > >>> diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c > >>> index 245523e..b3134ef 100644 > >>> --- a/drivers/gpu/drm/i915/intel_lpe_audio.c > >>> +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c > >>> @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv) > >>> goto err_free_irq; > >>> } > >>> > >>> + /* Enable DPAudio debug bits by default */ > >>> + if (IS_CHERRYVIEW(dev_priv)) { > > VLV too. And like I said we might need this in the powerwell code as > > well. You should make a test to see if the register value is retained > > across the display power well being turned off. Eg. simply disable all > > displays, check the log to make sure it really did turn off the display > > power well, then re-enable some displays, and finally check if the > > register value was retained or not). > VLV has DisplayPort? Yes. And as I said earlier the docs don't make any distinciton between HDMI and DP in the audio programming sequence. So based on the docs we should always unmute the amp manually. >I thought this was an addition on CHT, and I really > don't know of any devices with this combination of HDaudio disabled+DP. > I'd rather keep this CHT-only until we find a device where this can be > tested. > > On the powerwell, I could use more guidance. i tried this first solution > to see if streaming worked (and it did :-)). I don't mind moving the > code somewhere else but I have no idea where. 1. boot with drm.debug=0xe 1. 'intel_reg read 0x62f38' just after boot, to see that the chicken bit is as it should 2. disable all displays (eg. xset dpms force off) 3. grep 'power_well.*display' (make sure the last thing it says is "disabling" 4. turn on the displays (eg. xset dpms force on) 5. 'intel_reg read 0x62f38' to check the chicken bit again If it didn't survive then we need to set it in either in vlv_display_power_well_init(), or we could just set it whenever we set the AMP_MUTE bit for any of the ports. > > >>> + u32 chicken_bit; > >>> + > >>> + chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG); > >>> + I915_WRITE(VLV_AUD_CHICKEN_BIT_REG, > >>> + chicken_bit | CHICKEN_BIT_DBG_ENABLE); > >>> + } > >>> + > >>> return 0; > >>> err_free_irq: > >>> irq_free_desc(dev_priv->lpe_audio.irq); > >>> @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, > >>> pdata->tmds_clock_speed = tmds_clk_speed; > >>> if (link_rate) > >>> pdata->link_rate = link_rate; > >>> + > >>> + if (dp_output) { /* unmute the amp */ > > The spec doesn't distinquish DP vs. HDMI here. So I presume we should be > > able to do this always. > > I'll try to see if HDMI still works with this. We could tentatively add > unmute in all cases but I'll need to add a test for Baytrail (no PORT_D) > so in the end it's the same number of tests. > > > > > And I think we might want to mute things again when disabling audio. > I was wondering if there would be side effects of writing to a register > controlling a port if that port is not connected any longer. I'll give > it a try. Nothing apart from HPD circuitry really cares if there's anything connectect or not.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index a9ffc8d..ee90f64 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -2061,6 +2061,18 @@ enum skl_disp_power_wells { #define I915_HDMI_LPE_AUDIO_BASE (VLV_DISPLAY_BASE + 0x65000) #define I915_HDMI_LPE_AUDIO_SIZE 0x1000 +/* DisplayPort Audio w/ LPE */ +#define CHICKEN_BIT_DBG_ENABLE (1 << 0) +#define AMP_UNMUTE (1 << 1) +#define AUD_CHICKEN_BIT_REG 0x62F38 +#define AUD_PORT_EN_B_DBG 0x62F20 +#define AUD_PORT_EN_C_DBG 0x62F28 +#define AUD_PORT_EN_D_DBG 0x62F2C +#define VLV_AUD_CHICKEN_BIT_REG _MMIO(VLV_DISPLAY_BASE + AUD_CHICKEN_BIT_REG) +#define VLV_AUD_PORT_EN_B_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_B_DBG) +#define VLV_AUD_PORT_EN_C_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_C_DBG) +#define VLV_AUD_PORT_EN_D_DBG _MMIO(VLV_DISPLAY_BASE + AUD_PORT_EN_D_DBG) + #define GEN6_BSD_RNCID _MMIO(0x12198) #define GEN7_FF_THREAD_MODE _MMIO(0x20a0) diff --git a/drivers/gpu/drm/i915/intel_lpe_audio.c b/drivers/gpu/drm/i915/intel_lpe_audio.c index 245523e..b3134ef 100644 --- a/drivers/gpu/drm/i915/intel_lpe_audio.c +++ b/drivers/gpu/drm/i915/intel_lpe_audio.c @@ -248,6 +248,15 @@ static int lpe_audio_setup(struct drm_i915_private *dev_priv) goto err_free_irq; } + /* Enable DPAudio debug bits by default */ + if (IS_CHERRYVIEW(dev_priv)) { + u32 chicken_bit; + + chicken_bit = I915_READ(VLV_AUD_CHICKEN_BIT_REG); + I915_WRITE(VLV_AUD_CHICKEN_BIT_REG, + chicken_bit | CHICKEN_BIT_DBG_ENABLE); + } + return 0; err_free_irq: irq_free_desc(dev_priv->lpe_audio.irq); @@ -357,6 +366,24 @@ void intel_lpe_audio_notify(struct drm_i915_private *dev_priv, pdata->tmds_clock_speed = tmds_clk_speed; if (link_rate) pdata->link_rate = link_rate; + + if (dp_output) { /* unmute the amp */ + u32 audio_enable; + + if (port == PORT_B) { + audio_enable = I915_READ(VLV_AUD_PORT_EN_B_DBG); + I915_WRITE(VLV_AUD_PORT_EN_B_DBG, + audio_enable & ~AMP_UNMUTE); + } else if (port == PORT_C) { + audio_enable = I915_READ(VLV_AUD_PORT_EN_C_DBG); + I915_WRITE(VLV_AUD_PORT_EN_C_DBG, + audio_enable & ~AMP_UNMUTE); + } else if (port == PORT_D) { + audio_enable = I915_READ(VLV_AUD_PORT_EN_D_DBG); + I915_WRITE(VLV_AUD_PORT_EN_D_DBG, + audio_enable & ~AMP_UNMUTE); + } + } } else { memset(pdata->eld.eld_data, 0, HDMI_MAX_ELD_BYTES);
Enable chicken bit on LPE mode setup and unmute amp on notification FIXME: should these two phases done somewhere else? Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> --- drivers/gpu/drm/i915/i915_reg.h | 12 ++++++++++++ drivers/gpu/drm/i915/intel_lpe_audio.c | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+)