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

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

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@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@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@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.

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