Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755151AbcJEUd0 (ORCPT ); Wed, 5 Oct 2016 16:33:26 -0400 Received: from mga06.intel.com ([134.134.136.31]:64466 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752869AbcJEUdZ (ORCPT ); Wed, 5 Oct 2016 16:33:25 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,302,1473145200"; d="scan'208";a="17052233" Message-ID: <1475699598.2381.64.camel@intel.com> Subject: Re: [Intel-gfx] [PATCH 4/6] drm/i915/gen9: Make skl_wm_level per-plane From: Paulo Zanoni To: Lyude , intel-gfx@lists.freedesktop.org Cc: David Airlie , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Daniel Vetter Date: Wed, 05 Oct 2016 17:33:18 -0300 In-Reply-To: <1475681598-12081-5-git-send-email-cpaul@redhat.com> References: <1475681598-12081-1-git-send-email-cpaul@redhat.com> <1475681598-12081-5-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: 14071 Lines: 462 Em Qua, 2016-10-05 às 11:33 -0400, Lyude escreveu: > Having skl_wm_level contain all of the watermarks for each plane is > annoying since it prevents us from having any sort of object to > represent a single watermark level, something we take advantage of in > the next commit to cut down on all of the copy paste code in here. I'd like to start my review pointing that I really like this patch. I agree the current form is annoying. See below for some details. > > Signed-off-by: Lyude > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Matt Roper > --- >  drivers/gpu/drm/i915/i915_drv.h  |   6 +- >  drivers/gpu/drm/i915/intel_drv.h |   6 +- >  drivers/gpu/drm/i915/intel_pm.c  | 208 +++++++++++++++++---------- > ------------ >  3 files changed, 100 insertions(+), 120 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index d26e5999..0f97d43 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1648,9 +1648,9 @@ struct skl_wm_values { >  }; >   >  struct skl_wm_level { > - bool plane_en[I915_MAX_PLANES]; > - uint16_t plane_res_b[I915_MAX_PLANES]; > - uint8_t plane_res_l[I915_MAX_PLANES]; > + bool plane_en; > + uint16_t plane_res_b; > + uint8_t plane_res_l; >  }; >   >  /* > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 35ba282..d684f4f 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -468,9 +468,13 @@ struct intel_pipe_wm { >   bool sprites_scaled; >  }; >   > -struct skl_pipe_wm { > +struct skl_plane_wm { >   struct skl_wm_level wm[8]; >   struct skl_wm_level trans_wm; > +}; > + > +struct skl_pipe_wm { > + struct skl_plane_wm planes[I915_MAX_PLANES]; >   uint32_t linetime; >  }; >   > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index af96888..250f12d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3668,67 +3668,52 @@ static int >  skl_compute_wm_level(const struct drm_i915_private *dev_priv, >        struct skl_ddb_allocation *ddb, >        struct intel_crtc_state *cstate, > +      struct intel_plane *intel_plane, >        int level, >        struct skl_wm_level *result) >  { >   struct drm_atomic_state *state = cstate->base.state; >   struct intel_crtc *intel_crtc = to_intel_crtc(cstate- > >base.crtc); > - struct drm_plane *plane; > - struct intel_plane *intel_plane; > - struct intel_plane_state *intel_pstate; > + struct drm_plane *plane = &intel_plane->base; > + struct intel_plane_state *intel_pstate = NULL; >   uint16_t ddb_blocks; >   enum pipe pipe = intel_crtc->pipe; >   int ret; > + int i = skl_wm_plane_id(intel_plane); > + > + if (state) > + intel_pstate = > + intel_atomic_get_existing_plane_state(state, > +       intel_ > plane); >   >   /* > -  * We'll only calculate watermarks for planes that are > actually > -  * enabled, so make sure all other planes are set as > disabled. > +  * Note: If we start supporting multiple pending atomic > commits against > +  * the same planes/CRTC's in the future, plane->state will > no longer be > +  * the correct pre-state to use for the calculations here > and we'll > +  * need to change where we get the 'unchanged' plane data > from. > +  * > +  * For now this is fine because we only allow one queued > commit against > +  * a CRTC.  Even if the plane isn't modified by this > transaction and we > +  * don't have a plane lock, we still have the CRTC's lock, > so we know > +  * that no other transactions are racing with us to update > it. >    */ > - memset(result, 0, sizeof(*result)); > - > - for_each_intel_plane_mask(&dev_priv->drm, > -   intel_plane, > -   cstate->base.plane_mask) { > - int i = skl_wm_plane_id(intel_plane); > - > - plane = &intel_plane->base; > - intel_pstate = NULL; > - if (state) > - intel_pstate = > - intel_atomic_get_existing_plane_stat > e(state, > -      >   intel_plane); > + if (!intel_pstate) > + intel_pstate = to_intel_plane_state(plane->state); >   > - /* > -  * Note: If we start supporting multiple pending > atomic commits > -  * against the same planes/CRTC's in the future, > plane->state > -  * will no longer be the correct pre-state to use > for the > -  * calculations here and we'll need to change where > we get the > -  * 'unchanged' plane data from. > -  * > -  * For now this is fine because we only allow one > queued commit > -  * against a CRTC.  Even if the plane isn't modified > by this > -  * transaction and we don't have a plane lock, we > still have > -  * the CRTC's lock, so we know that no other > transactions are > -  * racing with us to update it. > -  */ > - if (!intel_pstate) > - intel_pstate = to_intel_plane_state(plane- > >state); > + WARN_ON(!intel_pstate->base.fb); >   > - WARN_ON(!intel_pstate->base.fb); > + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]); >   > - ddb_blocks = skl_ddb_entry_size(&ddb- > >plane[pipe][i]); > - > - ret = skl_compute_plane_wm(dev_priv, > -    cstate, > -    intel_pstate, > -    ddb_blocks, > -    level, > -    &result->plane_res_b[i], > -    &result->plane_res_l[i], > -    &result->plane_en[i]); > - if (ret) > - return ret; > - } > + ret = skl_compute_plane_wm(dev_priv, > +    cstate, > +    intel_pstate, > +    ddb_blocks, > +    level, > +    &result->plane_res_b, > +    &result->plane_res_l, > +    &result->plane_en); > + if (ret) > + return ret; >   >   return 0; >  } > @@ -3749,19 +3734,11 @@ skl_compute_linetime_wm(struct > intel_crtc_state *cstate) >  static void skl_compute_transition_wm(struct intel_crtc_state > *cstate, >         struct skl_wm_level *trans_wm > /* out */) >  { > - struct drm_crtc *crtc = cstate->base.crtc; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_plane *intel_plane; > - >   if (!cstate->base.active) >   return; >   >   /* Until we know more, just disable transition WMs */ > - for_each_intel_plane_on_crtc(crtc->dev, intel_crtc, > intel_plane) { > - int i = skl_wm_plane_id(intel_plane); > - > - trans_wm->plane_en[i] = false; > - } > + trans_wm->plane_en = false; >  } >   >  static int skl_build_pipe_wm(struct intel_crtc_state *cstate, > @@ -3770,19 +3747,33 @@ static int skl_build_pipe_wm(struct > intel_crtc_state *cstate, >  { >   struct drm_device *dev = cstate->base.crtc->dev; >   const struct drm_i915_private *dev_priv = to_i915(dev); > + struct intel_plane *intel_plane; > + struct skl_plane_wm *wm; >   int level, max_level = ilk_wm_max_level(dev); >   int ret; >   > - for (level = 0; level <= max_level; level++) { > - ret = skl_compute_wm_level(dev_priv, ddb, cstate, > -    level, &pipe_wm- > >wm[level]); > - if (ret) > - return ret; > + /* > +  * We'll only calculate watermarks for planes that are > actually > +  * enabled, so make sure all other planes are set as > disabled. > +  */ > + memset(pipe_wm->planes, 0, sizeof(pipe_wm->planes)); > + > + for_each_intel_plane_mask(&dev_priv->drm, > +   intel_plane, > +   cstate->base.plane_mask) { > + wm = &pipe_wm->planes[skl_wm_plane_id(intel_plane)]; > + > + for (level = 0; level <= max_level; level++) { > + ret = skl_compute_wm_level(dev_priv, ddb, > cstate, > +    intel_plane, > level, > +    &wm->wm[level]); > + if (ret) > + return ret; > + } > + skl_compute_transition_wm(cstate, &wm->trans_wm); >   } >   pipe_wm->linetime = skl_compute_linetime_wm(cstate); >   > - skl_compute_transition_wm(cstate, &pipe_wm->trans_wm); > - >   return 0; >  } >   > @@ -3792,50 +3783,56 @@ static void skl_compute_wm_results(struct > drm_device *dev, >      struct intel_crtc *intel_crtc) >  { >   int level, max_level = ilk_wm_max_level(dev); > + struct skl_plane_wm *plane_wm; >   enum pipe pipe = intel_crtc->pipe; >   uint32_t temp; >   int i; >   > - for (level = 0; level <= max_level; level++) { > - for (i = 0; i < intel_num_planes(intel_crtc); i++) { > + for (i = 0; i < intel_num_planes(intel_crtc); i++) { > + plane_wm = &p_wm->planes[i]; > + > + for (level = 0; level <= max_level; level++) { >   temp = 0; >   > - temp |= p_wm->wm[level].plane_res_l[i] << > + temp |= plane_wm->wm[level].plane_res_l << >   PLANE_WM_LINES_SHIFT; > - temp |= p_wm->wm[level].plane_res_b[i]; > - if (p_wm->wm[level].plane_en[i]) > + temp |= plane_wm->wm[level].plane_res_b; > + if (plane_wm->wm[level].plane_en) >   temp |= PLANE_WM_EN; >   >   r->plane[pipe][i][level] = temp; >   } >   > - temp = 0; > - > - temp |= p_wm->wm[level].plane_res_l[PLANE_CURSOR] << > PLANE_WM_LINES_SHIFT; > - temp |= p_wm->wm[level].plane_res_b[PLANE_CURSOR]; > + } >   > - if (p_wm->wm[level].plane_en[PLANE_CURSOR]) > + for (level = 0; level <= max_level; level++) { > + plane_wm = &p_wm->planes[PLANE_CURSOR]; > + temp = 0; > + temp |= plane_wm->wm[level].plane_res_l << > PLANE_WM_LINES_SHIFT; > + temp |= plane_wm->wm[level].plane_res_b; > + if (plane_wm->wm[level].plane_en) >   temp |= PLANE_WM_EN; >   >   r->plane[pipe][PLANE_CURSOR][level] = temp; > - >   } >   >   /* transition WMs */ >   for (i = 0; i < intel_num_planes(intel_crtc); i++) { > + plane_wm = &p_wm->planes[i]; >   temp = 0; > - temp |= p_wm->trans_wm.plane_res_l[i] << > PLANE_WM_LINES_SHIFT; > - temp |= p_wm->trans_wm.plane_res_b[i]; > - if (p_wm->trans_wm.plane_en[i]) > + temp |= plane_wm->trans_wm.plane_res_l << > PLANE_WM_LINES_SHIFT; > + temp |= plane_wm->trans_wm.plane_res_b; > + if (plane_wm->trans_wm.plane_en) >   temp |= PLANE_WM_EN; >   >   r->plane_trans[pipe][i] = temp; >   } >   > + plane_wm = &p_wm->planes[PLANE_CURSOR]; >   temp = 0; > - temp |= p_wm->trans_wm.plane_res_l[PLANE_CURSOR] << > PLANE_WM_LINES_SHIFT; > - temp |= p_wm->trans_wm.plane_res_b[PLANE_CURSOR]; > - if (p_wm->trans_wm.plane_en[PLANE_CURSOR]) > + temp |= plane_wm->trans_wm.plane_res_l << > PLANE_WM_LINES_SHIFT; > + temp |= plane_wm->trans_wm.plane_res_b; > + if (plane_wm->trans_wm.plane_en) >   temp |= PLANE_WM_EN; >   >   r->plane_trans[pipe][PLANE_CURSOR] = temp; > @@ -4262,44 +4259,24 @@ static void ilk_optimize_watermarks(struct > intel_crtc_state *cstate) >  static void skl_pipe_wm_active_state(uint32_t val, >        struct skl_pipe_wm *active, >        bool is_transwm, > -      bool is_cursor, >        int i, >        int level) >  { > + struct skl_plane_wm *plane_wm = &active->planes[i]; >   bool is_enabled = (val & PLANE_WM_EN) != 0; >   >   if (!is_transwm) { > - if (!is_cursor) { > - active->wm[level].plane_en[i] = is_enabled; > - active->wm[level].plane_res_b[i] = > - val & PLANE_WM_BLOCKS_MASK; > - active->wm[level].plane_res_l[i] = > - (val >> > PLANE_WM_LINES_SHIFT) & > - PLANE_WM_LINES_MASK; > - } else { > - active->wm[level].plane_en[PLANE_CURSOR] = > is_enabled; > - active->wm[level].plane_res_b[PLANE_CURSOR] > = > - val & PLANE_WM_BLOCKS_MASK; > - active->wm[level].plane_res_l[PLANE_CURSOR] > = > - (val >> > PLANE_WM_LINES_SHIFT) & > - PLANE_WM_LINES_MASK; > - } > + plane_wm->wm[level].plane_en = is_enabled; > + plane_wm->wm[level].plane_res_b = val & > PLANE_WM_BLOCKS_MASK; > + plane_wm->wm[level].plane_res_l = > + (val >> PLANE_WM_LINES_SHIFT) & > + PLANE_WM_LINES_MASK; Nitpick: you can join the two lines above and still stay under 80 columns. >   } else { > - if (!is_cursor) { > - active->trans_wm.plane_en[i] = is_enabled; > - active->trans_wm.plane_res_b[i] = > - val & PLANE_WM_BLOCKS_MASK; > - active->trans_wm.plane_res_l[i] = > - (val >> > PLANE_WM_LINES_SHIFT) & > - PLANE_WM_LINES_MASK; > - } else { > - active->trans_wm.plane_en[PLANE_CURSOR] = > is_enabled; > - active->trans_wm.plane_res_b[PLANE_CURSOR] = > - val & PLANE_WM_BLOCKS_MASK; > - active->trans_wm.plane_res_l[PLANE_CURSOR] = > - (val >> > PLANE_WM_LINES_SHIFT) & > - PLANE_WM_LINES_MASK; > - } > + plane_wm->trans_wm.plane_en = is_enabled; > + plane_wm->trans_wm.plane_res_b = val & > PLANE_WM_BLOCKS_MASK; > + plane_wm->trans_wm.plane_res_l = > + (val >> PLANE_WM_LINES_SHIFT) & > + PLANE_WM_LINES_MASK; Same here. >   } >  } >   > @@ -4338,20 +4315,19 @@ static void skl_pipe_wm_get_hw_state(struct > drm_crtc *crtc) >   for (level = 0; level <= max_level; level++) { >   for (i = 0; i < intel_num_planes(intel_crtc); i++) { >   temp = hw->plane[pipe][i][level]; > - skl_pipe_wm_active_state(temp, active, > false, > - false, i, level); > + skl_pipe_wm_active_state(temp, active, > false, i, level); >   } >   temp = hw->plane[pipe][PLANE_CURSOR][level]; > - skl_pipe_wm_active_state(temp, active, false, true, > i, level); > + skl_pipe_wm_active_state(temp, active, false, i, > level); While this is not wrong today, history shows that the number of planes increases over time, so we may at some point in the future add PLANE_D and more, so the code will become wrong. Just pass PLANE_CURSOR instead of "i" here and below. Also, this simplification could have been a separate patch. Everything else looks correct, so if you fix the detail above I'll provide a r-b tag. >   } >   >   for (i = 0; i < intel_num_planes(intel_crtc); i++) { >   temp = hw->plane_trans[pipe][i]; > - skl_pipe_wm_active_state(temp, active, true, false, > i, 0); > + skl_pipe_wm_active_state(temp, active, true, i, 0); >   } >   >   temp = hw->plane_trans[pipe][PLANE_CURSOR]; > - skl_pipe_wm_active_state(temp, active, true, true, i, 0); > + skl_pipe_wm_active_state(temp, active, true, i, 0); >   >   intel_crtc->wm.active.skl = *active; >  }