[Intel-gfx] [PATCH] drm/i915/lspcon: Fix Parade LSPCON scrambling fail

[Sharma, Shashank]

Regards

Shashank


On 10/13/2018 12:58 AM, Ville Syrjälä wrote:
> On Fri, Oct 12, 2018 at 10:17:57PM +0300, Ville Syrjälä wrote:
>> On Sat, Oct 13, 2018 at 12:26:57AM +0530, Sharma, Shashank wrote:
>>> Regards
>>>
>>> Shashank
>>>
>>>
>>> On 10/13/2018 12:08 AM, Ville Syrjala wrote:
>>>> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>>
>>>> The Parade LSPCON on KBL NUCs forgets to turn off scrambling/bit clock
>>>> rate when switching from a mode that needs them to a mode that does
>>>> not. This manifests as a "no signal" on my TV when I try to go from
>>>> 4k to 1080p for example. Resetting the SCDC register bits with
>>>> i2cset is sufficient to restore the picture to the screen.
>>>>
>>>> Here's the OUI/fw revision for the LSPCON chip in question:
>>>> DP branch: OUI 00-1c-f8 dev-ID 175IB0 HW-rev 1.0 SW-rev 7.32 quirks 0x0000
>>>>
>>>> Asking users to poke at SCDC with i2cset is a bit much, so
>>>> let's work around this in the driver. We don't need to go all
>>>> out here and compute whether scrambling is needed or not as
>>>> LSPCON will do that itself. If scrambling is actually
>>>> required LSPCON does not forget to enable it.
>>>>
>>>> Cc: Shashank Sharma <shashank.sharma at intel.com>
>>>> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_ddi.c  | 52 ++++++++++++++++++++++++++++---
>>>>    drivers/gpu/drm/i915/intel_drv.h  |  1 +
>>>>    drivers/gpu/drm/i915/intel_hdmi.c |  5 ++-
>>>>    3 files changed, 51 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>>> index 47960c92cbbf..ef502fc9add1 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -2984,6 +2984,23 @@ static void intel_ddi_pre_enable(struct intel_encoder *encoder,
>>>>    		intel_ddi_pre_enable_dp(encoder, crtc_state, conn_state);
>>>>    }
>>>>    
>>>> +static void intel_ddi_pre_enable_lspcon(struct intel_encoder *encoder,
>>>> +					const struct intel_crtc_state *crtc_state,
>>>> +					const struct drm_connector_state *conn_state)
>>>> +{
>>>> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>>> +
>>>> +	intel_ddi_pre_enable(encoder, crtc_state, conn_state);
>>>> +
>>>> +	/*
>>>> +	 * Parade LSPCON forgets to turn off scrambling/bit clock rate
>>>> +	 * when switching from a mode that needs them to a mode that
>>>> +	 * does not.
>>>> +	 */
>>>> +	intel_hdmi_handle_sink_scrambling(encoder, conn_state->connector,
>>>> +					  &intel_dp->aux.ddc, false, false);
>>>> +}
>>>> +
>>>>    static void intel_disable_ddi_buf(struct intel_encoder *encoder)
>>>>    {
>>>>    	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>> @@ -3086,6 +3103,23 @@ static void intel_ddi_post_disable(struct intel_encoder *encoder,
>>>>    					  old_crtc_state, old_conn_state);
>>>>    }
>>>>    
>>>> +static void intel_ddi_post_disable_lspcon(struct intel_encoder *encoder,
>>>> +					  const struct intel_crtc_state *old_crtc_state,
>>>> +					  const struct drm_connector_state *old_conn_state)
>>>> +{
>>>> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>>> +
>>>> +	/*
>>>> +	 * Parade LSPCON forgets to turn off scrambling/bit clock rate
>>>> +	 * when switching from a mode that needs them to a mode that
>>>> +	 * does not.
>>>> +	 */
>>>> +	intel_hdmi_handle_sink_scrambling(encoder, old_conn_state->connector,
>>>> +					  &intel_dp->aux.ddc, false, false);
>>>> +
>>>> +	intel_ddi_post_disable(encoder, old_crtc_state, old_conn_state);
>>>> +}
>>> Few thoughts:
>>> - Would it make more sense to move these 2 functions to intel_lspcon.c ?
>> Would require more non-statics. But I guess it might be nice to attempt
>> isolating it as much as possible.
>>
>>> -  And then add a lspcon->vendor == VENDOR_PARADE check, so that we will
>>> run the code only when needed.
>> Maybe. The ->vendor thing isn't in yet, and we'd have to backport it
>> as well. Is it big?
>>
>> Also at this time I have no idea whether Megachips LSPCON is similarly
>> bugged. Would need to find one and test it.
> Oh, and another open question is what happens if one of these chips is
> in a DP->HDMI 2.0 dongle. In that case we might end up needing the same
> workaround in the normal DP codepath as well :(
>
You are right, that would be a LSPCON in a dongle configuration (this 
one is motherboard down configuration), which we are not officially 
supporting yet.
That might come with its own complexity. I think we already have a BZ 
for such a Parade dongle adapter.

- Shashank