Re: [PATCH v5 5/6] drm/i915: enable scrambling

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2017-03-02 at 08:25 +0530, Sharma, Shashank wrote:
> Regards
> 
> Shashank
> 
> 
> On 3/1/2017 8:41 PM, Ville Syrjälä wrote:
> > On Tue, Feb 28, 2017 at 02:09:09PM +0530, Shashank Sharma wrote:
> > > Geminilake platform sports a native HDMI 2.0 controller, and is
> > > capable of driving pixel-clocks upto 594Mhz. HDMI 2.0 spec
> > > mendates scrambling for these higher clocks, for reduced RF footprint.
> > > 
> > > This patch checks if the monitor supports scrambling, and if required,
> > > enables it during the modeset.
> > > 
> > > V2: Addressed review comments from Ville:
> > > - Do not track scrambling status in DRM layer, track somewhere in
> > >    driver like in intel_crtc_state.
> > > - Don't talk to monitor at such a low layer, set monitor scrambling
> > >    in intel_enable_ddi() before enabling the port.
> > > 
> > > V3: Addressed review comments from Jani
> > >   - In comments, function names, use "sink" instead of "monitor",
> > >     so that the implementation could be close to the language of
> > >     HDMI spec.
> > > 
> > > V4: Addressed review comment from Maarten
> > >   - scrambling -> hdmi_scrambling
> > >     high_tmds_clock_ratio -> hdmi_high_tmds_clock_ratio
> > > 
> > > V5: Addressed review comments from Ville and Ander
> > >   - Do not modifiy the crtc_state after compute_config. Move all
> > >     scrambling and tmds_clock_ratio calcutations to compute_config.
> > >   - While setting scrambling for source/sink, do not check the
> > >     conditions again, just go by the crtc_state flags. This will
> > >     simplyfy the condition checks.
> > > 
> > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_reg.h   |  4 ++
> > >   drivers/gpu/drm/i915/intel_ddi.c  | 29 ++++++++++++
> > >   drivers/gpu/drm/i915/intel_drv.h  | 14 ++++++
> > >   drivers/gpu/drm/i915/intel_hdmi.c | 98 +++++++++++++++++++++++++++++++++++++++
> > >   4 files changed, 145 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > > index ce6f096..1759b64 100644
> > > --- a/drivers/gpu/drm/i915/i915_reg.h
> > > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > > @@ -7824,7 +7824,11 @@ enum {
> > >   #define  TRANS_DDI_EDP_INPUT_B_ONOFF	(5<<12)
> > >   #define  TRANS_DDI_EDP_INPUT_C_ONOFF	(6<<12)
> > >   #define  TRANS_DDI_DP_VC_PAYLOAD_ALLOC	(1<<8)
> > > +#define  TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE (1<<7)
> > > +#define  TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ (1<<6)
> > >   #define  TRANS_DDI_BFI_ENABLE		(1<<4)
> > > +#define  TRANS_DDI_HIGH_TMDS_CHAR_RATE	(1<<4)
> > > +#define  TRANS_DDI_HDMI_SCRAMBLING	(1<<0)
> > >   
> > >   /* DisplayPort Transport Control */
> > >   #define _DP_TP_CTL_A			0x64040
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > > index a7c08d7..24bc3a8 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1311,6 +1311,11 @@ void intel_ddi_enable_transcoder_func(struct drm_crtc *crtc)
> > >   			temp |= TRANS_DDI_MODE_SELECT_HDMI;
> > >   		else
> > >   			temp |= TRANS_DDI_MODE_SELECT_DVI;
> > > +
> > > +		if (IS_GEMINILAKE(dev_priv))
> > > +			temp = intel_hdmi_handle_source_scrambling(
> > > +				intel_encoder,
> > > +				intel_crtc->config, temp);
> > >   	} else if (type == INTEL_OUTPUT_ANALOG) {
> > >   		temp |= TRANS_DDI_MODE_SELECT_FDI;
> > >   		temp |= (intel_crtc->config->fdi_lanes - 1) << 1;
> > > @@ -1885,6 +1890,21 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder,
> > >   		struct intel_digital_port *intel_dig_port =
> > >   			enc_to_dig_port(encoder);
> > >   
> > > +		if (IS_GEMINILAKE(dev_priv)) {
> > > +			struct intel_crtc *crtc = to_intel_crtc(encoder->crtc);
> > > +			/*
> > > +			 * GLK sports a native HDMI 2.0 controller. If required
> > > +			 * clock rate is > 340 Mhz && scrambling is supported
> > > +			 * by sink, enable scrambling before enabling the
> > > +			 * HDMI 2.0 port. The sink can choose to disable the
> > > +			 * scrambling if it doesn't detect a scrambled within
> > > +			 * 100 ms.
> > > +			 */
> > > +			intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > +						conn_state->connector,
> > > +						crtc->config, true);
> > > +		}
> > > +
> > >   		/* In HDMI/DVI mode, the port width, and swing/emphasis values
> > >   		 * are ignored so nothing special needs to be done besides
> > >   		 * enabling the port.
> > > @@ -1912,11 +1932,20 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder,
> > >   			      struct drm_connector_state *old_conn_state)
> > >   {
> > >   	struct drm_encoder *encoder = &intel_encoder->base;
> > > +	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
> > >   	int type = intel_encoder->type;
> > >   
> > >   	if (old_crtc_state->has_audio)
> > >   		intel_audio_codec_disable(intel_encoder);
> > >   
> > > +	if ((type == INTEL_OUTPUT_HDMI) && IS_GEMINILAKE(dev_priv)) {
> > 
> >              ^                         ^
> > 
> > Pointless parens. Also I'm nor sure we want the platform check here.
> > Seems to me that we should be able to just explicitly configure the sink
> > on every platform, iff the sink supports SCDC of course.
> 
> This is interesting, what if I connect a HDMI 2.0 sink, which supports 
> scrambling to a GEN8 or non-lspcon
> GEN 9 device ? Shouldn't we check if the source also allows it ?
> > > +		struct intel_crtc *intel_crtc = to_intel_crtc(encoder->crtc);
> > > +
> > > +		intel_hdmi_handle_sink_scrambling(intel_encoder,
> > > +					old_conn_state->connector,
> > > +					intel_crtc->config, false);
> > > +	}
> > > +
> > >   	if (type == INTEL_OUTPUT_EDP) {
> > >   		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > >   
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 907fb00..0e53fcd 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -695,6 +695,12 @@ struct intel_crtc_state {
> > >   
> > >   	/* Gamma mode programmed on the pipe */
> > >   	uint32_t gamma_mode;
> > > +
> > > +	/* HDMI scrambling status (sink) */
> > > +	bool hdmi_scrambling;
> > > +
> > > +	/* HDMI High TMDS char rate ratio (sink) */
> > > +	bool hdmi_high_tmds_clock_ratio;
> > >   };
> > >   
> > >   struct vlv_wm_state {
> > > @@ -1626,6 +1632,14 @@ void intel_hdmi_init_connector(struct intel_digital_port *intel_dig_port,
> > >   bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > >   			       struct intel_crtc_state *pipe_config,
> > >   			       struct drm_connector_state *conn_state);
> > > +uint32_t
> > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > +					struct intel_crtc_state *config,
> > > +					uint32_t hdmi_config);
> > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > +					struct drm_connector *connector,
> > > +					struct intel_crtc_state *config,
> > > +					bool enable);
> > >   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 c2184f7..fdbeefd 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -34,6 +34,7 @@
> > >   #include <drm/drm_atomic_helper.h>
> > >   #include <drm/drm_crtc.h>
> > >   #include <drm/drm_edid.h>
> > > +#include <drm/drm_scdc_helper.h>
> > >   #include "intel_drv.h"
> > >   #include <drm/i915_drm.h>
> > >   #include <drm/intel_lpe_audio.h>
> > > @@ -1316,6 +1317,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > >   	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> > >   	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > >   	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > > +	struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
> > >   	int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> > >   	int clock_12bpc = clock_8bpc * 3 / 2;
> > >   	int desired_bpp;
> > > @@ -1385,6 +1387,16 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > >   
> > >   	pipe_config->lane_count = 4;
> > >   
> > > +	if (scdc->scrambling.supported) {
> > 
> > Here we should have a check whether the source support scrambling or not.
> 
> Ok, makes sense.
> > 
> > > +		if (scdc->scrambling.low_rates)
> > > +			pipe_config->hdmi_scrambling = true;
> > > +
> > > +		if (pipe_config->port_clock > 340000) {
> > > +			pipe_config->hdmi_scrambling = true;
> > > +			pipe_config->hdmi_high_tmds_clock_ratio = true;
> > > +		}
> > > +	}
> > > +
> > >   	return true;
> > >   }
> > >   
> > > @@ -1794,6 +1806,92 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
> > >   	intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_NONE;
> > >   }
> > >   
> > > +void intel_hdmi_handle_sink_scrambling(struct intel_encoder *intel_encoder,
> > > +				       struct drm_connector *connector,
> > > +				       struct intel_crtc_state *config,
> > > +				       bool enable)
> > > +{
> > > +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > +	struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > +	struct drm_scrambling *scrambling =
> > > +				&connector->display_info.hdmi.scdc.scrambling;
> > > +	struct i2c_adapter *adptr = intel_gmbus_get_adapter(dev_priv,
> > > +							  intel_hdmi->ddc_bus);
> > > +	bool ret;
> > > +
> > > +	if (!scrambling->supported)
> > > +		return;
> > > +
> > > +	DRM_DEBUG_KMS("Setting sink scrambling for enc:%s connector:%s\n",
> > > +			intel_encoder->base.name, connector->name);
> > > +
> > > +	if (enable) {
> > > +
> > > +		if (config->hdmi_high_tmds_clock_ratio) {
> > > +			/* Set TMDS bit clock ratio to 1/40 */
> > > +			ret = drm_scdc_set_high_tmds_clock_ratio(adptr, true);
> > > +			if (!ret) {
> > > +				DRM_ERROR("Set high TMDS ratio failed\n");
> > > +				return;
> > > +			}
> > > +		}
> > > +
> > > +		if (config->hdmi_scrambling) {
> > > +			/* Enable sink scrambling */
> > > +			ret = drm_scdc_set_scrambling(adptr, true);
> > > +			if (!ret) {
> > > +				DRM_ERROR("Can't enable sink scrambling\n");
> > > +				return;
> > > +			}
> > > +		}
> > > +
> > > +		DRM_DEBUG_KMS("sink scrambling enabled\n");
> > > +		return;
> > > +	}
> > > +
> > > +	if (config->hdmi_high_tmds_clock_ratio) {
> > > +		/* Set TMDS bit clock ratio back to 1/10 */
> > > +		ret = !(drm_scdc_set_high_tmds_clock_ratio(adptr, false));
> > > +		if (ret)
> > > +			DRM_ERROR("Reset high TMDS ratio failed\n");
> > > +	}
> > > +
> > > +	if (config->hdmi_scrambling) {
> > > +		/* Disable sink scrambling */
> > > +		ret = !(drm_scdc_set_scrambling(adptr, false));
> > > +		if (ret)
> > > +			DRM_ERROR("Disable sink scrambling failed\n");
> > > +	}
> > 
> > Seems a little complicated for what it's doing.
> > 
> > I'd go for something simple like:
> > 
> > foo(bool scrambling, bool clock_ratio)
> > {
> > 	set_scrambling(scrambling);
> > 	set_clock_ratio(clock_ratio);
> > }
> > 
> > enable()
> > {
> > 	foo(crtc_state->hdmi_scrambling, crtc_state->clock_ratio);
> > }
> > 
> > disable()
> > {
> > 	foo(false, false);
> > }
> 
> In the first look, I thought intel_hdmi_handle_sink_scrambling is your 
> foo, but later I realized,
> you dont want an if (enable) cond.... will take care of it.
> > 
> > > +
> > > +	DRM_DEBUG_KMS("sink scrambling disabled\n");
> > > +}
> > > +
> > > +uint32_t
> > > +intel_hdmi_handle_source_scrambling(struct intel_encoder *intel_encoder,
> > > +			struct intel_crtc_state *config, uint32_t hdmi_config)
> > > +{
> > > +	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> > > +	struct drm_connector *connector = &intel_hdmi->attached_connector->base;
> > > +
> > > +	DRM_DEBUG_KMS("Setting scrambling for enc:%s connector:%s\n",
> > > +			intel_encoder->base.name, connector->name);
> > > +
> > > +	hdmi_config &= ~(TRANS_DDI_HDMI_SCRAMBLING |
> > > +		TRANS_DDI_HIGH_TMDS_CHAR_RATE |
> > > +		TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > +		TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > +
> > > +	if (config->hdmi_scrambling)
> > > +		hdmi_config |= (TRANS_DDI_HDMI_SCRAMBLING |
> > > +			TRANS_DDI_HDMI_SCRAMBLER_RESET_FREQ |
> > > +			TRANS_DDI_HDMI_SCRAMBLER_CTS_ENABLE);
> > > +
> > > +	if (config->hdmi_high_tmds_clock_ratio)
> > > +		hdmi_config |= TRANS_DDI_HIGH_TMDS_CHAR_RATE;
> > 
> > IMO just inline these two things into the function that configures
> > this register. Having to jump around between functions is just
> > extra pain we don't need IMO.
> 
> Not really, this function is also making sure, that the scrambling bits 
> are cleared, 

There is no read-modify-write in intel_ddi_enable_transcoder_func(). Every bit
that is not explicitly set is cleared.

> and only
> enabled which is required. Also, this is making code more readable, 
> segregating scrambling
> functionalities, and better for debugging too. Lets keep this one if you 
> please allow.

I think the extra function makes it harder to read. The function
intel_ddi_enable_transcoder_func() only translates the current configuration to
the right bits that need to be programmed to TRANS_DDI_FUNC_CTL. That belongs in
intel_ddi.c.

To help with debugging, add HDMI scrambling prints to intel_dump_pipe_config().
That's actually more useful than the print in this function since it can also
tell us if scrambling was enabled for a configuration read out from hw state.

Ander

> > There should also be corresponding readout code for this in
> > intel_ddi_get_config(), and matching PIPE_CONF_CHECK stuff in
> > intel_pipe_config_compare().
> 
> Agree, will add that too.
> 
> - Shashank
> > > +
> > > +	return hdmi_config;
> > > +}
> > > +
> > >   static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> > >   			     enum port port)
> > >   {
> > > -- 
> > > 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