Re: [PATCH v3 1/9] drm/i915/skl: pass pipe_wm in skl_compute_(wm_level/plane_wm) functions

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qua, 2016-09-21 às 19:18 +0530, Mahesh Kumar escreveu:
> there are two main reason I want to pass complete pipe_wm to compute 
> function
> 
> 1. for transition WM calculation, else we need to pass 
> trans_wm.plane_res_b &  trans_wm.plane_res_l also as a parameter,
> 
>      that will increase the parameter-list. And trans_wm will be
> used 
> (calculation will happen) only during level-0. All other levels (1-
> 7) 
> these will be extra unused variables.
> 
>      If we want to calculate transition WM separately (after plane
> WM 
> calculation) there we will have to duplicate some of the code for WM 
> level-0 calculation.
> 
> 2. This will be used in Render compression WA for (Gen9 & CNL) where
> we 
> have to make sure WM level-(1-7) result blocks/lines are >= WM level-
> 0 
> result blocks/lines.

Well, I tried to check against the series and I really thought they
were not necessary.

Considering there's also some discussion whether we want transition WMs
or not (i.e., the HW guys say we don't want), can you please at least
move this patch to be right before the point where it is needed? This
way we'll be able to discuss & merge the others before.

Thanks,
Paulo

> 
> 
> agree, out_ variable should not be local variables.
> If you think above points can be addressed, let me know, will update 
> patches accordingly.
> 
> thanks,
> -Mahesh
> 
> On Tuesday 20 September 2016 05:47 PM, Paulo Zanoni wrote:
> > 
> > Em Sex, 2016-09-09 às 13:30 +0530, Kumar, Mahesh escreveu:
> > > 
> > > From: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > 
> > > This patch make use of plane_wm variable directly instead of
> > > passing
> > > skl_plane_wm struct. this way reduces number of argument
> > > requirement
> > > in watermark calculation functions.
> > > 
> > > It also gives more freedom of decision making to implement Bspec
> > > WM
> > > workarounds.
> > This is just my personal opinion, but I don't think this patch is
> > an
> > improvement to the codebase. The way things are currently
> > organized,
> > there's no risk that we may end up reading some variable that's
> > still
> > not set/computed, so there's less risk that some later patch may
> > end up
> > using information it shouldn't. Also, by having these explicit out
> > parameters, we clearly highlight what's the goal of the function:
> > output those 3 values, instead of writing I-don't-know-what to a
> > huge
> > struct.
> > 
> > Besides, the patch even keeps the out_ variables as local variables
> > instead of parameters, which makes things even more confusing IMHO,
> > since in_ and out_ variables are usually function parameters.
> > 
> > I also see that this patch is not necessary for the series. We
> > kinda
> > use the new pipe_wm variable at patch 9, but we could just go with
> > the
> > variables we have.
> > 
> > So, unless some new arguments are given, I'd suggest to just drop
> > this
> > patch.
> > 
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.kumar@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/intel_pm.c | 29 +++++++++++++++-----------
> > > ---
> > >   1 file changed, 15 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 86c6d66..3fdec4d 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3542,9 +3542,7 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   				struct intel_plane_state
> > > *intel_pstate,
> > >   				uint16_t ddb_allocation,
> > >   				int level,
> > > -				uint16_t *out_blocks, /* out */
> > > -				uint8_t *out_lines, /* out */
> > > -				bool *enabled /* out */)
> > > +				struct skl_pipe_wm *pipe_wm)
> > >   {
> > >   	struct drm_plane_state *pstate = &intel_pstate->base;
> > >   	struct drm_framebuffer *fb = pstate->fb;
> > > @@ -3557,6 +3555,11 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   	uint32_t width = 0, height = 0;
> > >   	uint32_t plane_pixel_rate;
> > >   	uint32_t y_tile_minimum, y_min_scanlines;
> > > +	int id = skl_wm_plane_id(to_intel_plane(pstate->plane));
> > > +	struct skl_wm_level *result = &pipe_wm->wm[level];
> > > +	uint16_t *out_blocks = &result->plane_res_b[id];
> > > +	uint8_t *out_lines = &result->plane_res_l[id];
> > > +	bool *enabled = &result->plane_en[id];
> > >   
> > >   	if (latency == 0 || !cstate->base.active ||
> > > !intel_pstate-
> > > > 
> > > > base.visible) {
> > >   		*enabled = false;
> > > @@ -3673,7 +3676,7 @@ skl_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >   		     struct skl_ddb_allocation *ddb,
> > >   		     struct intel_crtc_state *cstate,
> > >   		     int level,
> > > -		     struct skl_wm_level *result)
> > > +		     struct skl_pipe_wm *pipe_wm)
> > >   {
> > >   	struct drm_atomic_state *state = cstate->base.state;
> > >   	struct intel_crtc *intel_crtc = to_intel_crtc(cstate-
> > > > 
> > > > base.crtc);
> > > @@ -3684,12 +3687,6 @@ skl_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >   	enum pipe pipe = intel_crtc->pipe;
> > >   	int ret;
> > >   
> > > -	/*
> > > -	 * We'll only calculate watermarks for planes that are
> > > actually
> > > -	 * enabled, so make sure all other planes are set as
> > > disabled.
> > > -	 */
> > > -	memset(result, 0, sizeof(*result));
> > > -
> > >   	for_each_intel_plane_mask(&dev_priv->drm,
> > >   				  intel_plane,
> > >   				  cstate->base.plane_mask) {
> > > @@ -3727,9 +3724,7 @@ skl_compute_wm_level(const struct
> > > drm_i915_private *dev_priv,
> > >   					   intel_pstate,
> > >   					   ddb_blocks,
> > >   					   level,
> > > -					   &result-
> > > >plane_res_b[i],
> > > -					   &result-
> > > >plane_res_l[i],
> > > -					   &result-
> > > >plane_en[i]);
> > > +					   pipe_wm);
> > >   		if (ret)
> > >   			return ret;
> > >   	}
> > > @@ -3777,9 +3772,15 @@ static int skl_build_pipe_wm(struct
> > > intel_crtc_state *cstate,
> > >   	int level, max_level = ilk_wm_max_level(dev);
> > >   	int ret;
> > >   
> > > +	/*
> > > +	 * We'll only calculate watermarks for planes that are
> > > actually
> > > +	 * enabled, so make sure all other planes are set as
> > > disabled.
> > > +	 */
> > > +	memset(pipe_wm, 0, sizeof(*pipe_wm));
> > > +
> > >   	for (level = 0; level <= max_level; level++) {
> > >   		ret = skl_compute_wm_level(dev_priv, ddb,
> > > cstate,
> > > -					   level, &pipe_wm-
> > > > 
> > > > wm[level]);
> > > +					   level, pipe_wm);
> > >   		if (ret)
> > >   			return ret;
> > >   	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx