On Tue, Oct 16, 2018 at 08:22:26AM +0530, Sharma, Shashank wrote:
> 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@xxxxxxxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxx>
> >>> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >>> ---
> >>> 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.
Which means we have to guess as to which clock LSPCON will choose. Ie.
just another guess that we can get wrong at which point the bug will
be back. This patch has no such achilles heel.
>
> 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;
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
