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

[Manasi Navare <manasi.d.navare@intel.com>]

On Thu, Oct 25, 2018 at 05:15:34PM +0300, Ville Syrjälä wrote:
> 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@linux.intel.com>
> > Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> > Cc: Anusha Srivatsa <anusha.srivatsa@intel.com>
> > Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
> > Reviewed-by: Anusha Srivatsa <anusha.srivatsa@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.

Yes, might have added this during the early development cycles. Will remove it now
that it follows the functions that actually do it.
Thanks for pointing it out.

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

But if you look at the spec def of PIPE_DSS_CTL1 and 2, it actually
has a set of registers for Pipe B and different for Pipe C.
So in case of external DP VDSC engines are tied to per pipe
on ICL as well.

Only for eDP, it is tied to transcoder EDP and hence I look at the
transcoder eDP and use DSS_CTL1 or 2 else use PIPE_DSS_CTL1/2.

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

So add a macro something like this:
#define IS_TRANSCODER_EDP(crtc-state)       crtc_state->cpu_transcoder == TRANSCODER_EDP;

Is this what you are suggesting?

MAnasi

> > +	}
> > +	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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel