Em Sex, 2016-09-09 às 13:30 +0530, Kumar, Mahesh escreveu:
> From: Mahesh Kumar <mahesh1.ku...@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.ku...@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

Reply via email to