>-----Original Message----- >From: Intel-gfx [mailto:intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of >Shankar, Uma >Sent: Wednesday, September 23, 2015 8:19 PM >To: Nikula, Jani; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >Cc: Kumar, Shobhit >Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder >support in CRTC modeset > > > >>-----Original Message----- >>From: Nikula, Jani >>Sent: Wednesday, September 23, 2015 6:24 PM >>To: Shankar, Uma; intel-gfx@xxxxxxxxxxxxxxxxxxxxx >>Cc: Kumar, Shobhit; Deak, Imre; Sharma, Shashank; Shankar, Uma >>Subject: Re: [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder >>support in CRTC modeset >> >>On Tue, 01 Sep 2015, Uma Shankar <uma.shankar@xxxxxxxxx> 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. >> > >The pre_pll_enable callback was not being used earlier for any encoder in >haswell functions. >This is the reason we used it for DSI at place appropriate for DSI sequence. I will >remove the callback and put the code in encoder->enable callback itself. > >Regards, >Uma Shankar This callback should ideally be before pre_enable. I will update and resend the patch. This is the order followed for BYT/CHT as well. Regards, Uma Shankar The correct order for BXT is also pre_pll_enable, >>[1] http://mid.gmane.org/87twqlnw5k.fsf@xxxxxxxxx >> >> >>> encoder->enable(encoder); >>> intel_opregion_notify_encoder(encoder, true); >>> } >> > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@xxxxxxxxxxxxxxxxxxxxx >http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
