* [PATCH] drm/i915: add a "force-dvi" HDMI audio mode
@ 2012-02-08 2:48 Wu Fengguang
2012-02-13 19:22 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Wu Fengguang @ 2012-02-08 2:48 UTC (permalink / raw)
To: Jesse Barnes; +Cc: Andrea Arcangeli, intel-gfx, Wang Zhenyu
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@redhat.com>
Signed-off-by: Wu Fengguang <fengguang.wu@intel.com>
---
drivers/gpu/drm/i915/i915_drv.h | 7 +++++++
drivers/gpu/drm/i915/intel_hdmi.c | 8 +++++---
drivers/gpu/drm/i915/intel_modes.c | 4 +++-
3 files changed, 15 insertions(+), 4 deletions(-)
--- linux-next.orig/drivers/gpu/drm/i915/i915_drv.h 2012-01-31 20:44:59.000000000 +0800
+++ linux-next/drivers/gpu/drm/i915/i915_drv.h 2012-02-08 10:37:30.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-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;
- 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 2011-10-19 11:11:20.000000000 +0800
+++ linux-next/drivers/gpu/drm/i915/intel_modes.c 2012-02-08 10:37:30.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;
}
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: add a "force-dvi" HDMI audio mode 2012-02-08 2:48 [PATCH] drm/i915: add a "force-dvi" HDMI audio mode Wu Fengguang @ 2012-02-13 19:22 ` Daniel Vetter 2012-02-14 3:45 ` Wu Fengguang 0 siblings, 1 reply; 4+ messages in thread From: Daniel Vetter @ 2012-02-13 19:22 UTC (permalink / raw) To: Wu Fengguang; +Cc: Andrea Arcangeli, intel-gfx, Wang Zhenyu 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@redhat.com> > Signed-off-by: Wu Fengguang <fengguang.wu@intel.com> Jesse acked this patch on irc, but I've noticed a few little things. See below for the details. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > drivers/gpu/drm/i915/intel_hdmi.c | 8 +++++--- > drivers/gpu/drm/i915/intel_modes.c | 4 +++- > 3 files changed, 15 insertions(+), 4 deletions(-) > > --- linux-next.orig/drivers/gpu/drm/i915/i915_drv.h 2012-01-31 20:44:59.000000000 +0800 > +++ linux-next/drivers/gpu/drm/i915/i915_drv.h 2012-02-08 10:37:30.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-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; 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. 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. > > - 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 2011-10-19 11:11:20.000000000 +0800 > +++ linux-next/drivers/gpu/drm/i915/intel_modes.c 2012-02-08 10:37:30.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; > } > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: add a "force-dvi" HDMI audio mode 2012-02-13 19:22 ` Daniel Vetter @ 2012-02-14 3:45 ` Wu Fengguang 2012-02-14 9:02 ` Daniel Vetter 0 siblings, 1 reply; 4+ messages in thread From: Wu Fengguang @ 2012-02-14 3:45 UTC (permalink / raw) To: Daniel Vetter; +Cc: Andrea Arcangeli, intel-gfx, Wang Zhenyu 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@redhat.com> > > Signed-off-by: Wu Fengguang <fengguang.wu@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@redhat.com> Signed-off-by: Wu Fengguang <fengguang.wu@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; } ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: add a "force-dvi" HDMI audio mode 2012-02-14 3:45 ` Wu Fengguang @ 2012-02-14 9:02 ` Daniel Vetter 0 siblings, 0 replies; 4+ messages in thread From: Daniel Vetter @ 2012-02-14 9:02 UTC (permalink / raw) To: Wu Fengguang; +Cc: Andrea Arcangeli, intel-gfx, Wang Zhenyu On Tue, Feb 14, 2012 at 11:45:36AM +0800, Wu Fengguang wrote: > 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@redhat.com> > > > Signed-off-by: Wu Fengguang <fengguang.wu@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? Yep, you're right, should be enough. > Updated patch follows. Compile tested. Applied. You owe me a beer when it blows up because it's only compile-tested ;-) Cheers, Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-02-14 9:02 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-08 2:48 [PATCH] drm/i915: add a "force-dvi" HDMI audio mode Wu Fengguang 2012-02-13 19:22 ` Daniel Vetter 2012-02-14 3:45 ` Wu Fengguang 2012-02-14 9:02 ` Daniel Vetter
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.