Re: [Intel-gfx] [PATCH v6 5/6] drm/i915: enable scrambling

On Fri, 2017-03-03 at 17:33 +0530, Sharma, Shashank wrote:

Thanks for the review Ander. My comments inline.

Shashank

On 3/3/2017 3:22 PM, Ander Conselvan De Oliveira wrote:

...

On Fri, 2017-03-03 at 11:59 +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.

Signed-off-by: Shashank Sharma shashank.sharma@intel.com

drivers/gpu/drm/i915/i915_reg.h | 7 ++++ drivers/gpu/drm/i915/intel_ddi.c | 33 ++++++++++++++++ drivers/gpu/drm/i915/intel_display.c | 5 +++ drivers/gpu/drm/i915/intel_drv.h | 14 +++++++ drivers/gpu/drm/i915/intel_hdmi.c | 74 ++++++++++++++++++++++++++++++++++++ 5 files changed, 133 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 4906ce4d..f7891ac 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -7824,7 +7824,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)
  

Last line is misaligned.

Ok.

...

...

/* 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 a7c08d7..d0c6927 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1311,6 +1311,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc) temp |= TRANS_DDI_MODE_SELECT_HDMI; else temp |= TRANS_DDI_MODE_SELECT_DVI;

• if (IS_GEMINILAKE(dev_priv))
  

• 	temp = intel_hdmi_handle_source_scrambling(
  

• 		intel_encoder,
  

• 		intel_crtc->config, temp);
  

This whole hunk could just be:

• if (config->hdmi_scrambling)

• hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
  

• if (config->hdmi_high_tmds_clock_ratio)

• hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
  
  

No need to check for GLK here, since the compute config code already did.

As I was emphasizing in the previous review comments, having two separate functions, one to set source_scrambling and other to set sink_scrambling makes it easy to follow up, due to design symmetry, and in that case you need not to refer to bspec to find out which all registers be dealing with scrambling , just check for scrambling control functions. So I was insisting on keeping them.

But that design creates an asymmetry when it comes to checking all bits that are enabled in TRANS_DDI_FUNC_CTL() that involves function-hopping. Maybe it is just a matter of different taste. But for the more practical problem of finding what bits are involved in enabling scrambling, I think

git grep hdmi_scrambling

will do a good enough job.

But anyways, both you and Ville are insisting on keeping it inline, so lets keep it that way.

...

...

} else if (type == INTEL_OUTPUT_ANALOG) { temp |= TRANS_DDI_MODE_SELECT_FDI; temp |= (intel_crtc->config->fdi_lanes - 1) << 1; @@ -1885,6 +1890,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder, struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);

• if (IS_GEMINILAKE(dev_priv)) {
  

• 	struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
  

• 	/*
  

• 	 * GLK sports a native HDMI 2.0 controller. If required
  

• 	 * clock rate is > 340 Mhz && scrambling is supported
  

• 	 * by sink, enable scrambling before enabling the
  

• 	 * HDMI 2.0 port. The sink can choose to disable the
  

• 	 * scrambling if it doesn't detect a scrambled within
  

• 	 * 100 ms.
  

• 	 */
  

• 	intel_hdmi_handle_sink_scrambling(intel_encoder,
  

• 				conn_state->connector,
  

• 				crtc->config, true);
  

This function receives a pipe_config pointer as argument. Use that instead of crtc->config.

Ok.

...

...

• }
  

• /* In HDMI/DVI mode, the port width, and swing/emphasis values
  • are ignored so nothing special needs to be done besides
  • enabling the port.

@@ -1917,6 +1937,14 @@ 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) {

• struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
  

• intel_hdmi_handle_sink_scrambling(intel_encoder,
  

• 			old_conn_state->connector,
  

• 			intel_crtc->config, false);
  

• }
• if (type == INTEL_OUTPUT_EDP) { struct intel_dp *intel_dp = enc_to_intel_dp(encoder);

@@ -2044,6 +2072,11 @@ 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)
  

I missed this the first time, but we say hdmi_scrambling is enabled if any of those bits in the mask is set. Is that really what we want? Or should this check specifically for the TRANS_DDI_HDMI_SCRAMBLING bit or all the bits set?

...

...

• 	pipe_config->hdmi_scrambling = true;
  

• if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
  

• 	pipe_config->hdmi_high_tmds_clock_ratio = true;
  

Hmm, we might need a IS_GLK here, since these bits only exist in that platform. I believe they should read zero though, since they are listed are reserved. Dunno.

Hmm, I dont think there would be a way to set the reserved bits though, so we can keep it. In this way the only place we are checking and setting the platform is compute_config.

Yeah, with the change to check the state for non-GLK too, we get a WARN if this fails, so seems ok.

...

...

/* 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 99e8d9c..c017ed3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11662,6 +11662,11 @@ 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);

• if (IS_GEMINILAKE(dev_priv)) {

No need to check for GLK here. The configuration should never be set for other platforms. If it is set, that's a bug and we want verify_crtc_state() to tells us about it.

Agree, will remove this.

...

...

• 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 e6fbbee..baefbaa 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -695,6 +695,12 @@ struct intel_crtc_state {

/* Gamma mode programmed on the pipe */ uint32_t gamma_mode;

• /* HDMI scrambling status (sink) */

• bool hdmi_scrambling;

• /* HDMI High TMDS char rate ratio (sink) */

• bool hdmi_high_tmds_clock_ratio; };

  struct vlv_wm_state {

@@ -1626,6 +1632,14 @@ 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); +uint32_t +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,

• 			struct intel_crtc_state *config,
  

• 			uint32_t hdmi_config);
  

+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..0a1ae03 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,68 @@ static void intel_hdmi_destroy(struct drm_connector *connector) intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE; }

+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;
  

• 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");

+}

+uint32_t +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,

• 	struct intel_crtc_state *config, uint32_t hdmi_config)
  

+{

• struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
• struct drm_connector *connector = &intel_hdmi->attached_connector->base;
• DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",

• 	intel_encoder->base.name, connector->name);
  

Instead of debugging here, print the hdmi scrambling values in intel_dump_pipe_config().

I am also interested in which encoder and connector are we setting the values for, is it that bad :) ?

The debug for the modeset should tell you that, but IMO it would be ok to leave this extra debug message if the function would still exist.

Ander

...

...

• hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING_MASK |

• TRANS_DDI_HIGH_TMDS_CHAR_RATE);
  

This is unnecessary. The hunk below belongs in intel_ddi_enable_transcoder_func() and that function does not do read-modify- write. Any bit that is not explicitly set is zero. I think things are just simpler if this function doesn't exist.

Yeah, will do that.

...

Ander

...

• if (config->hdmi_scrambling)

• hdmi_config |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
  

• if (config->hdmi_high_tmds_clock_ratio)

• hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
  

• return hdmi_config;

+}

• static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv, enum port port) {