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

[Clint Taylor <clinton.a.taylor@intel.com>]

On 10/08/2018 03:33 AM, Ville Syrjälä wrote:
> 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.
I will optimize the statement.
> 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.
Section 6.1.2.4.1 of the HDMI 2.1 specification contains some 
information about the CTS testing. In normal video transmission there is 
one SSCP transmitted per field. Of course the HDMI 2.0 CTS doesn't 
mention a need for per line SSCP's that this bit enables.
>>   			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.
Agreed. There are more fixes on the way to correct enable/disable 
scrambling and the SCDC registers.
> I don't think the spec section numbers are particularly helpful without
> some indication as to which specification they refer to.
Will add specification version information.
>> +
>> +	/* 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.
I'm experimenting with an HDMI hotplug handler specifically for HDMI 2.0 
sinks. I would prefer not to wake up every 250ms, powering up the DDC 
lines, doing a single byte read, and sleeping again.
>
> 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.
The SCDC scramble_enable bit must be enabled within 100ms of sending 
scrambled data. Maybe compute_config would work.

-Clint


>> +
>>   	/* 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

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