[v8,5/6] drm/i915: enable scrambling

Message ID	1488980269-13046-6-git-send-email-shashank.sharma@intel.com (mailing list archive)
State	New, archived
Headers	show Return-Path: <intel-gfx-bounces@lists.freedesktop.org> From: Shashank Sharma <shashank.sharma@intel.com> To: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org, ander.conselvan.de.oliveira@intel.com, ville.syrjala@linux.intel.com Date: Wed, 8 Mar 2017 19:07:48 +0530 Message-Id: <1488980269-13046-6-git-send-email-shashank.sharma@intel.com> In-Reply-To: <1488980269-13046-1-git-send-email-shashank.sharma@intel.com> References: <1488980269-13046-1-git-send-email-shashank.sharma@intel.com> Cc: jose.abreu@synopsys.com, daniel.vetter@intel.com, treding@nvidia.com Subject: [Intel-gfx] [PATCH v8 5/6] drm/i915: enable scrambling Precedence: list MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" <intel-gfx-bounces@lists.freedesktop.org>

Sharma, Shashank March 8, 2017, 1:37 p.m. UTC

Geminilake platform sports a native HDMI 2.0 controller, and is
 capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
 mendates scrambling for these higher clocks, for reduced RF footprint.

 This patch checks if the monitor supports scrambling, and if required,
 enables it during the modeset.

 V2: Addressed review comments from Ville:
 - Do not track scrambling status in DRM layer, track somewhere in
   driver like in intel_crtc_state.
 - Don't talk to monitor at such a low layer, set monitor scrambling
   in intel_enable_ddi() before enabling the port.

 V3: Addressed review comments from Jani
 - In comments, function names, use "sink" instead of "monitor",
   so that the implementation could be close to the language of
   HDMI spec.

V4: Addressed review comment from Maarten
 - scrambling -> hdmi_scrambling
 - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio

V5: Addressed review comments from Ville and Ander
 - Do not modifiy the crtc_state after compute_config. Move all
   scrambling and tmds_clock_ratio calcutations to compute_config.
 - While setting scrambling for source/sink, do not check the
   conditions again, just go by the crtc_state flags. This will
   simplyfy the condition checks.

V6: Addressed review comments from Ville
 - Do not add IS_GLK check in disable/enable function, instead add it
   in compute_config, while setting state flags.
 - Remove unnecessary paranthesis.
 - Simplyfy handle_sink_scrambling function as suggested.
 - Add readout code for scrambling status in get_ddi_config and add a
   check for the same in pipe_config_compare.

V7: Addressed review comments from Ander/Ville
 - No separate function for source scrambling, make it inline
 - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
 - Do not add platform check while setting source scrambling
 - Use pipe_config instead of crtc->config to set sink scrambling
 - To readout scrambling status, Compare with SCRAMBLING_MASK
   not any of its bits
 - Remove platform check in intel_pipe_config_compare while checking
   scrambling status

V8: Fixed mege conflict, Addressed review comments from Ander
 - Remove the desciption/comment about scrambling fom the caller, move
   it to the function
 - Move the IS_GLK check into scrambling function
 - Fix alignment

Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
 drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
 drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
 5 files changed, 101 insertions(+)

Ville Syrjälä March 8, 2017, 5:17 p.m. UTC | #1

On Wed, Mar 08, 2017 at 07:07:48PM +0530, Shashank Sharma wrote:
>  Geminilake platform sports a native HDMI 2.0 controller, and is
>  capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>  mendates scrambling for these higher clocks, for reduced RF footprint.
> 
>  This patch checks if the monitor supports scrambling, and if required,
>  enables it during the modeset.
> 
>  V2: Addressed review comments from Ville:
>  - Do not track scrambling status in DRM layer, track somewhere in
>    driver like in intel_crtc_state.
>  - Don't talk to monitor at such a low layer, set monitor scrambling
>    in intel_enable_ddi() before enabling the port.
> 
>  V3: Addressed review comments from Jani
>  - In comments, function names, use "sink" instead of "monitor",
>    so that the implementation could be close to the language of
>    HDMI spec.
> 
> V4: Addressed review comment from Maarten
>  - scrambling -> hdmi_scrambling
>  - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> 
> V5: Addressed review comments from Ville and Ander
>  - Do not modifiy the crtc_state after compute_config. Move all
>    scrambling and tmds_clock_ratio calcutations to compute_config.
>  - While setting scrambling for source/sink, do not check the
>    conditions again, just go by the crtc_state flags. This will
>    simplyfy the condition checks.
> 
> V6: Addressed review comments from Ville
>  - Do not add IS_GLK check in disable/enable function, instead add it
>    in compute_config, while setting state flags.
>  - Remove unnecessary paranthesis.
>  - Simplyfy handle_sink_scrambling function as suggested.
>  - Add readout code for scrambling status in get_ddi_config and add a
>    check for the same in pipe_config_compare.
> 
> V7: Addressed review comments from Ander/Ville
>  - No separate function for source scrambling, make it inline
>  - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
>  - Do not add platform check while setting source scrambling
>  - Use pipe_config instead of crtc->config to set sink scrambling
>  - To readout scrambling status, Compare with SCRAMBLING_MASK
>    not any of its bits
>  - Remove platform check in intel_pipe_config_compare while checking
>    scrambling status
> 
> V8: Fixed mege conflict, Addressed review comments from Ander
>  - Remove the desciption/comment about scrambling fom the caller, move
>    it to the function
>  - Move the IS_GLK check into scrambling function
>  - Fix alignment
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
>  drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
>  drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 101 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc843f9..2d50fdc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7830,7 +7830,14 @@ enum {
>  #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>  #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>  #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>  #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
> +					| TRANS_DDI_HDMI_SCRAMBLING)
>  
>  /* DisplayPort Transport Control */
>  #define _DP_TP_CTL_A			0x64040
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0467676..b3e4c4a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
>  			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>  		else
>  			temp |= TRANS_DDI_MODE_SELECT_DVI;
> +
> +		if (crtc_state->hdmi_scrambling)
> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>  	} else if (type == INTEL_OUTPUT_ANALOG) {
>  		temp |= TRANS_DDI_MODE_SELECT_FDI;
>  		temp |= (crtc_state->fdi_lanes - 1) << 1;
> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>  		struct intel_digital_port *intel_dig_port =
>  			enc_to_dig_port(encoder);
>  
> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> +			conn_state->connector, pipe_config, true);
> +
>  		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>  		 * are ignored so nothing special needs to be done besides
>  		 * enabling the port.
> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(intel_encoder);
>  
> +	if (type == INTEL_OUTPUT_HDMI) {
> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> +			old_conn_state->connector, old_crtc_state, false);
> +	}
> +
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  
>  		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
>  			pipe_config->has_infoframe = true;
> +
> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
> +			pipe_config->hdmi_scrambling = true;
> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>  		/* fall through */
>  	case TRANS_DDI_MODE_SELECT_DVI:
>  		pipe_config->lane_count = 4;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e77ca7d..fb5e670 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
>  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		PIPE_CONF_CHECK_I(limited_color_range);
> +
> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
>  	PIPE_CONF_CHECK_I(has_infoframe);
>  
>  	PIPE_CONF_CHECK_I(has_audio);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3af20c1..50d680c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -729,6 +729,12 @@ struct intel_crtc_state {
>  
>  	/* bitmask of visible planes (enum plane_id) */
>  	u8 active_planes;
> +
> +	/* HDMI scrambling status */
> +	bool hdmi_scrambling;
> +
> +	/* HDMI High TMDS char rate ratio */
> +	bool hdmi_high_tmds_clock_ratio;
>  };
>  
>  struct intel_crtc {
> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *pipe_config,
>  			       struct drm_connector_state *conn_state);
> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> +					struct drm_connector *connector,
> +					struct intel_crtc_state *config,
> +					bool enable);
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>  
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index c2184f7..4585aa8 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_scdc_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include <drm/intel_lpe_audio.h>
> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
>  	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
>  	int clock_12bpc = clock_8bpc * 3 / 2;
>  	int desired_bpp;
> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->lane_count = 4;
>  
> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
> +		if (scdc->scrambling.low_rates)
> +			pipe_config->hdmi_scrambling = true;
> +
> +		if (pipe_config->port_clock > 340000) {
> +			pipe_config->hdmi_scrambling = true;
> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> +		}
> +	}
> +
>  	return true;
>  }
>  
> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>  }
>  
> +/*
> + * This function handles scrambling on HDMI 2.0 capable sinks.
> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
> + * it enables scrambling. This should be called before enabling the HDMI
> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
> + * detect a scrambled clock within 100 ms.
> + */
> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> +				       struct drm_connector *connector,
> +				       struct intel_crtc_state *config,
> +				       bool enable)
> +{
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct drm_scrambling *scrambling =
> +				&connector->display_info.hdmi.scdc.scrambling;
> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> +							  intel_hdmi->ddc_bus);
> +	bool ret;
> +
> +	if (!scrambling->supported)
> +		return;
> +
> +	if (!IS_GEMINILAKE(dev_priv))
> +		return;

I'd still like to see this getting done uncoditionally. That way we
might get a bit of extra testing for the SCDC helper no matter what
platform the user happens to own.

I presume the spec says the sink will reset these to some default value
on HPD or something like that? But I'm not sure trusting them to get
this right is a good idea because reality often doesn't match the
specs. So the safer option IMO would be always tell the sink exactly
what we want.

> +
> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> +			intel_encoder->base.name, connector->name);
> +
> +	if (config->hdmi_high_tmds_clock_ratio) {
> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
> +		if (!ret) {
> +			DRM_ERROR("Set TMDS ratio failed\n");
> +			return;
> +		}
> +	}
> +
> +	if (config->hdmi_scrambling) {
> +		/* Enable/disable sink scrambling */
> +		ret = drm_scdc_set_scrambling(adptr, enable);
> +		if (!ret) {
> +			DRM_ERROR("Set sink scrambling failed\n");
> +			return;
> +		}
> +	}
> +
> +	DRM_DEBUG_KMS("sink scrambling handled\n");
> +}
> +
>  static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>  			     enum port port)
>  {
> -- 
> 1.9.1

Sharma, Shashank March 8, 2017, 5:26 p.m. UTC | #2

Regards

Shashank


On 3/8/2017 7:17 PM, Ville Syrjälä wrote:
> On Wed, Mar 08, 2017 at 07:07:48PM +0530, Shashank Sharma wrote:
>>   Geminilake platform sports a native HDMI 2.0 controller, and is
>>   capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>>   mendates scrambling for these higher clocks, for reduced RF footprint.
>>
>>   This patch checks if the monitor supports scrambling, and if required,
>>   enables it during the modeset.
>>
>>   V2: Addressed review comments from Ville:
>>   - Do not track scrambling status in DRM layer, track somewhere in
>>     driver like in intel_crtc_state.
>>   - Don't talk to monitor at such a low layer, set monitor scrambling
>>     in intel_enable_ddi() before enabling the port.
>>
>>   V3: Addressed review comments from Jani
>>   - In comments, function names, use "sink" instead of "monitor",
>>     so that the implementation could be close to the language of
>>     HDMI spec.
>>
>> V4: Addressed review comment from Maarten
>>   - scrambling -> hdmi_scrambling
>>   - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
>>
>> V5: Addressed review comments from Ville and Ander
>>   - Do not modifiy the crtc_state after compute_config. Move all
>>     scrambling and tmds_clock_ratio calcutations to compute_config.
>>   - While setting scrambling for source/sink, do not check the
>>     conditions again, just go by the crtc_state flags. This will
>>     simplyfy the condition checks.
>>
>> V6: Addressed review comments from Ville
>>   - Do not add IS_GLK check in disable/enable function, instead add it
>>     in compute_config, while setting state flags.
>>   - Remove unnecessary paranthesis.
>>   - Simplyfy handle_sink_scrambling function as suggested.
>>   - Add readout code for scrambling status in get_ddi_config and add a
>>     check for the same in pipe_config_compare.
>>
>> V7: Addressed review comments from Ander/Ville
>>   - No separate function for source scrambling, make it inline
>>   - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
>>   - Do not add platform check while setting source scrambling
>>   - Use pipe_config instead of crtc->config to set sink scrambling
>>   - To readout scrambling status, Compare with SCRAMBLING_MASK
>>     not any of its bits
>>   - Remove platform check in intel_pipe_config_compare while checking
>>     scrambling status
>>
>> V8: Fixed mege conflict, Addressed review comments from Ander
>>   - Remove the desciption/comment about scrambling fom the caller, move
>>     it to the function
>>   - Move the IS_GLK check into scrambling function
>>   - Fix alignment
>>
>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
>>   drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
>>   drivers/gpu/drm/i915/intel_display.c |  3 ++
>>   drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
>>   drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
>>   5 files changed, 101 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index cc843f9..2d50fdc 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -7830,7 +7830,14 @@ enum {
>>   #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>>   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
>> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>>   #define  TRANS_DDI_BFI_ENABLE		(1<<4)
>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
>> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
>> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
>> +					| TRANS_DDI_HDMI_SCRAMBLING)
>>   
>>   /* DisplayPort Transport Control */
>>   #define _DP_TP_CTL_A			0x64040
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 0467676..b3e4c4a 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
>>   			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>>   		else
>>   			temp |= TRANS_DDI_MODE_SELECT_DVI;
>> +
>> +		if (crtc_state->hdmi_scrambling)
>> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
>> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
>> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>>   	} else if (type == INTEL_OUTPUT_ANALOG) {
>>   		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>   		temp |= (crtc_state->fdi_lanes - 1) << 1;
>> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>>   		struct intel_digital_port *intel_dig_port =
>>   			enc_to_dig_port(encoder);
>>   
>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>> +			conn_state->connector, pipe_config, true);
>> +
>>   		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>>   		 * are ignored so nothing special needs to be done besides
>>   		 * enabling the port.
>> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>>   	if (old_crtc_state->has_audio)
>>   		intel_audio_codec_disable(intel_encoder);
>>   
>> +	if (type == INTEL_OUTPUT_HDMI) {
>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>> +			old_conn_state->connector, old_crtc_state, false);
>> +	}
>> +
>>   	if (type == INTEL_OUTPUT_EDP) {
>>   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>   
>> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>>   
>>   		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
>>   			pipe_config->has_infoframe = true;
>> +
>> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
>> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
>> +			pipe_config->hdmi_scrambling = true;
>> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>>   		/* fall through */
>>   	case TRANS_DDI_MODE_SELECT_DVI:
>>   		pipe_config->lane_count = 4;
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e77ca7d..fb5e670 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
>>   	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>   	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>   		PIPE_CONF_CHECK_I(limited_color_range);
>> +
>> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
>> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
>>   	PIPE_CONF_CHECK_I(has_infoframe);
>>   
>>   	PIPE_CONF_CHECK_I(has_audio);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 3af20c1..50d680c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -729,6 +729,12 @@ struct intel_crtc_state {
>>   
>>   	/* bitmask of visible planes (enum plane_id) */
>>   	u8 active_planes;
>> +
>> +	/* HDMI scrambling status */
>> +	bool hdmi_scrambling;
>> +
>> +	/* HDMI High TMDS char rate ratio */
>> +	bool hdmi_high_tmds_clock_ratio;
>>   };
>>   
>>   struct intel_crtc {
>> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   			       struct intel_crtc_state *pipe_config,
>>   			       struct drm_connector_state *conn_state);
>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>> +					struct drm_connector *connector,
>> +					struct intel_crtc_state *config,
>> +					bool enable);
>>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>   
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>> index c2184f7..4585aa8 100644
>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>> @@ -34,6 +34,7 @@
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_crtc.h>
>>   #include <drm/drm_edid.h>
>> +#include <drm/drm_scdc_helper.h>
>>   #include "intel_drv.h"
>>   #include <drm/i915_drm.h>
>>   #include <drm/intel_lpe_audio.h>
>> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>>   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>   	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
>>   	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
>>   	int clock_12bpc = clock_8bpc * 3 / 2;
>>   	int desired_bpp;
>> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>   
>>   	pipe_config->lane_count = 4;
>>   
>> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
>> +		if (scdc->scrambling.low_rates)
>> +			pipe_config->hdmi_scrambling = true;
>> +
>> +		if (pipe_config->port_clock > 340000) {
>> +			pipe_config->hdmi_scrambling = true;
>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>> +		}
>> +	}
>> +
>>   	return true;
>>   }
>>   
>> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>   }
>>   
>> +/*
>> + * This function handles scrambling on HDMI 2.0 capable sinks.
>> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
>> + * it enables scrambling. This should be called before enabling the HDMI
>> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
>> + * detect a scrambled clock within 100 ms.
>> + */
>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>> +				       struct drm_connector *connector,
>> +				       struct intel_crtc_state *config,
>> +				       bool enable)
>> +{
>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>> +	struct drm_scrambling *scrambling =
>> +				&connector->display_info.hdmi.scdc.scrambling;
>> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
>> +							  intel_hdmi->ddc_bus);
>> +	bool ret;
>> +
>> +	if (!scrambling->supported)
>> +		return;
>> +
>> +	if (!IS_GEMINILAKE(dev_priv))
>> +		return;
> I'd still like to see this getting done uncoditionally. That way we
> might get a bit of extra testing for the SCDC helper no matter what
> platform the user happens to own.
Sure, seems a good idea. I just added this hear to address the review 
comment from Ander.
Also, I can move this IS_GLK condition to compute_config, and once we 
check this in compute_config
we might never have check it again.
> I presume the spec says the sink will reset these to some default value
> on HPD or something like that? But I'm not sure trusting them to get
> this right is a good idea because reality often doesn't match the
> specs. So the safer option IMO would be always tell the sink exactly
> what we want.
Yep, You are correct about the spec too, that it says if there is no 
scrambled clock post hpd_out for
100ms, the sink should disable scrambling. But connected modeset 
scenarios need our attention, where
for example we are switching mode from, 4k@60 (scrambling reqd) -> 
1080@60 (scrambling not reqd).
For such cases too, we have to tell sink to disable scrambling.
>
>> +
>> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
>> +			intel_encoder->base.name, connector->name);
>> +
>> +	if (config->hdmi_high_tmds_clock_ratio) {
>> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
>> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
>> +		if (!ret) {
>> +			DRM_ERROR("Set TMDS ratio failed\n");
>> +			return;
>> +		}
>> +	}
>> +
>> +	if (config->hdmi_scrambling) {
>> +		/* Enable/disable sink scrambling */
>> +		ret = drm_scdc_set_scrambling(adptr, enable);
>> +		if (!ret) {
>> +			DRM_ERROR("Set sink scrambling failed\n");
>> +			return;
>> +		}
>> +	}
>> +
>> +	DRM_DEBUG_KMS("sink scrambling handled\n");
>> +}
>> +
>>   static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>>   			     enum port port)
>>   {
>> -- 
>> 1.9.1

Ville Syrjälä March 8, 2017, 5:44 p.m. UTC | #3

On Wed, Mar 08, 2017 at 07:26:45PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/8/2017 7:17 PM, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2017 at 07:07:48PM +0530, Shashank Sharma wrote:
> >>   Geminilake platform sports a native HDMI 2.0 controller, and is
> >>   capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> >>   mendates scrambling for these higher clocks, for reduced RF footprint.
> >>
> >>   This patch checks if the monitor supports scrambling, and if required,
> >>   enables it during the modeset.
> >>
> >>   V2: Addressed review comments from Ville:
> >>   - Do not track scrambling status in DRM layer, track somewhere in
> >>     driver like in intel_crtc_state.
> >>   - Don't talk to monitor at such a low layer, set monitor scrambling
> >>     in intel_enable_ddi() before enabling the port.
> >>
> >>   V3: Addressed review comments from Jani
> >>   - In comments, function names, use "sink" instead of "monitor",
> >>     so that the implementation could be close to the language of
> >>     HDMI spec.
> >>
> >> V4: Addressed review comment from Maarten
> >>   - scrambling -> hdmi_scrambling
> >>   - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> >>
> >> V5: Addressed review comments from Ville and Ander
> >>   - Do not modifiy the crtc_state after compute_config. Move all
> >>     scrambling and tmds_clock_ratio calcutations to compute_config.
> >>   - While setting scrambling for source/sink, do not check the
> >>     conditions again, just go by the crtc_state flags. This will
> >>     simplyfy the condition checks.
> >>
> >> V6: Addressed review comments from Ville
> >>   - Do not add IS_GLK check in disable/enable function, instead add it
> >>     in compute_config, while setting state flags.
> >>   - Remove unnecessary paranthesis.
> >>   - Simplyfy handle_sink_scrambling function as suggested.
> >>   - Add readout code for scrambling status in get_ddi_config and add a
> >>     check for the same in pipe_config_compare.
> >>
> >> V7: Addressed review comments from Ander/Ville
> >>   - No separate function for source scrambling, make it inline
> >>   - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
> >>   - Do not add platform check while setting source scrambling
> >>   - Use pipe_config instead of crtc->config to set sink scrambling
> >>   - To readout scrambling status, Compare with SCRAMBLING_MASK
> >>     not any of its bits
> >>   - Remove platform check in intel_pipe_config_compare while checking
> >>     scrambling status
> >>
> >> V8: Fixed mege conflict, Addressed review comments from Ander
> >>   - Remove the desciption/comment about scrambling fom the caller, move
> >>     it to the function
> >>   - Move the IS_GLK check into scrambling function
> >>   - Fix alignment
> >>
> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
> >>   drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
> >>   drivers/gpu/drm/i915/intel_display.c |  3 ++
> >>   drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
> >>   drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
> >>   5 files changed, 101 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index cc843f9..2d50fdc 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -7830,7 +7830,14 @@ enum {
> >>   #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
> >>   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> >>   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> >> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> >> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> >>   #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> >> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> >> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> >> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
> >> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
> >> +					| TRANS_DDI_HDMI_SCRAMBLING)
> >>   
> >>   /* DisplayPort Transport Control */
> >>   #define _DP_TP_CTL_A			0x64040
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index 0467676..b3e4c4a 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
> >>   			temp |= TRANS_DDI_MODE_SELECT_HDMI;
> >>   		else
> >>   			temp |= TRANS_DDI_MODE_SELECT_DVI;
> >> +
> >> +		if (crtc_state->hdmi_scrambling)
> >> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> >> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
> >> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> >>   	} else if (type == INTEL_OUTPUT_ANALOG) {
> >>   		temp |= TRANS_DDI_MODE_SELECT_FDI;
> >>   		temp |= (crtc_state->fdi_lanes - 1) << 1;
> >> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> >>   		struct intel_digital_port *intel_dig_port =
> >>   			enc_to_dig_port(encoder);
> >>   
> >> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> >> +			conn_state->connector, pipe_config, true);
> >> +
> >>   		/* In HDMI/DVI mode, the port width, and swing/emphasis values
> >>   		 * are ignored so nothing special needs to be done besides
> >>   		 * enabling the port.
> >> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> >>   	if (old_crtc_state->has_audio)
> >>   		intel_audio_codec_disable(intel_encoder);
> >>   
> >> +	if (type == INTEL_OUTPUT_HDMI) {
> >> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> >> +			old_conn_state->connector, old_crtc_state, false);
> >> +	}
> >> +
> >>   	if (type == INTEL_OUTPUT_EDP) {
> >>   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>   
> >> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >>   
> >>   		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
> >>   			pipe_config->has_infoframe = true;
> >> +
> >> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> >> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
> >> +			pipe_config->hdmi_scrambling = true;
> >> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
> >> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> >>   		/* fall through */
> >>   	case TRANS_DDI_MODE_SELECT_DVI:
> >>   		pipe_config->lane_count = 4;
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index e77ca7d..fb5e670 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
> >>   	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> >>   	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >>   		PIPE_CONF_CHECK_I(limited_color_range);
> >> +
> >> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
> >> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
> >>   	PIPE_CONF_CHECK_I(has_infoframe);
> >>   
> >>   	PIPE_CONF_CHECK_I(has_audio);
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 3af20c1..50d680c 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -729,6 +729,12 @@ struct intel_crtc_state {
> >>   
> >>   	/* bitmask of visible planes (enum plane_id) */
> >>   	u8 active_planes;
> >> +
> >> +	/* HDMI scrambling status */
> >> +	bool hdmi_scrambling;
> >> +
> >> +	/* HDMI High TMDS char rate ratio */
> >> +	bool hdmi_high_tmds_clock_ratio;
> >>   };
> >>   
> >>   struct intel_crtc {
> >> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >>   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>   			       struct intel_crtc_state *pipe_config,
> >>   			       struct drm_connector_state *conn_state);
> >> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> >> +					struct drm_connector *connector,
> >> +					struct intel_crtc_state *config,
> >> +					bool enable);
> >>   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> >>   
> >>   
> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >> index c2184f7..4585aa8 100644
> >> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >> @@ -34,6 +34,7 @@
> >>   #include <drm/drm_atomic_helper.h>
> >>   #include <drm/drm_crtc.h>
> >>   #include <drm/drm_edid.h>
> >> +#include <drm/drm_scdc_helper.h>
> >>   #include "intel_drv.h"
> >>   #include <drm/i915_drm.h>
> >>   #include <drm/intel_lpe_audio.h>
> >> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>   	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> >>   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>   	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
> >>   	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> >>   	int clock_12bpc = clock_8bpc * 3 / 2;
> >>   	int desired_bpp;
> >> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>   
> >>   	pipe_config->lane_count = 4;
> >>   
> >> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
> >> +		if (scdc->scrambling.low_rates)
> >> +			pipe_config->hdmi_scrambling = true;
> >> +
> >> +		if (pipe_config->port_clock > 340000) {
> >> +			pipe_config->hdmi_scrambling = true;
> >> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> >> +		}
> >> +	}
> >> +
> >>   	return true;
> >>   }
> >>   
> >> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> >>   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>   }
> >>   
> >> +/*
> >> + * This function handles scrambling on HDMI 2.0 capable sinks.
> >> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
> >> + * it enables scrambling. This should be called before enabling the HDMI
> >> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
> >> + * detect a scrambled clock within 100 ms.
> >> + */
> >> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> >> +				       struct drm_connector *connector,
> >> +				       struct intel_crtc_state *config,
> >> +				       bool enable)
> >> +{
> >> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> >> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> >> +	struct drm_scrambling *scrambling =
> >> +				&connector->display_info.hdmi.scdc.scrambling;
> >> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> >> +							  intel_hdmi->ddc_bus);
> >> +	bool ret;
> >> +
> >> +	if (!scrambling->supported)
> >> +		return;
> >> +
> >> +	if (!IS_GEMINILAKE(dev_priv))
> >> +		return;
> > I'd still like to see this getting done uncoditionally. That way we
> > might get a bit of extra testing for the SCDC helper no matter what
> > platform the user happens to own.
> Sure, seems a good idea. I just added this hear to address the review 
> comment from Ander.
> Also, I can move this IS_GLK condition to compute_config, and once we 
> check this in compute_config
> we might never have check it again.

I thought you already had the check in there? Hmm. Looks that way to me.

> > I presume the spec says the sink will reset these to some default value
> > on HPD or something like that? But I'm not sure trusting them to get
> > this right is a good idea because reality often doesn't match the
> > specs. So the safer option IMO would be always tell the sink exactly
> > what we want.
> Yep, You are correct about the spec too, that it says if there is no 
> scrambled clock post hpd_out for
> 100ms, the sink should disable scrambling. But connected modeset 
> scenarios need our attention, where
> for example we are switching mode from, 4k@60 (scrambling reqd) -> 
> 1080@60 (scrambling not reqd).
> For such cases too, we have to tell sink to disable scrambling.
> >
> >> +
> >> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> >> +			intel_encoder->base.name, connector->name);
> >> +
> >> +	if (config->hdmi_high_tmds_clock_ratio) {
> >> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
> >> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
> >> +		if (!ret) {
> >> +			DRM_ERROR("Set TMDS ratio failed\n");
> >> +			return;
> >> +		}
> >> +	}
> >> +
> >> +	if (config->hdmi_scrambling) {
> >> +		/* Enable/disable sink scrambling */
> >> +		ret = drm_scdc_set_scrambling(adptr, enable);
> >> +		if (!ret) {
> >> +			DRM_ERROR("Set sink scrambling failed\n");
> >> +			return;
> >> +		}
> >> +	}
> >> +
> >> +	DRM_DEBUG_KMS("sink scrambling handled\n");
> >> +}
> >> +
> >>   static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> >>   			     enum port port)
> >>   {
> >> -- 
> >> 1.9.1

Sharma, Shashank March 8, 2017, 5:50 p.m. UTC | #4

Regards

Shashank


On 3/8/2017 7:44 PM, Ville Syrjälä wrote:
> On Wed, Mar 08, 2017 at 07:26:45PM +0200, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 3/8/2017 7:17 PM, Ville Syrjälä wrote:
>>> On Wed, Mar 08, 2017 at 07:07:48PM +0530, Shashank Sharma wrote:
>>>>    Geminilake platform sports a native HDMI 2.0 controller, and is
>>>>    capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>>>>    mendates scrambling for these higher clocks, for reduced RF footprint.
>>>>
>>>>    This patch checks if the monitor supports scrambling, and if required,
>>>>    enables it during the modeset.
>>>>
>>>>    V2: Addressed review comments from Ville:
>>>>    - Do not track scrambling status in DRM layer, track somewhere in
>>>>      driver like in intel_crtc_state.
>>>>    - Don't talk to monitor at such a low layer, set monitor scrambling
>>>>      in intel_enable_ddi() before enabling the port.
>>>>
>>>>    V3: Addressed review comments from Jani
>>>>    - In comments, function names, use "sink" instead of "monitor",
>>>>      so that the implementation could be close to the language of
>>>>      HDMI spec.
>>>>
>>>> V4: Addressed review comment from Maarten
>>>>    - scrambling -> hdmi_scrambling
>>>>    - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
>>>>
>>>> V5: Addressed review comments from Ville and Ander
>>>>    - Do not modifiy the crtc_state after compute_config. Move all
>>>>      scrambling and tmds_clock_ratio calcutations to compute_config.
>>>>    - While setting scrambling for source/sink, do not check the
>>>>      conditions again, just go by the crtc_state flags. This will
>>>>      simplyfy the condition checks.
>>>>
>>>> V6: Addressed review comments from Ville
>>>>    - Do not add IS_GLK check in disable/enable function, instead add it
>>>>      in compute_config, while setting state flags.
>>>>    - Remove unnecessary paranthesis.
>>>>    - Simplyfy handle_sink_scrambling function as suggested.
>>>>    - Add readout code for scrambling status in get_ddi_config and add a
>>>>      check for the same in pipe_config_compare.
>>>>
>>>> V7: Addressed review comments from Ander/Ville
>>>>    - No separate function for source scrambling, make it inline
>>>>    - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
>>>>    - Do not add platform check while setting source scrambling
>>>>    - Use pipe_config instead of crtc->config to set sink scrambling
>>>>    - To readout scrambling status, Compare with SCRAMBLING_MASK
>>>>      not any of its bits
>>>>    - Remove platform check in intel_pipe_config_compare while checking
>>>>      scrambling status
>>>>
>>>> V8: Fixed mege conflict, Addressed review comments from Ander
>>>>    - Remove the desciption/comment about scrambling fom the caller, move
>>>>      it to the function
>>>>    - Move the IS_GLK check into scrambling function
>>>>    - Fix alignment
>>>>
>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
>>>>    drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
>>>>    drivers/gpu/drm/i915/intel_display.c |  3 ++
>>>>    drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
>>>>    drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
>>>>    5 files changed, 101 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index cc843f9..2d50fdc 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -7830,7 +7830,14 @@ enum {
>>>>    #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>>>>    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>>>    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>>>>    #define  TRANS_DDI_BFI_ENABLE		(1<<4)
>>>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
>>>> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
>>>> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
>>>> +					| TRANS_DDI_HDMI_SCRAMBLING)
>>>>    
>>>>    /* DisplayPort Transport Control */
>>>>    #define _DP_TP_CTL_A			0x64040
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 0467676..b3e4c4a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
>>>>    			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>>>>    		else
>>>>    			temp |= TRANS_DDI_MODE_SELECT_DVI;
>>>> +
>>>> +		if (crtc_state->hdmi_scrambling)
>>>> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
>>>> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
>>>> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>>>>    	} else if (type == INTEL_OUTPUT_ANALOG) {
>>>>    		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>>>    		temp |= (crtc_state->fdi_lanes - 1) << 1;
>>>> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>>>>    		struct intel_digital_port *intel_dig_port =
>>>>    			enc_to_dig_port(encoder);
>>>>    
>>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>>>> +			conn_state->connector, pipe_config, true);
>>>> +
>>>>    		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>>>>    		 * are ignored so nothing special needs to be done besides
>>>>    		 * enabling the port.
>>>> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>>>>    	if (old_crtc_state->has_audio)
>>>>    		intel_audio_codec_disable(intel_encoder);
>>>>    
>>>> +	if (type == INTEL_OUTPUT_HDMI) {
>>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>>>> +			old_conn_state->connector, old_crtc_state, false);
>>>> +	}
>>>> +
>>>>    	if (type == INTEL_OUTPUT_EDP) {
>>>>    		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>>    
>>>> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>>>>    
>>>>    		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
>>>>    			pipe_config->has_infoframe = true;
>>>> +
>>>> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
>>>> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
>>>> +			pipe_config->hdmi_scrambling = true;
>>>> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
>>>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>>>>    		/* fall through */
>>>>    	case TRANS_DDI_MODE_SELECT_DVI:
>>>>    		pipe_config->lane_count = 4;
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index e77ca7d..fb5e670 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
>>>>    	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>>>    	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>>>    		PIPE_CONF_CHECK_I(limited_color_range);
>>>> +
>>>> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
>>>> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
>>>>    	PIPE_CONF_CHECK_I(has_infoframe);
>>>>    
>>>>    	PIPE_CONF_CHECK_I(has_audio);
>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>> index 3af20c1..50d680c 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -729,6 +729,12 @@ struct intel_crtc_state {
>>>>    
>>>>    	/* bitmask of visible planes (enum plane_id) */
>>>>    	u8 active_planes;
>>>> +
>>>> +	/* HDMI scrambling status */
>>>> +	bool hdmi_scrambling;
>>>> +
>>>> +	/* HDMI High TMDS char rate ratio */
>>>> +	bool hdmi_high_tmds_clock_ratio;
>>>>    };
>>>>    
>>>>    struct intel_crtc {
>>>> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>    			       struct intel_crtc_state *pipe_config,
>>>>    			       struct drm_connector_state *conn_state);
>>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>>>> +					struct drm_connector *connector,
>>>> +					struct intel_crtc_state *config,
>>>> +					bool enable);
>>>>    void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>>>    
>>>>    
>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> index c2184f7..4585aa8 100644
>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>> @@ -34,6 +34,7 @@
>>>>    #include <drm/drm_atomic_helper.h>
>>>>    #include <drm/drm_crtc.h>
>>>>    #include <drm/drm_edid.h>
>>>> +#include <drm/drm_scdc_helper.h>
>>>>    #include "intel_drv.h"
>>>>    #include <drm/i915_drm.h>
>>>>    #include <drm/intel_lpe_audio.h>
>>>> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>    	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>>>>    	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>    	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
>>>>    	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
>>>>    	int clock_12bpc = clock_8bpc * 3 / 2;
>>>>    	int desired_bpp;
>>>> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>    
>>>>    	pipe_config->lane_count = 4;
>>>>    
>>>> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
>>>> +		if (scdc->scrambling.low_rates)
>>>> +			pipe_config->hdmi_scrambling = true;
>>>> +
>>>> +		if (pipe_config->port_clock > 340000) {
>>>> +			pipe_config->hdmi_scrambling = true;
>>>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>>>> +		}
>>>> +	}
>>>> +
>>>>    	return true;
>>>>    }
>>>>    
>>>> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>>>    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>    }
>>>>    
>>>> +/*
>>>> + * This function handles scrambling on HDMI 2.0 capable sinks.
>>>> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
>>>> + * it enables scrambling. This should be called before enabling the HDMI
>>>> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
>>>> + * detect a scrambled clock within 100 ms.
>>>> + */
>>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>>>> +				       struct drm_connector *connector,
>>>> +				       struct intel_crtc_state *config,
>>>> +				       bool enable)
>>>> +{
>>>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>>>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>>>> +	struct drm_scrambling *scrambling =
>>>> +				&connector->display_info.hdmi.scdc.scrambling;
>>>> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
>>>> +							  intel_hdmi->ddc_bus);
>>>> +	bool ret;
>>>> +
>>>> +	if (!scrambling->supported)
>>>> +		return;
>>>> +
>>>> +	if (!IS_GEMINILAKE(dev_priv))
>>>> +		return;
>>> I'd still like to see this getting done uncoditionally. That way we
>>> might get a bit of extra testing for the SCDC helper no matter what
>>> platform the user happens to own.
>> Sure, seems a good idea. I just added this hear to address the review
>> comment from Ander.
>> Also, I can move this IS_GLK condition to compute_config, and once we
>> check this in compute_config
>> we might never have check it again.
> I thought you already had the check in there? Hmm. Looks that way to me.
Yep, I already have that (this shows I have too many versions of code in 
my mind too :P),
what I meant was, we can use some of those state checks here before 
enabling/disabling sink side scrambling.
But then I realized it won't be the unconditional stuff you actually 
want, so please rub my last comment off :-)

