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

[Shankar, Uma]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of
>Shankar, Uma
>Sent: Wednesday, September 23, 2015 8:19 PM
>To: Nikula, Jani; intel-gfx at lists.freedesktop.org
>Cc: Kumar, Shobhit
>Subject: Re: [Intel-gfx] [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 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

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@intel.com
>>
>>
>>>  		encoder->enable(encoder);
>>>  		intel_opregion_notify_encoder(encoder, true);
>>>  	}
>>
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx