[Intel-gfx] [PATCH v5] drm/i915: pass ELD to HDMI/DP audio driver

Sun Sep 4 18:06:11 PDT 2011

Dear Paul,

On Sun, Sep 04, 2011 at 07:11:54PM +0800, Paul Menzel wrote:
> Dear Wu,
> 
> 
> I hope that is your first name.

My first name is Fengguang. "LAST NAME, FIRSTNAME" is the convention
in Intel emails. However I often forgot this and pick whatever comes
first...and happily accept both Fengguang and Wu :)

> Am Sonntag, den 04.09.2011, 05:15 +0800 schrieb Wu Fengguang:
> > Changes from v4: remove a debug call to dump_stack().
> > Thanks to Bossart for catching this!
> 
> His first name is Pierre-Louis. I do not know how you address people at
> Intel though.

Thanks for the reminding!

> > ---
> 
> I think your format will confuse `git am`. Please always put that under
> the »---« under the Signed-off-by lines.

OK, good point!

> > Add ELD support for Intel Eaglelake, IbexPeak/Ironlake,
> > SandyBridge/CougarPoint and IvyBridge/PantherPoint chips.
> > 
> > ELD (EDID-Like Data) describes to the HDMI/DP audio driver the audio
> > capabilities of the plugged monitor. It's built and passed to audio
> > driver in 2 steps:
> > 
> > (1) at get_modes time, parse EDID and save ELD to drm_connector.eld[]
> > 
> > (2) at mode_set time, write drm_connector.eld[] to the Transcoder's hw
> >     ELD buffer and set the ELD_valid bit to inform HDMI/DP audio driver
> > 
> > ELD selection policy: it's possible for one encoder to be associated
> > with multiple connectors (ie. monitors), in which case the first found
> > ELD will be used. This policy may not be suitable for all users, but
> > let's start it simple first.
> > 
> > The impact of ELD selection policy: assume there are two monitors, one
> > supports stereo playback and the other has 8-channel output; cloned
> > display mode is used, so that the two monitors are associated with the
> > same internal encoder. If only the stereo playback capability is reported,
> > the user won't be able to start 8-channel playback; if the 8-channel ELD
> > is reported, then user space applications may send 8-channel samples
> > down, however the user may actually be listening to the 2-channel
> > monitor and not connecting speakers to the 8-channel monitor. Overall,
> > it's more safe to report maximum profiles to the user space, so that
> > the user can at least be able to do 8-channel playback if he want to.
> 
> s'he's/he'

Fixed.

> > This patch is tested OK on G45/HDMI, IbexPeak/HDMI and IvyBridge/HDMI+DP.
> 
> What is the correct way to test this patch. Just plug in the HDMI
> monitor and it should work out of the box?

Just plug in the HDMI/DP monitor, and run

        cat /proc/asound/card0/eld*

to check if the monitor name, HDMI/DP type, etc. show up correctly.

> > Minor imperfection: the GEN6_AUD_CNTL_ST/DIP_Port_Select field always
> > reads 0 (reserved). Without knowing the port number, I worked it around
> > by setting the ELD_valid bit for ALL the three ports. It's tested to not
> > be a problem, because the audio driver will find invalid ELD data and
> > hence rightfully abort, even when it sees the ELD_valid indicator.
> > 
> > Thanks to Zhenyu and Bossart for a lot of valuable help and testing.
> 
> Again the first name is Pierre-Louis or put Mr in front of it.

Got it, so the convention is either "Pierre-Louis", or "Mr. Bossart".

> > CC: Zhao Yakui <yakui.zhao at intel.com>
> > CC: Wang Zhenyu <zhenyu.z.wang at intel.com>
> > CC: Jeremy Bush <contractfrombelow at gmail.com>
> > CC: Christopher White <c.white at pulseforce.com>
> > CC: "Bossart, Pierre-louis" <pierre-louis.bossart at intel.com>
> 
> Pierre-Louis Bossart

Corrected.