- Shashank
>>> I presume the spec says the sink will reset these to some default value
>>> on HPD or something like that? But I'm not sure trusting them to get
>>> this right is a good idea because reality often doesn't match the
>>> specs. So the safer option IMO would be always tell the sink exactly
>>> what we want.
>> Yep, You are correct about the spec too, that it says if there is no
>> scrambled clock post hpd_out for
>> 100ms, the sink should disable scrambling. But connected modeset
>> scenarios need our attention, where
>> for example we are switching mode from, 4k@60 (scrambling reqd) ->
>> 1080@60 (scrambling not reqd).
>> For such cases too, we have to tell sink to disable scrambling.
>>>> +
>>>> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
>>>> +			intel_encoder->base.name, connector->name);
>>>> +
>>>> +	if (config->hdmi_high_tmds_clock_ratio) {
>>>> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
>>>> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
>>>> +		if (!ret) {
>>>> +			DRM_ERROR("Set TMDS ratio failed\n");
>>>> +			return;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (config->hdmi_scrambling) {
>>>> +		/* Enable/disable sink scrambling */
>>>> +		ret = drm_scdc_set_scrambling(adptr, enable);
>>>> +		if (!ret) {
>>>> +			DRM_ERROR("Set sink scrambling failed\n");
>>>> +			return;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	DRM_DEBUG_KMS("sink scrambling handled\n");
>>>> +}
>>>> +
>>>>    static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>>>>    			     enum port port)
>>>>    {
>>>> -- 
>>>> 1.9.1

Ville Syrjälä March 8, 2017, 5:56 p.m. UTC | #5

On Wed, Mar 08, 2017 at 07:50:45PM +0200, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/8/2017 7:44 PM, Ville Syrjälä wrote:
> > On Wed, Mar 08, 2017 at 07:26:45PM +0200, Sharma, Shashank wrote:
> >> Regards
> >>
> >> Shashank
> >>
> >>
> >> On 3/8/2017 7:17 PM, Ville Syrjälä wrote:
> >>> On Wed, Mar 08, 2017 at 07:07:48PM +0530, Shashank Sharma wrote:
> >>>>    Geminilake platform sports a native HDMI 2.0 controller, and is
> >>>>    capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> >>>>    mendates scrambling for these higher clocks, for reduced RF footprint.
> >>>>
> >>>>    This patch checks if the monitor supports scrambling, and if required,
> >>>>    enables it during the modeset.
> >>>>
> >>>>    V2: Addressed review comments from Ville:
> >>>>    - Do not track scrambling status in DRM layer, track somewhere in
> >>>>      driver like in intel_crtc_state.
> >>>>    - Don't talk to monitor at such a low layer, set monitor scrambling
> >>>>      in intel_enable_ddi() before enabling the port.
> >>>>
> >>>>    V3: Addressed review comments from Jani
> >>>>    - In comments, function names, use "sink" instead of "monitor",
> >>>>      so that the implementation could be close to the language of
> >>>>      HDMI spec.
> >>>>
> >>>> V4: Addressed review comment from Maarten
> >>>>    - scrambling -> hdmi_scrambling
> >>>>    - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> >>>>
> >>>> V5: Addressed review comments from Ville and Ander
> >>>>    - Do not modifiy the crtc_state after compute_config. Move all
> >>>>      scrambling and tmds_clock_ratio calcutations to compute_config.
> >>>>    - While setting scrambling for source/sink, do not check the
> >>>>      conditions again, just go by the crtc_state flags. This will
> >>>>      simplyfy the condition checks.
> >>>>
> >>>> V6: Addressed review comments from Ville
> >>>>    - Do not add IS_GLK check in disable/enable function, instead add it
> >>>>      in compute_config, while setting state flags.
> >>>>    - Remove unnecessary paranthesis.
> >>>>    - Simplyfy handle_sink_scrambling function as suggested.
> >>>>    - Add readout code for scrambling status in get_ddi_config and add a
> >>>>      check for the same in pipe_config_compare.
> >>>>
> >>>> V7: Addressed review comments from Ander/Ville
> >>>>    - No separate function for source scrambling, make it inline
> >>>>    - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
> >>>>    - Do not add platform check while setting source scrambling
> >>>>    - Use pipe_config instead of crtc->config to set sink scrambling
> >>>>    - To readout scrambling status, Compare with SCRAMBLING_MASK
> >>>>      not any of its bits
> >>>>    - Remove platform check in intel_pipe_config_compare while checking
> >>>>      scrambling status
> >>>>
> >>>> V8: Fixed mege conflict, Addressed review comments from Ander
> >>>>    - Remove the desciption/comment about scrambling fom the caller, move
> >>>>      it to the function
> >>>>    - Move the IS_GLK check into scrambling function
> >>>>    - Fix alignment
> >>>>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> >>>> ---
> >>>>    drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
> >>>>    drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
> >>>>    drivers/gpu/drm/i915/intel_display.c |  3 ++
> >>>>    drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
> >>>>    drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
> >>>>    5 files changed, 101 insertions(+)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >>>> index cc843f9..2d50fdc 100644
> >>>> --- a/drivers/gpu/drm/i915/i915_reg.h
> >>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >>>> @@ -7830,7 +7830,14 @@ enum {
> >>>>    #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
> >>>>    #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> >>>>    #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> >>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> >>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> >>>>    #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> >>>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> >>>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> >>>> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
> >>>> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
> >>>> +					| TRANS_DDI_HDMI_SCRAMBLING)
> >>>>    
> >>>>    /* DisplayPort Transport Control */
> >>>>    #define _DP_TP_CTL_A			0x64040
> >>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> index 0467676..b3e4c4a 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >>>> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
> >>>>    			temp |= TRANS_DDI_MODE_SELECT_HDMI;
> >>>>    		else
> >>>>    			temp |= TRANS_DDI_MODE_SELECT_DVI;
> >>>> +
> >>>> +		if (crtc_state->hdmi_scrambling)
> >>>> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> >>>> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
> >>>> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> >>>>    	} else if (type == INTEL_OUTPUT_ANALOG) {
> >>>>    		temp |= TRANS_DDI_MODE_SELECT_FDI;
> >>>>    		temp |= (crtc_state->fdi_lanes - 1) << 1;
> >>>> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> >>>>    		struct intel_digital_port *intel_dig_port =
> >>>>    			enc_to_dig_port(encoder);
> >>>>    
> >>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> >>>> +			conn_state->connector, pipe_config, true);
> >>>> +
> >>>>    		/* In HDMI/DVI mode, the port width, and swing/emphasis values
> >>>>    		 * are ignored so nothing special needs to be done besides
> >>>>    		 * enabling the port.
> >>>> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> >>>>    	if (old_crtc_state->has_audio)
> >>>>    		intel_audio_codec_disable(intel_encoder);
> >>>>    
> >>>> +	if (type == INTEL_OUTPUT_HDMI) {
> >>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> >>>> +			old_conn_state->connector, old_crtc_state, false);
> >>>> +	}
> >>>> +
> >>>>    	if (type == INTEL_OUTPUT_EDP) {
> >>>>    		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> >>>>    
> >>>> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
> >>>>    
> >>>>    		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
> >>>>    			pipe_config->has_infoframe = true;
> >>>> +
> >>>> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> >>>> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
> >>>> +			pipe_config->hdmi_scrambling = true;
> >>>> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
> >>>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> >>>>    		/* fall through */
> >>>>    	case TRANS_DDI_MODE_SELECT_DVI:
> >>>>    		pipe_config->lane_count = 4;
> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >>>> index e77ca7d..fb5e670 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>>> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
> >>>>    	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
> >>>>    	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> >>>>    		PIPE_CONF_CHECK_I(limited_color_range);
> >>>> +
> >>>> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
> >>>> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
> >>>>    	PIPE_CONF_CHECK_I(has_infoframe);
> >>>>    
> >>>>    	PIPE_CONF_CHECK_I(has_audio);
> >>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >>>> index 3af20c1..50d680c 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_drv.h
> >>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >>>> @@ -729,6 +729,12 @@ struct intel_crtc_state {
> >>>>    
> >>>>    	/* bitmask of visible planes (enum plane_id) */
> >>>>    	u8 active_planes;
> >>>> +
> >>>> +	/* HDMI scrambling status */
> >>>> +	bool hdmi_scrambling;
> >>>> +
> >>>> +	/* HDMI High TMDS char rate ratio */
> >>>> +	bool hdmi_high_tmds_clock_ratio;
> >>>>    };
> >>>>    
> >>>>    struct intel_crtc {
> >>>> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> >>>>    bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>>>    			       struct intel_crtc_state *pipe_config,
> >>>>    			       struct drm_connector_state *conn_state);
> >>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> >>>> +					struct drm_connector *connector,
> >>>> +					struct intel_crtc_state *config,
> >>>> +					bool enable);
> >>>>    void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> >>>>    
> >>>>    
> >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> index c2184f7..4585aa8 100644
> >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> >>>> @@ -34,6 +34,7 @@
> >>>>    #include <drm/drm_atomic_helper.h>
> >>>>    #include <drm/drm_crtc.h>
> >>>>    #include <drm/drm_edid.h>
> >>>> +#include <drm/drm_scdc_helper.h>
> >>>>    #include "intel_drv.h"
> >>>>    #include <drm/i915_drm.h>
> >>>>    #include <drm/intel_lpe_audio.h>
> >>>> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>>>    	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> >>>>    	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> >>>>    	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >>>> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
> >>>>    	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> >>>>    	int clock_12bpc = clock_8bpc * 3 / 2;
> >>>>    	int desired_bpp;
> >>>> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >>>>    
> >>>>    	pipe_config->lane_count = 4;
> >>>>    
> >>>> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
> >>>> +		if (scdc->scrambling.low_rates)
> >>>> +			pipe_config->hdmi_scrambling = true;
> >>>> +
> >>>> +		if (pipe_config->port_clock > 340000) {
> >>>> +			pipe_config->hdmi_scrambling = true;
> >>>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>>    	return true;
> >>>>    }
> >>>>    
> >>>> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> >>>>    	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >>>>    }
> >>>>    
> >>>> +/*
> >>>> + * This function handles scrambling on HDMI 2.0 capable sinks.
> >>>> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
> >>>> + * it enables scrambling. This should be called before enabling the HDMI
> >>>> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
> >>>> + * detect a scrambled clock within 100 ms.
> >>>> + */
> >>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> >>>> +				       struct drm_connector *connector,
> >>>> +				       struct intel_crtc_state *config,
> >>>> +				       bool enable)
> >>>> +{
> >>>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> >>>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> >>>> +	struct drm_scrambling *scrambling =
> >>>> +				&connector->display_info.hdmi.scdc.scrambling;
> >>>> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> >>>> +							  intel_hdmi->ddc_bus);
> >>>> +	bool ret;
> >>>> +
> >>>> +	if (!scrambling->supported)
> >>>> +		return;
> >>>> +
> >>>> +	if (!IS_GEMINILAKE(dev_priv))
> >>>> +		return;
> >>> I'd still like to see this getting done uncoditionally. That way we
> >>> might get a bit of extra testing for the SCDC helper no matter what
> >>> platform the user happens to own.
> >> Sure, seems a good idea. I just added this hear to address the review
> >> comment from Ander.
> >> Also, I can move this IS_GLK condition to compute_config, and once we
> >> check this in compute_config
> >> we might never have check it again.
> > I thought you already had the check in there? Hmm. Looks that way to me.
> Yep, I already have that (this shows I have too many versions of code in 
> my mind too :P),
> what I meant was, we can use some of those state checks here before 
> enabling/disabling sink side scrambling.
> But then I realized it won't be the unconditional stuff you actually 
> want, so please rub my last comment off :-)

Well I think it should work just fine if you simply pass both into this
function as bools, and then in the disable path we pass explicit 'false'
for both, and in the enable path we just pass config->whatever.

> 
> - Shashank
> >>> I presume the spec says the sink will reset these to some default value
> >>> on HPD or something like that? But I'm not sure trusting them to get
> >>> this right is a good idea because reality often doesn't match the
> >>> specs. So the safer option IMO would be always tell the sink exactly
> >>> what we want.
> >> Yep, You are correct about the spec too, that it says if there is no
> >> scrambled clock post hpd_out for
> >> 100ms, the sink should disable scrambling. But connected modeset
> >> scenarios need our attention, where
> >> for example we are switching mode from, 4k@60 (scrambling reqd) ->
> >> 1080@60 (scrambling not reqd).
> >> For such cases too, we have to tell sink to disable scrambling.
> >>>> +
> >>>> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> >>>> +			intel_encoder->base.name, connector->name);
> >>>> +
> >>>> +	if (config->hdmi_high_tmds_clock_ratio) {
> >>>> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
> >>>> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
> >>>> +		if (!ret) {
> >>>> +			DRM_ERROR("Set TMDS ratio failed\n");
> >>>> +			return;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	if (config->hdmi_scrambling) {
> >>>> +		/* Enable/disable sink scrambling */
> >>>> +		ret = drm_scdc_set_scrambling(adptr, enable);
> >>>> +		if (!ret) {
> >>>> +			DRM_ERROR("Set sink scrambling failed\n");
> >>>> +			return;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	DRM_DEBUG_KMS("sink scrambling handled\n");
> >>>> +}
> >>>> +
> >>>>    static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> >>>>    			     enum port port)
> >>>>    {
> >>>> -- 
> >>>> 1.9.1

Sharma, Shashank March 8, 2017, 6 p.m. UTC | #6

Regards

Shashank


On 3/8/2017 7:56 PM, Ville Syrjälä wrote:
> On Wed, Mar 08, 2017 at 07:50:45PM +0200, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 3/8/2017 7:44 PM, Ville Syrjälä wrote:
>>> On Wed, Mar 08, 2017 at 07:26:45PM +0200, Sharma, Shashank wrote:
>>>> Regards
>>>>
>>>> Shashank
>>>>
>>>>
>>>> On 3/8/2017 7:17 PM, Ville Syrjälä wrote:
>>>>> On Wed, Mar 08, 2017 at 07:07:48PM +0530, Shashank Sharma wrote:
>>>>>>     Geminilake platform sports a native HDMI 2.0 controller, and is
>>>>>>     capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>>>>>>     mendates scrambling for these higher clocks, for reduced RF footprint.
>>>>>>
>>>>>>     This patch checks if the monitor supports scrambling, and if required,
>>>>>>     enables it during the modeset.
>>>>>>
>>>>>>     V2: Addressed review comments from Ville:
>>>>>>     - Do not track scrambling status in DRM layer, track somewhere in
>>>>>>       driver like in intel_crtc_state.
>>>>>>     - Don't talk to monitor at such a low layer, set monitor scrambling
>>>>>>       in intel_enable_ddi() before enabling the port.
>>>>>>
>>>>>>     V3: Addressed review comments from Jani
>>>>>>     - In comments, function names, use "sink" instead of "monitor",
>>>>>>       so that the implementation could be close to the language of
>>>>>>       HDMI spec.
>>>>>>
>>>>>> V4: Addressed review comment from Maarten
>>>>>>     - scrambling -> hdmi_scrambling
>>>>>>     - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
>>>>>>
>>>>>> V5: Addressed review comments from Ville and Ander
>>>>>>     - Do not modifiy the crtc_state after compute_config. Move all
>>>>>>       scrambling and tmds_clock_ratio calcutations to compute_config.
>>>>>>     - While setting scrambling for source/sink, do not check the
>>>>>>       conditions again, just go by the crtc_state flags. This will
>>>>>>       simplyfy the condition checks.
>>>>>>
>>>>>> V6: Addressed review comments from Ville
>>>>>>     - Do not add IS_GLK check in disable/enable function, instead add it
>>>>>>       in compute_config, while setting state flags.
>>>>>>     - Remove unnecessary paranthesis.
>>>>>>     - Simplyfy handle_sink_scrambling function as suggested.
>>>>>>     - Add readout code for scrambling status in get_ddi_config and add a
>>>>>>       check for the same in pipe_config_compare.
>>>>>>
>>>>>> V7: Addressed review comments from Ander/Ville
>>>>>>     - No separate function for source scrambling, make it inline
>>>>>>     - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
>>>>>>     - Do not add platform check while setting source scrambling
>>>>>>     - Use pipe_config instead of crtc->config to set sink scrambling
>>>>>>     - To readout scrambling status, Compare with SCRAMBLING_MASK
>>>>>>       not any of its bits
>>>>>>     - Remove platform check in intel_pipe_config_compare while checking
>>>>>>       scrambling status
>>>>>>
>>>>>> V8: Fixed mege conflict, Addressed review comments from Ander
>>>>>>     - Remove the desciption/comment about scrambling fom the caller, move
>>>>>>       it to the function
>>>>>>     - Move the IS_GLK check into scrambling function
>>>>>>     - Fix alignment
>>>>>>
>>>>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>>>>> ---
>>>>>>     drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
>>>>>>     drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
>>>>>>     drivers/gpu/drm/i915/intel_display.c |  3 ++
>>>>>>     drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
>>>>>>     drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
>>>>>>     5 files changed, 101 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>>>> index cc843f9..2d50fdc 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>>>> @@ -7830,7 +7830,14 @@ enum {
>>>>>>     #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>>>>>>     #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>>>>>>     #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
>>>>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
>>>>>> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>>>>>>     #define  TRANS_DDI_BFI_ENABLE		(1<<4)
>>>>>> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
>>>>>> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
>>>>>> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
>>>>>> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
>>>>>> +					| TRANS_DDI_HDMI_SCRAMBLING)
>>>>>>     
>>>>>>     /* DisplayPort Transport Control */
>>>>>>     #define _DP_TP_CTL_A			0x64040
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> index 0467676..b3e4c4a 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>>>> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
>>>>>>     			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>>>>>>     		else
>>>>>>     			temp |= TRANS_DDI_MODE_SELECT_DVI;
>>>>>> +
>>>>>> +		if (crtc_state->hdmi_scrambling)
>>>>>> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
>>>>>> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
>>>>>> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>>>>>>     	} else if (type == INTEL_OUTPUT_ANALOG) {
>>>>>>     		temp |= TRANS_DDI_MODE_SELECT_FDI;
>>>>>>     		temp |= (crtc_state->fdi_lanes - 1) << 1;
>>>>>> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>>>>>>     		struct intel_digital_port *intel_dig_port =
>>>>>>     			enc_to_dig_port(encoder);
>>>>>>     
>>>>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>>>>>> +			conn_state->connector, pipe_config, true);
>>>>>> +
>>>>>>     		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>>>>>>     		 * are ignored so nothing special needs to be done besides
>>>>>>     		 * enabling the port.
>>>>>> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>>>>>>     	if (old_crtc_state->has_audio)
>>>>>>     		intel_audio_codec_disable(intel_encoder);
>>>>>>     
>>>>>> +	if (type == INTEL_OUTPUT_HDMI) {
>>>>>> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
>>>>>> +			old_conn_state->connector, old_crtc_state, false);
>>>>>> +	}
>>>>>> +
>>>>>>     	if (type == INTEL_OUTPUT_EDP) {
>>>>>>     		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>>>>>>     
>>>>>> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>>>>>>     
>>>>>>     		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
>>>>>>     			pipe_config->has_infoframe = true;
>>>>>> +
>>>>>> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
>>>>>> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
>>>>>> +			pipe_config->hdmi_scrambling = true;
>>>>>> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
>>>>>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>>>>>>     		/* fall through */
>>>>>>     	case TRANS_DDI_MODE_SELECT_DVI:
>>>>>>     		pipe_config->lane_count = 4;
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>>>> index e77ca7d..fb5e670 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>>>> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
>>>>>>     	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>>>>>>     	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>>>>>>     		PIPE_CONF_CHECK_I(limited_color_range);
>>>>>> +
>>>>>> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
>>>>>> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
>>>>>>     	PIPE_CONF_CHECK_I(has_infoframe);
>>>>>>     
>>>>>>     	PIPE_CONF_CHECK_I(has_audio);
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>>>>> index 3af20c1..50d680c 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>>>> @@ -729,6 +729,12 @@ struct intel_crtc_state {
>>>>>>     
>>>>>>     	/* bitmask of visible planes (enum plane_id) */
>>>>>>     	u8 active_planes;
>>>>>> +
>>>>>> +	/* HDMI scrambling status */
>>>>>> +	bool hdmi_scrambling;
>>>>>> +
>>>>>> +	/* HDMI High TMDS char rate ratio */
>>>>>> +	bool hdmi_high_tmds_clock_ratio;
>>>>>>     };
>>>>>>     
>>>>>>     struct intel_crtc {
>>>>>> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>>>>>>     bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>>>     			       struct intel_crtc_state *pipe_config,
>>>>>>     			       struct drm_connector_state *conn_state);
>>>>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>>>>>> +					struct drm_connector *connector,
>>>>>> +					struct intel_crtc_state *config,
>>>>>> +					bool enable);
>>>>>>     void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>>>>>>     
>>>>>>     
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>>> index c2184f7..4585aa8 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>>>>> @@ -34,6 +34,7 @@
>>>>>>     #include <drm/drm_atomic_helper.h>
>>>>>>     #include <drm/drm_crtc.h>
>>>>>>     #include <drm/drm_edid.h>
>>>>>> +#include <drm/drm_scdc_helper.h>
>>>>>>     #include "intel_drv.h"
>>>>>>     #include <drm/i915_drm.h>
>>>>>>     #include <drm/intel_lpe_audio.h>
>>>>>> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>>>     	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>>>>>>     	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>>>>     	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>>>> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
>>>>>>     	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
>>>>>>     	int clock_12bpc = clock_8bpc * 3 / 2;
>>>>>>     	int desired_bpp;
>>>>>> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>>>>>     
>>>>>>     	pipe_config->lane_count = 4;
>>>>>>     
>>>>>> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
>>>>>> +		if (scdc->scrambling.low_rates)
>>>>>> +			pipe_config->hdmi_scrambling = true;
>>>>>> +
>>>>>> +		if (pipe_config->port_clock > 340000) {
>>>>>> +			pipe_config->hdmi_scrambling = true;
>>>>>> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>>     	return true;
>>>>>>     }
>>>>>>     
>>>>>> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>>>>>>     	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>>>>>>     }
>>>>>>     
>>>>>> +/*
>>>>>> + * This function handles scrambling on HDMI 2.0 capable sinks.
>>>>>> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
>>>>>> + * it enables scrambling. This should be called before enabling the HDMI
>>>>>> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
>>>>>> + * detect a scrambled clock within 100 ms.
>>>>>> + */
>>>>>> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
>>>>>> +				       struct drm_connector *connector,
>>>>>> +				       struct intel_crtc_state *config,
>>>>>> +				       bool enable)
>>>>>> +{
>>>>>> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
>>>>>> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
>>>>>> +	struct drm_scrambling *scrambling =
>>>>>> +				&connector->display_info.hdmi.scdc.scrambling;
>>>>>> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
>>>>>> +							  intel_hdmi->ddc_bus);
>>>>>> +	bool ret;
>>>>>> +
>>>>>> +	if (!scrambling->supported)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (!IS_GEMINILAKE(dev_priv))
>>>>>> +		return;
>>>>> I'd still like to see this getting done uncoditionally. That way we
>>>>> might get a bit of extra testing for the SCDC helper no matter what
>>>>> platform the user happens to own.
>>>> Sure, seems a good idea. I just added this hear to address the review
>>>> comment from Ander.
>>>> Also, I can move this IS_GLK condition to compute_config, and once we
>>>> check this in compute_config
>>>> we might never have check it again.
>>> I thought you already had the check in there? Hmm. Looks that way to me.
>> Yep, I already have that (this shows I have too many versions of code in
>> my mind too :P),
>> what I meant was, we can use some of those state checks here before
>> enabling/disabling sink side scrambling.
>> But then I realized it won't be the unconditional stuff you actually
>> want, so please rub my last comment off :-)
> Well I think it should work just fine if you simply pass both into this
> function as bools, and then in the disable path we pass explicit 'false'
> for both, and in the enable path we just pass config->whatever.
Humm, let me try this.

