Re: [Intel-gfx] [PATCH] drm/i915: add a force-dvi HDMI audio mode

2012-02-13 Thread Daniel Vetter 
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 aarca...@redhat.com
 Signed-off-by: Wu Fengguang fengguang...@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.0 +0800
 +++ linux-next/drivers/gpu/drm/i915/i915_drv.h2012-02-08 
 10:37:30.0 +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.0 +0800
 +++ linux-next/drivers/gpu/drm/i915/intel_hdmi.c  2012-02-08 
 10:37:30.0 +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.c2011-10-19 
 11:11:20.0 +0800
 +++ linux-next/drivers/gpu/drm/i915/intel_modes.c 2012-02-08 
 10:37:30.0 +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: dan...@ffwll.ch
Mobile: +41 (0)79 365 57 48
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: add a force-dvi HDMI audio mode

2012-02-13 Thread Wu Fengguang 
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 aarca...@redhat.com
  Signed-off-by: Wu Fengguang fengguang...@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.0 +0800
  +++ linux-next/drivers/gpu/drm/i915/intel_hdmi.c2012-02-08 
  10:37:30.0 +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 aarca...@redhat.com
Signed-off-by: Wu Fengguang fengguang...@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.0 +0800
+++ linux-next/drivers/gpu/drm/i915/i915_drv.h  2012-02-14 11:21:03.0 
+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.0 +0800
+++ linux-next/drivers/gpu/drm/i915/intel_hdmi.c2012-02-14 
11:25:16.0 +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 =
+

[Intel-gfx] [PATCH] drm/i915: add a force-dvi HDMI audio mode

2012-02-07 Thread Wu Fengguang 
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 aarca...@redhat.com
Signed-off-by: Wu Fengguang fengguang...@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.0 +0800
+++ linux-next/drivers/gpu/drm/i915/i915_drv.h  2012-02-08 10:37:30.0 
+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.0 +0800
+++ linux-next/drivers/gpu/drm/i915/intel_hdmi.c2012-02-08 
10:37:30.0 +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.0 +0800
+++ linux-next/drivers/gpu/drm/i915/intel_modes.c   2012-02-08 
10:37:30.0 +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