I found my check for audio capability within CEA extension might has some conflict with Adam's patch to add more detailed mode from CEA ext block. But I haven't found Adam's patch in next tree, so I just kept my current change.
This fixed incorrect probe for DP monitor's audio. Fengguang helped to test this set. We got correct DP sound on Eizo EV2333W, and HDMI audio also worked with no regression found.
Patch is against Chris's drm-intel-next branch.
thanks.
Zhenyu Wang (4): drm/edid: add helper function to detect monitor audio capability drm/i915: Enable DisplayPort audio drm/i915: Enable HDMI audio for monitor with audio support drm/i915: add new param to force audio on or off for HDMI/DP port
drivers/gpu/drm/drm_edid.c | 92 +++++++++++++++++++++++++++++++------ drivers/gpu/drm/i915/i915_drv.c | 3 + drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_dp.c | 64 +++++++++++++++---------- drivers/gpu/drm/i915/intel_hdmi.c | 13 ++++-- include/drm/drm_crtc.h | 1 + 6 files changed, 130 insertions(+), 44 deletions(-)
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 --- 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_hdmi_monitor - detect whether monitor is hdmi. - * @edid: monitor EDID information - * - * Parse the CEA extension according to CEA-861-B. - * Return true if HDMI, false if not or unknown. + * Search EDID for CEA extension block. */ -bool drm_detect_hdmi_monitor(struct edid *edid) +static u8 *drm_find_cea_extension(struct edid *edid) { - char *edid_ext = NULL; - int i, hdmi_id; - int start_offset, end_offset; - bool is_hdmi = false; + u8 *edid_ext = NULL; + int i;
/* No EDID or EDID extensions */ if (edid == NULL || edid->extensions == 0) - goto end; + return NULL;
/* Find CEA extension */ for (i = 0; i < edid->extensions; i++) { - edid_ext = (char *)edid + EDID_LENGTH * (i + 1); - /* This block is CEA extension */ - if (edid_ext[0] == 0x02) + edid_ext = (u8 *)edid + EDID_LENGTH * (i + 1); + if (edid_ext[0] == CEA_EXT) break; }
if (i == edid->extensions) + return NULL; + + return edid_ext; +} + +/** + * drm_detect_hdmi_monitor - detect whether monitor is hdmi. + * @edid: monitor EDID information + * + * Parse the CEA extension according to CEA-861-B. + * Return true if HDMI, false if not or unknown. + */ +bool drm_detect_hdmi_monitor(struct edid *edid) +{ + u8 *edid_ext; + int i, hdmi_id; + int start_offset, end_offset; + bool is_hdmi = false; + + edid_ext = drm_find_cea_extension(edid); + if (!edid_ext) goto end;
/* Data block offset in CEA extension block */ @@ -1326,6 +1343,53 @@ end: EXPORT_SYMBOL(drm_detect_hdmi_monitor);
/** + * 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) +{ + 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); + + if (has_audio) { + DRM_DEBUG_KMS("Monitor has basic audio support\n"); + goto end; + } + + /* Data block offset in CEA extension block */ + start_offset = 4; + end_offset = edid_ext[2]; + + for (i = start_offset; i < end_offset; + i += ((edid_ext[i] & 0x1f) + 1)) { + if ((edid_ext[i] >> 5) == AUDIO_BLOCK) { + has_audio = true; + for (j = 1; j < (edid_ext[i] & 0x1f); j += 3) + DRM_DEBUG_KMS("CEA audio format %d\n", + (edid_ext[i + j] >> 3) & 0xf); + goto end; + } + } +end: + return has_audio; +} +EXPORT_SYMBOL(drm_detect_monitor_audio); + +/** * drm_add_edid_modes - add modes from EDID data, if available * @connector: connector we're probing * @edid: edid data diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c9f3cc5..284ca90 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -754,6 +754,7 @@ extern int drm_mode_gamma_get_ioctl(struct drm_device *dev, extern int drm_mode_gamma_set_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern bool drm_detect_hdmi_monitor(struct edid *edid); +extern bool drm_detect_monitor_audio(struct edid *edid); extern int drm_mode_page_flip_ioctl(struct drm_device *dev, void *data, struct drm_file *file_priv); extern struct drm_display_mode *drm_cvt_mode(struct drm_device *dev,
[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
On 2010.09.19 08:38:07 +0100, Chris Wilson wrote:
This should be cc'ed for Adam Jackson's attention as well.
yep, I think I cc-ed ajax in git-send-mail command line...I'll amend the commit.
+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.)
yeah, that looks better. thanks.
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.
That's also the problem we stuck for quite some time, and until recently we got reply on this issue. Adam is right, 'basic audio' bit might not be reliable, so we try to search audio block too, if there's any existence, we will accept 'basic audio' support as well.
Thanks for the review!
On Sun, 2010-09-19 at 15:56 +0800, Zhenyu Wang wrote:
On 2010.09.19 08:38:07 +0100, Chris Wilson wrote:
This should be cc'ed for Adam Jackson's attention as well.
yep, I think I cc-ed ajax in git-send-mail command line...I'll amend the commit.
+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.)
yeah, that looks better. thanks.
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.
That's also the problem we stuck for quite some time, and until recently we got reply on this issue. Adam is right, 'basic audio' bit might not be reliable, so we try to search audio block too, if there's any existence, we will accept 'basic audio' support as well.
Also, would you be able to export drm_find_cea_extension(), I have pending nouveau patches that will require this symbol (to build a HDA ELD from the EDID data for HDMI/DP audio).
With that, you can add my Reviewed-by :)
Thanks, Ben.
Thanks for the review!
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
This will turn on DP audio output by checking monitor's audio capability.
Tested-by: Wu Fengguang fengguang.wu@intel.com Signed-off-by: Zhenyu Wang zhenyuw@linux.intel.com --- drivers/gpu/drm/i915/intel_dp.c | 61 +++++++++++++++++++++++---------------- 1 files changed, 36 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 208a4ec..81fca1e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1444,39 +1444,50 @@ intel_dp_detect(struct drm_connector *connector) struct drm_i915_private *dev_priv = dev->dev_private; uint32_t temp, bit; enum drm_connector_status status; + struct edid *edid = NULL;
intel_dp->has_audio = false;
- if (HAS_PCH_SPLIT(dev)) - return ironlake_dp_detect(connector); + if (HAS_PCH_SPLIT(dev)) { + status = ironlake_dp_detect(connector); + } else { + switch (intel_dp->output_reg) { + case DP_B: + bit = DPB_HOTPLUG_INT_STATUS; + break; + case DP_C: + bit = DPC_HOTPLUG_INT_STATUS; + break; + case DP_D: + bit = DPD_HOTPLUG_INT_STATUS; + break; + default: + return connector_status_unknown; + }
- switch (intel_dp->output_reg) { - case DP_B: - bit = DPB_HOTPLUG_INT_STATUS; - break; - case DP_C: - bit = DPC_HOTPLUG_INT_STATUS; - break; - case DP_D: - bit = DPD_HOTPLUG_INT_STATUS; - break; - default: - return connector_status_unknown; - } + temp = I915_READ(PORT_HOTPLUG_STAT);
- temp = I915_READ(PORT_HOTPLUG_STAT); + if ((temp & bit) == 0) + return connector_status_disconnected;
- if ((temp & bit) == 0) - return connector_status_disconnected; + status = connector_status_disconnected; + if (intel_dp_aux_native_read(intel_dp, 0x000, intel_dp->dpcd, + sizeof (intel_dp->dpcd)) == sizeof (intel_dp->dpcd)) + { + if (intel_dp->dpcd[0] != 0) + status = connector_status_connected; + } + }
- status = connector_status_disconnected; - if (intel_dp_aux_native_read(intel_dp, - 0x000, intel_dp->dpcd, - sizeof (intel_dp->dpcd)) == sizeof (intel_dp->dpcd)) - { - if (intel_dp->dpcd[0] != 0) - status = connector_status_connected; + if (status == connector_status_connected) { + edid = drm_get_edid(connector, intel_dp->base.ddc_bus); + if (edid) { + intel_dp->has_audio = drm_detect_monitor_audio(edid); + connector->display_info.raw_edid = NULL; + kfree(edid); + } } + return status; }
On Sun, 19 Sep 2010 14:52:07 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
This will turn on DP audio output by checking monitor's audio capability.
Tested-by: Wu Fengguang fengguang.wu@intel.com Signed-off-by: Zhenyu Wang zhenyuw@linux.intel.com
drivers/gpu/drm/i915/intel_dp.c | 61 +++++++++++++++++++++++---------------- 1 files changed, 36 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 208a4ec..81fca1e 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -1444,39 +1444,50 @@ intel_dp_detect(struct drm_connector *connector) struct drm_i915_private *dev_priv = dev->dev_private; uint32_t temp, bit; enum drm_connector_status status;
struct edid *edid = NULL;
intel_dp->has_audio = false;
- if (HAS_PCH_SPLIT(dev))
return ironlake_dp_detect(connector);
- if (HAS_PCH_SPLIT(dev)) {
status = ironlake_dp_detect(connector);- } else {
switch (intel_dp->output_reg) {case DP_B:bit = DPB_HOTPLUG_INT_STATUS;break;case DP_C:bit = DPC_HOTPLUG_INT_STATUS;break;case DP_D:bit = DPD_HOTPLUG_INT_STATUS;break;default:return connector_status_unknown;}
Move the switch and register check into a separate function, so the main detect body becomes: if (HAS_PCH_SPLIT(dev)) status = ironlake_dp_detect(connector); else status = g4x_dp_detect(connector); if (status != connector_status_connected) return status;
On 2010.09.19 08:43:30 +0100, Chris Wilson wrote:
Move the switch and register check into a separate function, so the main detect body becomes: if (HAS_PCH_SPLIT(dev)) status = ironlake_dp_detect(connector); else status = g4x_dp_detect(connector); if (status != connector_status_connected) return status;
ok, will do that.
thanks.
Rely on monitor's audio capability to turn on audio output for HDMI.
Tested-by: Wu Fengguang fengguang.wu@intel.com Signed-off-by: Zhenyu Wang zhenyuw@linux.intel.com --- drivers/gpu/drm/i915/intel_hdmi.c | 12 ++++++++---- 1 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 783924c..90184e6 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -41,6 +41,7 @@ struct intel_hdmi { struct intel_encoder base; u32 sdvox_reg; bool has_hdmi_sink; + bool has_audio; };
static struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder) @@ -71,11 +72,12 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder, if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC) sdvox |= SDVO_HSYNC_ACTIVE_HIGH;
- if (intel_hdmi->has_hdmi_sink) { + /* Required on CPT */ + if (intel_hdmi->has_hdmi_sink && HAS_PCH_CPT(dev)) + sdvox |= HDMI_MODE_SELECT; + + if (intel_hdmi->has_audio) sdvox |= SDVO_AUDIO_ENABLE; - if (HAS_PCH_CPT(dev)) - sdvox |= HDMI_MODE_SELECT; - }
if (intel_crtc->pipe == 1) { if (HAS_PCH_CPT(dev)) @@ -152,12 +154,14 @@ intel_hdmi_detect(struct drm_connector *connector) enum drm_connector_status status = connector_status_disconnected;
intel_hdmi->has_hdmi_sink = false; + intel_hdmi->has_audio = false; edid = drm_get_edid(connector, intel_hdmi->base.ddc_bus);
if (edid) { if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); + intel_hdmi->has_audio = drm_detect_monitor_audio(edid); } connector->display_info.raw_edid = NULL; kfree(edid);
Two reasons to add this param, one is for some AV device requiring HDMI input for audio, but some device might not have sane EDID info to enable audio on HDMI port. Another is for testing purpose.
Signed-off-by: Zhenyu Wang zhenyuw@linux.intel.com --- drivers/gpu/drm/i915/i915_drv.c | 3 +++ drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_dp.c | 3 ++- drivers/gpu/drm/i915/intel_hdmi.c | 3 ++- 4 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index dffc1bc..95277e5 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -48,6 +48,9 @@ module_param_named(powersave, i915_powersave, int, 0400); unsigned int i915_lvds_downclock = 0; module_param_named(lvds_downclock, i915_lvds_downclock, int, 0400);
+unsigned int i915_force_audio = -1; +module_param_named(force_audio, i915_force_audio, int, 0400); + static struct drm_driver driver; extern int intel_agp_enabled;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index b0692c4..495182e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -844,6 +844,7 @@ extern int i915_max_ioctl; extern unsigned int i915_fbpercrtc; extern unsigned int i915_powersave; extern unsigned int i915_lvds_downclock; +extern unsigned int i915_force_audio;
extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 81fca1e..77b45e7 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -736,7 +736,8 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode, intel_dp->DP |= DP_PORT_WIDTH_4; break; } - if (intel_dp->has_audio) + if (i915_force_audio == 1 || + (i915_force_audio != 0 && intel_dp->has_audio)) intel_dp->DP |= DP_AUDIO_OUTPUT_ENABLE;
memset(intel_dp->link_configuration, 0, DP_LINK_CONFIGURATION_SIZE); diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index 90184e6..20af171 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -76,7 +76,8 @@ static void intel_hdmi_mode_set(struct drm_encoder *encoder, if (intel_hdmi->has_hdmi_sink && HAS_PCH_CPT(dev)) sdvox |= HDMI_MODE_SELECT;
- if (intel_hdmi->has_audio) + if (i915_force_audio == 1 || + (i915_force_audio != 0 && intel_hdmi->has_audio)) sdvox |= SDVO_AUDIO_ENABLE;
if (intel_crtc->pipe == 1) {
On Sun, 19 Sep 2010 14:52:09 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
Two reasons to add this param, one is for some AV device requiring HDMI input for audio, but some device might not have sane EDID info to enable audio on HDMI port. Another is for testing purpose.
I was thinking along the lines of a connector property for force_audio. -Chris
On 2010.09.19 09:14:53 +0100, Chris Wilson wrote:
On Sun, 19 Sep 2010 14:52:09 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
Two reasons to add this param, one is for some AV device requiring HDMI input for audio, but some device might not have sane EDID info to enable audio on HDMI port. Another is for testing purpose.
I was thinking along the lines of a connector property for force_audio.
oh, right, that's much better. sorry, I forgot about it completely. ;)
On Sun, 19 Sep 2010 16:22:06 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
On 2010.09.19 09:14:53 +0100, Chris Wilson wrote:
On Sun, 19 Sep 2010 14:52:09 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
Two reasons to add this param, one is for some AV device requiring HDMI input for audio, but some device might not have sane EDID info to enable audio on HDMI port. Another is for testing purpose.
I was thinking along the lines of a connector property for force_audio.
oh, right, that's much better. sorry, I forgot about it completely. ;)
No worries, I've written up a couple of patches to add the connector properties on top of your series. So just a couple of minor tweaks and getting Adam's seal of approval for the core changes and I'll apply the series to next. Thanks! -Chris
On Sun, 2010-09-19 at 14:52 +0800, Zhenyu Wang wrote:
I found my check for audio capability within CEA extension might has some conflict with Adam's patch to add more detailed mode from CEA ext block. But I haven't found Adam's patch in next tree, so I just kept my current change.
This fixed incorrect probe for DP monitor's audio. Fengguang helped to test this set. We got correct DP sound on Eizo EV2333W, and HDMI audio also worked with no regression found.
Patch is against Chris's drm-intel-next branch.
Ignore my previous r-b.
Reviewed-by: Adam Jackson ajax@redhat.com
- ajax
On Sun, 19 Sep 2010 14:52:05 +0800, Zhenyu Wang zhenyuw@linux.intel.com wrote:
I found my check for audio capability within CEA extension might has some conflict with Adam's patch to add more detailed mode from CEA ext block. But I haven't found Adam's patch in next tree, so I just kept my current change.
This fixed incorrect probe for DP monitor's audio. Fengguang helped to test this set. We got correct DP sound on Eizo EV2333W, and HDMI audio also worked with no regression found.
Applied. Thanks for the reminder, Zhenyu. -Chris
dri-devel@lists.freedesktop.org
-
Adam Jackson -
Ben Skeggs -
Chris Wilson -
Zhenyu Wang