[Intel-gfx] [BXT MIPI PATCH v3 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Shankar, Uma]

>-----Original Message-----
>From: Nikula, Jani
>Sent: Wednesday, September 23, 2015 6:24 PM
>To: Shankar, Uma; intel-gfx at lists.freedesktop.org
>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 at intel.com> 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

>[1] http://mid.gmane.org/87twqlnw5k.fsf@intel.com
>
>
>>  		encoder->enable(encoder);
>>  		intel_opregion_notify_encoder(encoder, true);
>>  	}
>