Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752409AbcJKThF (ORCPT ); Tue, 11 Oct 2016 15:37:05 -0400 Received: from mga05.intel.com ([192.55.52.43]:60594 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752319AbcJKThD (ORCPT ); Tue, 11 Oct 2016 15:37:03 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,330,1473145200"; d="scan'208";a="19026530" Message-ID: <1476214616.2460.15.camel@intel.com> Subject: Re: [PATCH v2 03/10] drm/i915/gen9: Make skl_wm_level per-plane From: Paulo Zanoni To: Lyude , intel-gfx@lists.freedesktop.org Cc: Ville =?ISO-8859-1?Q?Syrj=E4l=E4?= , Daniel Vetter , Jani Nikula , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Date: Tue, 11 Oct 2016 16:36:56 -0300 In-Reply-To: <1475885497-6094-4-git-send-email-cpaul@redhat.com> References: <1475885497-6094-1-git-send-email-cpaul@redhat.com> <1475885497-6094-4-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: 12967 Lines: 425 Em Sex, 2016-10-07 às 20:11 -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. > > Changes since v1: > - Style nitpicks > - Fix accidental usage of i vs. PLANE_CURSOR > - Split out skl_pipe_wm_active_state simplification into separate > patch > > Signed-off-by: Lyude Reviewed-by: Paulo Zanoni > Reviewed-by: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  drivers/gpu/drm/i915/i915_drv.h  |   6 +- >  drivers/gpu/drm/i915/intel_drv.h |   6 +- >  drivers/gpu/drm/i915/intel_pm.c  | 207 +++++++++++++++++++-------- > ------------ >  3 files changed, 111 insertions(+), 108 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > b/drivers/gpu/drm/i915/i915_drv.h > index e9d035ea..0287c93 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1649,9 +1649,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 cc5d5e9..4c2ebcd 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3670,67 +3670,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)); > + if (!intel_pstate) > + intel_pstate = to_intel_plane_state(plane->state); >   > - 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); > + WARN_ON(!intel_pstate->base.fb); >   > - /* > -  * 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); > + ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]); >   > - WARN_ON(!intel_pstate->base.fb); > - > - 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; >  } > @@ -3751,19 +3736,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, > @@ -3772,19 +3749,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; >  } >   > @@ -3794,50 +3785,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; > @@ -4278,35 +4275,37 @@ static void skl_pipe_wm_active_state(uint32_t > val, >   >   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; > + active->planes[i].wm[level].plane_en = > is_enabled; > + active->planes[i].wm[level].plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active->planes[i].wm[level].plane_res_l = > + (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; > + active- > >planes[PLANE_CURSOR].wm[level].plane_en = > + is_enabled; > + active- > >planes[PLANE_CURSOR].wm[level].plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active- > >planes[PLANE_CURSOR].wm[level].plane_res_l = > + (val >> PLANE_WM_LINES_SHIFT) & > + PLANE_WM_LINES_MASK; >   } >   } 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; > + active->planes[i].trans_wm.plane_en = > is_enabled; > + active->planes[i].trans_wm.plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active->planes[i].trans_wm.plane_res_l = > + (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; > + active- > >planes[PLANE_CURSOR].trans_wm.plane_en = > + is_enabled; > + active- > >planes[PLANE_CURSOR].trans_wm.plane_res_b = > + val & PLANE_WM_BLOCKS_MASK; > + active- > >planes[PLANE_CURSOR].trans_wm.plane_res_l = > + (val >> PLANE_WM_LINES_SHIFT) & > + PLANE_WM_LINES_MASK; >   } >   } >  }