[01/15] drm/i915: only disable DDI sound if intel_crtc->eld_vld

Commit Message

Paulo Zanoni March 6, 2013, 11:03 p.m. UTC

From: Paulo Zanoni <[e-mail]>

We already have the same check on intel_enable_ddi. This patch
prevents "unclaimed register" messages when the power well is
disabled.

Signed-off-by: Paulo Zanoni <[e-mail]>
---
 drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Ville Syrjala March 7, 2013, 9:31 a.m. UTC | #1

On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <[e-mail]>
> 
> We already have the same check on intel_enable_ddi. This patch
> prevents "unclaimed register" messages when the power well is
> disabled.
> 
> Signed-off-by: Paulo Zanoni <[e-mail]>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 56bb7cb..cd2f519 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  		ironlake_edp_backlight_off(intel_dp);
>  	}
>  
> -	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) {
> +		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);
> +	}

We set eld_vld=false before disabling the crtc in intel_crtc_disable().
I think you need to rearrange that so that we clear eld_vld only
after ->crtc_disable has been called.

>  }
>  
>  int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> [e-mail]
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Daniel Vetter March 17, 2013, 8:17 p.m. UTC | #2

On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <[e-mail]>
> > 
> > We already have the same check on intel_enable_ddi. This patch
> > prevents "unclaimed register" messages when the power well is
> > disabled.
> > 
> > Signed-off-by: Paulo Zanoni <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 56bb7cb..cd2f519 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >  		ironlake_edp_backlight_off(intel_dp);
> >  	}
> >  
> > -	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) {
> > +		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);
> > +	}
> 
> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> I think you need to rearrange that so that we clear eld_vld only
> after ->crtc_disable has been called.

Just looked a bit through the code here and ->eld_vld is another case of
where the ddi encoder needs to know something which only the connector
really knows currently. Since in our dp/hdmi/sdvo code we just check
<connector>->has_audio in the respective modeset function.

And it's not really the only place where the apparently common ddi
functions are just if ladders gropping around in connector details.

I guess we need to eventually clean this mess up, once things have settled
a bit, since I fear the duplication it icky little bugs this fragmentation
of state keeping might (or probably will) cause.

Ideas welcome ;-)

Cheers, Daniel

Daniel Vetter March 17, 2013, 8:23 p.m. UTC | #3

On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <[e-mail]>
> > 
> > We already have the same check on intel_enable_ddi. This patch
> > prevents "unclaimed register" messages when the power well is
> > disabled.
> > 
> > Signed-off-by: Paulo Zanoni <[e-mail]>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 56bb7cb..cd2f519 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >  		ironlake_edp_backlight_off(intel_dp);
> >  	}
> >  
> > -	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) {
> > +		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);
> > +	}
> 
> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> I think you need to rearrange that so that we clear eld_vld only
> after ->crtc_disable has been called.

I've forgotten to drop my bikeshed on the patch itself:

This looks a bit fishy since currently we assume that disabling something
just works (especially clearing a few registers). And I don't really
understand how we can hit unclaimed register issues since the pipe should
be enabled when we call this function here ...

So either transcoder eDP doesn't have audio, in which case I think it'd be
better to check for that here (plus ensure that we yell at callers for
integrated eDP in e.g. hsw_write_eld). Or it _does_ have audio, but the
audio stuff is in the power well. In which case we need to add a check.

Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
don't want to fire up the work machine ;-)

Cheers, Daniel

Paulo Zanoni March 20, 2013, 10:03 p.m. UTC | #4

Hi

2013/3/17 Daniel Vetter <[e-mail]>:
> On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
>> On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <[e-mail]>
>> >
>> > We already have the same check on intel_enable_ddi. This patch
>> > prevents "unclaimed register" messages when the power well is
>> > disabled.
>> >
>> > Signed-off-by: Paulo Zanoni <[e-mail]>
>> > ---
>> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> > index 56bb7cb..cd2f519 100644
>> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>> >             ironlake_edp_backlight_off(intel_dp);
>> >     }
>> >
>> > -   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) {
>> > +           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);
>> > +   }
>>
>> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
>> I think you need to rearrange that so that we clear eld_vld only
>> after ->crtc_disable has been called.
>
> I've forgotten to drop my bikeshed on the patch itself:
>
> This looks a bit fishy since currently we assume that disabling something
> just works (especially clearing a few registers). And I don't really
> understand how we can hit unclaimed register issues since the pipe should
> be enabled when we call this function here ...

