[Intel-gfx] [PATCH v6 24/28] drm/i915/dp: Configure Display stream splitter registers during DSC enable

[Ville Syrjälä]

On Wed, Oct 24, 2018 at 03:28:36PM -0700, Manasi Navare wrote:
> Display Stream Splitter registers need to be programmed to enable
> the joiner if two DSC engines are used and also to enable
> the left and the right DSC engines. This happens as part of
> the DSC enabling routine in the source in atomic commit.
> 
> v3:
> * Use cpu_transcoder instead of encoder->type (Ville)
> v2:
> * Rebase (Manasi)
> 
> Cc: Jani Nikula <jani.nikula at linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
> Cc: Anusha Srivatsa <anusha.srivatsa at intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
> Reviewed-by: Anusha Srivatsa <anusha.srivatsa at intelcom>
> ---
>  drivers/gpu/drm/i915/intel_vdsc.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_vdsc.c b/drivers/gpu/drm/i915/intel_vdsc.c
> index 4b4b812d68f3..8b46619aae15 100644
> --- a/drivers/gpu/drm/i915/intel_vdsc.c
> +++ b/drivers/gpu/drm/i915/intel_vdsc.c
> @@ -1010,6 +1010,12 @@ static void intel_dp_send_dsc_pps_sdp(struct intel_encoder *encoder,
>  void intel_dsc_enable(struct intel_encoder *encoder,
>  		      struct intel_crtc_state *crtc_state)
>  {
> +	struct intel_crtc *crtc = to_intel_crtc(crtc_state->base.crtc);
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	enum pipe pipe = crtc->pipe;
> +	i915_reg_t dss_ctl1_reg, dss_ctl2_reg;
> +	u32 dss_ctl1_val = 0;
> +	u32 dss_ctl2_val = 0;
>  
>  	if (!crtc_state->dsc_params.compression_enable)
>  		return;
> @@ -1018,5 +1024,21 @@ void intel_dsc_enable(struct intel_encoder *encoder,
>  
>  	intel_dp_send_dsc_pps_sdp(encoder, crtc_state);
>  
> +	/* Configure DSS_CTL registers for DSC */

This comment seems redundant.

> +	if (crtc_state->cpu_transcoder == TRANSCODER_EDP) {
> +		dss_ctl1_reg = DSS_CTL1;
> +		dss_ctl2_reg = DSS_CTL2;
> +	} else {
> +		dss_ctl1_reg = ICL_PIPE_DSS_CTL1(pipe);
> +		dss_ctl2_reg = ICL_PIPE_DSS_CTL2(pipe);

Shouldn't these be cpu_transcoder too? Yeah, it's the same thing
essentially, but I think it's better to use the thing that
actually matches the hardware.

I wonder if it would even make sense to do the trans==EDP check
in the macro as well. Would avoid cluttering the code with 
details like this. The macro wouldn't be the prettiest thing
ever, but that more or less holds for all reg macros.

> +	}
> +	dss_ctl2_val |= LEFT_BRANCH_VDSC_ENABLE;
> +	if (crtc_state->dsc_params.dsc_split) {
> +		dss_ctl2_val |= RIGHT_BRANCH_VDSC_ENABLE;
> +		dss_ctl1_val |= JOINER_ENABLE;
> +	}
> +	I915_WRITE(dss_ctl1_reg, dss_ctl1_val);
> +	I915_WRITE(dss_ctl2_reg, dss_ctl2_val);
> +
>  	return;
>  }
> -- 
> 2.18.0

-- 
Ville Syrjälä
Intel