All of lore.kernel.org
 help / color / mirror / Atom feed
* [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.