[Intel-gfx] [PATCH v2 04/22] drm/i915/dp: Update the link bpp limits for DSC mode

Imre Deak imre.deak at intel.com
Mon Sep 4 11:08:38 UTC 2023


On Mon, Sep 04, 2023 at 06:48:25AM +0300, Ville Syrjälä wrote:
> On Thu, Aug 24, 2023 at 11:04:59AM +0300, Imre Deak wrote:
> > In non-DSC mode the link bpp can be set in 2*3 bpp steps in the pipe bpp
> > range, while in DSC mode it can be set in 1/16 bpp steps to any value
> > up to the maximum pipe bpp. Update the limits accordingly in both modes
> > to prepare for a follow-up patch which may need to reduce the max link
> > bpp value and starts to check the link bpp limits in DSC mode as well.
> > 
> > While at it add more detail to the link limit debug print and print it
> > also for DSC mode.
> > 
> > v2:
> > - Add to_bpp_frac_dec() instead of open coding it. (Jani)
> > 
> > Cc: Jani Nikula <jani.nikula at linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  .../drm/i915/display/intel_display_types.h    |  5 ++
> >  drivers/gpu/drm/i915/display/intel_dp.c       | 89 +++++++++++++++----
> >  drivers/gpu/drm/i915/display/intel_dp.h       |  6 ++
> >  drivers/gpu/drm/i915/display/intel_dp_mst.c   | 23 +++--
> >  4 files changed, 101 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > index 5875eff5012ce..a0a404967b5d2 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > @@ -2113,6 +2113,11 @@ static inline int to_bpp_int(int bpp_x16)
> >  	return bpp_x16 >> 4;
> >  }
> >  
> > +static inline int to_bpp_frac_dec(int bpp_x16)
> > +{
> > +	return (bpp_x16 & 0xf) * 625;
> > +}
> 
> This gives me the impression that this would be somehow
> generally useful, but I presume we only use it for the printk?
> So maybe should just have some printk FMT+ARG macros for
> this stuff?

Yes, only used by printks. Make sense to define the FMT+ARG helpers at
one place, can add these here.

