[Intel-gfx] [PATCH] drm/i915/lspcon: Fix Parade LSPCON scrambling fail
Sharma, Shashank
shashank.sharma at intel.com
Tue Oct 16 02:52:26 UTC 2018
Regards
Shashank
On 10/13/2018 12:47 AM, 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?
The LSPCON patches are merged now, we can directly use the vendor check
now.
> Also at this time I have no idea whether Megachips LSPCON is similarly
> bugged. Would need to find one and test it.
That would be best, we will have to do that anyways for compliance
issues sooner or later :-)
>> - Also, we should check if scrambling is enabled, there might be a case
>> where we are driving a HDMI 2.0 display (scrambling->supported = 1) but
>> current mode is 1080 P.
> As explained in the commit message this is not needed. And currently we
> have no idea whether LSPCON will enable scrambling or not. Adding code
> to determine that would mean second guessing what LSPCON will do. I
> don't see any benefit in doing that.
Humm, I guess this can be determined with few checks, and HDMI spec
mendates:
- clock above 340Mhz ? LSPCON must enable scrambling
- clock below 340Mhz && monitor supports scrambling below 340Mhz ?
LSPCON should enable scrambling : LSPCON mustn't enable scrambling.
This will make sure that we are doing what's recommended by spec.
- Shashank
>> - Shashank
>>> +
>>> void intel_ddi_fdi_post_disable(struct intel_encoder *encoder,
>>> const struct intel_crtc_state *old_crtc_state,
>>> const struct drm_connector_state *old_conn_state)
>>> @@ -3146,9 +3180,11 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
>>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>> struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>>> struct drm_connector *connector = conn_state->connector;
>>> + struct i2c_adapter *adapter = intel_gmbus_get_adapter(dev_priv,
>>> + dig_port->hdmi.ddc_bus);
>>> enum port port = encoder->port;
>>>
>>> - if (!intel_hdmi_handle_sink_scrambling(encoder, connector,
>>> + if (!intel_hdmi_handle_sink_scrambling(encoder, connector, adapter,
>>> crtc_state->hdmi_high_tmds_clock_ratio,
>>> crtc_state->hdmi_scrambling))
>>> DRM_ERROR("[CONNECTOR:%d:%s] Failed to configure sink scrambling/TMDS bit clock ratio\n",
>>> @@ -3243,13 +3279,17 @@ static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
>>> const struct intel_crtc_state *old_crtc_state,
>>> const struct drm_connector_state *old_conn_state)
>>> {
>>> + struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>> struct drm_connector *connector = old_conn_state->connector;
>>> + struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>>> + struct i2c_adapter *adapter = intel_gmbus_get_adapter(dev_priv,
>>> + dig_port->hdmi.ddc_bus);
>>>
>>> if (old_crtc_state->has_audio)
>>> intel_audio_codec_disable(encoder,
>>> old_crtc_state, old_conn_state);
>>>
>>> - if (!intel_hdmi_handle_sink_scrambling(encoder, connector,
>>> + if (!intel_hdmi_handle_sink_scrambling(encoder, connector, adapter,
>>> false, false))
>>> DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Failed to reset sink scrambling/TMDS bit clock ratio\n",
>>> connector->base.id, connector->name);
>>> @@ -3862,17 +3902,21 @@ void intel_ddi_init(struct drm_i915_private *dev_priv, enum port port)
>>> }
>>>
>>> if (init_lspcon) {
>>> - if (lspcon_init(intel_dig_port))
>>> + if (lspcon_init(intel_dig_port)) {
>>> /* TODO: handle hdmi info frame part */
>>> DRM_DEBUG_KMS("LSPCON init success on port %c\n",
>>> port_name(port));
>>> - else
>>> +
>>> + intel_encoder->pre_enable = intel_ddi_pre_enable_lspcon;
>>> + intel_encoder->post_disable = intel_ddi_post_disable_lspcon;
>>> + } else {
>>> /*
>>> * LSPCON init faied, but DP init was success, so
>>> * lets try to drive as DP++ port.
>>> */
>>> DRM_ERROR("LSPCON init failed on port %c\n",
>>> port_name(port));
>>> + }
>>> }
>>>
>>> return;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index e321fc698ae1..d0a06fcc80c0 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1866,6 +1866,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
>>> struct drm_connector_state *conn_state);
>>> bool intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
>>> struct drm_connector *connector,
>>> + struct i2c_adapter *adapter,
>>> bool high_tmds_clock_ratio,
>>> bool scrambling);
>>> 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 2c53efc463e6..bf6f571b674b 100644
>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c
>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
>>> @@ -2114,6 +2114,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>>> * intel_hdmi_handle_sink_scrambling: handle sink scrambling/clock ratio setup
>>> * @encoder: intel_encoder
>>> * @connector: drm_connector
>>> + * @adapter: i2c adapter for the ddc bus
>>> * @high_tmds_clock_ratio = bool to indicate if the function needs to set
>>> * or reset the high tmds clock ratio for scrambling
>>> * @scrambling: bool to Indicate if the function needs to set or reset
>>> @@ -2130,15 +2131,13 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
>>> */
>>> bool intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
>>> struct drm_connector *connector,
>>> + struct i2c_adapter *adapter,
>>> bool high_tmds_clock_ratio,
>>> bool scrambling)
>>> {
>>> struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>> - struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
>>> struct drm_scrambling *sink_scrambling =
>>> &connector->display_info.hdmi.scdc.scrambling;
>>> - struct i2c_adapter *adapter =
>>> - intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
>>>
>>> if (!sink_scrambling->supported)
>>> return true;
More information about the Intel-gfx
mailing list