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

[Mahesh Kumar <mahesh1.kumar@xxxxxxxxx>]

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.


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@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx