Re: [PATCH 07/11] drm/i915: move ddb_blocks to be a watermark parameter

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Oct 22, 2018 at 03:29:22PM -0700, Paulo Zanoni wrote:
> Em Qui, 2018-10-18 às 16:41 +0300, Ville Syrjälä escreveu:
> > On Tue, Oct 16, 2018 at 03:01:29PM -0700, Paulo Zanoni wrote:
> > > The goal of struct skl_wm_params is to cache every watermark
> > > parameter so the other functions can just use them without worrying
> > > about the appropriate place to fetch each parameter requested by
> > > the
> > > spec, and without having to recompute parameters that are used in
> > > different steps of the calculation.
> > > 
> > > The ddb_blocks parameter is one that is used by both the the plane
> > > watermarks and the transition watermarks. Move ddb_blocks to the
> > > parameter struct so we can simplify the code.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_pm.c | 44 ++++++++++++++++-------------
> > > ------------
> > >  2 files changed, 18 insertions(+), 27 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4b1e8471609b..b32d680d9bf0 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1255,6 +1255,7 @@ struct skl_wm_params {
> > >  	bool rc_surface;
> > >  	bool is_planar;
> > >  	uint32_t width;
> > > +	uint16_t ddb_blocks;
> > >  	uint8_t cpp;
> > >  	uint32_t plane_pixel_rate;
> > >  	uint32_t y_min_scanlines;
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index f388bfa99a97..4053f4a68657 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4522,11 +4522,13 @@ static int
> > >  skl_compute_plane_wm_params(const struct drm_i915_private
> > > *dev_priv,
> > >  			    struct intel_crtc_state *cstate,
> > >  			    const struct intel_plane_state
> > > *intel_pstate,
> > > +			    const struct skl_ddb_allocation *ddb,
> > >  			    struct skl_wm_params *wp, int
> > > plane_id)
> > >  {
> > >  	struct intel_plane *plane = to_intel_plane(intel_pstate-
> > > >base.plane);
> > >  	const struct drm_plane_state *pstate = &intel_pstate-
> > > >base;
> > >  	const struct drm_framebuffer *fb = pstate->fb;
> > > +	enum pipe pipe = to_intel_crtc(cstate->base.crtc)->pipe;
> > >  	uint32_t interm_pbpl;
> > >  	struct intel_atomic_state *state =
> > >  		to_intel_atomic_state(cstate->base.state);
> > > @@ -4624,13 +4626,16 @@ skl_compute_plane_wm_params(const struct
> > > drm_i915_private *dev_priv,
> > >  	wp->linetime_us = fixed16_to_u32_round_up(
> > >  					intel_get_linetime_us(csta
> > > te));
> > >  
> > > +	wp->ddb_blocks = plane_id ?
> > 
> > Ugh. That 'plane_id' really should be renamed to something else.
> > 'color_plane' would be the newfangled name I started to use
> > elsewhere.
> 
> I completely agree: there's plane_id and plane->id and they mean
> different things.
> 
> If you're already using color_plane I can totally go with it so we have
> consistency. But, well, for RGB planes, plane 0 does contain color
> information... But can I suggest us to use "subplane" terminology when
> talking about the planes of a plane? What about subplane_index?
> Something that does not use the word "plane" is probably better.

What is "planes of a plane"?

There are drm_planes, hardware display planes (1:1 with drm_plane
for us), and there are the planes of a planar framebuffer (luma
plane, chroma plane, etc.). The latter is what I christened
"color plane".

> 
> > 
> > Patch looks good to me.
> > Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > 
> > > +		     skl_ddb_entry_size(&ddb-
> > > >uv_plane[pipe][plane->id]) :
> > > +		     skl_ddb_entry_size(&ddb->plane[pipe][plane-
> > > >id]);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > >  static int skl_compute_plane_wm(const struct drm_i915_private
> > > *dev_priv,
> > >  				struct intel_crtc_state *cstate,
> > >  				const struct intel_plane_state
> > > *intel_pstate,
> > > -				uint16_t ddb_allocation,
> > >  				int level,
> > >  				const struct skl_wm_params *wp,
> > >  				const struct skl_wm_level
> > > *result_prev,
> > > @@ -4674,7 +4679,7 @@ static int skl_compute_plane_wm(const struct
> > > drm_i915_private *dev_priv,
> > >  		     wp->dbuf_block_size < 1) &&
> > >  		     (wp->plane_bytes_per_line / wp-
> > > >dbuf_block_size < 1)) {
> > >  			selected_result = method2;
> > > -		} else if (ddb_allocation >=
> > > +		} else if (wp->ddb_blocks >=
> > >  			 fixed16_to_u32_round_up(wp-
> > > >plane_blocks_per_line)) {
> > >  			if (INTEL_GEN(dev_priv) == 9 &&
> > >  			    !IS_GEMINILAKE(dev_priv))
> > > @@ -4747,8 +4752,8 @@ static int skl_compute_plane_wm(const struct
> > > drm_i915_private *dev_priv,
> > >  	}
> > >  
> > >  	if ((level > 0 && res_lines > 31) ||
> > > -	    res_blocks >= ddb_allocation ||
> > > -	    min_disp_buf_needed >= ddb_allocation) {
> > > +	    res_blocks >= wp->ddb_blocks ||
> > > +	    min_disp_buf_needed >= wp->ddb_blocks) {
> > >  		result->plane_en = false;
> > >  
> > >  		/*
> > > @@ -4763,7 +4768,7 @@ static int skl_compute_plane_wm(const struct
> > > drm_i915_private *dev_priv,
> > >  			DRM_DEBUG_KMS("Requested display
> > > configuration exceeds system watermark limitations\n");
> > >  			DRM_DEBUG_KMS("[PLANE:%d:%s] blocks
> > > required = %u/%u, lines required = %u/31\n",
> > >  				      plane->base.id, plane->name,
> > > -				      res_blocks, ddb_allocation,
> > > res_lines);
> > > +				      res_blocks, wp->ddb_blocks,
> > > res_lines);
> > >  			return -EINVAL;
> > >  		}
> > >  	}
> > > @@ -4789,26 +4794,15 @@ static int skl_compute_plane_wm(const
> > > struct drm_i915_private *dev_priv,
> > >  
> > >  static int
> > >  skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > > -		      struct skl_ddb_allocation *ddb,
> > >  		      struct intel_crtc_state *cstate,
> > >  		      const struct intel_plane_state
> > > *intel_pstate,
> > >  		      const struct skl_wm_params *wm_params,
> > >  		      struct skl_plane_wm *wm,
> > >  		      int plane_id)
> > >  {
> > > -	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > > >base.crtc);
> > > -	struct drm_plane *plane = intel_pstate->base.plane;
> > > -	struct intel_plane *intel_plane = to_intel_plane(plane);
> > > -	uint16_t ddb_blocks;
> > > -	enum pipe pipe = intel_crtc->pipe;
> > >  	int level, max_level = ilk_wm_max_level(dev_priv);
> > > -	enum plane_id intel_plane_id = intel_plane->id;
> > >  	int ret;
> > >  
> > > -	ddb_blocks = plane_id ?
> > > -		     skl_ddb_entry_size(&ddb-
> > > >uv_plane[pipe][intel_plane_id]) :
> > > -		     skl_ddb_entry_size(&ddb-
> > > >plane[pipe][intel_plane_id]);
> > > -
> > >  	for (level = 0; level <= max_level; level++) {
> > >  		struct skl_wm_level *result = plane_id ? &wm-
> > > >uv_wm[level] :
> > >  							  &wm-
> > > >wm[level];
> > > @@ -4823,7 +4817,6 @@ skl_compute_wm_levels(const struct
> > > drm_i915_private *dev_priv,
> > >  		ret = skl_compute_plane_wm(dev_priv,
> > >  					   cstate,
> > >  					   intel_pstate,
> > > -					   ddb_blocks,
> > >  					   level,
> > >  					   wm_params,
> > >  					   result_prev,
> > > @@ -4863,7 +4856,6 @@ skl_compute_linetime_wm(struct
> > > intel_crtc_state *cstate)
> > >  static void skl_compute_transition_wm(struct intel_crtc_state
> > > *cstate,
> > >  				      struct skl_wm_params *wp,
> > >  				      struct skl_wm_level *wm_l0,
> > > -				      uint16_t ddb_allocation,
> > >  				      struct skl_wm_level
> > > *trans_wm /* out */)
> > >  {
> > >  	struct drm_device *dev = cstate->base.crtc->dev;
> > > @@ -4914,7 +4906,7 @@ static void skl_compute_transition_wm(struct
> > > intel_crtc_state *cstate,
> > >  
> > >  	res_blocks += 1;
> > >  
> > > -	if (res_blocks < ddb_allocation) {
> > > +	if (res_blocks < wp->ddb_blocks) {
> > >  		trans_wm->plane_res_b = res_blocks;
> > >  		trans_wm->plane_en = true;
> > >  		return;
> > > @@ -4947,37 +4939,35 @@ static int skl_build_pipe_wm(struct
> > > intel_crtc_state *cstate,
> > >  						to_intel_plane_sta
> > > te(pstate);
> > >  		enum plane_id plane_id = to_intel_plane(plane)-
> > > >id;
> > >  		struct skl_wm_params wm_params;
> > > -		enum pipe pipe = to_intel_crtc(cstate->base.crtc)-
> > > >pipe;
> > > -		uint16_t ddb_blocks;
> > >  
> > >  		wm = &pipe_wm->planes[plane_id];
> > > -		ddb_blocks = skl_ddb_entry_size(&ddb-
> > > >plane[pipe][plane_id]);
> > >  
> > >  		ret = skl_compute_plane_wm_params(dev_priv,
> > > cstate,
> > > -						  intel_pstate,
> > > &wm_params, 0);
> > > +						  intel_pstate,
> > > ddb,
> > > +						  &wm_params, 0);
> > >  		if (ret)
> > >  			return ret;
> > >  
> > >  		if (!wm_params.plane_visible)
> > >  			continue;
> > >  
> > > -		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> > > +		ret = skl_compute_wm_levels(dev_priv, cstate,
> > >  					    intel_pstate,
> > > &wm_params, wm, 0);
> > >  		if (ret)
> > >  			return ret;
> > >  
> > >  		skl_compute_transition_wm(cstate, &wm_params, &wm-
> > > >wm[0],
> > > -					  ddb_blocks, &wm-
> > > >trans_wm);
> > > +					  &wm->trans_wm);
> > >  
> > >  		/* uv plane watermarks must also be validated for
> > > NV12/Planar */
> > >  		if (wm_params.is_planar) {
> > >  			ret =
> > > skl_compute_plane_wm_params(dev_priv, cstate,
> > > -							  intel_ps
> > > tate,
> > > +							  intel_ps
> > > tate, ddb,
> > >  							  &wm_para
> > > ms, 1);
> > >  			if (ret)
> > >  				return ret;
> > >  
> > > -			ret = skl_compute_wm_levels(dev_priv, ddb,
> > > cstate,
> > > +			ret = skl_compute_wm_levels(dev_priv,
> > > cstate,
> > >  						    intel_pstate,
> > > &wm_params,
> > >  						    wm, 1);
> > >  			if (ret)
> > > -- 
> > > 2.14.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux