Re: [Intel-gfx] [PATCH 1/4] drm/edid: add helper function to detect monitor audio capability

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

[Lets see if I have a working vpn connection today...]

On Sun, 19 Sep 2010 14:52:06 +0800, Zhenyu Wang <zhenyuw@linux.intel.com> wrote:
> To help to determine if digital display port needs to enable
> audio output or not. This one adds a helper to get monitor's
> audio capability via EDID CEA extension block.
> 
> Tested-by: Wu Fengguang <fengguang.wu@intel.com>
> Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>

This should be cc'ed for Adam Jackson's attention as well.

> ---
>  drivers/gpu/drm/drm_edid.c |   92 +++++++++++++++++++++++++++++++++++++-------
>  include/drm/drm_crtc.h     |    1 +
>  2 files changed, 79 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 96e9631..7f356af 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -1268,34 +1268,51 @@ add_detailed_modes(struct drm_connector *connector, struct edid *edid,
>  }
>  
>  #define HDMI_IDENTIFIER 0x000C03
> +#define AUDIO_BLOCK	0x01
>  #define VENDOR_BLOCK    0x03
> +#define EDID_BASIC_AUDIO	(1 << 6)
> +
>  /**
> + * drm_detect_monitor_audio - check monitor audio capability
> + *
> + * Monitor should have CEA extension block.
> + * If monitor has 'basic audio', but no CEA audio blocks, it's 'basic
> + * audio' only. If there is any audio extension block and supported
> + * audio format, assume at least 'basic audio' support, even if 'basic
> + * audio' is not defined in EDID.
> + *
> + */
> +bool drm_detect_monitor_audio(struct edid *edid)

drm_edid_has_monitor_audio()? That is a little more self-descriptive.
(I also think drm_detect_hdmi_monitor is poorly named.)

> +{
> +	u8 *edid_ext;
> +	int i, j;
> +	bool has_audio = false;
> +	int start_offset, end_offset;
> +
> +	edid_ext = drm_find_cea_extension(edid);
> +	if (!edid_ext)
> +		goto end;
> +
> +	has_audio = ((edid_ext[3] & EDID_BASIC_AUDIO) != 0);
Too many brackets do not lead to code clarity. ;-)
> +
> +	if (has_audio) {

The last time Adam had a chance to comment on this EDID check, he
suggested that we cannot rely on has_audio being a reliable indicator of
the sink's properties. If the EDID has no audio support, then return
early, otherwise perform the secondary check that extension block contains
audio data.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre