[PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[mengdong.lin]

From: Mukesh <mukeshx.arora@intel.com>

The code defines a new function intel_disable_audio() to implement the
entire audio codec disable sequence, called by intel_disable_ddi() in
DDI port disablement. This audio codec disbale sequence is implemented
as per the recommendation of the Bspec.

[Patch modified by Mendong to apply to both HDMI and DP port.]

Signed-off-by: Mukesh Arora <mukeshx.arora@intel.com>
Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index b042ee5..2946fe7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1088,6 +1088,45 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 	I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
 }
 
+/* audio codec disable sequence, as per Bspec */
+void intel_disable_audio(struct intel_encoder *intel_encoder)
+{
+	int type = intel_encoder->type;
+	struct drm_encoder *encoder = &intel_encoder->base;
+	struct drm_device *dev = encoder->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
+	int pipe = intel_crtc->pipe;
+	int aud_config = HSW_AUD_CFG(pipe);
+	u32 temp;
+
+	/* disable timestamps */
+	temp = I915_READ(aud_config);
+	if (type == INTEL_OUTPUT_HDMI)
+		temp &= ~AUD_CONFIG_N_VALUE_INDEX;
+	else if (type == INTEL_OUTPUT_DISPLAYPORT)
+		temp |= AUD_CONFIG_N_VALUE_INDEX;
+	else
+		return;
+	temp |= AUD_CONFIG_N_PROG_ENABLE;
+	temp &= ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE);
+	I915_WRITE(aud_config, temp);
+
+	/* Disable ELDV and ELD buffer */
+	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+	temp &= ~(AUDIO_ELD_VALID_A << (pipe * 4));
+	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
+
+	/* Wait for 2 vertical blanks */
+	intel_wait_for_vblank(dev, pipe);
+	intel_wait_for_vblank(dev, pipe);
+
+	/* Disable audio PD. This is optional as per Bspec.  */
+	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+	temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
+	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
+}
+
 static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 {
 	struct drm_encoder *encoder = &intel_encoder->base;
@@ -1132,18 +1171,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	struct drm_encoder *encoder = &intel_encoder->base;
 	struct drm_crtc *crtc = encoder->crtc;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
 	int type = intel_encoder->type;
-	struct drm_device *dev = encoder->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	uint32_t tmp;
 
-	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
-		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
-			 (pipe * 4));
-		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
-	}
+	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP)
+		intel_disable_audio(intel_encoder);
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-- 
1.8.1.2

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Ben Widawsky]

On Thu, Aug 29, 2013 at 07:50:20PM -0400, mengdong.lin@intel.com wrote:
> From: Mukesh <mukeshx.arora@intel.com>
> 
> The code defines a new function intel_disable_audio() to implement the
> entire audio codec disable sequence, called by intel_disable_ddi() in
> DDI port disablement. This audio codec disbale sequence is implemented
> as per the recommendation of the Bspec.
> 
> [Patch modified by Mendong to apply to both HDMI and DP port.]
> 
> Signed-off-by: Mukesh Arora <mukeshx.arora@intel.com>
> Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index b042ee5..2946fe7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1088,6 +1088,45 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>  	I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
>  }
>  
> +/* audio codec disable sequence, as per Bspec */
> +void intel_disable_audio(struct intel_encoder *intel_encoder)
> +{
> +	int type = intel_encoder->type;
> +	struct drm_encoder *encoder = &intel_encoder->base;
> +	struct drm_device *dev = encoder->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> +	int pipe = intel_crtc->pipe;
> +	int aud_config = HSW_AUD_CFG(pipe);
> +	u32 temp;
> +

I don't know what it means, but where is the : "Disable sample
fabrication" step?

> +	/* disable timestamps */
> +	temp = I915_READ(aud_config);
> +	if (type == INTEL_OUTPUT_HDMI)
> +		temp &= ~AUD_CONFIG_N_VALUE_INDEX;
> +	else if (type == INTEL_OUTPUT_DISPLAYPORT)
> +		temp |= AUD_CONFIG_N_VALUE_INDEX;
> +	else
> +		return;
> +	temp |= AUD_CONFIG_N_PROG_ENABLE;
> +	temp &= ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE);
> +	I915_WRITE(aud_config, temp);
> +
> +	/* Disable ELDV and ELD buffer */
> +	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> +	temp &= ~(AUDIO_ELD_VALID_A << (pipe * 4));
> +	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> +

POSTING_READ or a comment explaining why you don't need one.

> +	/* Wait for 2 vertical blanks */
> +	intel_wait_for_vblank(dev, pipe);
> +	intel_wait_for_vblank(dev, pipe);
> +
> +	/* Disable audio PD. This is optional as per Bspec.  */
Please add the comment when software might want to skip this step (it
seems relevant to me for future programming)
> +	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> +	temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> +	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> +}
> +

The bspec isn't clear about any implied waits in the steps, but it's
probably safer to add a POSTING_READ after each I915_WRITE, unless you
specifically know the HW doesn't need the last write to have landed.

With all comments addressed, it's:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

>  static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  {
>  	struct drm_encoder *encoder = &intel_encoder->base;
> @@ -1132,18 +1171,10 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  	struct drm_encoder *encoder = &intel_encoder->base;
>  	struct drm_crtc *crtc = encoder->crtc;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> -	int pipe = intel_crtc->pipe;
>  	int type = intel_encoder->type;
> -	struct drm_device *dev = encoder->dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -	uint32_t tmp;
>  
> -	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
> -		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> -		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
> -			 (pipe * 4));
> -		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> -	}
> +	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP)
> +		intel_disable_audio(intel_encoder);
>  
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);

This hunk looks fine.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Daniel Vetter]

On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> +       /* Wait for 2 vertical blanks */
> +       intel_wait_for_vblank(dev, pipe);
> +       intel_wait_for_vblank(dev, pipe);
> +
> +       /* Disable audio PD. This is optional as per Bspec.  */
> +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);

If this is optional do we really need the two vblank waits above?
Adding them just for fun when we generally try to rip out as many
vblank waits as possible from the modeset code isn't all that great
...

Also I'd really like to see the audio stuff being tracked in the pipe
config instead of splattering these different ad-hoc state bits like
intel_crtc->eld_vld all over the place.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Ville Syrjälä]

On Wed, Sep 04, 2013 at 08:50:13PM +0200, Daniel Vetter wrote:
> On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> > +       /* Wait for 2 vertical blanks */
> > +       intel_wait_for_vblank(dev, pipe);
> > +       intel_wait_for_vblank(dev, pipe);
> > +
> > +       /* Disable audio PD. This is optional as per Bspec.  */
> > +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> 
> If this is optional do we really need the two vblank waits above?
> Adding them just for fun when we generally try to rip out as many
> vblank waits as possible from the modeset code isn't all that great
> ...

One idea I had for these kinds of vblank waits (there also one required
for IPS for instance) is that we might just sample a vblank counter
after the first step, then at the latest point we can, we'd wait for the
frame counter to have passed the sampled vaoue + whatever extra is
needed. That might allow us to do other stuff in parallel while the
required number of vblanks will elapese.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Daniel Vetter]

On Thu, Sep 05, 2013 at 02:33:20PM +0300, Ville Syrjälä wrote:
> On Wed, Sep 04, 2013 at 08:50:13PM +0200, Daniel Vetter wrote:
> > On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> > > +       /* Wait for 2 vertical blanks */
> > > +       intel_wait_for_vblank(dev, pipe);
> > > +       intel_wait_for_vblank(dev, pipe);
> > > +
> > > +       /* Disable audio PD. This is optional as per Bspec.  */
> > > +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > > +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > > +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> > 
> > If this is optional do we really need the two vblank waits above?
> > Adding them just for fun when we generally try to rip out as many
> > vblank waits as possible from the modeset code isn't all that great
> > ...
> 
> One idea I had for these kinds of vblank waits (there also one required
> for IPS for instance) is that we might just sample a vblank counter
> after the first step, then at the latest point we can, we'd wait for the
> frame counter to have passed the sampled vaoue + whatever extra is
> needed. That might allow us to do other stuff in parallel while the
> required number of vblanks will elapese.

My solution for this is to have vblank work items that we can use to chain
off all these things. We also need them for pageflips e.g. when
re-enabling fbc or similar stuff. The problem is a bit that for switching
things off like in a modeset the synchronization can get hairy and will be
little-tested. For enabling as long as we share the code with the nuclear
pageflip code we should be fine though ...

Hence why I think we should try rather hard to avoid these vblank waits in
the first place.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Ville Syrjälä]

