From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 01/15] drm/i915: only disable DDI sound if intel_crtc->eld_vld Date: Sun, 17 Mar 2013 21:23:44 +0100 Message-ID: <20130317202344.GJ9021@phenom.ffwll.local> References: <1362611003-4823-1-git-send-email-przanoni@gmail.com> <1362611003-4823-2-git-send-email-przanoni@gmail.com> <20130307093123.GQ4469@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-ee0-f47.google.com (mail-ee0-f47.google.com [74.125.83.47]) by gabe.freedesktop.org (Postfix) with ESMTP id 70FE8E5C0E for ; Sun, 17 Mar 2013 13:21:00 -0700 (PDT) Received: by mail-ee0-f47.google.com with SMTP id e52so2271263eek.6 for ; Sun, 17 Mar 2013 13:20:59 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130307093123.GQ4469@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: "Runyan, Arthur J" , intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrj=E4l=E4 wrote: > On Wed, Mar 06, 2013 at 08:03:08PM -0300, Paulo Zanoni wrote: > > From: Paulo Zanoni > > = > > 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 > > --- > > 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/in= tel_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_encod= er *intel_encoder) > > ironlake_edp_backlight_off(intel_dp); > > } > > = > > - tmp =3D I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > > - tmp &=3D ~((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 =3D I915_READ(HSW_AUD_PIN_ELD_CP_VLD); > > + tmp &=3D ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << > > + (pipe * 4)); > > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp); > > + } > = > We set eld_vld=3Dfalse 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 -- = Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch