Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757265AbcJMV0K (ORCPT ); Thu, 13 Oct 2016 17:26:10 -0400 Received: from mx1.redhat.com ([209.132.183.28]:53798 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756713AbcJMV0B (ORCPT ); Thu, 13 Oct 2016 17:26:01 -0400 Message-ID: <1476393954.24414.2.camel@redhat.com> Subject: Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values From: Lyude Paul To: Paulo Zanoni , Maarten Lankhorst , intel-gfx@lists.freedesktop.org Cc: David Airlie , Daniel Vetter , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Date: Thu, 13 Oct 2016 17:25:54 -0400 In-Reply-To: <1476389242.2478.33.camel@intel.com> References: <1475885497-6094-1-git-send-email-cpaul@redhat.com> <1475885497-6094-6-git-send-email-cpaul@redhat.com> <1476389043.2478.30.camel@intel.com> <1476389242.2478.33.camel@intel.com> Organization: Red Hat Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Thu, 13 Oct 2016 21:25:56 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 9677 Lines: 251 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 > > > > Reviewed-by: Maarten Lankhorst > > > om > > > > > > > > > > > > > > Cc: Ville Syrjälä > > > > Cc: Paulo Zanoni > > > > --- > > > >  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 > > 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@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx