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

[Paul Menzel]

Dear Wu,


I hope that is your first name.

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.

> ---

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

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

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

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

> 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

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

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?

> +#define GEN6_CP_READYB			(1 << 1)
> +
> +#define GEN7_HDMIW_HDMIEDID_A		0xE5050
> +#define GEN7_AUD_CNTRL_ST_A		0xE50B4
> +#define GEN7_AUD_CNTRL_ST2		0xE50C0
> +
>  #endif /* _I915_REG_H_ */

[…]


Thanks,

Paul