Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values

From: Lyude Paul
Date: Thu Oct 13 2016 - 17:26:12 EST

Next message: Al Viro: "Re: [PATCH] namei: revert old behaviour for filename_lookup with LOOKUP_PARENT flag"
Previous message: Sean Young: "Re: [PATCH] [media] dib0700: Fix uninitialized protocol for NEC repeat codes"
In reply to: Paulo Zanoni: "Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Your is SAGV related, and when we don't make the SAGV happy people's
machines usually hang. So I'm definitely for your patches getting
merged first

On Thu, 2016-10-13 at 17:07 -0300, Paulo Zanoni wrote:
> Em Qui, 2016-10-13 Ãs 17:04 -0300, Paulo Zanoni escreveu:
> >
> > Em Qui, 2016-10-13 Ãs 15:39 +0200, Maarten Lankhorst escreveu:
> > >
> > >
> > > Op 08-10-16 om 02:11 schreef Lyude:
> > > >
> > > >
> > > >
> > > > Now that we've make skl_wm_levels make a little more sense, we
> > > > can
> > > > remove all of the redundant wm information. Up until now we'd
> > > > been
> > > > storing two copies of all of the skl watermarks: one being the
> > > > skl_pipe_wm structs, the other being the global wm struct in
> > > > drm_i915_private containing the raw register values. This is
> > > > confusing
> > > > and problematic, since it means we're prone to accidentally
> > > > letting
> > > > the
> > > > two copies go out of sync. So, get rid of all of the functions
> > > > responsible for computing the register values and just use a
> > > > single
> > > > helper, skl_write_wm_level(), to convert and write the new
> > > > watermarks on
> > > > the fly.
> > > >
> > > > Changes since v1:
> > > > - Fixup skl_write_wm_level()
> > > > - Fixup skl_wm_level_from_reg_val()
> > > > - Don't forget to copy *active to intel_crtc->wm.active.skl
> > > >
> > > > Signed-off-by: Lyude <cpaul@xxxxxxxxxx>
> > > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxx
> > > > om
> > > > >
> > > > >
> > > > Cc: Ville SyrjÃlÃ <ville.syrjala@xxxxxxxxxxxxxxx>
> > > > Cc: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> > > > ---
> > > > Âdrivers/gpu/drm/i915/i915_drv.hÂÂÂÂÂÂ|ÂÂÂ2 -
> > > > Âdrivers/gpu/drm/i915/intel_display.c |ÂÂ14 ++-
> > > > Âdrivers/gpu/drm/i915/intel_drv.hÂÂÂÂÂ|ÂÂÂ6 +-
> > > > Âdrivers/gpu/drm/i915/intel_pm.cÂÂÂÂÂÂ| 204 ++++++++++++-------
> > > > --
> > > > --
> > > > ------------
> > > > Âdrivers/gpu/drm/i915/intel_sprite.cÂÂ|ÂÂÂ8 +-
> > > > Â5 files changed, 90 insertions(+), 144 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > index 0287c93..76583b2 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation {
> > > > Âstruct skl_wm_values {
> > > > Â unsigned dirty_pipes;
> > > > Â struct skl_ddb_allocation ddb;
> > > > - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8];
> > > > - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES];
> > > > Â};
> > > > Â
> > > > Âstruct skl_wm_level {
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index a71d05a..39400a0 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3378,6 +3378,8 @@ static void
> > > > skylake_update_primary_plane(struct drm_plane *plane,
> > > > Â struct intel_crtc *intel_crtc =
> > > > to_intel_crtc(crtc_state-
> > > > >
> > > > >
> > > > > base.crtc);
> > > > Â struct drm_framebuffer *fb = plane_state->base.fb;
> > > > Â const struct skl_wm_values *wm = &dev_priv-
> > > > >
> > > > >
> > > > > wm.skl_results;
> > > > + const struct skl_plane_wm *p_wm =
> > > > + &crtc_state->wm.skl.optimal.planes[0];
> > > > Â int pipe = intel_crtc->pipe;
> > > > Â u32 plane_ctl;
> > > > Â unsigned int rotation = plane_state->base.rotation;
> > > > @@ -3414,7 +3416,7 @@ static void
> > > > skylake_update_primary_plane(struct drm_plane *plane,
> > > > Â intel_crtc->adjusted_y = src_y;
> > > > Â
> > > > Â if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc-
> > > > >base))
> > > > - skl_write_plane_wm(intel_crtc, wm, 0);
> > > > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb,
> > > > 0);
> > > > Â
> > > > Â I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl);
> > > > Â I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) |
> > > > src_x);
> > > > @@ -3448,6 +3450,8 @@ static void
> > > > skylake_disable_primary_plane(struct drm_plane *primary,
> > > > Â struct drm_device *dev = crtc->dev;
> > > > Â struct drm_i915_private *dev_priv = to_i915(dev);
> > > > Â struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > + struct intel_crtc_state *cstate =
> > > > to_intel_crtc_state(crtc->state);
> > > > + const struct skl_plane_wm *p_wm = &cstate-
> > > > >
> > > > >
> > > > > wm.skl.optimal.planes[0];
> > > > Â int pipe = intel_crtc->pipe;
> > > > Â
> > > > Â /*
> > > > @@ -3455,7 +3459,8 @@ static void
> > > > skylake_disable_primary_plane(struct drm_plane *primary,
> > > > Â Â* plane's visiblity isn't actually changing neither
> > > > is
> > > > its watermarks.
> > > > Â Â*/
> > > > Â if (!crtc->primary->state->visible)
> > > > - skl_write_plane_wm(intel_crtc, &dev_priv-
> > > > >
> > > > >
> > > > > wm.skl_results, 0);
> > > > + skl_write_plane_wm(intel_crtc, p_wm,
> > > > + ÂÂÂ&dev_priv-
> > > > >
> > > > > wm.skl_results.ddb,
> > > > 0);
> > > > Â
> > > > Â I915_WRITE(PLANE_CTL(pipe, 0), 0);
> > > > Â I915_WRITE(PLANE_SURF(pipe, 0), 0);
> > > > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct
> > > > drm_crtc *crtc, u32 base,
> > > > Â struct drm_device *dev = crtc->dev;
> > > > Â struct drm_i915_private *dev_priv = to_i915(dev);
> > > > Â struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > > > + struct intel_crtc_state *cstate =
> > > > to_intel_crtc_state(crtc->state);
> > > > Â const struct skl_wm_values *wm = &dev_priv-
> > > > >
> > > > >
> > > > > wm.skl_results;
> > > > + const struct skl_plane_wm *p_wm =
> > > > + &cstate->wm.skl.optimal.planes[PLANE_CURSOR];
> > > > Â int pipe = intel_crtc->pipe;
> > > > Â uint32_t cntl = 0;
> > > > Â
> > > > Â if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes &
> > > > drm_crtc_mask(crtc))
> > > > - skl_write_cursor_wm(intel_crtc, wm);
> > > > + skl_write_cursor_wm(intel_crtc, p_wm, &wm-
> > > > >ddb);
> > > > Â
> > > > Â if (plane_state && plane_state->base.visible) {
> > > > Â cntl = MCURSOR_GAMMA_ENABLE;
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > > > b/drivers/gpu/drm/i915/intel_drv.h
> > > > index d684f4f..958dc72 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1765,9 +1765,11 @@ bool skl_ddb_allocation_equals(const
> > > > struct
> > > > skl_ddb_allocation *old,
> > > > Âbool skl_ddb_allocation_overlaps(struct drm_atomic_state
> > > > *state,
> > > > Â Âstruct intel_crtc
> > > > *intel_crtc);
> > > > Âvoid skl_write_cursor_wm(struct intel_crtc *intel_crtc,
> > > > - Âconst struct skl_wm_values *wm);
> > > > + Âconst struct skl_plane_wm *wm,
> > > > + Âconst struct skl_ddb_allocation
> > > > *ddb);
> > > > Âvoid skl_write_plane_wm(struct intel_crtc *intel_crtc,
> > > > - const struct skl_wm_values *wm,
> > > > + const struct skl_plane_wm *wm,
> > > > + const struct skl_ddb_allocation *ddb,
> > > > Â int plane);
> > > > Âuint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state
> > > > *pipe_config);
> > > > Âbool ilk_disable_lp_wm(struct drm_device *dev);
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 5dbaf12..5cb537c 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -3000,6 +3000,8 @@ bool intel_can_enable_sagv(struct
> > > > drm_atomic_state *state)
> > > > Â struct drm_i915_private *dev_priv = to_i915(dev);
> > > > Â struct intel_atomic_state *intel_state =
> > > > to_intel_atomic_state(state);
> > > > Â struct drm_crtc *crtc;
> > > > + struct intel_crtc_state *cstate;
> > > > + struct skl_plane_wm *wm;
> > > > Â enum pipe pipe;
> > > > Â int level, plane;
> > > > Â
> > > > @@ -3020,18 +3022,21 @@ bool intel_can_enable_sagv(struct
> > > > drm_atomic_state *state)
> > > > Â /* Since we're now guaranteed to only have one active
> > > > CRTC... */
> > > > Â pipe = ffs(intel_state->active_crtcs) - 1;
> > > > Â crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > > > + cstate = intel_atomic_get_crtc_state(state,
> > > > to_intel_crtc(crtc));
> > > Wrong cstate here, should be crtc->state since it's committed and
> > > crtc_state is the old state.
> >
> > Ah... How many instances of the "you're looking at the wrong state
> > struct" have we spotted/fixed ever since the introduction of the
> > atomic
> > code? Can't we try to do something more human-proof in order to
> > avoid
> > these problems from happening again and again?
> >
> > Like for example copying the old states to a specific structure
> > like
> > drm_atomic_state->old_states_which_you_probably_shouldn_be_using,
> > and
> > then setting the CRTC/connector/plane states in the
> > drm_atomic_state
> > struct to NULL so bugs will be much easier to spot?
> >
> > Anyway, with this problem fixed, feel free to add:
> > Reviewed-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> Oh, I forgot to mention: this patch has a conflict with my
> "unconditionally apply the memory workaround" patch, so we may need
> to
> decide which one gets merged first. Of course I'd suggest my patch to
> be merged first since I want it to land in stable, but I'm always
> biased towards my patches. If you agree, I can even volunteer myself
> to
> solve the conflicts of this patch later when applying.
>
> >
> >
> > >
> > >
> > >
> > > ~Maarten
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Next message: Al Viro: "Re: [PATCH] namei: revert old behaviour for filename_lookup with LOOKUP_PARENT flag"
Previous message: Sean Young: "Re: [PATCH] [media] dib0700: Fix uninitialized protocol for NEC repeat codes"
In reply to: Paulo Zanoni: "Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]