On Thu, Sep 05, 2013 at 01:45:47PM +0200, Daniel Vetter wrote:
> On Thu, Sep 05, 2013 at 02:33:20PM +0300, Ville Syrjälä wrote:
> > On Wed, Sep 04, 2013 at 08:50:13PM +0200, Daniel Vetter wrote:
> > > On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> > > > +       /* Wait for 2 vertical blanks */
> > > > +       intel_wait_for_vblank(dev, pipe);
> > > > +       intel_wait_for_vblank(dev, pipe);
> > > > +
> > > > +       /* Disable audio PD. This is optional as per Bspec.  */
> > > > +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > > > +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > > > +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> > > 
> > > If this is optional do we really need the two vblank waits above?
> > > Adding them just for fun when we generally try to rip out as many
> > > vblank waits as possible from the modeset code isn't all that great
> > > ...
> > 
> > One idea I had for these kinds of vblank waits (there also one required
> > for IPS for instance) is that we might just sample a vblank counter
> > after the first step, then at the latest point we can, we'd wait for the
> > frame counter to have passed the sampled vaoue + whatever extra is
> > needed. That might allow us to do other stuff in parallel while the
> > required number of vblanks will elapese.
> 
> My solution for this is to have vblank work items that we can use to chain
> off all these things. We also need them for pageflips e.g. when
> re-enabling fbc or similar stuff. The problem is a bit that for switching
> things off like in a modeset the synchronization can get hairy and will be
> little-tested. For enabling as long as we share the code with the nuclear
> pageflip code we should be fine though ...
> 
> Hence why I think we should try rather hard to avoid these vblank waits in
> the first place.

Yeah, for enabling we definitely want to execute all that stuff
asynchronously.

The disable case is more problematic. For atomic pageflip we might get
away with returning -EAGAIN or something, but we have to block in the
full modeset case unless we want to move the entire modeset to a work
queue. So this trick would mostly be relevant for the disable during
modeset case.

But of course we could do that stuff via some kind of
schedule_vblank_work(currentl_vbl_count + X) and synchronize_vblank_work()
just before the critical step that needs the vblank work to be done. But
the locking and bugs might be more complicated in this case.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Daniel Vetter]

On Thu, Sep 05, 2013 at 03:21:01PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 05, 2013 at 01:45:47PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 05, 2013 at 02:33:20PM +0300, Ville Syrjälä wrote:
> > > On Wed, Sep 04, 2013 at 08:50:13PM +0200, Daniel Vetter wrote:
> > > > On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> > > > > +       /* Wait for 2 vertical blanks */
> > > > > +       intel_wait_for_vblank(dev, pipe);
> > > > > +       intel_wait_for_vblank(dev, pipe);
> > > > > +
> > > > > +       /* Disable audio PD. This is optional as per Bspec.  */
> > > > > +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > > > > +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > > > > +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> > > > 
> > > > If this is optional do we really need the two vblank waits above?
> > > > Adding them just for fun when we generally try to rip out as many
> > > > vblank waits as possible from the modeset code isn't all that great
> > > > ...
> > > 
> > > One idea I had for these kinds of vblank waits (there also one required
> > > for IPS for instance) is that we might just sample a vblank counter
> > > after the first step, then at the latest point we can, we'd wait for the
> > > frame counter to have passed the sampled vaoue + whatever extra is
> > > needed. That might allow us to do other stuff in parallel while the
> > > required number of vblanks will elapese.
> > 
> > My solution for this is to have vblank work items that we can use to chain
> > off all these things. We also need them for pageflips e.g. when
> > re-enabling fbc or similar stuff. The problem is a bit that for switching
> > things off like in a modeset the synchronization can get hairy and will be
> > little-tested. For enabling as long as we share the code with the nuclear
> > pageflip code we should be fine though ...
> > 
> > Hence why I think we should try rather hard to avoid these vblank waits in
> > the first place.
> 
> Yeah, for enabling we definitely want to execute all that stuff
> asynchronously.
> 
> The disable case is more problematic. For atomic pageflip we might get
> away with returning -EAGAIN or something, but we have to block in the
> full modeset case unless we want to move the entire modeset to a work
> queue. So this trick would mostly be relevant for the disable during
> modeset case.
> 
> But of course we could do that stuff via some kind of
> schedule_vblank_work(currentl_vbl_count + X) and synchronize_vblank_work()
> just before the critical step that needs the vblank work to be done. But
> the locking and bugs might be more complicated in this case.

Yeah we'll end up doing a maze of what will essentially be async tasklets
and callbacks. A declarative way to do that would imo be a requirement,
but such fanciness is hard to pull of in C without turning into something
really ugly ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Lin, Mengdong]

Hi Ben,

Thank you very much for the review. I'm sorry I missed the feedback for quite a long time.
Please see comments below ...

> -----Original Message-----
> From: Ben Widawsky [mailto:ben@bwidawsk.net]
> Sent: Thursday, September 05, 2013 1:03 AM
> To: Lin, Mengdong
> Cc: intel-gfx@lists.freedesktop.org; Arora, MukeshX
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/hsw: Add display Audio codec
> disable sequence for Haswell
> 
> On Thu, Aug 29, 2013 at 07:50:20PM -0400, mengdong.lin@intel.com wrote:
> > From: Mukesh <mukeshx.arora@intel.com>
> >
> > The code defines a new function intel_disable_audio() to implement the
> > entire audio codec disable sequence, called by intel_disable_ddi() in
> > DDI port disablement. This audio codec disbale sequence is implemented
> > as per the recommendation of the Bspec.
> >
> > [Patch modified by Mendong to apply to both HDMI and DP port.]
> >
> > Signed-off-by: Mukesh Arora <mukeshx.arora@intel.com>
> > Signed-off-by: Mengdong Lin <mengdong.lin@intel.com>
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index b042ee5..2946fe7 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1088,6 +1088,45 @@ static void intel_ddi_post_disable(struct
> intel_encoder *intel_encoder)
> >  	I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);  }
> >
> > +/* audio codec disable sequence, as per Bspec */ void
> > +intel_disable_audio(struct intel_encoder *intel_encoder) {
> > +	int type = intel_encoder->type;
> > +	struct drm_encoder *encoder = &intel_encoder->base;
> > +	struct drm_device *dev = encoder->dev;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > +	int pipe = intel_crtc->pipe;
> > +	int aud_config = HSW_AUD_CFG(pipe);
> > +	u32 temp;
> > +
> 
> I don't know what it means, but where is the : "Disable sample fabrication"
> step?

No, there is no "Disable sample fabrication" step, because sample fabrication is never enabled.
I'll add a comment in code about this.

> > +	/* disable timestamps */
> > +	temp = I915_READ(aud_config);
> > +	if (type == INTEL_OUTPUT_HDMI)
> > +		temp &= ~AUD_CONFIG_N_VALUE_INDEX;
> > +	else if (type == INTEL_OUTPUT_DISPLAYPORT)
> > +		temp |= AUD_CONFIG_N_VALUE_INDEX;
> > +	else
> > +		return;
> > +	temp |= AUD_CONFIG_N_PROG_ENABLE;
> > +	temp &=
> ~(AUD_CONFIG_UPPER_N_VALUE|AUD_CONFIG_LOWER_N_VALUE);
> > +	I915_WRITE(aud_config, temp);
> > +
> > +	/* Disable ELDV and ELD buffer */
> > +	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > +	temp &= ~(AUDIO_ELD_VALID_A << (pipe * 4));
> > +	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> > +
> 
> POSTING_READ or a comment explaining why you don't need one.

I'll add POST_READ.

> > +	/* Wait for 2 vertical blanks */
> > +	intel_wait_for_vblank(dev, pipe);
> > +	intel_wait_for_vblank(dev, pipe);
> > +
> > +	/* Disable audio PD. This is optional as per Bspec.  */
> Please add the comment when software might want to skip this step (it seems
> relevant to me for future programming)
> > +	temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > +	temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > +	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp); }
> > +
> 
> The bspec isn't clear about any implied waits in the steps, but it's probably
> safer to add a POSTING_READ after each I915_WRITE, unless you specifically
> know the HW doesn't need the last write to have landed.
> 
> With all comments addressed, it's:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Okay. Thanks!
Mengdong

> >  static void intel_enable_ddi(struct intel_encoder *intel_encoder)  {
> >  	struct drm_encoder *encoder = &intel_encoder->base; @@ -1132,18
> > +1171,10 @@ static void intel_disable_ddi(struct intel_encoder
> *intel_encoder)
> >  	struct drm_encoder *encoder = &intel_encoder->base;
> >  	struct drm_crtc *crtc = encoder->crtc;
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > -	int pipe = intel_crtc->pipe;
> >  	int type = intel_encoder->type;
> > -	struct drm_device *dev = encoder->dev;
> > -	struct drm_i915_private *dev_priv = dev->dev_private;
> > -	uint32_t tmp;
> >
> > -	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
> > -		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > -		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
> > -			 (pipe * 4));
> > -		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> > -	}
> > +	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP)
> > +		intel_disable_audio(intel_encoder);
> >
> >  	if (type == INTEL_OUTPUT_EDP) {
> >  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> 
> This hunk looks fine.
> 
> --
> Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Lin, Mengdong]

Hi Daniel and Ville,

Thanks for your feedback and please see comments below ...
I'm very sorry I missed your mails for a long time.

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Thursday, September 05, 2013 2:50 AM
> To: Lin, Mengdong
> Cc: intel-gfx; Arora, MukeshX
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/hsw: Add display Audio codec
> disable sequence for Haswell
> 
> On Fri, Aug 30, 2013 at 1:50 AM,  <mengdong.lin@intel.com> wrote:
> > +       /* Wait for 2 vertical blanks */
> > +       intel_wait_for_vblank(dev, pipe);
> > +       intel_wait_for_vblank(dev, pipe);
> > +
> > +       /* Disable audio PD. This is optional as per Bspec.  */
> > +       temp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> > +       temp &= ~(AUDIO_OUTPUT_ENABLE_A << (pipe * 4));
> > +       I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, temp);
> 
> If this is optional do we really need the two vblank waits above?
> Adding them just for fun when we generally try to rip out as many vblank waits
> as possible from the modeset code isn't all that great ...

The two vblank wait is requested by b-spec, not optional.
Can we reserve them now until we got confirmation from HW owner that's safe to remove them?
Or until we have better implementation of vblank wait?

> Also I'd really like to see the audio stuff being tracked in the pipe config instead
> of splattering these different ad-hoc state bits like intel_crtc->eld_vld all over
> the place.
> -Daniel

How about adding a flag "has_audio" to intel_crtc->config?
If okay, I'll write a patch to clean up checking on intel_crtc->eld_vld here and there.

Thanks
Mengdong

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Daniel Vetter]

On Mon, Sep 23, 2013 at 10:52 AM, Lin, Mengdong <mengdong.lin@intel.com> wrote:
>> Also I'd really like to see the audio stuff being tracked in the pipe config instead
>> of splattering these different ad-hoc state bits like intel_crtc->eld_vld all over
>> the place.
>> -Daniel
>
> How about adding a flag "has_audio" to intel_crtc->config?
> If okay, I'll write a patch to clean up checking on intel_crtc->eld_vld here and there.

That's actually my plan: HDMI/DP encoders should set config->has_audio
in their ->compute_config if we have audio enabled, and everything
else should then just check intel_crtc->config.has_audio. If you do
the patch for hsw I'll volunteer to convert older platforms.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH v2] drm/i915/hsw: Add display Audio codec disable sequence for Haswell

[Lin, Mengdong]

> -----Original Message-----
> From: daniel.vetter@ffwll.ch [mailto:daniel.vetter@ffwll.ch] On Behalf Of
> Daniel Vetter
> Sent: Monday, September 23, 2013 4:57 PM
> To: Lin, Mengdong
> Cc: ville.syrjala@linux.intel.com; intel-gfx; Arora, MukeshX
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/hsw: Add display Audio codec
> disable sequence for Haswell
> 
> On Mon, Sep 23, 2013 at 10:52 AM, Lin, Mengdong <mengdong.lin@intel.com>
> wrote:
> >> Also I'd really like to see the audio stuff being tracked in the pipe
> >> config instead of splattering these different ad-hoc state bits like
> >> intel_crtc->eld_vld all over the place.
> >> -Daniel
> >
> > How about adding a flag "has_audio" to intel_crtc->config?
> > If okay, I'll write a patch to clean up checking on intel_crtc->eld_vld here and
> there.
> 
> That's actually my plan: HDMI/DP encoders should set config->has_audio in
> their ->compute_config if we have audio enabled, and everything else should
> then just check intel_crtc->config.has_audio. If you do the patch for hsw I'll
> volunteer to convert older platforms.
> -Daniel
> --

Okay. I'll do the patch for HSW.

Thanks
Mengdong