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
