Re: [PATCH v3 3/4] drm/i915: Enable/disable TMDS output buffers in DP++ adaptor as needed

["Sharma, Shashank" <shashank.sharma@intel.com>]

On 5/4/2016 5:19 PM, Ville Syrjälä wrote:
> On Wed, May 04, 2016 at 03:43:30PM +0530, Sharma, Shashank wrote:
>>
>> On 5/3/2016 12:38 AM, ville.syrjala@linux.intel.com wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> To save a bit of power, let's try to turn off the TMDS output buffers
>>> in DP++ adaptors when we're not driving the port.
>>>
>>> v2: Let's not forget DDI, toss in a debug message while at it
>>> v3: Just do the TMDS output control based on adaptor type. With the
>>>       helper getting passed the type, we wouldn't actually have to
>>>       check at all in the driver, but the check eliminates the debug
>>>       output more honest
>>>
>>> Cc: stable@vger.kernel.org
>>> Cc: Tore Anderson <tore@fud.no>
>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> Cc: Shashank Sharma <shashank.sharma@intel.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/intel_ddi.c  | 12 ++++++++++++
>>>    drivers/gpu/drm/i915/intel_drv.h  |  1 +
>>>    drivers/gpu/drm/i915/intel_hdmi.c | 20 ++++++++++++++++++++
>>>    3 files changed, 33 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index 422ec81ef59b..a3600704e6d4 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -1601,6 +1601,12 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>>>    	enum port port = intel_ddi_get_encoder_port(intel_encoder);
>>>    	int type = intel_encoder->type;
>>>
>>> +	if (type == INTEL_OUTPUT_HDMI) {
>> how to handle type2 active adapters here, which can show type = DP ?
>
> The dual mode concept shouldn't apply to active adapters. Those, as you
> say, appear as DP and so will be handled as if they were native DP sinks
> more or less.
>
> Or are you aware of active DP dongles that also implement the dual mode
> standatd? Apart from LSPCON of course. Actually I'm not sure how LSPCON
> will handle this. If we drive it in PCON mode, would it still respect
> the dual mode register settings, including the TMDS OE# state?
>
I cross checked LSPCON specs, it respects TMDS_OE (0x80, 0x20) register 
only in LS mode (like stype 2 adapter), by putting it in low power 
state. So I guess you are right, we have to handle LSPCON separately, 
based on if it's in LS mode or PCON mode.  May be while re-basing LSPCON 
layer on top of this new series, I can think of this.

Reviewed-by: Shashank Sharma <shashank.sharma@intel.com>

Regards
Shashank
>>> +		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>>> +
>>> +		intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
>>> +	}
>>> +
>>>    	intel_prepare_ddi_buffer(intel_encoder);
>>>
>>>    	if (type == INTEL_OUTPUT_EDP) {
>>> @@ -1667,6 +1673,12 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
>>>    					DPLL_CTRL2_DDI_CLK_OFF(port)));
>>>    	else if (INTEL_INFO(dev)->gen < 9)
>>>    		I915_WRITE(PORT_CLK_SEL(port), PORT_CLK_SEL_NONE);
>>> +
>>> +	if (type == INTEL_OUTPUT_HDMI) {
>> Same as above.
>>
>> Regards
>> Shashank
>>> +		struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(encoder);
>>> +
>>> +		intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
>>> +	}
>>>    }
>>>
>>>    static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index e94d18fb2ff1..dbcb80c70e2e 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1419,6 +1419,7 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>>    struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>    			       struct intel_crtc_state *pipe_config);
>>> +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>>
>>>
>>>    /* intel_lvds.c */
>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>> index 31ca11134294..c4d93e6b4bed 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -836,6 +836,22 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
>>>    	intel_hdmi_set_hdmi_infoframe(encoder, adjusted_mode);
>>>    }
>>>
>>> +void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable)
>>> +{
>>> +	struct drm_i915_private *dev_priv = to_i915(intel_hdmi_to_dev(hdmi));
>>> +	struct i2c_adapter *adapter =
>>> +		intel_gmbus_get_adapter(dev_priv, hdmi->ddc_bus);
>>> +
>>> +	if (hdmi->dp_dual_mode.type < DRM_DP_DUAL_MODE_TYPE2_DVI)
>>> +		return;
>>> +
>>> +	DRM_DEBUG_KMS("%s DP dual mode adaptor TMDS output\n",
>>> +		      enable ? "Enabling" : "Disabling");
>>> +
>>> +	drm_dp_dual_mode_set_tmds_output(hdmi->dp_dual_mode.type,
>>> +					 adapter, enable);
>>> +}
>>> +
>>>    static void intel_hdmi_prepare(struct intel_encoder *encoder)
>>>    {
>>>    	struct drm_device *dev = encoder->base.dev;
>>> @@ -845,6 +861,8 @@ static void intel_hdmi_prepare(struct intel_encoder *encoder)
>>>    	const struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
>>>    	u32 hdmi_val;
>>>
>>> +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, true);
>>> +
>>>    	hdmi_val = SDVO_ENCODING_HDMI;
>>>    	if (!HAS_PCH_SPLIT(dev) && crtc->config->limited_color_range)
>>>    		hdmi_val |= HDMI_COLOR_RANGE_16_235;
>>> @@ -1142,6 +1160,8 @@ static void intel_disable_hdmi(struct intel_encoder *encoder)
>>>    	}
>>>
>>>    	intel_hdmi->set_infoframes(&encoder->base, false, NULL);
>>> +
>>> +	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
>>>    }
>>>
>>>    static void g4x_disable_hdmi(struct intel_encoder *encoder)
>>>
>