[Intel-gfx] [PATCH 1/6] drm/i915: Introduce proper dbuf state

[Lisovskiy, Stanislav]

On Mon, 2020-02-17 at 16:45 +0200, Ville Syrjälä wrote:
> On Mon, Feb 17, 2020 at 08:46:40AM +0000, Lisovskiy, Stanislav wrote:
> > On Thu, 2020-02-13 at 20:47 +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > 
> > > Add a global state to track the dbuf slices. Gets rid of all the
> > > nasty
> > > coupling between state->modeset and dbuf recompulation. Also we
> > > can
> > > now
> > > totally nuke state->active_pipe_changes.
> > > 
> > > dev_priv->wm.distrust_bios_wm still remains, but should probably
> > > also
> > > get nuked from orbit later. Just didn't spend any significant
> > > time
> > > pondering how to go about. The obvious thing would be to just
> > > recompute
> > > the thing every time.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  69 ++++---
> > >  .../drm/i915/display/intel_display_power.c    |   4 +-
> > >  .../drm/i915/display/intel_display_types.h    |  13 --
> > >  drivers/gpu/drm/i915/i915_drv.h               |  11 +-
> > >  drivers/gpu/drm/i915/intel_pm.c               | 185
> > > ++++++++++++--
> > > ----
> > >  drivers/gpu/drm/i915/intel_pm.h               |  22 +++
> > >  6 files changed, 205 insertions(+), 99 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > index e09d3c93c52b..e331ab900336 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -7558,6 +7558,8 @@ static void
> > > intel_crtc_disable_noatomic(struct
> > > intel_crtc *crtc,
> > >  		to_intel_bw_state(dev_priv->bw_obj.state);
> > >  	struct intel_cdclk_state *cdclk_state =
> > >  		to_intel_cdclk_state(dev_priv->cdclk.obj.state);
> > > +	struct intel_dbuf_state *dbuf_state =
> > > +		to_intel_dbuf_state(dev_priv->dbuf.obj.state);
> > >  	struct intel_crtc_state *crtc_state =
> > >  		to_intel_crtc_state(crtc->base.state);
> > >  	enum intel_display_power_domain domain;
> > > @@ -7630,6 +7632,8 @@ static void
> > > intel_crtc_disable_noatomic(struct
> > > intel_crtc *crtc,
> > >  	cdclk_state->min_voltage_level[pipe] = 0;
> > >  	cdclk_state->active_pipes &= ~BIT(pipe);
> > >  
> > > +	dbuf_state->active_pipes &= ~BIT(pipe);
> > > +
> > 
> > May be I'm wrong(so being not offensive here :) ), but feels kind
> > of
> > redundant to have active_pipes in cdclk_state and at the same time
> > in
> > dbuf_state. Why can't it be still 
> > in some more general state, which is inherited/used/aggregated by
> > those
> > dbuf and cdclk states? Otherwise you will have to do this duplicate
> > code initializations which I see here, for example if there would
> > be
> > even more states you have then three or more similar
> > initializations
> > here,
> > doing the same thing. This pretty much increases probability that
> > somewhere in the code, you will eventually forget to do all
> > initializations(well I guess you understand).
> > Or if you will have to update the behavior, based on active_pipes
> > here
> > somehow, you will also have to change the duplicate code all over
> > the
> > place.
> 
> The problem with putting such things in some central place is that
> then
> we get everything coupled together with said state. You get annoying
> ordering requirements on which order you compute those things etc.
> IMO better to just encapsulate each thing as much as possible. This
> way
> you don't have to think at all what those other states are doing with
> their lives.
> 
> The readout is a bit ugly yes, but we could provide a small helper
> for that. It would still probably have somewhat annoying ordering
> constraints though since we perhaps don't want to do actual readout
> of active_pipes multiple times.

I agree, regarding central place - coupling everything together would
be a step back to what we had before. Another option which we could use
is to not have any "central"  state at all(what you are already
actually doing) but many dedicated "orthogonal" states which contain
only the stuff which is required and related to their semantics.

Like you have bw state and all the stuff _only_ related to that,
dbuf state, sagv state, modeset state, which would have active_pipes 
and so on. Each of those are global state object which you swap 
on commit state. And no central state. Of course you are going to have
a dependencies - but we anyway have them. At least when you have those
as explicit objects, the dependencies can be then explicitly
identified. 
For instance dbuf_state and sagv_state would depend on modeset_state
might require some info from modeset_state however if there hasn't been
any changes we don't have to update modeset_state, but only dbuf_state 
if there were some plane changes. sagv state then would depend on both
dbuf state changes(obviously) and also modeset. In practice it mostly
will mean that we just read necessary info from those, however if those
which we depend on had changed then we certainly need to update
dependent objects as well. 

I.e would be call to have some explicit hiearchy like:

	   (modeset state object changed)
               |                    |
               |                    |
(dbuf/wm state state changed) (bw state changed)
               |                   |
               |                   |
(sagv state changed)         (sagv state changed)

So we would have well identified relashionships regarding
which state needs which information from some states and
also changes in which states require recalculation in other
states.

We anyway can't get rid of those dependencies, they exist even
now - however the problem is that currently they are implicit
and not sometimes clearly visible, which brings problems. 

Stan


>