From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: add a "force-dvi" HDMI audio mode Date: Tue, 14 Feb 2012 10:02:23 +0100 Message-ID: <20120214090223.GA4097@phenom.ffwll.local> References: <20120208024820.GA15982@localhost> <20120213192201.GK5301@phenom.ffwll.local> <20120214034536.GA25300@localhost> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ww0-f41.google.com (mail-ww0-f41.google.com [74.125.82.41]) by gabe.freedesktop.org (Postfix) with ESMTP id 674859E80F for ; Tue, 14 Feb 2012 01:02:14 -0800 (PST) Received: by wgbdt11 with SMTP id dt11so3764806wgb.0 for ; Tue, 14 Feb 2012 01:02:13 -0800 (PST) Content-Disposition: inline In-Reply-To: <20120214034536.GA25300@localhost> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Wu Fengguang Cc: Andrea Arcangeli , intel-gfx@lists.freedesktop.org, Wang Zhenyu List-Id: intel-gfx@lists.freedesktop.org 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 > > > Signed-off-by: Wu Fengguang > > > > 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