- Shashank
>> - Shashank
>>>>> I presume the spec says the sink will reset these to some default value
>>>>> on HPD or something like that? But I'm not sure trusting them to get
>>>>> this right is a good idea because reality often doesn't match the
>>>>> specs. So the safer option IMO would be always tell the sink exactly
>>>>> what we want.
>>>> Yep, You are correct about the spec too, that it says if there is no
>>>> scrambled clock post hpd_out for
>>>> 100ms, the sink should disable scrambling. But connected modeset
>>>> scenarios need our attention, where
>>>> for example we are switching mode from, 4k@60 (scrambling reqd) ->
>>>> 1080@60 (scrambling not reqd).
>>>> For such cases too, we have to tell sink to disable scrambling.
>>>>>> +
>>>>>> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
>>>>>> +			intel_encoder->base.name, connector->name);
>>>>>> +
>>>>>> +	if (config->hdmi_high_tmds_clock_ratio) {
>>>>>> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
>>>>>> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
>>>>>> +		if (!ret) {
>>>>>> +			DRM_ERROR("Set TMDS ratio failed\n");
>>>>>> +			return;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	if (config->hdmi_scrambling) {
>>>>>> +		/* Enable/disable sink scrambling */
>>>>>> +		ret = drm_scdc_set_scrambling(adptr, enable);
>>>>>> +		if (!ret) {
>>>>>> +			DRM_ERROR("Set sink scrambling failed\n");
>>>>>> +			return;
>>>>>> +		}
>>>>>> +	}
>>>>>> +
>>>>>> +	DRM_DEBUG_KMS("sink scrambling handled\n");
>>>>>> +}
>>>>>> +
>>>>>>     static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>>>>>>     			     enum port port)
>>>>>>     {
>>>>>> -- 
>>>>>> 1.9.1

Ander Conselvan de Oliveira March 9, 2017, 7:46 a.m. UTC | #7

On Wed, 2017-03-08 at 19:07 +0530, Shashank Sharma wrote:
>  Geminilake platform sports a native HDMI 2.0 controller, and is
>  capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
>  mendates scrambling for these higher clocks, for reduced RF footprint.
> 
>  This patch checks if the monitor supports scrambling, and if required,
>  enables it during the modeset.
> 
>  V2: Addressed review comments from Ville:
>  - Do not track scrambling status in DRM layer, track somewhere in
>    driver like in intel_crtc_state.
>  - Don't talk to monitor at such a low layer, set monitor scrambling
>    in intel_enable_ddi() before enabling the port.
> 
>  V3: Addressed review comments from Jani
>  - In comments, function names, use "sink" instead of "monitor",
>    so that the implementation could be close to the language of
>    HDMI spec.
> 
> V4: Addressed review comment from Maarten
>  - scrambling -> hdmi_scrambling
>  - high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> 
> V5: Addressed review comments from Ville and Ander
>  - Do not modifiy the crtc_state after compute_config. Move all
>    scrambling and tmds_clock_ratio calcutations to compute_config.
>  - While setting scrambling for source/sink, do not check the
>    conditions again, just go by the crtc_state flags. This will
>    simplyfy the condition checks.
> 
> V6: Addressed review comments from Ville
>  - Do not add IS_GLK check in disable/enable function, instead add it
>    in compute_config, while setting state flags.
>  - Remove unnecessary paranthesis.
>  - Simplyfy handle_sink_scrambling function as suggested.
>  - Add readout code for scrambling status in get_ddi_config and add a
>    check for the same in pipe_config_compare.
> 
> V7: Addressed review comments from Ander/Ville
>  - No separate function for source scrambling, make it inline
>  - Align the last line of the macro TRANS_DDI_HDMI_SCRAMBLING_MASK
>  - Do not add platform check while setting source scrambling
>  - Use pipe_config instead of crtc->config to set sink scrambling
>  - To readout scrambling status, Compare with SCRAMBLING_MASK
>    not any of its bits
>  - Remove platform check in intel_pipe_config_compare while checking
>    scrambling status
> 
> V8: Fixed mege conflict, Addressed review comments from Ander
>  - Remove the desciption/comment about scrambling fom the caller, move
>    it to the function
>  - Move the IS_GLK check into scrambling function
>  - Fix alignment
> 
> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |  7 ++++
>  drivers/gpu/drm/i915/intel_ddi.c     | 19 +++++++++++
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h     | 10 ++++++
>  drivers/gpu/drm/i915/intel_hdmi.c    | 62 ++++++++++++++++++++++++++++++++++++
>  5 files changed, 101 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index cc843f9..2d50fdc 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7830,7 +7830,14 @@ enum {
>  #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
>  #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
>  #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
>  #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> +#define  TRANS_DDI_HDMI_SCRAMBLING_MASK (TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE \
> +					| TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ \
> +					| TRANS_DDI_HDMI_SCRAMBLING)
>  
>  /* DisplayPort Transport Control */
>  #define _DP_TP_CTL_A			0x64040
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 0467676..b3e4c4a 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1309,6 +1309,11 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
>  			temp |= TRANS_DDI_MODE_SELECT_HDMI;
>  		else
>  			temp |= TRANS_DDI_MODE_SELECT_DVI;
> +
> +		if (crtc_state->hdmi_scrambling)
> +			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> +		if (crtc_state->hdmi_high_tmds_clock_ratio)
> +			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>  	} else if (type == INTEL_OUTPUT_ANALOG) {
>  		temp |= TRANS_DDI_MODE_SELECT_FDI;
>  		temp |= (crtc_state->fdi_lanes - 1) << 1;
> @@ -1882,6 +1887,9 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
>  		struct intel_digital_port *intel_dig_port =
>  			enc_to_dig_port(encoder);
>  
> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> +			conn_state->connector, pipe_config, true);
> +

