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 at gmail.com> Thanks, Rodrigo. On Mon, Jan 21, 2013 at 11:11 AM, Ville Syrj?l? <ville.syrjala at 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 at linux.intel.com] >> > Sent: Friday, January 18, 2013 9:14 PM >> > To: Wang, Xingchao >> > Cc: intel-gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch; Zanoni, Paulo R >> > Subject: Re: [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 at linux.intel.com] >> > > > Sent: Friday, January 18, 2013 6:46 PM >> > > > To: Wang, Xingchao >> > > > Cc: intel-gfx at lists.freedesktop.org; daniel.vetter at ffwll.ch; Zanoni, >> > > > Paulo R >> > > > Subject: Re: [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 at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Rodrigo Vivi Blog: http://blog.vivi.eng.br
