Re: [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder support in CRTC modeset

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 05 Oct 2015, "Shankar, Uma" <uma.shankar@intel.com> wrote:
>>-----Original Message-----
>>From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
>>Sent: Friday, October 2, 2015 6:05 PM
>>To: Shankar, Uma
>>Cc: intel-gfx@lists.freedesktop.org; Kumar, Shobhit
>>Subject: Re: [Intel-gfx] [BXT MIPI PATCH v5 05/14] drm/i915/bxt: DSI encoder
>>support in CRTC modeset
>>
>>On Thu, Oct 01, 2015 at 10:23:49PM +0530, Uma Shankar wrote:
>>> From: Shashank Sharma <shashank.sharma@intel.com>
>>>
>>> SKL and BXT qualifies the HAS_DDI() check, and hence haswell modeset
>>> functions are re-used for modeset sequence. But DDI interface doesn't
>>> include support for DSI.
>>> This patch adds:
>>> 1. cases for DSI encoder, in those modeset functions and allows
>>>    a CRTC modeset
>>> 2. Adds call to pre_pll enabled from CRTC modeset function. Nothing
>>>    needs to be done as such in CRTC for DSI encoder, as PLL, clock
>>>    and and transcoder programming will be taken care in encoder's
>>>    pre_enable and pre_pll_enable function.
>>>
>>> v2: Fixed Jani's review comments. Added INVALID_PORT for non DDI
>>>     encoder like DSI for platforms having HAS_DDI as true.
>>>
>>> v3: Rebased on latest drm-nightly branch. Added a WARN_ON for invalid
>>>     encoder.
>>>
>>> v4: WARN_ON for invalid encoder is refactored as per Jani's suggestion.
>>>     Fixed the sequence for pre_pll_enable.
>>>
>>> v5: Protected DDI code paths in case of DSI encoder calls.
>>>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> Signed-off-by: Uma Shankar <uma.shankar@intel.com>
>>
>>Ok, after this patch we get stuff like this:
>>
>>	for_each_encoder_on_crtc(dev, crtc, encoder) {
>>		if (encoder->pre_pll_enable)
>>			encoder->pre_pll_enable(encoder);
>>		if (encoder->pre_enable)
>>			encoder->pre_enable(encoder);
>>	}
>>
>>	if (intel_crtc->config->has_pch_encoder) {
>>		intel_set_pch_fifo_underrun_reporting(dev_priv,
>>TRANSCODER_A,
>>						      true);
>>		dev_priv->display.fdi_link_train(crtc);
>>	}
>>
>>	if (!is_dsi)
>>		intel_ddi_enable_pipe_clock(intel_crtc);
>>
>>1. Please remove pre_pll_enable again, we don't need 2 callbacks in exactly the
>>same spot. Yes this might mean that you need special bxt_ versions of that in the
>>dsi encoder, we have that everywhere.
>>
>>2. the has_pch_encoder is already something encoder-specific (it's exclusively
>>used by the HSW LPT CRT encoder). Now we have another one of those for the
>>!is_dsi case. These special-cases should be moved into the
>>encoder->pre_enable callbacks, that's what they're for.
>>
>>I'm not going to block these patches are (18months is already ridiculous), but I
>>want this cleanup done. Uma, can you pls own this? If you can't do it yourself
>>please escalate to Indranil so he can find someone.
>>
>>Thanks, Daniel
>>
>
> Hi Daniel,
> I will discuss with Indranil and will get this done. 
>
> Thanks for your support in getting the video mode patches merged.

I'll do the cleanups, don't worry about it.

BR,
Jani.


>
> Regards,
> Uma Shankar
>
>>> ---
>>>  drivers/gpu/drm/i915/intel_ddi.c      |    7 +++++--
>>>  drivers/gpu/drm/i915/intel_display.c  |   21 +++++++++++++++------
>>>  drivers/gpu/drm/i915/intel_opregion.c |    9 +++++++--
>>>  3 files changed, 27 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c
>>> b/drivers/gpu/drm/i915/intel_ddi.c
>>> index cacb07b..7b7f544 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -390,8 +390,10 @@ void intel_prepare_ddi(struct drm_device *dev)
>>>  		enum port port;
>>>  		bool supports_hdmi;
>>>
>>> -		ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port);
>>> +		if (intel_encoder->type == INTEL_OUTPUT_DSI)
>>> +			continue;
>>>
>>> +		ddi_get_encoder_port(intel_encoder, &intel_dig_port, &port);
>>>  		if (visited[port])
>>>  			continue;
>>>
>>> @@ -1779,7 +1781,8 @@ bool intel_ddi_get_hw_state(struct intel_encoder
>>> *encoder,  void intel_ddi_enable_pipe_clock(struct intel_crtc
>>> *intel_crtc)  {
>>>  	struct drm_crtc *crtc = &intel_crtc->base;
>>> -	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>>> +	struct drm_device *dev = crtc->dev;
>>> +	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  	struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>>>  	enum port port = intel_ddi_get_encoder_port(intel_encoder);
>>>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>>> b/drivers/gpu/drm/i915/intel_display.c
>>> index b8e0310..ea0f533 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>> @@ -4991,6 +4991,7 @@ static void haswell_crtc_enable(struct drm_crtc
>>*crtc)
>>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>  	struct intel_encoder *encoder;
>>> +	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>>>  	int pipe = intel_crtc->pipe;
>>>
>>>  	WARN_ON(!crtc->state->enable);
>>> @@ -5023,9 +5024,12 @@ static void haswell_crtc_enable(struct drm_crtc
>>*crtc)
>>>  	intel_crtc->active = true;
>>>
>>>  	intel_set_cpu_fifo_underrun_reporting(dev_priv, pipe, true);
>>> -	for_each_encoder_on_crtc(dev, crtc, encoder)
>>> +	for_each_encoder_on_crtc(dev, crtc, encoder) {
>>> +		if (encoder->pre_pll_enable)
>>> +			encoder->pre_pll_enable(encoder);
>>>  		if (encoder->pre_enable)
>>>  			encoder->pre_enable(encoder);
>>> +	}
>>>
>>>  	if (intel_crtc->config->has_pch_encoder) {
>>>  		intel_set_pch_fifo_underrun_reporting(dev_priv,
>>TRANSCODER_A, @@
>>> -5033,7 +5037,8 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>>  		dev_priv->display.fdi_link_train(crtc);
>>>  	}
>>>
>>> -	intel_ddi_enable_pipe_clock(intel_crtc);
>>> +	if (!is_dsi)
>>> +		intel_ddi_enable_pipe_clock(intel_crtc);
>>>
>>>  	if (INTEL_INFO(dev)->gen == 9)
>>>  		skylake_pfit_update(intel_crtc, 1); @@ -5049,7 +5054,8 @@
>>static
>>> void haswell_crtc_enable(struct drm_crtc *crtc)
>>>  	intel_crtc_load_lut(crtc);
>>>
>>>  	intel_ddi_set_pipe_settings(crtc);
>>> -	intel_ddi_enable_transcoder_func(crtc);
>>> +	if (!is_dsi)
>>> +		intel_ddi_enable_transcoder_func(crtc);
>>>
>>>  	intel_update_watermarks(crtc);
>>>  	intel_enable_pipe(intel_crtc);
>>> @@ -5057,7 +5063,7 @@ 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);
>>> @@ -5159,6 +5165,7 @@ static void haswell_crtc_disable(struct drm_crtc
>>*crtc)
>>>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>>  	struct intel_encoder *encoder;
>>>  	enum transcoder cpu_transcoder = intel_crtc->config->cpu_transcoder;
>>> +	bool is_dsi = intel_pipe_has_type(intel_crtc, INTEL_OUTPUT_DSI);
>>>
>>>  	if (!intel_crtc->active)
>>>  		return;
>>> @@ -5179,7 +5186,8 @@ static void haswell_crtc_disable(struct drm_crtc
>>*crtc)
>>>  	if (intel_crtc->config->dp_encoder_is_mst)
>>>  		intel_ddi_set_vc_payload_alloc(crtc, false);
>>>
>>> -	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>>> +	if (!is_dsi)
>>> +		intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>>>
>>>  	if (INTEL_INFO(dev)->gen == 9)
>>>  		skylake_pfit_update(intel_crtc, 0); @@ -5188,7 +5196,8 @@
>>static
>>> void haswell_crtc_disable(struct drm_crtc *crtc)
>>>  	else
>>>  		MISSING_CASE(INTEL_INFO(dev)->gen);
>>>
>>> -	intel_ddi_disable_pipe_clock(intel_crtc);
>>> +	if (!is_dsi)
>>> +		intel_ddi_disable_pipe_clock(intel_crtc);
>>>
>>>  	if (intel_crtc->config->has_pch_encoder) {
>>>  		lpt_disable_pch_transcoder(dev_priv);
>>> diff --git a/drivers/gpu/drm/i915/intel_opregion.c
>>> b/drivers/gpu/drm/i915/intel_opregion.c
>>> index 4813374..db518ef 100644
>>> --- a/drivers/gpu/drm/i915/intel_opregion.c
>>> +++ b/drivers/gpu/drm/i915/intel_opregion.c
>>> @@ -334,8 +334,12 @@ int intel_opregion_notify_encoder(struct
>>intel_encoder *intel_encoder,
>>>  	if (!HAS_DDI(dev))
>>>  		return 0;
>>>
>>> -	port = intel_ddi_get_encoder_port(intel_encoder);
>>> -	if (port == PORT_E) {
>>> +	if (intel_encoder->type == INTEL_OUTPUT_DSI)
>>> +		port = 0;
>>> +	else
>>> +		port = intel_ddi_get_encoder_port(intel_encoder);
>>> +
>>> +	if (port == PORT_E)  {
>>>  		port = 0;
>>>  	} else {
>>>  		parm |= 1 << port;
>>> @@ -356,6 +360,7 @@ int intel_opregion_notify_encoder(struct
>>intel_encoder *intel_encoder,
>>>  		type = DISPLAY_TYPE_EXTERNAL_FLAT_PANEL;
>>>  		break;
>>>  	case INTEL_OUTPUT_EDP:
>>> +	case INTEL_OUTPUT_DSI:
>>>  		type = DISPLAY_TYPE_INTERNAL_FLAT_PANEL;
>>>  		break;
>>>  	default:
>>> --
>>> 1.7.9.5
>>>
>>> _______________________________________________
>>> Intel-gfx mailing list
>>> Intel-gfx@lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>--
>>Daniel Vetter
>>Software Engineer, Intel Corporation
>>http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx