Re: [PATCH] /drm/i915/hdmi: SCDC Scrambling enable without CTS mode

From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: clinton.a.taylor@intel.com
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] /drm/i915/hdmi: SCDC Scrambling enable without CTS mode
Date: Mon, 8 Oct 2018 13:33:49 +0300	[thread overview]
Message-ID: <20181008103349.GB9144@intel.com> (raw)
In-Reply-To: <1538777924-27255-1-git-send-email-clinton.a.taylor@intel.com>

On Fri, Oct 05, 2018 at 03:18:44PM -0700, clinton.a.taylor@intel.com wrote:
> From: Clint Taylor <clinton.a.taylor@intel.com>
> 
> Setting the SCDC scrambling CTS mode causes HDMI Link Layer protocol tests
> HF1-12 and HF1-13 to fail. Added "Source Shall" entries from SCDC
> section before enabling scrambling.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107895
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107896
> Signed-off-by: Clint Taylor <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  | 6 +++---
>  drivers/gpu/drm/i915/intel_hdmi.c | 8 ++++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 9e82281..a1b877f 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1872,7 +1872,7 @@ void intel_ddi_enable_transcoder_func(const struct intel_crtc_state *crtc_state)
>  			temp |= TRANS_DDI_MODE_SELECT_DVI;
>  
>  		if (crtc_state->hdmi_scrambling)
> -			temp |= TRANS_DDI_HDMI_SCRAMBLING_MASK;
> +			temp |= TRANS_DDI_HDMI_SCRAMBLING;
>  		if (crtc_state->hdmi_high_tmds_clock_ratio)
>  			temp |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
>  	} else if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_ANALOG)) {
> @@ -3394,8 +3394,8 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  		if (intel_dig_port->infoframe_enabled(encoder, pipe_config))
>  			pipe_config->has_infoframe = true;
>  
> -		if ((temp & TRANS_DDI_HDMI_SCRAMBLING_MASK) ==
> -			TRANS_DDI_HDMI_SCRAMBLING_MASK)
> +		if ((temp & TRANS_DDI_HDMI_SCRAMBLING) ==
> +			TRANS_DDI_HDMI_SCRAMBLING)

It's a single bit so 'temp & TRANS_DDI_HDMI_SCRAMBLING' will do.

The spec isn't particularly clear about the CTS enable bit, but judging
from the name I guess you should only enable it when doing compliance
testing.

>  			pipe_config->hdmi_scrambling = true;
>  		if (temp & TRANS_DDI_HIGH_TMDS_CHAR_RATE)
>  			pipe_config->hdmi_high_tmds_clock_ratio = true;
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 454f570..d181d67 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -2148,6 +2148,14 @@ bool intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
>  		      connector->base.id, connector->name,
>  		      yesno(scrambling), high_tmds_clock_ratio ? 40 : 10);
>  
> +	/* SCDC source version 10.4.1.2 */
> +	if (drm_scdc_writeb(adapter, SCDC_SOURCE_VERSION, 0x01) < 0)
> +		DRM_DEBUG_KMS("Unable to set SCDC Source Version register\n");

These look unrelated to the scrambler fix, so should be a separate patch.

I don't think the spec section numbers are particularly helpful without
some indication as to which specification they refer to.

> +
> +	/* Clear SCDC CONFIG_0 10.4.1.6 - RR_Enable Polling Only */
> +	if (drm_scdc_writeb(adapter, SCDC_CONFIG_0, 0x00) < 0)
> +		DRM_DEBUG_KMS("Unable to set SCDC CONFIG_0 register\n");

The spec is unfortunately vague about this stuff. It sort of implies
that polling is optional, but then it says that if either source or sink
doesn't set the RR bit then polling must be used, which to me seems
like polling is in fact mandatory.

The spec allows for a max polling interval of 250 ms. I don't particulary
cherish waking up every 250ms whenver a HDMI 2.0 sink is hooked up. I
guess maybe we could limit it to times when the link is actually active,
but it still feels very wasteful to poll for something that should
basically never happen.

This is rather like the eDP dpcd polling when hpd isn't support.
Except IIRC the eDP polling is actually opitonal and we haven't
bothered to implement it. But I'm not even sure whether there are
any machines w/o eDP hpd hooked up.

Anyway, back to the patch itself. It seems to me that we should
probably be configuring this stuff during detect rather than
during crtc enable.

> +
>  	/* Set TMDS bit clock ratio to 1/40 or 1/10, and enable/disable scrambling */
>  	return drm_scdc_set_high_tmds_clock_ratio(adapter,
>  						  high_tmds_clock_ratio) &&
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx