On Tue, 01 Sep 2015, Uma Shankar <[email protected]> wrote: > @@ -5057,13 +5060,16 @@ static void haswell_crtc_enable(struct drm_crtc *crtc) > if (intel_crtc->config->has_pch_encoder) > lpt_pch_enable(crtc); > > - if (intel_crtc->config->dp_encoder_is_mst) > + if (intel_crtc->config->dp_encoder_is_mst && !is_dsi) > intel_ddi_set_vc_payload_alloc(crtc, true); > > assert_vblank_disabled(crtc); > drm_crtc_vblank_on(crtc); > > for_each_encoder_on_crtc(dev, crtc, encoder) { > + if (encoder->pre_pll_enable) > + encoder->pre_pll_enable(encoder); > +
We should not modify the crtc enable sequence to accommodate the needs of one encoder type only. The hook names should be taken as describing roughly when in the sequence they are called, not necessarily what they must do for each encoder. If an encoder requires a different ordering or sequence, it should handle this in what it does in its hooks - and this may possibly need to be different on each platform. Here, the ->pre_pll_enable call is added after the ->pre_enable call, making the sequence of calls surprising. Also, there is no point in calling ->pre_pll_enable in the same loop as ->enable; the encoder could just as well do everything in ->enable. Indeed, this may be what you should do on Broxton. I'm willing to ignore [1] if Daniel thinks my worry is unwarranted, but this part must be fixed. BR, Jani. [1] http://mid.gmane.org/[email protected] > encoder->enable(encoder); > intel_opregion_notify_encoder(encoder, true); > } -- Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
