From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH v18 2/8] drm/i915: Introduce skl_plane_wm_level accessor.
Date: Thu, 27 Feb 2020 17:51:52 +0200 [thread overview]
Message-ID: <20200227155152.GP13686@intel.com> (raw)
In-Reply-To: <20200224153240.9047-3-stanislav.lisovskiy@intel.com>
On Mon, Feb 24, 2020 at 05:32:34PM +0200, Stanislav Lisovskiy wrote:
> For future Gen12 SAGV implementation we need to
> seemlessly alter wm levels calculated, depending
> on whether we are allowed to enable SAGV or not.
>
> So this accessor will give additional flexibility
> to do that.
>
> Currently this accessor is still simply working
> as "pass-through" function. This will be changed
> in next coming patches from this series.
>
> v2: - plane_id -> plane->id(Ville Syrjälä)
When did I say that? Can't find a previous review of this patch.
Anywyas, that change seems to cause a lot of needless noise into the
patch, and atm I can't see why we'd require it.
> - Moved wm_level var to have more local scope
> (Ville Syrjälä)
> - Renamed yuv to color_plane(Ville Syrjälä) in
> skl_plane_wm_level
>
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> drivers/gpu/drm/i915/intel_pm.c | 120 +++++++++++++++++++++-----------
> 1 file changed, 81 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d6933e382657..e1d167429489 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4548,6 +4548,18 @@ icl_get_total_relative_data_rate(struct intel_crtc_state *crtc_state,
> return total_data_rate;
> }
>
> +static const struct skl_wm_level *
> +skl_plane_wm_level(struct intel_plane *plane,
> + const struct intel_crtc_state *crtc_state,
nit: I'd put the crtc_state as the first parameter as that's the thing
we're operating on. The other stuff just specifies which piece we want
to dig out.
> + int level,
> + int color_plane)
> +{
> + const struct skl_plane_wm *wm =
> + &crtc_state->wm.skl.optimal.planes[plane->id];
> +
> + return color_plane ? &wm->uv_wm[level] : &wm->wm[level];
> +}
> +
> static int
> skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> {
> @@ -4560,7 +4572,7 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> u16 total[I915_MAX_PLANES] = {};
> u16 uv_total[I915_MAX_PLANES] = {};
> u64 total_data_rate;
> - enum plane_id plane_id;
> + struct intel_plane *plane;
> int num_active;
> u64 plane_data_rate[I915_MAX_PLANES] = {};
> u64 uv_plane_data_rate[I915_MAX_PLANES] = {};
> @@ -4612,22 +4624,28 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> */
> for (level = ilk_wm_max_level(dev_priv); level >= 0; level--) {
> blocks = 0;
> - for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> - const struct skl_plane_wm *wm =
> - &crtc_state->wm.skl.optimal.planes[plane_id];
>
> - if (plane_id == PLANE_CURSOR) {
> - if (wm->wm[level].min_ddb_alloc > total[PLANE_CURSOR]) {
> + for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) {
> + const struct skl_wm_level *wm_level;
> + const struct skl_wm_level *wm_uv_level;
> +
> + wm_level = skl_plane_wm_level(plane, crtc_state,
> + level, false);
> + wm_uv_level = skl_plane_wm_level(plane, crtc_state,
> + level, true);
false/true aren't particularly sensible color plane indices.
> +
> + if (plane->id == PLANE_CURSOR) {
> + if (wm_level->min_ddb_alloc > total[PLANE_CURSOR]) {
> drm_WARN_ON(&dev_priv->drm,
> - wm->wm[level].min_ddb_alloc != U16_MAX);
> + wm_level->min_ddb_alloc != U16_MAX);
> blocks = U32_MAX;
> break;
> }
> continue;
> }
>
> - blocks += wm->wm[level].min_ddb_alloc;
> - blocks += wm->uv_wm[level].min_ddb_alloc;
> + blocks += wm_level->min_ddb_alloc;
> + blocks += wm_uv_level->min_ddb_alloc;
> }
>
> if (blocks <= alloc_size) {
> @@ -4649,13 +4667,18 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> * watermark level, plus an extra share of the leftover blocks
> * proportional to its relative data rate.
> */
> - for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> - const struct skl_plane_wm *wm =
> - &crtc_state->wm.skl.optimal.planes[plane_id];
> + for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) {
> + const struct skl_wm_level *wm_level;
> + const struct skl_wm_level *wm_uv_level;
> u64 rate;
> u16 extra;
>
> - if (plane_id == PLANE_CURSOR)
> + wm_level = skl_plane_wm_level(plane, crtc_state,
> + level, false);
> + wm_uv_level = skl_plane_wm_level(plane, crtc_state,
> + level, true);
> +
> + if (plane->id == PLANE_CURSOR)
> continue;
>
> /*
> @@ -4665,22 +4688,22 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> if (total_data_rate == 0)
> break;
>
> - rate = plane_data_rate[plane_id];
> + rate = plane_data_rate[plane->id];
> extra = min_t(u16, alloc_size,
> DIV64_U64_ROUND_UP(alloc_size * rate,
> total_data_rate));
> - total[plane_id] = wm->wm[level].min_ddb_alloc + extra;
> + total[plane->id] = wm_level->min_ddb_alloc + extra;
> alloc_size -= extra;
> total_data_rate -= rate;
>
> if (total_data_rate == 0)
> break;
>
> - rate = uv_plane_data_rate[plane_id];
> + rate = uv_plane_data_rate[plane->id];
> extra = min_t(u16, alloc_size,
> DIV64_U64_ROUND_UP(alloc_size * rate,
> total_data_rate));
> - uv_total[plane_id] = wm->uv_wm[level].min_ddb_alloc + extra;
> + uv_total[plane->id] = wm_uv_level->min_ddb_alloc + extra;
> alloc_size -= extra;
> total_data_rate -= rate;
> }
> @@ -4688,29 +4711,29 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
>
> /* Set the actual DDB start/end points for each plane */
> start = alloc->start;
> - for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) {
> struct skl_ddb_entry *plane_alloc =
> - &crtc_state->wm.skl.plane_ddb_y[plane_id];
> + &crtc_state->wm.skl.plane_ddb_y[plane->id];
> struct skl_ddb_entry *uv_plane_alloc =
> - &crtc_state->wm.skl.plane_ddb_uv[plane_id];
> + &crtc_state->wm.skl.plane_ddb_uv[plane->id];
>
> - if (plane_id == PLANE_CURSOR)
> + if (plane->id == PLANE_CURSOR)
> continue;
>
> /* Gen11+ uses a separate plane for UV watermarks */
> drm_WARN_ON(&dev_priv->drm,
> - INTEL_GEN(dev_priv) >= 11 && uv_total[plane_id]);
> + INTEL_GEN(dev_priv) >= 11 && uv_total[plane->id]);
>
> /* Leave disabled planes at (0,0) */
> - if (total[plane_id]) {
> + if (total[plane->id]) {
> plane_alloc->start = start;
> - start += total[plane_id];
> + start += total[plane->id];
> plane_alloc->end = start;
> }
>
> - if (uv_total[plane_id]) {
> + if (uv_total[plane->id]) {
> uv_plane_alloc->start = start;
> - start += uv_total[plane_id];
> + start += uv_total[plane->id];
> uv_plane_alloc->end = start;
> }
> }
> @@ -4722,9 +4745,16 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> * that aren't actually possible.
> */
> for (level++; level <= ilk_wm_max_level(dev_priv); level++) {
> - for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) {
> + const struct skl_wm_level *wm_level;
> + const struct skl_wm_level *wm_uv_level;
> struct skl_plane_wm *wm =
> - &crtc_state->wm.skl.optimal.planes[plane_id];
> + &crtc_state->wm.skl.optimal.planes[plane->id];
> +
> + wm_level = skl_plane_wm_level(plane, crtc_state,
> + level, false);
> + wm_uv_level = skl_plane_wm_level(plane, crtc_state,
> + level, true);
>
> /*
> * We only disable the watermarks for each plane if
> @@ -4738,9 +4768,10 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> * planes must be enabled before the level will be used."
> * So this is actually safe to do.
> */
> - if (wm->wm[level].min_ddb_alloc > total[plane_id] ||
> - wm->uv_wm[level].min_ddb_alloc > uv_total[plane_id])
> - memset(&wm->wm[level], 0, sizeof(wm->wm[level]));
> + if (wm_level->min_ddb_alloc > total[plane->id] ||
> + wm_uv_level->min_ddb_alloc > uv_total[plane->id])
> + memset(&wm->wm[level], 0,
> + sizeof(struct skl_wm_level));
>
> /*
> * Wa_1408961008:icl, ehl
> @@ -4748,9 +4779,14 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> */
> if (IS_GEN(dev_priv, 11) &&
> level == 1 && wm->wm[0].plane_en) {
> - wm->wm[level].plane_res_b = wm->wm[0].plane_res_b;
> - wm->wm[level].plane_res_l = wm->wm[0].plane_res_l;
> - wm->wm[level].ignore_lines = wm->wm[0].ignore_lines;
> + wm_level = skl_plane_wm_level(plane, crtc_state,
> + 0, false);
> + wm->wm[level].plane_res_b =
> + wm_level->plane_res_b;
> + wm->wm[level].plane_res_l =
> + wm_level->plane_res_l;
> + wm->wm[level].ignore_lines =
> + wm_level->ignore_lines;
> }
> }
> }
> @@ -4759,11 +4795,11 @@ skl_allocate_pipe_ddb(struct intel_crtc_state *crtc_state)
> * Go back and disable the transition watermark if it turns out we
> * don't have enough DDB blocks for it.
> */
> - for_each_plane_id_on_crtc(intel_crtc, plane_id) {
> + for_each_intel_plane_on_crtc(&dev_priv->drm, intel_crtc, plane) {
> struct skl_plane_wm *wm =
> - &crtc_state->wm.skl.optimal.planes[plane_id];
> + &crtc_state->wm.skl.optimal.planes[plane->id];
>
> - if (wm->trans_wm.plane_res_b >= total[plane_id])
> + if (wm->trans_wm.plane_res_b >= total[plane->id])
> memset(&wm->trans_wm, 0, sizeof(wm->trans_wm));
> }
>
> @@ -5354,10 +5390,13 @@ void skl_write_plane_wm(struct intel_plane *plane,
> &crtc_state->wm.skl.plane_ddb_y[plane_id];
> const struct skl_ddb_entry *ddb_uv =
> &crtc_state->wm.skl.plane_ddb_uv[plane_id];
> + const struct skl_wm_level *wm_level;
These can be in tighter scope.
>
> for (level = 0; level <= max_level; level++) {
> + wm_level = skl_plane_wm_level(plane, crtc_state, level, false);
> +
> skl_write_wm_level(dev_priv, PLANE_WM(pipe, plane_id, level),
> - &wm->wm[level]);
> + wm_level);
> }
> skl_write_wm_level(dev_priv, PLANE_WM_TRANS(pipe, plane_id),
> &wm->trans_wm);
> @@ -5388,10 +5427,13 @@ void skl_write_cursor_wm(struct intel_plane *plane,
> &crtc_state->wm.skl.optimal.planes[plane_id];
> const struct skl_ddb_entry *ddb =
> &crtc_state->wm.skl.plane_ddb_y[plane_id];
> + const struct skl_wm_level *wm_level;
>
> for (level = 0; level <= max_level; level++) {
> + wm_level = skl_plane_wm_level(plane, crtc_state, level, false);
> +
> skl_write_wm_level(dev_priv, CUR_WM(pipe, level),
> - &wm->wm[level]);
> + wm_level);
> }
> skl_write_wm_level(dev_priv, CUR_WM_TRANS(pipe), &wm->trans_wm);
>
> --
> 2.24.1.485.gad05a3d8e5
--
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2020-02-27 15:52 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-02-24 15:32 [Intel-gfx] [PATCH v18 0/8] Refactor Gen11+ SAGV support Stanislav Lisovskiy
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 1/8] drm/i915: Start passing latency as parameter Stanislav Lisovskiy
2020-02-27 16:28 ` Ville Syrjälä
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 2/8] drm/i915: Introduce skl_plane_wm_level accessor Stanislav Lisovskiy
2020-02-27 15:51 ` Ville Syrjälä [this message]
2020-02-28 12:23 ` Lisovskiy, Stanislav
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 3/8] drm/i915: Add intel_bw_get_*_state helpers Stanislav Lisovskiy
2020-02-27 15:53 ` Ville Syrjälä
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 4/8] drm/i915: Introduce more *_state_changed indicators Stanislav Lisovskiy
2020-02-25 14:57 ` Stanislav Lisovskiy
2020-02-27 16:12 ` Ville Syrjälä
2020-02-28 8:56 ` Lisovskiy, Stanislav
2020-02-28 16:12 ` Ville Syrjälä
2020-02-29 9:34 ` Lisovskiy, Stanislav
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 5/8] drm/i915: Refactor intel_can_enable_sagv Stanislav Lisovskiy
2020-02-25 14:59 ` Stanislav Lisovskiy
2020-02-27 11:46 ` Stanislav Lisovskiy
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 6/8] drm/i915: Added required new PCode commands Stanislav Lisovskiy
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 7/8] drm/i915: Restrict qgv points which don't have enough bandwidth Stanislav Lisovskiy
2020-02-25 15:00 ` Stanislav Lisovskiy
2020-02-27 16:20 ` Ville Syrjälä
2020-03-02 13:15 ` Lisovskiy, Stanislav
2020-02-24 15:32 ` [Intel-gfx] [PATCH v18 8/8] drm/i915: Enable SAGV support for Gen12 Stanislav Lisovskiy
2020-02-24 18:37 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Refactor Gen11+ SAGV support Patchwork
2020-02-24 18:39 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-02-24 19:04 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2020-02-26 22:00 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Refactor Gen11+ SAGV support (rev5) Patchwork
2020-02-26 22:02 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-02-26 22:26 ` [Intel-gfx] ✗ Fi.CI.BAT: failure " Patchwork
2020-02-27 15:33 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Refactor Gen11+ SAGV support (rev6) Patchwork
2020-02-27 15:35 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2020-02-27 15:56 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-02-28 17:22 ` [Intel-gfx] ✗ Fi.CI.IGT: failure " Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200227155152.GP13686@intel.com \
--to=ville.syrjala@linux.intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=stanislav.lisovskiy@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox