Re: [PATCH v2] drm/i915: HDMI/DP - ELD info refresh support for Haswell

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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