[Intel-gfx] [PATCH 5/5] drm/i915: Move stuff from haswell_crtc_disable() into encoder .post_disable()

Souza, Jose jose.souza at intel.com
Tue Dec 17 17:33:05 UTC 2019


On Fri, 2019-12-13 at 21:52 +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Move all of haswell_crtc_disable() into the encoder
> .post_disable() hooks. Now we're left with just
> calling the .disable() and .post_disable() hooks
> back to back.
> 
> I chose to move the code into the .post_disable() hook instead
> of the .enable() hook as most of the sequence is currently

s/.enable()/.disable()

> implemented in the .post_enable() hook.
> 

s/post_enable()/.post_disable()

> We should collapse it all down to just one hook and then the
> encoders can drive the modeset sequence fully. But that may
> need some further refactoring as we currently call the
> ddi .post_disable() hook from mst code and we can't just
> replace that with a call to the ddi .disable() hook.
> 
> Should also follow up with similar treatment for the enable
> sequence but let's start here where it's easier.

Pretty good change, removed the feature checks from
haswell_crtc_disable() and moved to the right place.

With this changes I can do what you asked and move the code in
intel_dp_mst_post_trans_disabled() to intel_mst_post_disable_dp().

With the typos above fixed:

Reviewed-by: José Roberto de Souza <jose.souza at intel.com>

Thanks

