Re: [PATCH v2] drm/i915: Don't spew errors when resetting HDMI scrambling/bit clock ratio fails

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 22, 2018 at 08:42:26PM +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/22/2018 7:35 PM, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > When we're disabling the HDMI link we try to reset the scrambling and
> > TMDS bit clock ratio back to the default values. This will fail if the
> > sink has already been disconnected. Thus we should not print an error
> > message when resetting the scrambling/TMDS bit clock ratio fail during
> > disable. During enable we do want the error, and during disable we may
> > still want to know what happended for debug purposes so let's use
> > DRM_DEBUG_KMS() there.
> Yep, this reminds me of initial patch set of scrambling series :-)
> >
> > v2: Remember them consts
> >
> > Cc: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105644
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105655
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/intel_ddi.c  |  11 ++--
> >   drivers/gpu/drm/i915/intel_drv.h  |  10 ++--
> >   drivers/gpu/drm/i915/intel_hdmi.c | 104 +++++++++++++++++++++++++-------------
> >   3 files changed, 78 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 8c2d778560f0..e151c073debb 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -2426,10 +2426,8 @@ static void intel_enable_ddi_hdmi(struct intel_encoder *encoder,
> >   	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> >   	enum port port = encoder->port;
> >   
> > -	intel_hdmi_handle_sink_scrambling(encoder,
> > -					  conn_state->connector,
> > -					  crtc_state->hdmi_high_tmds_clock_ratio,
> > -					  crtc_state->hdmi_scrambling);
> > +	intel_hdmi_sink_scrambling_enable(encoder,
> > +					  crtc_state, conn_state);
> >   
> >   	/* Display WA #1143: skl,kbl,cfl */
> >   	if (IS_GEN9_BC(dev_priv)) {
> > @@ -2524,9 +2522,8 @@ static void intel_disable_ddi_hdmi(struct intel_encoder *encoder,
> >   		intel_audio_codec_disable(encoder,
> >   					  old_crtc_state, old_conn_state);
> >   
> > -	intel_hdmi_handle_sink_scrambling(encoder,
> > -					  old_conn_state->connector,
> > -					  false, false);
> > +	intel_hdmi_sink_scrambling_enable(encoder,
> > +					  old_crtc_state, old_conn_state);
> Calling enable again ? this must be a disable call.

Doh.

> >   }
> >   
> >   static void intel_disable_ddi(struct intel_encoder *encoder,
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index a215aa78b0be..63f0919aa7c8 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1783,10 +1783,12 @@ struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder);
> >   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> >   			       struct intel_crtc_state *pipe_config,
> >   			       struct drm_connector_state *conn_state);
> > -void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > -				       struct drm_connector *connector,
> > -				       bool high_tmds_clock_ratio,
> > -				       bool scrambling);
> > +void intel_hdmi_sink_scrambling_enable(struct intel_encoder *encoder,
> > +				       const struct intel_crtc_state *crtc_state,
> > +				       const struct drm_connector_state *conn_state);
> > +void intel_hdmi_sink_scrambling_disable(struct intel_encoder *encoder,
> > +					const struct intel_crtc_state *old_crtc_state,
> > +					const struct drm_connector_state *old_conn_state);
> >   void intel_dp_dual_mode_set_tmds_output(struct intel_hdmi *hdmi, bool enable);
> >   void intel_infoframe_init(struct intel_digital_port *intel_dig_port);
> >   
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 1baef4ac7ecb..6b61b3d1ac08 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -2068,55 +2068,87 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c
> >   	connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> >   }
> >   
> > -/*
> > - * intel_hdmi_handle_sink_scrambling: handle sink scrambling/clock ratio setup
> > - * @encoder: intel_encoder
> > - * @connector: drm_connector
> > - * @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
> > - *  sink scrambling
> > - *
> > - * This function handles scrambling on HDMI 2.0 capable sinks.
> > - * If required clock rate is > 340 Mhz && scrambling is supported by sink
> > - * it enables scrambling. This should be called before enabling the HDMI
> > - * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
> > - * detect a scrambled clock within 100 ms.
> > - */
> > -void intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
> > -				       struct drm_connector *connector,
> > -				       bool high_tmds_clock_ratio,
> > -				       bool scrambling)
> > +static int intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
> > +					     struct drm_connector *connector,
> > +					     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_i915_private *dev_priv = connector->dev->dev_private;
> >   	struct drm_scrambling *sink_scrambling =
> >   				&connector->display_info.hdmi.scdc.scrambling;
> > -	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> > -							   intel_hdmi->ddc_bus);
> > +	struct i2c_adapter *adapter =
> > +		intel_gmbus_get_adapter(dev_priv, intel_hdmi->ddc_bus);
> >   	bool ret;
> >   
> >   	if (!sink_scrambling->supported)
> > -		return;
> > +		return 0;
> >   
> > -	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > -		      encoder->base.name, connector->name);
> > +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] scrambling=%s, TMDS bit clock ratio=1/%dX\n",
> > +		      connector->base.id, connector->name,
> > +		      yesno(scrambling), high_tmds_clock_ratio ? 40 : 10);
> >   
> >   	/* Set TMDS bit clock ratio to 1/40 or 1/10 */
> > -	ret = drm_scdc_set_high_tmds_clock_ratio(adptr, high_tmds_clock_ratio);
> > -	if (!ret) {
> > -		DRM_ERROR("Set TMDS ratio failed\n");
> > -		return;
> > -	}
> > +	ret = drm_scdc_set_high_tmds_clock_ratio(adapter,
> > +						 high_tmds_clock_ratio);
> > +	if (ret)
> > +		return ret;
> >   
> >   	/* Enable/disable sink scrambling */
> > -	ret = drm_scdc_set_scrambling(adptr, scrambling);
> > -	if (!ret) {
> > -		DRM_ERROR("Set sink scrambling failed\n");
> > -		return;
> > -	}
> > +	ret = drm_scdc_set_scrambling(adapter, scrambling);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return 0;
> > +}
> >   
> > -	DRM_DEBUG_KMS("sink scrambling handled\n");
> > +/*
> > + * intel_hdmi_sink_scrambling_enable: configure sink scrambling/TMDS bit clock ratio
> > + * @crtc_state: new crtc state
> > + * @conn_state: new connector state
> > + *
> > + * Configure scrambling and TMDS bit clock ratio on HDMI 2.0 capable sinks.
> > + * If required clock rate is > 340 Mhz && scrambling is supported by sink
> > + * it enables scrambling. This should be called before enabling the HDMI
> > + * 2.0 port, as the sink can choose to disable the scrambling if it doesn't
> > + * detect a scrambled clock within 100 ms.
> > + */
> > +void intel_hdmi_sink_scrambling_enable(struct intel_encoder *encoder,
> > +				       const struct intel_crtc_state *crtc_state,
> > +				       const struct drm_connector_state *conn_state)
> > +{
> > +	struct drm_connector *connector = conn_state->connector;
> > +	int ret;
> > +
> > +	ret = intel_hdmi_handle_sink_scrambling(encoder, connector,
> > +						crtc_state->hdmi_high_tmds_clock_ratio,
> > +						crtc_state->hdmi_scrambling);
> > +	if (ret)
> > +		DRM_ERROR("[CONNECTOR:%d:%s] Failed to configure sink scrambling/TMDS bit clock ratio\n",
> > +			  connector->base.id, connector->name);
> > +}
> > +
> > +/*
> > + * intel_hdmi_sink_scrambling_disable: handle sink scrambling/clock ratio setup during disable
> > + * @old_crtc_state: old crtc state
> > + * @old_conn_state: old connector state
> > + *
> > + * Reset scrambling and TMDS bit clock ratio on HDMI 2.0 capable sinks.
> > + * We do this to make sure the sink returns to the default settings once
> > + * we're done using it.
> > + */
> > +void intel_hdmi_sink_scrambling_disable(struct intel_encoder *encoder,
> > +					const struct intel_crtc_state *old_crtc_state,
> > +					const struct drm_connector_state *old_conn_state)
> > +{
> > +	struct drm_connector *connector = old_conn_state->connector;
> > +	int ret;
> > +
> > +	ret = intel_hdmi_handle_sink_scrambling(encoder, connector,
> > +						false, false);
> > +	if (ret)
> > +		DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Failed to reset sink scrambling/TMDS bit clock ratio\n",
> > +			      connector->base.id, connector->name);
> >   }
> I think we can still keep only intel_hdmi_handle_sink_scrambling() 
> function instead of having separate disable() and enable() functions, as 
> the only difference between the two is one is printing error, other is 
> kms(). How about
> - keep only intel_hdmi_handle_sink_scrambling, but remove any error 
> messges, and return ret as you have done in this patch.
> - transfer the error message into ddi_enable_hdmi(error here) and 
> ddi_disable_hdmi(kms here)

I suppose that'd work fine since we don't have other users for this.

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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux