Re: [PATCH 4/4] drm/i915: move infoframe setting to after port enable

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Thu, 3 Apr 2014 22:55:24 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Apr 3, 2014 at 6:49 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> >> >  static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder,
> >> > @@ -738,9 +736,13 @@ static void intel_enable_hdmi(struct intel_encoder *encoder)
> >> >     struct drm_i915_private *dev_priv = dev->dev_private;
> >> >     struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
> >> >     struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> >> > +   struct drm_display_mode *adjusted_mode =
> >> > +           &intel_crtc->config.adjusted_mode;
> >> >     u32 temp;
> >> >     u32 enable_bits = SDVO_ENABLE;
> >> >
> >> > +   intel_hdmi->set_infoframes(&encoder->base, adjusted_mode);
> >> > +
> >> >     if (intel_hdmi->has_audio)
> >> >             enable_bits |= SDVO_AUDIO_ENABLE;
> >>
> >> That kind of change tends to freak out Paulo, our master of infoframes. Do
> >> doecs really state that this is how stuff should work in general, or is
> >> this just a gm45/vlv thing? Or vlv only?
> >>
> >> /me remembers how often we've burnt our hands here
> >
> > Hey infoframe emission was totally broken for awhile due to a generic
> > change, and we didn't notice that right away. :)
> >
> > But yeah I'd prefer to test this on multiple platforms first, but don't
> > have that capability.  It does pass on BYT though, and the logic should
> > be similar to IBX, so this change ought to be safe.  It's easy to
> > revert too and make platform specific if we get regression reports, but
> > I expect it to fix weird issues instead of introducing new ones, based
> > on the infoframe analyzer results we have from BYT.
> 
> Yeah I guess the number of users who actual use a gm45 or so with a TV
> is probably still bigger than all the byt platforms out there :( If
> Paulo can ack this I'll happily merge. Paulo, can you please take a
> quick look?
> 
> Also you make this sound like it's a regression, but the patch is
> missing cc:stable and a sha1 citation of the offending commit. Jesse,
> can you please fix this?

No it's not a regression, we had an earlier regression on infoframes
though that seemed to have gone unnoticed for awhile when looking at
the logs and testing here...  It's fixed now though.

-- 
Jesse Barnes, Intel Open Source Technology Center