> 
> > +
> >  static inline int to_bpp_x16(int bpp)
> >  {
> >  	return bpp << 4;
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> > index c580472c06b85..9ce861a7fd418 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> > @@ -2189,16 +2189,68 @@ int intel_dp_dsc_compute_config(struct intel_dp *intel_dp,
> >  	return 0;
> >  }
> >  
> > -static void
> > +/**
> > + * intel_dp_compute_config_link_bpp_limits - compute output link bpp limits
> > + * @intel_dp: intel DP
> > + * @crtc_state: crtc state
> > + * @dsc: DSC compression mode
> > + * @limits: link configuration limits
> > + *
> > + * Calculates the output link min, max bpp values in @limits based on the
> > + * pipe bpp range, @crtc_state and @dsc mode.
> > + *
> > + * Returns %true in case of success.
> > + */
> > +bool
> > +intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
> > +					const struct intel_crtc_state *crtc_state,
> > +					bool dsc,
> > +					struct link_config_limits *limits)
> > +{
> > +	struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> > +	const struct drm_display_mode *adjusted_mode =
> > +		&crtc_state->hw.adjusted_mode;
> > +	const struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> > +	const struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
> > +	int max_link_bpp_x16;
> > +
> > +	max_link_bpp_x16 = to_bpp_x16(limits->pipe.max_bpp);
> > +
> > +	if (!dsc) {
> > +		max_link_bpp_x16 = rounddown(max_link_bpp_x16, to_bpp_x16(2 * 3));
> > +
> > +		if (max_link_bpp_x16 < to_bpp_x16(limits->pipe.min_bpp))
> > +			return false;
> 
> Quite a few to_bpp_x16()'s in there. Seems like it would a bit simpler
> to just do that once at the end.

At the moment yes, but in a later patch max_link_bpp_x16 starts out as
crtc_state->max_link_bpp_x16 limited value (with a non-zero fractional
part).

> 
> > +
> > +		limits->link.min_bpp_x16 = to_bpp_x16(limits->pipe.min_bpp);
> > +	} else {
> > +		limits->link.min_bpp_x16 = 0;
> 
> Why is that zero? Don't we now have some helpers to fill
> this stuff correctly?

At the moment it's calculated only later in
intel_edp_dsc_compute_pipe_bpp() /  intel_dp_dsc_compute_pipe_bpp().

It should be inited already here, but I wanted to do that only as a
follow-up, since there's been other DSC changes from Ankit still under
review. Is that ok, adding a TODO: here?

> 
> > +	}
> > +
> > +	limits->link.max_bpp_x16 = max_link_bpp_x16;
> > +
> > +	drm_dbg_kms(&i915->drm,
> > +		    "[ENCODER:%d:%s][CRTC:%d:%s] DP link limits: pixel clock %d kHz DSC %s max lanes %d max rate %d max pipe_bpp %d max link_bpp %d.%04d\n",
> > +		    encoder->base.base.id, encoder->base.name,
> > +		    crtc->base.base.id, crtc->base.name,
> > +		    adjusted_mode->crtc_clock,
> > +		    dsc ? "on" : "off",
> > +		    limits->max_lane_count,
> > +		    limits->max_rate,
> > +		    limits->pipe.max_bpp,
> > +		    to_bpp_int(limits->link.max_bpp_x16),
> > +		    to_bpp_frac_dec(limits->link.max_bpp_x16));
> > +
> > +	return true;
> > +}
> > +
> > +static bool
> >  intel_dp_compute_config_limits(struct intel_dp *intel_dp,
> >  			       struct intel_crtc_state *crtc_state,
> >  			       bool respect_downstream_limits,
> > +			       bool dsc,
> >  			       struct link_config_limits *limits)
> >  {
> > -	struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -	const struct drm_display_mode *adjusted_mode =
> > -		&crtc_state->hw.adjusted_mode;
> > -
> >  	limits->min_rate = intel_dp_common_rate(intel_dp, 0);
> >  	limits->max_rate = intel_dp_max_link_rate(intel_dp);
> >  
> > @@ -2224,13 +2276,10 @@ intel_dp_compute_config_limits(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_adjust_compliance_config(intel_dp, crtc_state, limits);
> >  
> > -	limits->link.min_bpp_x16 = to_bpp_x16(limits->pipe.min_bpp);
> > -	limits->link.max_bpp_x16 = to_bpp_x16(limits->pipe.max_bpp);
> > -
> > -	drm_dbg_kms(&i915->drm, "DP link computation with max lane count %i "
> > -		    "max rate %d max bpp %d pixel clock %iKHz\n",
> > -		    limits->max_lane_count, limits->max_rate,
> > -		    to_bpp_int(limits->link.max_bpp_x16), adjusted_mode->crtc_clock);
> > +	return intel_dp_compute_config_link_bpp_limits(intel_dp,
> > +						       crtc_state,
> > +						       dsc,
> > +						       limits);
> >  }
> >  
> >  static int
> > @@ -2249,9 +2298,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  	bool dsc_needed;
> >  	int ret = 0;
> >  
> > -	intel_dp_compute_config_limits(intel_dp, pipe_config,
> > -				       respect_downstream_limits, &limits);
> > -
> >  	if (intel_dp_need_bigjoiner(intel_dp, adjusted_mode->crtc_hdisplay,
> >  				    adjusted_mode->crtc_clock))
> >  		pipe_config->bigjoiner_pipes = GENMASK(crtc->pipe + 1, crtc->pipe);
> > @@ -2263,7 +2309,11 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  	 */
> >  	joiner_needs_dsc = DISPLAY_VER(i915) < 13 && pipe_config->bigjoiner_pipes;
> >  
> > -	dsc_needed = joiner_needs_dsc || intel_dp->force_dsc_en;
> > +	dsc_needed = joiner_needs_dsc || intel_dp->force_dsc_en ||
> > +		     !intel_dp_compute_config_limits(intel_dp, pipe_config,
> > +						     respect_downstream_limits,
> > +						     false,
> > +						     &limits);
> >  
> >  	if (!dsc_needed) {
> >  		/*
> > @@ -2280,6 +2330,13 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >  		drm_dbg_kms(&i915->drm, "Try DSC (fallback=%s, joiner=%s, force=%s)\n",
> >  			    str_yes_no(ret), str_yes_no(joiner_needs_dsc),
> >  			    str_yes_no(intel_dp->force_dsc_en));
> > +
> > +		if (!intel_dp_compute_config_limits(intel_dp, pipe_config,
> > +						    respect_downstream_limits,
> > +						    true,
> > +						    &limits))
> > +			return -EINVAL;
> > +
> >  		ret = intel_dp_dsc_compute_config(intel_dp, pipe_config,
> >  						  conn_state, &limits, 64, true);
> >  		if (ret < 0)
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> > index ebc7f4e60c777..0b8a4bbef8f7d 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> > @@ -153,4 +153,10 @@ void intel_dp_phy_test(struct intel_encoder *encoder);
> >  void intel_dp_wait_source_oui(struct intel_dp *intel_dp);
> >  int intel_dp_output_bpp(enum intel_output_format output_format, int bpp);
> >  
> > +bool
> > +intel_dp_compute_config_link_bpp_limits(struct intel_dp *intel_dp,
> > +					const struct intel_crtc_state *crtc_state,
> > +					bool dsc,
> > +					struct link_config_limits *limits);
> > +
> >  #endif /* __INTEL_DP_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > index c077b999ccb74..2b78a3a8966f3 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> > @@ -308,9 +308,10 @@ static bool intel_dp_mst_has_audio(const struct drm_connector_state *conn_state)
> >  		return intel_conn_state->force_audio == HDMI_AUDIO_ON;
> >  }
> >  
> > -static void
> > +static bool
> >  intel_dp_mst_compute_config_limits(struct intel_dp *intel_dp,
> >  				   struct intel_crtc_state *crtc_state,
> > +				   bool dsc,
> >  				   struct link_config_limits *limits)
> >  {
> >  	/*
> > @@ -336,8 +337,10 @@ intel_dp_mst_compute_config_limits(struct intel_dp *intel_dp,
> >  
> >  	intel_dp_adjust_compliance_config(intel_dp, crtc_state, limits);
> >  
> > -	limits->link.min_bpp_x16 = to_bpp_x16(limits->pipe.min_bpp);
> > -	limits->link.max_bpp_x16 = to_bpp_x16(limits->pipe.max_bpp);
> > +	return intel_dp_compute_config_link_bpp_limits(intel_dp,
> > +						       crtc_state,
> > +						       dsc,
> > +						       limits);
> >  }
> >  
> >  static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> > @@ -364,9 +367,11 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  		intel_dp_mst_has_audio(conn_state) &&
> >  		intel_audio_compute_config(encoder, pipe_config, conn_state);
> >  
> > -	intel_dp_mst_compute_config_limits(intel_dp, pipe_config, &limits);
> > -
> > -	dsc_needed = intel_dp->force_dsc_en;
> > +	dsc_needed = intel_dp->force_dsc_en ||
> > +		     !intel_dp_mst_compute_config_limits(intel_dp,
> > +							 pipe_config,
> > +							 false,
> > +							 &limits);
> >  
> >  	if (!dsc_needed) {
> >  		ret = intel_dp_mst_compute_link_config(encoder, pipe_config,
> > @@ -385,6 +390,12 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder,
> >  			    str_yes_no(ret),
> >  			    str_yes_no(intel_dp->force_dsc_en));
> >  
> > +		if (!intel_dp_mst_compute_config_limits(intel_dp,
> > +							pipe_config,
> > +							true,
> > +							&limits))
> > +			return -EINVAL;
> > +
> >  		/*
> >  		 * FIXME: As bpc is hardcoded to 8, as mentioned above,
> >  		 * WARN and ignore the debug flag force_dsc_bpc for now.
> > -- 
> > 2.37.2
> 
> -- 
> Ville Syrjälä
> Intel


More information about the Intel-gfx mailing list