Re: [Intel-gfx] [PATCH] drm/i915/dsi: fix DSS CTL register offsets for TGL+

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Mar 01, 2023 at 05:14:09PM +0200, Jani Nikula wrote:
> On TGL+ the DSS control registers are at different offsets, and there's
> one per pipe. Fix the offsets to fix dual link DSI for TGL+.
> 
> There would be helpers for this in the DSC code, but just do the quick
> fix now for DSI. Long term, we should probably move all the DSS handling
> into intel_vdsc.c, so exporting the helpers seems counter-productive.

I'm not entirely happy with intel_vdsc.c since it handles
both the hardware VDSC block (which includes DSS, and so
also uncompressed joiner and MSO), and also some actual
DSC calculations/etc. Might be nice to have a cleaner
split of some sort.

That also reminds me that MSO+dsc/joiner is probably going
to fail miserably given that neither side knows about the
other and both poke the DSS registers.

> 
> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/8232
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/display/icl_dsi.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> index b5316715bb3b..5a17ab3f0d1a 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -277,9 +277,21 @@ static void configure_dual_link_mode(struct intel_encoder *encoder,
>  {
>  	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;
>  
> -	dss_ctl1 = intel_de_read(dev_priv, 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);

Side note: should get rid of this rmw to make sure the thing
fully configuerd the way we want...

Anyways, this seems fine for now:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  	dss_ctl1 |= SPLITTER_ENABLE;
>  	dss_ctl1 &= ~OVERLAP_PIXELS_MASK;
>  	dss_ctl1 |= OVERLAP_PIXELS(intel_dsi->pixel_overlap);
> @@ -299,14 +311,14 @@ static void configure_dual_link_mode(struct intel_encoder *encoder,
>  
>  		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, RIGHT_DL_BUF_TARGET_DEPTH_MASK,
> +		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, dss_ctl1);
> +	intel_de_write(dev_priv, dss_ctl1_reg, dss_ctl1);
>  }
>  
>  /* aka DSI 8X clock */
> -- 
> 2.39.1

-- 
Ville Syrjälä
Intel