Re: [PATCH v5] drm/i915: pass ELD to HDMI/DP audio driver

[Chris Wilson <chris@chris-wilson.co.uk>]

On Sun, 4 Sep 2011 05:15:10 +0800, Wu Fengguang <fengguang.wu@intel.com> wrote:
> Changes from v4: remove a debug call to dump_stack().
> Thanks to Bossart for catching this!
> ---
> 
> 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.
> 
> This patch is tested OK on G45/HDMI, IbexPeak/HDMI and IvyBridge/HDMI+DP.
> 
> 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.
> 

> --- linux-next.orig/drivers/gpu/drm/i915/intel_display.c	2011-09-02 15:59:31.000000000 +0800
> +++ linux-next/drivers/gpu/drm/i915/intel_display.c	2011-09-04 05:11:44.000000000 +0800
> +static void ironlake_write_eld(struct drm_connector *connector,
> +				     struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	uint8_t *eld = connector->eld;
> +	uint32_t eldv;
> +	uint32_t i;
> +	int len;
> +	int hdmiw_hdmiedid;
> +	int aud_cntl_st;
> +	int aud_cntrl_st2;
> +
> +	if (IS_IVYBRIDGE(connector->dev)) {
> +		hdmiw_hdmiedid = GEN7_HDMIW_HDMIEDID_A;
> +		aud_cntl_st = GEN7_AUD_CNTRL_ST_A;
> +		aud_cntrl_st2 = GEN7_AUD_CNTRL_ST2;
> +	} else {
> +		hdmiw_hdmiedid = GEN6_HDMIW_HDMIEDID_A;
> +		aud_cntl_st = GEN6_AUD_CNTL_ST_A;
> +		aud_cntrl_st2 = GEN6_AUD_CNTL_ST2;
> +	}

This register naming is inconsistent with its intent. If these registers
were indeed introduced on Ironlake as you seem to imply by using them
with Ironlake, you should label them as GEN5 and not GEN6.

This patch should be split between adding the core drm functionality to
build the ELD and the introduction of i915 support.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre