Em Qui, 2018-10-18 às 16:55 +0300, Ville Syrjälä escreveu:
> On Tue, Oct 16, 2018 at 03:01:30PM -0700, Paulo Zanoni wrote:
> > Print a more generic "failed to compute watermark levels" whenever
> > any
> > of skl_compute_wm_levels() fail, and print only the specific error
> > message for the specific cases. This allows us to stop passing
> > pstate
> > everywhere, making the watermarks computation code a little less
> > dependent on random intel state structs.
>
> Nothing random about those structs. They are *the* state.
What I'm about to say is all probably a reflex of my own incompetence
to understand the flows of our code, but here it goes.
1. There's duplication in those structs. At any given point of our
source code, should you use state structs passed as parameters, or
should you use object->state (e.g., intel_crtc->state)? Sometimes one
thing is the new state and the other thing is the old state, sometimes
it is the opposite, and checking which one is which is never trivial. I
always have to go back to intel_display.c and try to find that point in
the modeset where we assign crtc_config to crtc->config and then try to
figure out if my code runs before or after that point. And I'm never
100% confident I'm using the correct struct.
2. There's a lot of duplication in members of those structs. There are
like 5 different things that could mean "pixel rate" (for real mode,
for adjusted mode, considering/not-considering scaling, rotation, etc),
there are many things that could mean "source width", etc. So when you
need a specific value, it's not always clear where to get it from in
our driver.
3. The names inside the watermarks calculation page (or anywhere else
in our spec) don't easily translate to any of those struct members
mentioned in item 2. E.g., plane pixel rate.
I mean, look at how many times the spec (not only for watermarks, but
for other stuff too, like FBC and PSR) had wording like "source size"
and we fetched the value from one place, but then we learned that we
needed to fetch the value from this other place that was the same in
most cases except for these X and Y corner cases. You reviewed some of
those patches.
So, to conclude the argument, the nice thing about struct skl_wm_params
is that it allows us to have a single point where we translate "i915
terminology" to "watermarks calculation algorithm terminology", so we
can fix that single place if/when needed. And then everywhere else in
watermarks code we just fetch the value from this struct without having
to worry "at this point in the sequence, where should this value come
from?", allowing us to focus on the algorithm specifically. On top of
that, there's the fact that we only compute things once instead of up
to 8 times per plane (7 levels + transition watermarks).
Of course, the downside is that we address the problem of "too many
places to fetch information from" by introducing a new place where
information is fetched from, so I definitely can't argue that the
current solution is good either. I would really like to know what are
your proposals here: if we decide to remove the params struct, would
you suggest we simply fetch everything directly from the passed structs
and recompute all the values that are computed multiple times? I'm
totally willing to implement something better if you have it in your
head.
> Using them directly rather than duplicating informationa in the
> wm_params struct would probably make the code look a bit less alien.
>
> But given that the information is duplicated I guess we don't have to
> pass in the plane state. So the patch seems fine in that sense.
>
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Thanks for the reviews!
>
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c | 27 ++++++++++++---------------
> > 1 file changed, 12 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index 4053f4a68657..1290efc64869 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4635,13 +4635,11 @@ skl_compute_plane_wm_params(const struct
> > drm_i915_private *dev_priv,
> >
> > static int skl_compute_plane_wm(const struct drm_i915_private
> > *dev_priv,
> > struct intel_crtc_state *cstate,
> > - const struct intel_plane_state
> > *intel_pstate,
> > int level,
> > const struct skl_wm_params *wp,
> > const struct skl_wm_level
> > *result_prev,
> > struct skl_wm_level *result /* out
> > */)
> > {
> > - const struct drm_plane_state *pstate = &intel_pstate-
> > >base;
> > uint32_t latency = dev_priv->wm.skl_latency[level];
> > uint_fixed_16_16_t method1, method2;
> > uint_fixed_16_16_t selected_result;
> > @@ -4763,11 +4761,7 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> > if (level) {
> > return 0;
> > } else {
> > - struct drm_plane *plane = pstate->plane;
> > -
> > - DRM_DEBUG_KMS("Requested display
> > configuration exceeds system watermark limitations\n");
> > - DRM_DEBUG_KMS("[PLANE:%d:%s] blocks
> > required = %u/%u, lines required = %u/31\n",
> > - plane->base.id, plane->name,
> > + DRM_DEBUG_KMS("Requested display
> > configuration exceeds system watermark limitations: blocks required
> > = %u/%u, lines required = %u/31\n",
> > res_blocks, wp->ddb_blocks,
> > res_lines);
> > return -EINVAL;
> > }
> > @@ -4795,7 +4789,6 @@ static int skl_compute_plane_wm(const struct
> > drm_i915_private *dev_priv,
> > static int
> > skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> > struct intel_crtc_state *cstate,
> > - const struct intel_plane_state
> > *intel_pstate,
> > const struct skl_wm_params *wm_params,
> > struct skl_plane_wm *wm,
> > int plane_id)
> > @@ -4816,7 +4809,6 @@ skl_compute_wm_levels(const struct
> > drm_i915_private *dev_priv,
> >
> > ret = skl_compute_plane_wm(dev_priv,
> > cstate,
> > - intel_pstate,
> > level,
> > wm_params,
> > result_prev,
> > @@ -4951,10 +4943,13 @@ static int skl_build_pipe_wm(struct
> > intel_crtc_state *cstate,
> > if (!wm_params.plane_visible)
> > continue;
> >
> > - ret = skl_compute_wm_levels(dev_priv, cstate,
> > - intel_pstate,
> > &wm_params, wm, 0);
> > - if (ret)
> > + ret = skl_compute_wm_levels(dev_priv, cstate,
> > &wm_params, wm,
> > + 0);
> > + if (ret) {
> > + DRM_DEBUG_KMS("[PLANE:%d:%s] failed to
> > compute watermark levels\n",
> > + plane->base.id, plane-
> > >name);
> > return ret;
> > + }
> >
> > skl_compute_transition_wm(cstate, &wm_params, &wm-
> > >wm[0],
> > &wm->trans_wm);
> > @@ -4968,10 +4963,12 @@ static int skl_build_pipe_wm(struct
> > intel_crtc_state *cstate,
> > return ret;
> >
> > ret = skl_compute_wm_levels(dev_priv,
> > cstate,
> > - intel_pstate,
> > &wm_params,
> > - wm, 1);
> > - if (ret)
> > + &wm_params,
> > wm, 1);
> > + if (ret) {
> > + DRM_DEBUG_KMS("[PLANE:%d:%s]
> > failed to compute planar watermark levels\n",
> > + plane->base.id,
> > plane->name);
> > return ret;
> > + }
> > }
> > }
> >
> > --
> > 2.14.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
