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