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

[Jani Nikula]

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.


[1] http://mid.gmane.org/87twqlnw5k.fsf@intel.com


>  		encoder->enable(encoder);
>  		intel_opregion_notify_encoder(encoder, true);
>  	}

-- 
Jani Nikula, Intel Open Source Technology Center