On Mon, Feb 13, 2012 at 08:22:01PM +0100, Daniel Vetter wrote:
> On Wed, Feb 08, 2012 at 10:48:20AM +0800, Wu Fengguang wrote:
> > When HDMI-DVI converter is used, it's not only necessary to turn off
> > audio, but also to disable HDMI_MODE_SELECT and video infoframe. Since
> > the DVI mode is mainly tied to audio functionality from end user POV,
> > add a new "force-dvi" audio mode:
> >
> > xrandr --output HDMI1 --set audio force-dvi
> >
> > Note that most users won't need to set this and happily rely on the
> > EDID based DVI auto detection.
> >
> > Reported-by: Andrea Arcangeli <aarcange at redhat.com>
> > Signed-off-by: Wu Fengguang <fengguang.wu at intel.com>
>
> Jesse acked this patch on irc, but I've noticed a few little things. See
> below for the details.
OK, thanks!
> > --- linux-next.orig/drivers/gpu/drm/i915/intel_hdmi.c 2012-01-07 20:47:34.000000000 +0800
> > +++ linux-next/drivers/gpu/drm/i915/intel_hdmi.c 2012-02-08 10:37:30.000000000 +0800
> > @@ -339,7 +339,9 @@ intel_hdmi_detect(struct drm_connector *
> > if (edid) {
> > if (edid->input & DRM_EDID_INPUT_DIGITAL) {
> > status = connector_status_connected;
> > - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
> > + if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
> > + 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;
> > @@ -415,8 +417,8 @@ intel_hdmi_set_property(struct drm_conne
> > else
> > has_audio = i > 0;
>
> All these comparison with i use magic values instead of your new enum
> definitions. Can you replace these with the enums like you've done below,
> i.e. for the above if block:
>
> if (i == HDMI_AUDIO_AUOT)
> ...
> else if (i == HDMI_AUDIO_ON)
> has_audio = true;
Done.
> intel_hdmi_detect is also a place that checks these values (with
> intel_hdmi->force_audio) - please use the enum values there, too. Also
> please change the type of intel_hdmi->force_audio from int to the enum.
Done.
> Another thing I've noticed is that in intel_hdmi_detect you don't force
> has_hdmi_sink to false if the force_dvi option is set.
It's implicitly initialized to false at the beginning. Isn't that enough?
Updated patch follows. Compile tested.
Thanks,
Fengguang
---
Subject: drm/i915: add an "force-dvi" HDMI audio mode
Date: Fri Jan 06 11:04:00 CST 2012
When HDMI-DVI converter is used, it's not only necessary to turn off
audio, but also to disable HDMI_MODE_SELECT and video infoframe. Since
the DVI mode is mainly tied to audio functionality from end user POV,
add a new "force-dvi" audio mode:
xrandr --output HDMI1 --set audio force-dvi
Note that most users won't need to set this and happily rely on the EDID
based DVI auto detection.
Reported-by: Andrea Arcangeli <aarcange at redhat.com>
Signed-off-by: Wu Fengguang <fengguang.wu at intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
drivers/gpu/drm/i915/intel_hdmi.c | 21 ++++++++++++---------
drivers/gpu/drm/i915/intel_modes.c | 4 +++-
3 files changed, 22 insertions(+), 10 deletions(-)
--- linux-next.orig/drivers/gpu/drm/i915/i915_drv.h 2012-02-08 18:44:59.000000000 +0800
+++ linux-next/drivers/gpu/drm/i915/i915_drv.h 2012-02-14 11:21:03.000000000 +0800
@@ -749,6 +749,13 @@ typedef struct drm_i915_private {
struct drm_property *force_audio_property;
} drm_i915_private_t;
+enum hdmi_force_audio {
+ HDMI_AUDIO_OFF_DVI = -2, /* no aux data for HDMI-DVI converter */
+ HDMI_AUDIO_OFF, /* force turn off HDMI audio */
+ HDMI_AUDIO_AUTO, /* trust EDID */
+ HDMI_AUDIO_ON, /* force turn on HDMI audio */
+};
+
enum i915_cache_level {
I915_CACHE_NONE,
I915_CACHE_LLC,
--- linux-next.orig/drivers/gpu/drm/i915/intel_hdmi.c 2012-02-08 18:44:59.000000000 +0800
+++ linux-next/drivers/gpu/drm/i915/intel_hdmi.c 2012-02-14 11:25:16.000000000 +0800
@@ -44,7 +44,7 @@ struct intel_hdmi {
uint32_t color_range;
bool has_hdmi_sink;
bool has_audio;
- int force_audio;
+ enum hdmi_force_audio force_audio;
void (*write_infoframe)(struct drm_encoder *encoder,
struct dip_infoframe *frame);
};
@@ -339,7 +339,9 @@ intel_hdmi_detect(struct drm_connector *
if (edid) {
if (edid->input & DRM_EDID_INPUT_DIGITAL) {
status = connector_status_connected;
- intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
+ if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
+ 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;
@@ -347,8 +349,9 @@ intel_hdmi_detect(struct drm_connector *
}
if (status == connector_status_connected) {
- if (intel_hdmi->force_audio)
- intel_hdmi->has_audio = intel_hdmi->force_audio > 0;
+ if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
+ intel_hdmi->has_audio =
+ (intel_hdmi->force_audio == HDMI_AUDIO_ON);
}
return status;
@@ -402,7 +405,7 @@ intel_hdmi_set_property(struct drm_conne
return ret;
if (property == dev_priv->force_audio_property) {
- int i = val;
+ enum hdmi_force_audio i = val;
bool has_audio;
if (i == intel_hdmi->force_audio)
@@ -410,13 +413,13 @@ intel_hdmi_set_property(struct drm_conne
intel_hdmi->force_audio = i;
- if (i == 0)
+ if (i == HDMI_AUDIO_AUTO)
has_audio = intel_hdmi_detect_audio(connector);
else
- has_audio = i > 0;
+ has_audio = (i == HDMI_AUDIO_ON);
- if (has_audio == intel_hdmi->has_audio)
- return 0;
+ if (i == HDMI_AUDIO_OFF_DVI)
+ intel_hdmi->has_hdmi_sink = 0;
intel_hdmi->has_audio = has_audio;
goto done;
--- linux-next.orig/drivers/gpu/drm/i915/intel_modes.c 2012-02-08 15:18:15.000000000 +0800
+++ linux-next/drivers/gpu/drm/i915/intel_modes.c 2012-02-14 11:21:03.000000000 +0800
@@ -84,6 +84,7 @@ int intel_ddc_get_modes(struct drm_conne
}
static const char *force_audio_names[] = {
+ "force-dvi",
"off",
"auto",
"on",
@@ -106,7 +107,8 @@ intel_attach_force_audio_property(struct
return;
for (i = 0; i < ARRAY_SIZE(force_audio_names); i++)
- drm_property_add_enum(prop, i, i-1, force_audio_names[i]);
+ drm_property_add_enum(prop, i, i-2,
+ force_audio_names[i]);
dev_priv->force_audio_property = prop;
}