> > Signed-off-by: Ben Skeggs <bskeggs at redhat.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu at intel.com>
> > ---
> >  drivers/gpu/drm/drm_edid.c           |  171 +++++++++++++++++++++++++
> >  drivers/gpu/drm/i915/i915_drv.h      |    2 
> >  drivers/gpu/drm/i915/i915_reg.h      |   25 +++
> >  drivers/gpu/drm/i915/intel_display.c |  131 +++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_dp.c      |    6 
> >  drivers/gpu/drm/i915/intel_drv.h     |    2 
> >  drivers/gpu/drm/i915/intel_hdmi.c    |    3 
> >  drivers/gpu/drm/i915/intel_modes.c   |    2 
> >  include/drm/drm_crtc.h               |    9 +
> >  include/drm/drm_edid.h               |    9 +
> >  10 files changed, 358 insertions(+), 2 deletions(-)
> 
> Some more style things follow.
> 
> […]
> 
> > +/**
> > + * drm_av_sync_delay - HDMI/DP sink audio-video sync delay in milli-seconds
> > + * @connector: connector associated with the HDMI/DP sink
> > + * @mode: the display mode
> > + */
> > +int drm_av_sync_delay(struct drm_connector *connector,
> > +		      struct drm_display_mode *mode)
> > +{
> > +	int i = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> > +	int a, v;
> > +
> > +	if (!connector->latency_present[0])
> > +		return 0;
> > +	if (!connector->latency_present[1])
> > +		i = 0;
> > +
> > +	a = connector->audio_latency[i];
> > +	v = connector->video_latency[i];
> > +
> > +	/*
> > +	 * HDMI/DP sink doesn't support audio or video?
> > +	 */
> > +	if (a == 255 || v == 255)
> > +		return 0;
> > +
> > +	/*
> > +	 * Convert raw edid values to milli-seconds.
> 
> s/edid/EDID/ (nitpick)
> s/milli-seconds/millisecond/

Fixed.

> http://www.merriam-webster.com/dictionary/millisecond
> 
> > +	 * Treat unknown latency as 0ms.
> > +	 */
> > +	if (a)
> > +		a = min(2 * (a - 1), 500);
> > +	if (v)
> > +		v = min(2 * (v - 1), 500);
> > +
> > +	return max(v - a, 0);
> > +}
> > +EXPORT_SYMBOL(drm_av_sync_delay);
> 
> […]
> 
> > --- linux-next.orig/drivers/gpu/drm/i915/i915_reg.h	2011-09-02 15:59:31.000000000 +0800
> > +++ linux-next/drivers/gpu/drm/i915/i915_reg.h	2011-09-02 15:59:40.000000000 +0800
> > @@ -3470,4 +3470,29 @@
> >  #define GEN6_PCODE_DATA				0x138128
> >  #define   GEN6_PCODE_FREQ_IA_RATIO_SHIFT	8
> >  
> > +#define G4X_AUD_VID_DID			0x62020
> > +#define INTEL_AUDIO_DEVCL		0x808629FB
> 
> Alignment two lines above. Separate clean up patch to fix alignment to
> send before?
> 
> > +#define INTEL_AUDIO_DEVBLC		0x80862801
> > +#define INTEL_AUDIO_DEVCTG		0x80862802
> > +
> > +#define G4X_AUD_CNTL_ST			0x620B4
> 
> Alignment?
> 
> > +#define G4X_ELDV_DEVCL_DEVBLC		(1 << 13)
> > +#define G4X_ELDV_DEVCTG			(1 << 14)
> 
> Dito?
> 
> > +#define G4X_ELD_ADDR			(0xf << 5)
> > +#define G4X_ELD_ACK			(1 << 4)
> > +#define G4X_HDMIW_HDMIEDID		0x6210C
> > +
> > +#define GEN6_HDMIW_HDMIEDID_A		0xE2050
> > +#define GEN6_AUD_CNTL_ST_A		0xE20B4
> > +#define GEN6_ELD_BUFFER_SIZE		(0x1f << 10)
> > +#define GEN6_ELD_ADDRESS		(0x1f << 5)
> > +#define GEN6_ELD_ACK			(1 << 4)
> > +#define GEN6_AUD_CNTL_ST2		0xE20C0
> > +#define GEN6_ELD_VALIDB			(1 << 0)
> 
> Dito?

The alignments are actually good in the source code.  It's the leading
"+" in the patch file that makes some of the lines "appear" to be
unaligned.

Thanks for the careful review!

Regards,
Fengguang