Re: [PATCH 08/11] drm/i915: reorganize the error message for invalid watermarks

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Oct 22, 2018 at 03:22:27PM -0700, Paulo Zanoni wrote:
> 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.

That was solved with the mlankhorst iterators. So no more foo->state,
except during readout since we don't currently allocate new states
for that.

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

And Maarten has been busy elimninating crtc->config as well. Not too
many uses left I hope.

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

I don't recall many corner cases. Well, I guess you can count the
cursor as one. But that's about it I think.

Rotation was maybe the one that caused the most confusion but I think
that was because the spec was very vague. One actually has to think
a bit to figure out if it's referring to the fb orientation or the
scanout orientation.

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

That can usually be solved with functions as well.

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

The "let's not recompute the same thing many times" argument may
have some merit. Though most things are perhaps cheap enough to not
matter much. And we could always stick more stuff into the plane/crtc
states.

But I don't have any concrete proposals at this time. I've tried to
stay away from the skl wm code as much as possible so as to not get
an urge to rewrite it :P

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx