Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756448AbcJMUES (ORCPT ); Thu, 13 Oct 2016 16:04:18 -0400 Received: from mga04.intel.com ([192.55.52.120]:17060 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751352AbcJMUEJ (ORCPT ); Thu, 13 Oct 2016 16:04:09 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,489,1473145200"; d="scan'208";a="19323003" Message-ID: <1476389043.2478.30.camel@intel.com> Subject: Re: [Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values From: Paulo Zanoni To: Maarten Lankhorst , Lyude , intel-gfx@lists.freedesktop.org Cc: David Airlie , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Daniel Vetter Date: Thu, 13 Oct 2016 17:04:03 -0300 In-Reply-To: References: <1475885497-6094-1-git-send-email-cpaul@redhat.com> <1475885497-6094-6-git-send-email-cpaul@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.20.5 (3.20.5-1.fc24) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7702 Lines: 186 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 > > 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 > > ~Maarten