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 at redhat.com>
> > > Signed-off-by: Wu Fengguang <fengguang.wu at 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
