2014-04-02 14:08 GMT-03:00 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: > Needs to happen after clock is running or it doesn't behave correctly. > > Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> > --- > drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index fb9839b..a4ca63b6 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -669,8 +669,6 @@ static void intel_hdmi_mode_set(struct intel_encoder *encoder) > > I915_WRITE(intel_hdmi->hdmi_reg, hdmi_val); > POSTING_READ(intel_hdmi->hdmi_reg); > - > - intel_hdmi->set_infoframes(&encoder->base, adjusted_mode); > } > > 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; > Due to my past, I am not a person who should be reviewing these patches because I have a tendency to fear that a single variable rename will break everybody's machines at this point of the code :) My problem with this patch is that now we do the same thing at 3 different points depending on the platform: - For VLV, we will enable infoframes at encoder->pre_enable - For ILK and friends, we will enable infoframes at encoder->enable - For HSW+, we will still enable infoframes at encoder->modeset I'd really like to have a standard behavior here :) Also, for ILK/SNB/IVB, we run encoder->enable after the very end of the modeset sequence, and I suspect the pipe is already running at that point (since the we already called intel_enable_pipe, we already ran intel_enable_planes, and we also called ironlake_pch_enable). For that case, we probably need to implement all those "wait for the exact VSYNC moment before touching register" restrictions that are described on the spec. Or we find a way to also call set_infoframes at pre_enable on these platforms. Did we test these patches on the ILK+ family? I also remember a lot of bugs that would only be seen after suspend/resume, so I suggest testing it too :) The good thing is that moving register writes away from the mode_set callbacks is good IMHO since at some point we'll want crtc_enable to fully setup the HW, so runtime PM will be able to work without requiring a crtc_mode_set call. But you need to patch HSW+ too for that :P This is not an ack, nack nor a reviewed-by :) If you are still confident, feel free to go ahead. Thanks, Paulo > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
