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

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

On Wed, 01 Mar 2023, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 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>

Thanks, pushed to din.

BR,
Jani.

>
>>  	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

-- 
Jani Nikula, Intel Open Source Graphics Center