On Fri, 27 Jan 2017, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> > ---
>> > 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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