The audio registers are on the power well. My test case is: eDP panel
without sound. We don't hit unclaimed registers at the ->enable
function because it's protected by eld_vld (which is false, because we
don't have sound), but then at the disable path we just
unconditionally touch the registers which are on the power down well,
so "unclaimed register".

>
> So either transcoder eDP doesn't have audio, in which case I think it'd be
> better to check for that here (plus ensure that we yell at callers for
> integrated eDP in e.g. hsw_write_eld).

The eld_vld is the "check for audio" which you're asking for.

> Or it _does_ have audio, but the
> audio stuff is in the power well. In which case we need to add a check.

We need to patch haswell_modeset_global_resources to enable the power
well in case eDP has sound, but this is an unrelated bug with a
different patch. It's on my TODO list too.

Still, Ville's comment is valid, so I need to resend.

>
> Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
> don't want to fire up the work machine ;-)
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> [phone] - http://blog.ffwll.ch

Daniel Vetter March 20, 2013, 10:24 p.m. UTC | #5

On Wed, Mar 20, 2013 at 07:03:25PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/17 Daniel Vetter <[e-mail]>:
> > On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> >> On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote:
> >> > From: Paulo Zanoni <[e-mail]>
> >> >
> >> > We already have the same check on intel_enable_ddi. This patch
> >> > prevents "unclaimed register" messages when the power well is
> >> > disabled.
> >> >
> >> > Signed-off-by: Paulo Zanoni <[e-mail]>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
> >> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> > index 56bb7cb..cd2f519 100644
> >> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >> >             ironlake_edp_backlight_off(intel_dp);
> >> >     }
> >> >
> >> > -   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) {
> >> > +           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);
> >> > +   }
> >>
> >> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> >> I think you need to rearrange that so that we clear eld_vld only
> >> after ->crtc_disable has been called.
> >
> > I've forgotten to drop my bikeshed on the patch itself:
> >
> > This looks a bit fishy since currently we assume that disabling something
> > just works (especially clearing a few registers). And I don't really
> > understand how we can hit unclaimed register issues since the pipe should
> > be enabled when we call this function here ...
> 
> The audio registers are on the power well. My test case is: eDP panel
> without sound. We don't hit unclaimed registers at the ->enable
> function because it's protected by eld_vld (which is false, because we
> don't have sound), but then at the disable path we just
> unconditionally touch the registers which are on the power down well,
> so "unclaimed register".
> 
> >
> > So either transcoder eDP doesn't have audio, in which case I think it'd be
> > better to check for that here (plus ensure that we yell at callers for
> > integrated eDP in e.g. hsw_write_eld).
> 
> The eld_vld is the "check for audio" which you're asking for.

Ok, now I've been slightly less lazy and quickly checked the docs. If I'm
reading the diagrams and audio connections correctly, then there's _no_
audio support at all for the eDP transcoder. Which means we'd need to
protect both the enable and disable side in trancoder != TRANS_EDP checks.

That's what I've meant with proper check. The eld_vld thing is a bit
ad-hoc and imo due to our ddi encoder/connector split: Only the connector
has the edid and so knows whether the sync claims to support audio, but we
need that information in the ddi encoder functions.

But since our current ddi code is full of such little control inversions
where the ddi code has to dig out some piece of information about the
current connector from somewhere, I don't care and postpone this to a time
when hsw support has really settled.
-Daniel

> 
> > Or it _does_ have audio, but the
> > audio stuff is in the power well. In which case we need to add a check.
> 
> We need to patch haswell_modeset_global_resources to enable the power
> well in case eDP has sound, but this is an unrelated bug with a
> different patch. It's on my TODO list too.
> 
> Still, Ville's comment is valid, so I need to resend.
> 
> >
> > Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
> > don't want to fire up the work machine ;-)
> >
> > Cheers, Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > [phone] - http://blog.ffwll.ch
> 
> 
> 
> -- 
> Paulo Zanoni

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 56bb7cb..cd2f519 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1347,9 +1347,12 @@  static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 		ironlake_edp_backlight_off(intel_dp);
 	}
 
-	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) {
+		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);
+	}
 }
 
 int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)