I'd prefer if we followed the coding style here (and everywhere!), i.e., 

		intel_hdmi_handle_sink_scrambling(intel_encoder,
						  conn_state->connector,
						  pipe_config, true);

If you want to make this neater, you could add an extra variable for the
connector and throw in a clean up patch before this one changing the
"intel_encoder" variables to be called just "encoder". Since we prefer to have
the most specialized type as function arguments and we only support intel
hardware, we don't need to constantly remind ourselves that this is an intel
encoder.

$ grep -o -E 'struct intel_encoder \*(intel_)?encoder' *.[ch] | \
	cut -d: -f2 | sort | uniq -c
    257 struct intel_encoder *encoder
     69 struct intel_encoder *intel_encoder

Could change that to 270 vs 56 by cleaning intel_ddi.c up.

>  		/* In HDMI/DVI mode, the port width, and swing/emphasis values
>  		 * are ignored so nothing special needs to be done besides
>  		 * enabling the port.
> @@ -1914,6 +1922,11 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
>  	if (old_crtc_state->has_audio)
>  		intel_audio_codec_disable(intel_encoder);
>  
> +	if (type == INTEL_OUTPUT_HDMI) {
> +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> +			old_conn_state->connector, old_crtc_state, false);
> +	}
> +

Ditto.

>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
>  
> @@ -2040,6 +2053,12 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  
>  		if (intel_hdmi->infoframe_enabled(&encoder->base, pipe_config))
>  			pipe_config->has_infoframe = true;
> +
> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> +				TRANS_DDI_HDMI_SCRAMBLING_MASK)
> +			pipe_config->hdmi_scrambling = true;
> +		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
>  		/* fall through */
>  	case TRANS_DDI_MODE_SELECT_DVI:
>  		pipe_config->lane_count = 4;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e77ca7d..fb5e670 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11701,6 +11701,9 @@ static void __printf(3, 4)
>  	if ((INTEL_GEN(dev_priv) < 8 && !IS_HASWELL(dev_priv)) ||
>  	    IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
>  		PIPE_CONF_CHECK_I(limited_color_range);
> +
> +	PIPE_CONF_CHECK_I(hdmi_scrambling);
> +	PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
>  	PIPE_CONF_CHECK_I(has_infoframe);
>  
>  	PIPE_CONF_CHECK_I(has_audio);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 3af20c1..50d680c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -729,6 +729,12 @@ struct intel_crtc_state {
>  
>  	/* bitmask of visible planes (enum plane_id) */
>  	u8 active_planes;
> +
> +	/* HDMI scrambling status */
> +	bool hdmi_scrambling;
> +
> +	/* HDMI High TMDS char rate ratio */
> +	bool hdmi_high_tmds_clock_ratio;
>  };
>  
>  struct intel_crtc {
> @@ -1623,6 +1629,10 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
>  bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  			       struct intel_crtc_state *pipe_config,
>  			       struct drm_connector_state *conn_state);
> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> +					struct drm_connector *connector,
> +					struct intel_crtc_state *config,
> +					bool enable);
>  void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
>  
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index c2184f7..4585aa8 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -34,6 +34,7 @@
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_scdc_helper.h>
>  #include "intel_drv.h"
>  #include <drm/i915_drm.h>
>  #include <drm/intel_lpe_audio.h>
> @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
>  	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
>  	int clock_12bpc = clock_8bpc * 3 / 2;
>  	int desired_bpp;
> @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>  
>  	pipe_config->lane_count = 4;
>  
> +	if (scdc->scrambling.supported && IS_GEMINILAKE(dev_priv)) {
> +		if (scdc->scrambling.low_rates)
> +			pipe_config->hdmi_scrambling = true;
> +
> +		if (pipe_config->port_clock > 340000) {
> +			pipe_config->hdmi_scrambling = true;
> +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> +		}
> +	}
> +
>  	return true;
>  }
>  
> @@ -1794,6 +1806,56 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
>  	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
>  }
>  
> +/*
> + * This function handles scrambling on HDMI 2.0 capable sinks.
> + * If required clock rate is > 340 Mhz && scrambling is supported by sink
> + * it enables scrambling. This should be called before enabling the HDMI
> + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
> + * detect a scrambled clock within 100 ms.
> + */

I actually meant to put this in kernel doc format. It is standard practice to
document functions that are not static, even though we don't seem to follow this
for intel_hdmi.c. Have to start somewhere.

Ander

> +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> +				       struct drm_connector *connector,
> +				       struct intel_crtc_state *config,
> +				       bool enable)
> +{
> +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> +	struct drm_scrambling *scrambling =
> +				&connector->display_info.hdmi.scdc.scrambling;
> +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> +							  intel_hdmi->ddc_bus);
> +	bool ret;
> +
> +	if (!scrambling->supported)
> +		return;
> +
> +	if (!IS_GEMINILAKE(dev_priv))
> +		return;
> +
> +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> +			intel_encoder->base.name, connector->name);
> +
> +	if (config->hdmi_high_tmds_clock_ratio) {
> +		/* Set TMDS bit clock ratio to 1/40 or 1/10 */
> +		ret = drm_scdc_set_high_tmds_clock_ratio(adptr, enable);
> +		if (!ret) {
> +			DRM_ERROR("Set TMDS ratio failed\n");
> +			return;
> +		}
> +	}
> +
> +	if (config->hdmi_scrambling) {
> +		/* Enable/disable sink scrambling */
> +		ret = drm_scdc_set_scrambling(adptr, enable);
> +		if (!ret) {
> +			DRM_ERROR("Set sink scrambling failed\n");
> +			return;
> +		}
> +	}
> +
> +	DRM_DEBUG_KMS("sink scrambling handled\n");
> +}
> +
>  static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
>  			     enum port port)
>  {

[v8,5/6] drm/i915: enable scrambling

Commit Message

Comments

Patch