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

[Daniel Vetter <daniel@ffwll.ch>]

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