| Message ID | 1358477515-7323-1-git-send-email-xingchao.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: > ELD info should be updated dynamically according to hot plug event. > For haswell chip, clear/set the eld valid bit and output enable bit > from callback intel_disable/eanble_ddi(). Hmm. Is it OK to set the ELD valid bit if the ELD hasn't actually been written? And besides these bits are already set by haswell_write_eld(). > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> > --- > drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index f02b3fe..7ce4728 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -1274,10 +1274,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) > static void intel_enable_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; > struct drm_device *dev = encoder->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > enum port port = intel_ddi_get_encoder_port(intel_encoder); > int type = intel_encoder->type; > + int tmp; > > if (type == INTEL_OUTPUT_HDMI) { > /* In HDMI/DVI mode, the port width, and swing/emphasis values > @@ -1290,18 +1294,32 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) > > ironlake_edp_backlight_on(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); > } > > 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; > + int tmp; > > if (type == INTEL_OUTPUT_EDP) { > struct intel_dp *intel_dp = enc_to_intel_dp(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); > } > > int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv) > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Friday, January 18, 2013 6:46 PM > To: Wang, Xingchao > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, Paulo R > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info refresh > support for Haswell > > On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: > > ELD info should be updated dynamically according to hot plug event. > > For haswell chip, clear/set the eld valid bit and output enable bit > > from callback intel_disable/eanble_ddi(). > > Hmm. Is it OK to set the ELD valid bit if the ELD hasn't actually been written? This triggers an unsolicited event to ALSA driver which continue to read ELD info. > > And besides these bits are already set by haswell_write_eld(). Intel_disable/enable_ddi() was called even after haswell_write_eld(), so we need set the bits again. Thanks --xingchao > > > > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> > > --- > > drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++++++++ > > 1 file changed, 18 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index f02b3fe..7ce4728 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -1274,10 +1274,14 @@ static void intel_ddi_post_disable(struct > > intel_encoder *intel_encoder) static void intel_enable_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; > > struct drm_device *dev = encoder->dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > enum port port = intel_ddi_get_encoder_port(intel_encoder); > > int type = intel_encoder->type; > > + int tmp; > > > > if (type == INTEL_OUTPUT_HDMI) { > > /* In HDMI/DVI mode, the port width, and swing/emphasis values > @@ > > -1290,18 +1294,32 @@ static void intel_enable_ddi(struct intel_encoder > > *intel_encoder) > > > > ironlake_edp_backlight_on(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); > > } > > > > 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; > > + int tmp; > > > > if (type == INTEL_OUTPUT_EDP) { > > struct intel_dp *intel_dp = enc_to_intel_dp(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); > > } > > > > int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv) > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC
On Fri, Jan 18, 2013 at 12:00:10PM +0000, Wang, Xingchao wrote: > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Friday, January 18, 2013 6:46 PM > > To: Wang, Xingchao > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, Paulo R > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info refresh > > support for Haswell > > > > On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: > > > ELD info should be updated dynamically according to hot plug event. > > > For haswell chip, clear/set the eld valid bit and output enable bit > > > from callback intel_disable/eanble_ddi(). > > > > Hmm. Is it OK to set the ELD valid bit if the ELD hasn't actually been written? > > This triggers an unsolicited event to ALSA driver which continue to read ELD info. I take it we don't want that to happen? > > And besides these bits are already set by haswell_write_eld(). > > Intel_disable/enable_ddi() was called even after haswell_write_eld(), so we need set the bits again. For the mode set sequence only intel_enable_ddi() is called after haswell_write_eld(). This is how the sequence should end up looking with the current code: intel_set_mode() -> haswell_crtc_disable() -> intel_disable_ddi() -> intel_crtc_mode_set() -> haswell_crtc_mode_set() -> intel_ddi_mode_set() -> intel_write_eld() -> haswell_write_eld() -> haswell_crtc_enable() -> intel_enable_ddi() But for DPMS on->off->on there would be calls to haswell_crtc_disable() and haswell_crtc_enable() w/o calls to haswell_write_eld(). I suppose this is the problem you want to fix? So, perhaps there should be a flag somewhere that would be cleared at the beginning of the mode_set operation, and then intel_write_eld() should set the flag when the ELD was written succesfully. intel_enable_ddi() could check the flag and only set the ELD valid bit when the flag is set. Should non-ddi platforms do something similar as well? > Thanks > --xingchao > > > > > > > > Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++++++++ > > > 1 file changed, 18 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > > b/drivers/gpu/drm/i915/intel_ddi.c > > > index f02b3fe..7ce4728 100644 > > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > > @@ -1274,10 +1274,14 @@ static void intel_ddi_post_disable(struct > > > intel_encoder *intel_encoder) static void intel_enable_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; > > > struct drm_device *dev = encoder->dev; > > > struct drm_i915_private *dev_priv = dev->dev_private; > > > enum port port = intel_ddi_get_encoder_port(intel_encoder); > > > int type = intel_encoder->type; > > > + int tmp; > > > > > > if (type == INTEL_OUTPUT_HDMI) { > > > /* In HDMI/DVI mode, the port width, and swing/emphasis values > > @@ > > > -1290,18 +1294,32 @@ static void intel_enable_ddi(struct intel_encoder > > > *intel_encoder) > > > > > > ironlake_edp_backlight_on(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); > > > } > > > > > > 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; > > > + int tmp; > > > > > > if (type == INTEL_OUTPUT_EDP) { > > > struct intel_dp *intel_dp = enc_to_intel_dp(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); > > > } > > > > > > int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv) > > > -- > > > 1.7.9.5 > > > > > > _______________________________________________ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC
Hi Ville Syrjälä, > -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Friday, January 18, 2013 9:14 PM > To: Wang, Xingchao > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, Paulo R > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info refresh > support for Haswell > > On Fri, Jan 18, 2013 at 12:00:10PM +0000, Wang, Xingchao wrote: > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > Sent: Friday, January 18, 2013 6:46 PM > > > To: Wang, Xingchao > > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, > > > Paulo R > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info > > > refresh support for Haswell > > > > > > On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: > > > > ELD info should be updated dynamically according to hot plug event. > > > > For haswell chip, clear/set the eld valid bit and output enable > > > > bit from callback intel_disable/eanble_ddi(). > > > > > > Hmm. Is it OK to set the ELD valid bit if the ELD hasn't actually been > written? > > > > This triggers an unsolicited event to ALSA driver which continue to read ELD > info. > > I take it we don't want that to happen? > > > > And besides these bits are already set by haswell_write_eld(). > > > > Intel_disable/enable_ddi() was called even after haswell_write_eld(), so we > need set the bits again. > > For the mode set sequence only intel_enable_ddi() is called after > haswell_write_eld(). > > This is how the sequence should end up looking with the current > code: > > intel_set_mode() > -> haswell_crtc_disable() > -> intel_disable_ddi() > -> intel_crtc_mode_set() > -> haswell_crtc_mode_set() > -> intel_ddi_mode_set() > -> intel_write_eld() > -> haswell_write_eld() > -> haswell_crtc_enable() > -> intel_enable_ddi() > > > But for DPMS on->off->on there would be calls to haswell_crtc_disable() and > haswell_crtc_enable() w/o calls to haswell_write_eld(). I suppose this is the > problem you want to fix? Exactly. Intel_disable_ddi() was called after haswell_write_eld() only once during system bootup. I added a flag to monitor ELD valid status. It will be set in haswell_write_eld() and cleared in intel_disable_ddi(). In intel_enable_ddi() , will check this flag and set the ELD valid bit again only if the flag was cleared before. I had tested the new patch and it works well. I will send out the updated patch later. > > So, perhaps there should be a flag somewhere that would be cleared at the > beginning of the mode_set operation, and then intel_write_eld() should set the > flag when the ELD was written succesfully. > intel_enable_ddi() could check the flag and only set the ELD valid bit when the > flag is set. > > Should non-ddi platforms do something similar as well? Would you share more ideas on this? I have not noticed the potential issues here. Thanks --xingchao
On Mon, Jan 21, 2013 at 05:40:23AM +0000, Wang, Xingchao wrote: > Hi Ville Syrjälä, > > > -----Original Message----- > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > Sent: Friday, January 18, 2013 9:14 PM > > To: Wang, Xingchao > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, Paulo R > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info refresh > > support for Haswell > > > > On Fri, Jan 18, 2013 at 12:00:10PM +0000, Wang, Xingchao wrote: > > > > > > > -----Original Message----- > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > > Sent: Friday, January 18, 2013 6:46 PM > > > > To: Wang, Xingchao > > > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, > > > > Paulo R > > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info > > > > refresh support for Haswell > > > > > > > > On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: > > > > > ELD info should be updated dynamically according to hot plug event. > > > > > For haswell chip, clear/set the eld valid bit and output enable > > > > > bit from callback intel_disable/eanble_ddi(). > > > > > > > > Hmm. Is it OK to set the ELD valid bit if the ELD hasn't actually been > > written? > > > > > > This triggers an unsolicited event to ALSA driver which continue to read ELD > > info. > > > > I take it we don't want that to happen? > > > > > > And besides these bits are already set by haswell_write_eld(). > > > > > > Intel_disable/enable_ddi() was called even after haswell_write_eld(), so we > > need set the bits again. > > > > For the mode set sequence only intel_enable_ddi() is called after > > haswell_write_eld(). > > > > This is how the sequence should end up looking with the current > > code: > > > > intel_set_mode() > > -> haswell_crtc_disable() > > -> intel_disable_ddi() > > -> intel_crtc_mode_set() > > -> haswell_crtc_mode_set() > > -> intel_ddi_mode_set() > > -> intel_write_eld() > > -> haswell_write_eld() > > -> haswell_crtc_enable() > > -> intel_enable_ddi() > > > > > > But for DPMS on->off->on there would be calls to haswell_crtc_disable() and > > haswell_crtc_enable() w/o calls to haswell_write_eld(). I suppose this is the > > problem you want to fix? > > Exactly. Intel_disable_ddi() was called after haswell_write_eld() only once during system bootup. > I added a flag to monitor ELD valid status. It will be set in haswell_write_eld() and cleared in intel_disable_ddi(). > In intel_enable_ddi() , will check this flag and set the ELD valid bit again only if the flag was cleared before. But that would make the code behave almost the same as the old patch, no? It would make intel_enable_ddi() set the ELD valid bit, even if ELD wasn't written. My idea was that intel_disable_ddi() would not clear the flag, and intel_enable_ddi() would only write the ELD valid bit if the flag is set. That way we never write the ELD valid bit to the register when there is no valid ELD. So something likes this perhaps: intel_ddi_mode_set() { ... + intel_encoder->eld_valid = false; ... intel_write_eld(); ... } haswell_write_eld() { ... + intel_encoder->eld_valid = true; } intel_enable_ddi() { ... + if (intel_encoder->eld_valid) + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD); } intel_disable_ddi() { ... + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD) } One extra idea would be to have something like 'uint32_t eld_reg' instead of 'bool eld_valid'. That was you could just do 'I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, intel_encoder->eld_reg);' in intel_eanble_ddi() without having to recompute the register value. And intel_encodr->eld_reg would be updated with the new value in haswell_write_eld(). > I had tested the new patch and it works well. I will send out the updated patch later. > > > > So, perhaps there should be a flag somewhere that would be cleared at the > > beginning of the mode_set operation, and then intel_write_eld() should set the > > flag when the ELD was written succesfully. > > intel_enable_ddi() could check the flag and only set the ELD valid bit when the > > flag is set. > > > > Should non-ddi platforms do something similar as well? > > Would you share more ideas on this? I have not noticed the potential issues here. No ideas really. I was just wondering if we need to clear/set some ELD bits in the encoder disable/enable functions for older HW as well. At least we seem to have similar registers for older HW: G4X_AUD_CNTL_ST IBX_AUD_CNTL_ST2 CPT_AUD_CNTRL_ST2 But now I'm wondering if we're possibly overdesigning this thing. Would it be enough to simply clear the ELD valid bit at the beginning of intel_mode_set()? That way the bit would always be cleared when the display is really disabled, but it would not be touched when DPMS state is changed. I don't know which is more appropriate for DPMS off state; should the ELD valid bit be left alone, or should it be cleared? So something like this: intel_set_mode() { ... + for_each_intel_crtc_masked(dev, disable_pipes|prepare_pipes, intel_crtc) + intel_crtc_clear_eld(&intel_crtc->base); for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) intel_crtc_disable(&intel_crtc->base); ... } Where intel_crtc_clear_eld() would call some function similar to write_eld(), except that function would only clear the ELD valid bit.
Hi Wang, Sorry for not getting back you sooner. Thanks for clarifications and update. About current pipe sorry for my confusion... Maybe to avoid confusions like this and make it more clear we could have macros like: #define AUDIO_OUTPUT_ENABLE(pipe) (AUDIO_OUTPUT_ENABLE_A << (pipe * 4)) #define AUDIO_ELD_VALID(pipe) (AUDIO_ELD_VALID_A << (pipe * 4)) but maybe it is just me... since I notice there are code already using the style you used... About the flag logic I have nothing to add. I didn't see any problem in write it even when the eld wasn't written yet. Anyways, I don't see any actual issue on the current patch so feel free decide about remaining questions by yourself and use Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> Thanks, Rodrigo. On Mon, Jan 21, 2013 at 11:11 AM, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Jan 21, 2013 at 05:40:23AM +0000, Wang, Xingchao wrote: >> Hi Ville Syrjälä, >> >> > -----Original Message----- >> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> > Sent: Friday, January 18, 2013 9:14 PM >> > To: Wang, Xingchao >> > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, Paulo R >> > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info refresh >> > support for Haswell >> > >> > On Fri, Jan 18, 2013 at 12:00:10PM +0000, Wang, Xingchao wrote: >> > > >> > > > -----Original Message----- >> > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >> > > > Sent: Friday, January 18, 2013 6:46 PM >> > > > To: Wang, Xingchao >> > > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, >> > > > Paulo R >> > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info >> > > > refresh support for Haswell >> > > > >> > > > On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: >> > > > > ELD info should be updated dynamically according to hot plug event. >> > > > > For haswell chip, clear/set the eld valid bit and output enable >> > > > > bit from callback intel_disable/eanble_ddi(). >> > > > >> > > > Hmm. Is it OK to set the ELD valid bit if the ELD hasn't actually been >> > written? >> > > >> > > This triggers an unsolicited event to ALSA driver which continue to read ELD >> > info. >> > >> > I take it we don't want that to happen? >> > >> > > > And besides these bits are already set by haswell_write_eld(). >> > > >> > > Intel_disable/enable_ddi() was called even after haswell_write_eld(), so we >> > need set the bits again. >> > >> > For the mode set sequence only intel_enable_ddi() is called after >> > haswell_write_eld(). >> > >> > This is how the sequence should end up looking with the current >> > code: >> > >> > intel_set_mode() >> > -> haswell_crtc_disable() >> > -> intel_disable_ddi() >> > -> intel_crtc_mode_set() >> > -> haswell_crtc_mode_set() >> > -> intel_ddi_mode_set() >> > -> intel_write_eld() >> > -> haswell_write_eld() >> > -> haswell_crtc_enable() >> > -> intel_enable_ddi() >> > >> > >> > But for DPMS on->off->on there would be calls to haswell_crtc_disable() and >> > haswell_crtc_enable() w/o calls to haswell_write_eld(). I suppose this is the >> > problem you want to fix? >> >> Exactly. Intel_disable_ddi() was called after haswell_write_eld() only once during system bootup. >> I added a flag to monitor ELD valid status. It will be set in haswell_write_eld() and cleared in intel_disable_ddi(). >> In intel_enable_ddi() , will check this flag and set the ELD valid bit again only if the flag was cleared before. > > But that would make the code behave almost the same as the old patch, no? > It would make intel_enable_ddi() set the ELD valid bit, even if ELD > wasn't written. > > My idea was that intel_disable_ddi() would not clear the flag, and > intel_enable_ddi() would only write the ELD valid bit if the flag is set. > That way we never write the ELD valid bit to the register when there is > no valid ELD. > > So something likes this perhaps: > > intel_ddi_mode_set() > { > ... > + intel_encoder->eld_valid = false; > ... > intel_write_eld(); > ... > } > > haswell_write_eld() > { > ... > + intel_encoder->eld_valid = true; > } > > intel_enable_ddi() > { > ... > + if (intel_encoder->eld_valid) > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD); > } > > intel_disable_ddi() > { > ... > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD) > } > > > One extra idea would be to have something like 'uint32_t eld_reg' > instead of 'bool eld_valid'. That was you could just do > 'I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, intel_encoder->eld_reg);' in > intel_eanble_ddi() without having to recompute the register value. > And intel_encodr->eld_reg would be updated with the new value > in haswell_write_eld(). > >> I had tested the new patch and it works well. I will send out the updated patch later. >> > >> > So, perhaps there should be a flag somewhere that would be cleared at the >> > beginning of the mode_set operation, and then intel_write_eld() should set the >> > flag when the ELD was written succesfully. >> > intel_enable_ddi() could check the flag and only set the ELD valid bit when the >> > flag is set. >> > >> > Should non-ddi platforms do something similar as well? >> >> Would you share more ideas on this? I have not noticed the potential issues here. > No ideas really. I was just wondering if we need to clear/set some ELD > bits in the encoder disable/enable functions for older HW as well. > > At least we seem to have similar registers for older HW: > G4X_AUD_CNTL_ST > IBX_AUD_CNTL_ST2 > CPT_AUD_CNTRL_ST2 > > > But now I'm wondering if we're possibly overdesigning this thing. Would > it be enough to simply clear the ELD valid bit at the beginning of > intel_mode_set()? That way the bit would always be cleared when the > display is really disabled, but it would not be touched when DPMS state > is changed. I don't know which is more appropriate for DPMS off state; > should the ELD valid bit be left alone, or should it be cleared? > > So something like this: > intel_set_mode() > { > ... > + for_each_intel_crtc_masked(dev, disable_pipes|prepare_pipes, intel_crtc) > + intel_crtc_clear_eld(&intel_crtc->base); > > for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) > intel_crtc_disable(&intel_crtc->base); > ... > } > > Where intel_crtc_clear_eld() would call some function similar to > write_eld(), except that function would only clear the ELD valid bit. > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br
> -----Original Message----- > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > Sent: Monday, January 21, 2013 9:11 PM > To: Wang, Xingchao > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, Paulo R > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info refresh > support for Haswell > > On Mon, Jan 21, 2013 at 05:40:23AM +0000, Wang, Xingchao wrote: > > Hi Ville Syrjälä, > > > > > -----Original Message----- > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > Sent: Friday, January 18, 2013 9:14 PM > > > To: Wang, Xingchao > > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; Zanoni, > > > Paulo R > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info > > > refresh support for Haswell > > > > > > On Fri, Jan 18, 2013 at 12:00:10PM +0000, Wang, Xingchao wrote: > > > > > > > > > -----Original Message----- > > > > > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] > > > > > Sent: Friday, January 18, 2013 6:46 PM > > > > > To: Wang, Xingchao > > > > > Cc: intel-gfx@lists.freedesktop.org; daniel.vetter@ffwll.ch; > > > > > Zanoni, Paulo R > > > > > Subject: Re: [Intel-gfx] [PATCH v2] drm/i915: HDMI/DP - ELD info > > > > > refresh support for Haswell > > > > > > > > > > On Fri, Jan 18, 2013 at 10:51:55AM +0800, Wang Xingchao wrote: > > > > > > ELD info should be updated dynamically according to hot plug event. > > > > > > For haswell chip, clear/set the eld valid bit and output > > > > > > enable bit from callback intel_disable/eanble_ddi(). > > > > > > > > > > Hmm. Is it OK to set the ELD valid bit if the ELD hasn't > > > > > actually been > > > written? > > > > > > > > This triggers an unsolicited event to ALSA driver which continue > > > > to read ELD > > > info. > > > > > > I take it we don't want that to happen? > > > > > > > > And besides these bits are already set by haswell_write_eld(). > > > > > > > > Intel_disable/enable_ddi() was called even after > > > > haswell_write_eld(), so we > > > need set the bits again. > > > > > > For the mode set sequence only intel_enable_ddi() is called after > > > haswell_write_eld(). > > > > > > This is how the sequence should end up looking with the current > > > code: > > > > > > intel_set_mode() > > > -> haswell_crtc_disable() > > > -> intel_disable_ddi() > > > -> intel_crtc_mode_set() > > > -> haswell_crtc_mode_set() > > > -> intel_ddi_mode_set() > > > -> intel_write_eld() > > > -> haswell_write_eld() > > > -> haswell_crtc_enable() > > > -> intel_enable_ddi() > > > > > > > > > But for DPMS on->off->on there would be calls to > > > haswell_crtc_disable() and > > > haswell_crtc_enable() w/o calls to haswell_write_eld(). I suppose > > > this is the problem you want to fix? > > > > Exactly. Intel_disable_ddi() was called after haswell_write_eld() only once > during system bootup. > > I added a flag to monitor ELD valid status. It will be set in haswell_write_eld() > and cleared in intel_disable_ddi(). > > In intel_enable_ddi() , will check this flag and set the ELD valid bit again only if > the flag was cleared before. > > But that would make the code behave almost the same as the old patch, no? As there's DPMS state change during bootup, the ELD valid bit was cleared in intel_disable_ddi(), So we have to set that bit again in intel_enable_ddi(), otherwise there's no eld info after system bootup. I changed the logic based on your suggestion below, didot clear intel_crtc->eld_vld in intel_disable_ddi(). > It would make intel_enable_ddi() set the ELD valid bit, even if ELD wasn't > written. So the basic rule is to ONLY set ELD valid bit when ELD was really written in haswell_write_eld(). > > My idea was that intel_disable_ddi() would not clear the flag, and > intel_enable_ddi() would only write the ELD valid bit if the flag is set. > That way we never write the ELD valid bit to the register when there is no valid > ELD. > > So something likes this perhaps: > > intel_ddi_mode_set() > { Should it be intel_set_mode()? > ... > + intel_encoder->eld_valid = false; > ... > intel_write_eld(); > ... > } > > haswell_write_eld() > { > ... > + intel_encoder->eld_valid = true; > } > > intel_enable_ddi() > { > ... > + if (intel_encoder->eld_valid) > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD); > } > > intel_disable_ddi() > { > ... > + I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD) > } > > > One extra idea would be to have something like 'uint32_t eld_reg' > instead of 'bool eld_valid'. That was you could just do > 'I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, intel_encoder->eld_reg);' in > intel_eanble_ddi() without having to recompute the register value. > And intel_encodr->eld_reg would be updated with the new value in > haswell_write_eld(). I think using eld_vld is better as in intel_enable_ddi() we need this flag to decide whether to update the ELD valid bit. And the register value may change based on different pipe setting, it's more efficient to just read out the register value. > > > I had tested the new patch and it works well. I will send out the updated > patch later. > > > > > > So, perhaps there should be a flag somewhere that would be cleared > > > at the beginning of the mode_set operation, and then > > > intel_write_eld() should set the flag when the ELD was written succesfully. > > > intel_enable_ddi() could check the flag and only set the ELD valid > > > bit when the flag is set. > > > > > > Should non-ddi platforms do something similar as well? > > > > Would you share more ideas on this? I have not noticed the potential issues > here. > No ideas really. I was just wondering if we need to clear/set some ELD bits in > the encoder disable/enable functions for older HW as well. > > At least we seem to have similar registers for older HW: > G4X_AUD_CNTL_ST > IBX_AUD_CNTL_ST2 > CPT_AUD_CNTRL_ST2 > Hmm, there's already patch to make similar work: commit b98b60167279df3acac9422c3c9820d9ebbcf9fb Author: Wang Xingchao <xingchao.wang@intel.com> Date: Thu Sep 13 07:43:22 2012 +0800 drm/i915: HDMI - Clear Audio Enable bit for Hot Plug > > But now I'm wondering if we're possibly overdesigning this thing. Would it be > enough to simply clear the ELD valid bit at the beginning of intel_mode_set()? > That way the bit would always be cleared when the display is really disabled, > but it would not be touched when DPMS state is changed. I don't know which is > more appropriate for DPMS off state; should the ELD valid bit be left alone, or > should it be cleared? For DPMS state changed, the eld was not cleared so I think it's safe to keep the eld_vld flag be true. For hot plug event, the monitor was removed and ELD may change until next monitor inserted, so just clear the flag. Is that right? Do you think it's the right place to clear the eld_vld flag in haswell_crtc_disable()? thanks --xingchao > > So something like this: > intel_set_mode() > { > ... > + for_each_intel_crtc_masked(dev, disable_pipes|prepare_pipes, > intel_crtc) > + intel_crtc_clear_eld(&intel_crtc->base); > > for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc) > intel_crtc_disable(&intel_crtc->base); > ... > } > > Where intel_crtc_clear_eld() would call some function similar to write_eld(), > except that function would only clear the ELD valid bit. > > -- > Ville Syrjälä > Intel OTC
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index f02b3fe..7ce4728 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1274,10 +1274,14 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder) static void intel_enable_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; struct drm_device *dev = encoder->dev; struct drm_i915_private *dev_priv = dev->dev_private; enum port port = intel_ddi_get_encoder_port(intel_encoder); int type = intel_encoder->type; + int tmp; if (type == INTEL_OUTPUT_HDMI) { /* In HDMI/DVI mode, the port width, and swing/emphasis values @@ -1290,18 +1294,32 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder) ironlake_edp_backlight_on(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); } 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; + int tmp; if (type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(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); } int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
ELD info should be updated dynamically according to hot plug event. For haswell chip, clear/set the eld valid bit and output enable bit from callback intel_disable/eanble_ddi(). Signed-off-by: Wang Xingchao <xingchao.wang@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)