Re: [PATCH 04/12] drm/i915/icl_dsi: Move helpers to configure dsi dual link to intel_dss

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 26 Aug 2024, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Move the function to configure dss_ctl for dual_link dsi to intel_dss
> files. While at it, use struct intel_display wherever possible.

Also here, I prefer splitting code movement from other changes. It's
just so much easier to review, and so much easier to figure out what
went wrong if a bisection ever lands here.

>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c   | 55 ++----------------------
>  drivers/gpu/drm/i915/display/intel_dss.c | 51 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dss.h |  3 ++
>  3 files changed, 58 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 5ad5011e1fee..ab59638a8c6c 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -44,7 +44,7 @@
>  #include "intel_de.h"
>  #include "intel_dsi.h"
>  #include "intel_dsi_vbt.h"
> -#include "intel_dss_regs.h"
> +#include "intel_dss.h"
>  #include "intel_panel.h"
>  #include "intel_vdsc.h"
>  #include "skl_scaler.h"
> @@ -274,55 +274,6 @@ static void dsi_program_swing_and_deemphasis(struct intel_encoder *encoder)
>  	}
>  }
>  
> -static void configure_dual_link_mode(struct intel_encoder *encoder,
> -				     const struct intel_crtc_state *pipe_config)
> -{
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> -	struct intel_dsi *intel_dsi = enc_to_intel_dsi(encoder);
> -	i915_reg_t dss_ctl1_reg, dss_ctl2_reg;
> -	u32 dss_ctl1;
> -
> -	/* FIXME: Move all DSS handling to intel_vdsc.c */
> -	if (DISPLAY_VER(dev_priv) >= 12) {
> -		struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> -
> -		dss_ctl1_reg = ICL_PIPE_DSS_CTL1(crtc->pipe);
> -		dss_ctl2_reg = ICL_PIPE_DSS_CTL2(crtc->pipe);
> -	} else {
> -		dss_ctl1_reg = DSS_CTL1;
> -		dss_ctl2_reg = DSS_CTL2;
> -	}
> -
> -	dss_ctl1 = intel_de_read(dev_priv, dss_ctl1_reg);
> -	dss_ctl1 |= SPLITTER_ENABLE;
> -	dss_ctl1 &= ~OVERLAP_PIXELS_MASK;
> -	dss_ctl1 |= OVERLAP_PIXELS(intel_dsi->pixel_overlap);
> -
> -	if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK) {
> -		const struct drm_display_mode *adjusted_mode =
> -					&pipe_config->hw.adjusted_mode;
> -		u16 hactive = adjusted_mode->crtc_hdisplay;
> -		u16 dl_buffer_depth;
> -
> -		dss_ctl1 &= ~DUAL_LINK_MODE_INTERLEAVE;
> -		dl_buffer_depth = hactive / 2 + intel_dsi->pixel_overlap;
> -
> -		if (dl_buffer_depth > MAX_DL_BUFFER_TARGET_DEPTH)
> -			drm_err(&dev_priv->drm,
> -				"DL buffer depth exceed max value\n");
> -
> -		dss_ctl1 &= ~LEFT_DL_BUF_TARGET_DEPTH_MASK;
> -		dss_ctl1 |= LEFT_DL_BUF_TARGET_DEPTH(dl_buffer_depth);
> -		intel_de_rmw(dev_priv, dss_ctl2_reg, RIGHT_DL_BUF_TARGET_DEPTH_MASK,
> -			     RIGHT_DL_BUF_TARGET_DEPTH(dl_buffer_depth));
> -	} else {
> -		/* Interleave */
> -		dss_ctl1 |= DUAL_LINK_MODE_INTERLEAVE;
> -	}
> -
> -	intel_de_write(dev_priv, dss_ctl1_reg, dss_ctl1);
> -}
> -
>  /* aka DSI 8X clock */
>  static int afe_clk(struct intel_encoder *encoder,
>  		   const struct intel_crtc_state *crtc_state)
> @@ -791,7 +742,9 @@ gen11_dsi_configure_transcoder(struct intel_encoder *encoder,
>  		}
>  
>  		/* configure stream splitting */
> -		configure_dual_link_mode(encoder, pipe_config);
> +		intel_dss_configure_dsi_dual_link_mode(encoder, pipe_config,
> +						       intel_dsi->dual_link,
> +						       intel_dsi->pixel_overlap);
>  	}
>  
>  	for_each_dsi_port(port, intel_dsi->ports) {
> diff --git a/drivers/gpu/drm/i915/display/intel_dss.c b/drivers/gpu/drm/i915/display/intel_dss.c
> index 9cb89fe656cb..f44dcac84aa4 100644
> --- a/drivers/gpu/drm/i915/display/intel_dss.c
> +++ b/drivers/gpu/drm/i915/display/intel_dss.c
> @@ -7,6 +7,7 @@
>  #include "i915_reg_defs.h"
>  #include "intel_de.h"
>  #include "intel_display_types.h"
> +#include "intel_dsi.h"
>  #include "intel_dss.h"
>  #include "intel_dss_regs.h"
>  
> @@ -85,3 +86,53 @@ void intel_dss_configure_mso(const struct intel_crtc_state *crtc_state)
>  		     SPLITTER_ENABLE | SPLITTER_CONFIGURATION_MASK |
>  		     OVERLAP_PIXELS_MASK, dss1);
>  }
> +
> +void intel_dss_configure_dsi_dual_link_mode(struct intel_encoder *encoder,
> +					    const struct intel_crtc_state *pipe_config,
> +					    u8 dual_link,
> +					    u8 pixel_overlap)
> +{
> +	struct intel_display *display = to_intel_display(encoder);
> +	i915_reg_t dss_ctl1_reg, dss_ctl2_reg;
> +	u32 dss_ctl1;
> +
> +	if (DISPLAY_VER(display) >= 12) {
> +		struct intel_crtc *crtc = to_intel_crtc(pipe_config->uapi.crtc);
> +
> +		dss_ctl1_reg = ICL_PIPE_DSS_CTL1(crtc->pipe);
> +		dss_ctl2_reg = ICL_PIPE_DSS_CTL2(crtc->pipe);
> +	} else {
> +		dss_ctl1_reg = DSS_CTL1;
> +		dss_ctl2_reg = DSS_CTL2;
> +	}
> +
> +	dss_ctl1 = intel_de_read(display, dss_ctl1_reg);
> +	dss_ctl1 |= SPLITTER_ENABLE;
> +	dss_ctl1 &= ~OVERLAP_PIXELS_MASK;
> +	dss_ctl1 |= OVERLAP_PIXELS(pixel_overlap);
> +
> +	if (dual_link == DSI_DUAL_LINK_FRONT_BACK) {
> +		struct drm_i915_private *i915 = to_i915(display->drm);

You don't need that.

> +		const struct drm_display_mode *adjusted_mode =
> +					&pipe_config->hw.adjusted_mode;
> +		u16 hactive = adjusted_mode->crtc_hdisplay;
> +		u16 dl_buffer_depth;
> +
> +		dss_ctl1 &= ~DUAL_LINK_MODE_INTERLEAVE;
> +		dl_buffer_depth = hactive / 2 + pixel_overlap;
> +
> +		if (dl_buffer_depth > MAX_DL_BUFFER_TARGET_DEPTH)
> +			drm_err(&i915->drm,

display->drm

> +				"DL buffer depth exceed max value\n");
> +
> +		dss_ctl1 &= ~LEFT_DL_BUF_TARGET_DEPTH_MASK;
> +		dss_ctl1 |= LEFT_DL_BUF_TARGET_DEPTH(dl_buffer_depth);
> +		intel_de_rmw(display, dss_ctl2_reg, RIGHT_DL_BUF_TARGET_DEPTH_MASK,
> +			     RIGHT_DL_BUF_TARGET_DEPTH(dl_buffer_depth));
> +	} else {
> +		/* Interleave */
> +		dss_ctl1 |= DUAL_LINK_MODE_INTERLEAVE;
> +	}
> +
> +	intel_de_write(display, dss_ctl1_reg, dss_ctl1);
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dss.h b/drivers/gpu/drm/i915/display/intel_dss.h
> index 0571ee2a19f9..0c5d652d46f5 100644
> --- a/drivers/gpu/drm/i915/display/intel_dss.h
> +++ b/drivers/gpu/drm/i915/display/intel_dss.h
> @@ -16,6 +16,9 @@ u8 intel_dss_splitter_pipe_mask(struct intel_display *display);
>  void intel_dss_get_mso_config(struct intel_encoder *encoder,
>  			      struct intel_crtc_state *pipe_config);
>  void intel_dss_configure_mso(const struct intel_crtc_state *crtc_state);
> +void intel_dss_configure_dsi_dual_link_mode(struct intel_encoder *encoder,
> +					    const struct intel_crtc_state *pipe_config,
> +					    u8 dual_link, u8 pixel_overlap);

Nitpick, I think I'd name this intel_dss_dsi_dual_link_mode_configure()
or something.

>  
>  #endif /* __INTEL_DSS_H__ */

-- 
Jani Nikula, Intel