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

[Wang, Xingchao]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com]
> Sent: Monday, January 21, 2013 9:11 PM
> To: Wang, Xingchao
> Cc: intel-gfx at lists.freedesktop.org; daniel.vetter at 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 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: [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 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: [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 at 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