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

[Wang, Xingchao]

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.
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