> 
> Cc: José Roberto de Souza <jose.souza at intel.com>
> Cc: Manasi Navare <manasi.d.navare at intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c       | 12 +++++
>  drivers/gpu/drm/i915/display/intel_crt.c     |  8 +++
>  drivers/gpu/drm/i915/display/intel_ddi.c     | 35 ++++++++++++
>  drivers/gpu/drm/i915/display/intel_display.c | 57 +++---------------
> --
>  drivers/gpu/drm/i915/display/intel_display.h |  4 ++
>  drivers/gpu/drm/i915/display/intel_dp_mst.c  | 11 ++++
>  drivers/gpu/drm/i915/display/vlv_dsi.c       | 10 +++-
>  7 files changed, 86 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 03aa92d317a2..006b1a297e6f 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -1251,6 +1251,17 @@ static void gen11_dsi_disable(struct
> intel_encoder *encoder,
>  	gen11_dsi_disable_io_power(encoder);
>  }
>  
> +static void gen11_dsi_post_disable(struct intel_encoder *encoder,
> +				   const struct intel_crtc_state
> *old_crtc_state,
> +				   const struct drm_connector_state
> *old_conn_state)
> +{
> +	intel_crtc_vblank_off(old_crtc_state);
> +
> +	intel_dsc_disable(old_crtc_state);
> +
> +	skylake_scaler_disable(old_crtc_state);
> +}
> +
>  static enum drm_mode_status gen11_dsi_mode_valid(struct
> drm_connector *connector,
>  						 struct
> drm_display_mode *mode)
>  {
> @@ -1697,6 +1708,7 @@ void icl_dsi_init(struct drm_i915_private
> *dev_priv)
>  	encoder->pre_pll_enable = gen11_dsi_pre_pll_enable;
>  	encoder->pre_enable = gen11_dsi_pre_enable;
>  	encoder->disable = gen11_dsi_disable;
> +	encoder->post_disable = gen11_dsi_post_disable;
>  	encoder->port = port;
>  	encoder->get_config = gen11_dsi_get_config;
>  	encoder->update_pipe = intel_panel_update_backlight;
> diff --git a/drivers/gpu/drm/i915/display/intel_crt.c
> b/drivers/gpu/drm/i915/display/intel_crt.c
> index 50624b8f064d..b2b1336ecdb6 100644
> --- a/drivers/gpu/drm/i915/display/intel_crt.c
> +++ b/drivers/gpu/drm/i915/display/intel_crt.c
> @@ -241,6 +241,14 @@ static void hsw_post_disable_crt(struct
> intel_encoder *encoder,
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  
> +	intel_crtc_vblank_off(old_crtc_state);
> +
> +	intel_disable_pipe(old_crtc_state);
> +
> +	intel_ddi_disable_transcoder_func(old_crtc_state);
> +
> +	ironlake_pfit_disable(old_crtc_state);
> +
>  	intel_ddi_disable_pipe_clock(old_crtc_state);
>  
>  	pch_post_disable_crt(encoder, old_crtc_state, old_conn_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c
> b/drivers/gpu/drm/i915/display/intel_ddi.c
> index cac0be47e500..fa40ba7cbcad 100644
> --- a/drivers/gpu/drm/i915/display/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/display/intel_ddi.c
> @@ -3851,6 +3851,25 @@ static void intel_ddi_post_disable_hdmi(struct
> intel_encoder *encoder,
>  	intel_dp_dual_mode_set_tmds_output(intel_hdmi, false);
>  }
>  
> +static void icl_disable_transcoder_port_sync(const struct
> intel_crtc_state *old_crtc_state)
> +{
> +	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> >uapi.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	i915_reg_t reg;
> +	u32 trans_ddi_func_ctl2_val;
> +
> +	if (old_crtc_state->master_transcoder == INVALID_TRANSCODER)
> +		return;
> +
> +	DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave
> Transcoder %s\n",
> +		      transcoder_name(old_crtc_state->cpu_transcoder));
> +
> +	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> +	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> +				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
> +	I915_WRITE(reg, trans_ddi_func_ctl2_val);
> +}
> +
>  static void intel_ddi_post_disable(struct intel_encoder *encoder,
>  				   const struct intel_crtc_state
> *old_crtc_state,
>  				   const struct drm_connector_state
> *old_conn_state)
> @@ -3860,6 +3879,22 @@ static void intel_ddi_post_disable(struct
> intel_encoder *encoder,
>  	enum phy phy = intel_port_to_phy(dev_priv, encoder->port);
>  	bool is_tc_port = intel_phy_is_tc(dev_priv, phy);
>  
> +	intel_crtc_vblank_off(old_crtc_state);
> +
> +	intel_disable_pipe(old_crtc_state);
> +
> +	if (INTEL_GEN(dev_priv) >= 11)
> +		icl_disable_transcoder_port_sync(old_crtc_state);
> +
> +	intel_ddi_disable_transcoder_func(old_crtc_state);
> +
> +	intel_dsc_disable(old_crtc_state);
> +
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		skylake_scaler_disable(old_crtc_state);
> +	else
> +		ironlake_pfit_disable(old_crtc_state);
> +
>  	/*
>  	 * When called from DP MST code:
>  	 * - old_conn_state will be NULL
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> b/drivers/gpu/drm/i915/display/intel_display.c
> index df69e4cd4707..05502f1df3ad 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -167,7 +167,6 @@ static void chv_prepare_pll(struct intel_crtc
> *crtc,
>  static void intel_crtc_init_scalers(struct intel_crtc *crtc,
>  				    struct intel_crtc_state
> *crtc_state);
>  static void skylake_pfit_enable(const struct intel_crtc_state
> *crtc_state);
> -static void ironlake_pfit_disable(const struct intel_crtc_state
> *old_crtc_state);
>  static void ironlake_pfit_enable(const struct intel_crtc_state
> *crtc_state);
>  static void intel_modeset_setup_hw_state(struct drm_device *dev,
>  					 struct drm_modeset_acquire_ctx
> *ctx);
> @@ -1825,7 +1824,7 @@ static void intel_crtc_vblank_on(const struct
> intel_crtc_state *crtc_state)
>  	drm_crtc_vblank_on(&crtc->base);
>  }
>  
> -static void intel_crtc_vblank_off(const struct intel_crtc_state
> *crtc_state)
> +void intel_crtc_vblank_off(const struct intel_crtc_state
> *crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  
> @@ -1891,7 +1890,7 @@ static void intel_enable_pipe(const struct
> intel_crtc_state *new_crtc_state)
>  		intel_wait_for_pipe_scanline_moving(crtc);
>  }
>  
> -static void intel_disable_pipe(const struct intel_crtc_state
> *old_crtc_state)
> +void intel_disable_pipe(const struct intel_crtc_state
> *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> >uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -4582,25 +4581,6 @@ static void icl_enable_trans_port_sync(const
> struct intel_crtc_state *crtc_state
>  		   trans_ddi_func_ctl2_val);
>  }
>  
> -static void icl_disable_transcoder_port_sync(const struct
> intel_crtc_state *old_crtc_state)
> -{
> -	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> >uapi.crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	i915_reg_t reg;
> -	u32 trans_ddi_func_ctl2_val;
> -
> -	if (old_crtc_state->master_transcoder == INVALID_TRANSCODER)
> -		return;
> -
> -	DRM_DEBUG_KMS("Disabling Transcoder Port Sync on Slave
> Transcoder %s\n",
> -		      transcoder_name(old_crtc_state->cpu_transcoder));
> -
> -	reg = TRANS_DDI_FUNC_CTL2(old_crtc_state->cpu_transcoder);
> -	trans_ddi_func_ctl2_val = ~(PORT_SYNC_MODE_ENABLE |
> -				    PORT_SYNC_MODE_MASTER_SELECT_MASK);
> -	I915_WRITE(reg, trans_ddi_func_ctl2_val);
> -}
> -
>  static void intel_fdi_normal_train(struct intel_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->base.dev;
> @@ -5771,7 +5751,7 @@ static int skl_update_scaler_plane(struct
> intel_crtc_state *crtc_state,
>  	return 0;
>  }
>  
> -static void skylake_scaler_disable(const struct intel_crtc_state
> *old_crtc_state)
> +void skylake_scaler_disable(const struct intel_crtc_state
> *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> >uapi.crtc);
>  	int i;
> @@ -6668,7 +6648,7 @@ static void haswell_crtc_enable(struct
> intel_atomic_state *state,
>  	}
>  }
>  
> -static void ironlake_pfit_disable(const struct intel_crtc_state
> *old_crtc_state)
> +void ironlake_pfit_disable(const struct intel_crtc_state
> *old_crtc_state)
>  {
>  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state-
> >uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> @@ -6743,32 +6723,11 @@ static void ironlake_crtc_disable(struct
> intel_atomic_state *state,
>  static void haswell_crtc_disable(struct intel_atomic_state *state,
>  				 struct intel_crtc *crtc)
>  {
> -	const struct intel_crtc_state *old_crtc_state =
> -		intel_atomic_get_old_crtc_state(state, crtc);
> -	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> -	enum transcoder cpu_transcoder = old_crtc_state-
> >cpu_transcoder;
> -
> +	/*
> +	 * FIXME collapse everything to one hook.
> +	 * Need care with mst->ddi interactions.
> +	 */
>  	intel_encoders_disable(state, crtc);
> -
> -	intel_crtc_vblank_off(old_crtc_state);
> -
> -	/* XXX: Do the pipe assertions at the right place for BXT DSI.
> */
> -	if (!transcoder_is_dsi(cpu_transcoder))
> -		intel_disable_pipe(old_crtc_state);
> -
> -	if (INTEL_GEN(dev_priv) >= 11)
> -		icl_disable_transcoder_port_sync(old_crtc_state);
> -
> -	if (!transcoder_is_dsi(cpu_transcoder))
> -		intel_ddi_disable_transcoder_func(old_crtc_state);
> -
> -	intel_dsc_disable(old_crtc_state);
> -
> -	if (INTEL_GEN(dev_priv) >= 9)
> -		skylake_scaler_disable(old_crtc_state);
> -	else
> -		ironlake_pfit_disable(old_crtc_state);
> -
>  	intel_encoders_post_disable(state, crtc);
>  }
>  
> diff --git a/drivers/gpu/drm/i915/display/intel_display.h
> b/drivers/gpu/drm/i915/display/intel_display.h
> index 327376810f66..ff496cfbd4ab 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.h
> +++ b/drivers/gpu/drm/i915/display/intel_display.h
> @@ -485,6 +485,7 @@ enum phy intel_port_to_phy(struct
> drm_i915_private *i915, enum port port);
>  bool is_trans_port_sync_mode(const struct intel_crtc_state *state);
>  
>  void intel_plane_destroy(struct drm_plane *plane);
> +void intel_disable_pipe(const struct intel_crtc_state
> *old_crtc_state);
>  void i830_enable_pipe(struct drm_i915_private *dev_priv, enum pipe
> pipe);
>  void i830_disable_pipe(struct drm_i915_private *dev_priv, enum pipe
> pipe);
>  enum pipe intel_crtc_pch_transcoder(struct intel_crtc *crtc);
> @@ -518,6 +519,7 @@ enum tc_port intel_port_to_tc(struct
> drm_i915_private *dev_priv,
>  int intel_get_pipe_from_crtc_id_ioctl(struct drm_device *dev, void
> *data,
>  				      struct drm_file *file_priv);
>  u32 intel_crtc_get_vblank_counter(struct intel_crtc *crtc);
> +void intel_crtc_vblank_off(const struct intel_crtc_state
> *crtc_state);
>  
>  int ironlake_get_lanes_required(int target_clock, int link_bw, int
> bpp);
>  void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
> @@ -576,6 +578,8 @@ void intel_crtc_arm_fifo_underrun(struct
> intel_crtc *crtc,
>  
>  u16 skl_scaler_calc_phase(int sub, int scale, bool chroma_center);
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
> +void skylake_scaler_disable(const struct intel_crtc_state
> *old_crtc_state);
> +void ironlake_pfit_disable(const struct intel_crtc_state
> *old_crtc_state);
>  u32 glk_plane_color_ctl(const struct intel_crtc_state *crtc_state,
>  			const struct intel_plane_state *plane_state);
>  u32 glk_plane_color_ctl_crtc(const struct intel_crtc_state
> *crtc_state);
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 8bdbb15799ee..7aa0975c33b7 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -241,6 +241,17 @@ static void intel_mst_post_disable_dp(struct
> intel_encoder *encoder,
>  	intel_dp->active_mst_links--;
>  	last_mst_stream = intel_dp->active_mst_links == 0;
>  
> +	intel_crtc_vblank_off(old_crtc_state);
> +
> +	intel_disable_pipe(old_crtc_state);
> +
> +	intel_ddi_disable_transcoder_func(old_crtc_state);
> +
> +	if (INTEL_GEN(dev_priv) >= 9)
> +		skylake_scaler_disable(old_crtc_state);
> +	else
> +		ironlake_pfit_disable(old_crtc_state);
> +
>  	/*
>  	 * From TGL spec: "If multi-stream slave transcoder: Configure
>  	 * Transcoder Clock Select to direct no clock to the
> transcoder"
> diff --git a/drivers/gpu/drm/i915/display/vlv_dsi.c
> b/drivers/gpu/drm/i915/display/vlv_dsi.c
> index 8398a265b6a3..21e820299107 100644
> --- a/drivers/gpu/drm/i915/display/vlv_dsi.c
> +++ b/drivers/gpu/drm/i915/display/vlv_dsi.c
> @@ -882,8 +882,8 @@ static void intel_dsi_clear_device_ready(struct
> intel_encoder *encoder)
>  }
>  
>  static void intel_dsi_post_disable(struct intel_encoder *encoder,
> -				   const struct intel_crtc_state
> *pipe_config,
> -				   const struct drm_connector_state
> *conn_state)
> +				   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 intel_dsi *intel_dsi = enc_to_intel_dsi(&encoder->base);
> @@ -892,6 +892,12 @@ static void intel_dsi_post_disable(struct
> intel_encoder *encoder,
>  
>  	DRM_DEBUG_KMS("\n");
>  
> +	if (IS_GEN9_LP(dev_priv)) {
> +		intel_crtc_vblank_off(old_crtc_state);
> +
> +		skylake_scaler_disable(old_crtc_state);
> +	}
> +
>  	if (is_vid_mode(intel_dsi)) {
>  		for_each_dsi_port(port, intel_dsi->ports)
>  			vlv_dsi_wait_for_fifo_empty(intel_dsi, port);


More information about the Intel-gfx mailing list