From: "Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>
To: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Nuke intel_bw_calc_min_cdclk()
Date: Tue, 1 Feb 2022 10:52:39 +0200 [thread overview]
Message-ID: <20220201085239.GA9569@intel.com> (raw)
In-Reply-To: <20220118092354.11631-12-ville.syrjala@linux.intel.com>
On Tue, Jan 18, 2022 at 11:23:50AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> intel_bw_calc_min_cdclk() is entirely pointless. All it manages to do is
> somehow conflate the per-pipe min cdclk with dbuf min cdclk. There is no
> (at least documented) dbuf min cdclk limit on pre-skl so let's just get
> rid of all this confusion.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
I think we constantly have a bit contradictional attitude towards such situation.
From one perspective you can say, that those kind of "leagcy" callbacks are
pointless, from the other hand one might say. that we need to have a unified
approach for all platforms and I think we got, some legacy handlers for old
platforms for similar purpose as well.
I'm fine with both approaches, however for example when I was submitting
that patch, I was asked by reviewer to add this kind of legacy callback, so that we have
a "uniform" approach.
We just then need to have some standard agreement on those, which doesn't
depend on today's cosmic radiation levels :)
Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
> drivers/gpu/drm/i915/display/intel_bw.c | 49 ++--------------------
> drivers/gpu/drm/i915/display/intel_bw.h | 1 -
> drivers/gpu/drm/i915/display/intel_cdclk.c | 31 +-------------
> 3 files changed, 5 insertions(+), 76 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> index 93feab671c29..a3f169686f14 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.c
> +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> @@ -715,7 +715,7 @@ static void skl_crtc_calc_dbuf_bw(struct intel_bw_state *bw_state,
> }
> }
>
> -int skl_bw_calc_min_cdclk(struct intel_atomic_state *state)
> +int intel_bw_calc_min_cdclk(struct intel_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> struct intel_bw_state *new_bw_state = NULL;
> @@ -726,6 +726,9 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state)
> enum pipe pipe;
> int i;
>
> + if (DISPLAY_VER(dev_priv) < 9)
> + return 0;
> +
> for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> new_bw_state = intel_atomic_get_bw_state(state);
> if (IS_ERR(new_bw_state))
> @@ -770,50 +773,6 @@ int skl_bw_calc_min_cdclk(struct intel_atomic_state *state)
> return 0;
> }
>
> -int intel_bw_calc_min_cdclk(struct intel_atomic_state *state)
> -{
> - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> - struct intel_bw_state *new_bw_state = NULL;
> - struct intel_bw_state *old_bw_state = NULL;
> - const struct intel_crtc_state *crtc_state;
> - struct intel_crtc *crtc;
> - int min_cdclk = 0;
> - enum pipe pipe;
> - int i;
> -
> - for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> - new_bw_state = intel_atomic_get_bw_state(state);
> - if (IS_ERR(new_bw_state))
> - return PTR_ERR(new_bw_state);
> -
> - old_bw_state = intel_atomic_get_old_bw_state(state);
> - }
> -
> - if (!old_bw_state)
> - return 0;
> -
> - for_each_pipe(dev_priv, pipe) {
> - struct intel_cdclk_state *cdclk_state;
> -
> - cdclk_state = intel_atomic_get_new_cdclk_state(state);
> - if (!cdclk_state)
> - return 0;
> -
> - min_cdclk = max(cdclk_state->min_cdclk[pipe], min_cdclk);
> - }
> -
> - new_bw_state->min_cdclk = min_cdclk;
> -
> - if (new_bw_state->min_cdclk != old_bw_state->min_cdclk) {
> - int ret = intel_atomic_lock_global_state(&new_bw_state->base);
> -
> - if (ret)
> - return ret;
> - }
> -
> - return 0;
> -}
> -
> int intel_bw_atomic_check(struct intel_atomic_state *state)
> {
> struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> index 46c6eecbd917..57eb755d298a 100644
> --- a/drivers/gpu/drm/i915/display/intel_bw.h
> +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> @@ -65,6 +65,5 @@ void intel_bw_crtc_update(struct intel_bw_state *bw_state,
> int icl_pcode_restrict_qgv_points(struct drm_i915_private *dev_priv,
> u32 points_mask);
> int intel_bw_calc_min_cdclk(struct intel_atomic_state *state);
> -int skl_bw_calc_min_cdclk(struct intel_atomic_state *state);
>
> #endif /* __INTEL_BW_H__ */
> diff --git a/drivers/gpu/drm/i915/display/intel_cdclk.c b/drivers/gpu/drm/i915/display/intel_cdclk.c
> index 7e20967307df..078dc6e1ee34 100644
> --- a/drivers/gpu/drm/i915/display/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/display/intel_cdclk.c
> @@ -70,7 +70,6 @@ struct intel_cdclk_funcs {
> void (*set_cdclk)(struct drm_i915_private *i915,
> const struct intel_cdclk_config *cdclk_config,
> enum pipe pipe);
> - int (*bw_calc_min_cdclk)(struct intel_atomic_state *state);
> int (*modeset_calc_cdclk)(struct intel_cdclk_state *state);
> u8 (*calc_voltage_level)(int cdclk);
> };
> @@ -81,12 +80,6 @@ void intel_cdclk_get_cdclk(struct drm_i915_private *dev_priv,
> dev_priv->cdclk_funcs->get_cdclk(dev_priv, cdclk_config);
> }
>
> -static int intel_cdclk_bw_calc_min_cdclk(struct intel_atomic_state *state)
> -{
> - struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> - return dev_priv->cdclk_funcs->bw_calc_min_cdclk(state);
> -}
> -
> static void intel_cdclk_set_cdclk(struct drm_i915_private *dev_priv,
> const struct intel_cdclk_config *cdclk_config,
> enum pipe pipe)
> @@ -2680,7 +2673,7 @@ int intel_cdclk_atomic_check(struct intel_atomic_state *state,
> old_cdclk_state->force_min_cdclk != new_cdclk_state->force_min_cdclk)
> *need_cdclk_calc = true;
>
> - ret = intel_cdclk_bw_calc_min_cdclk(state);
> + ret = intel_bw_calc_min_cdclk(state);
> if (ret)
> return ret;
>
> @@ -3069,7 +3062,6 @@ u32 intel_read_rawclk(struct drm_i915_private *dev_priv)
> static const struct intel_cdclk_funcs tgl_cdclk_funcs = {
> .get_cdclk = bxt_get_cdclk,
> .set_cdclk = bxt_set_cdclk,
> - .bw_calc_min_cdclk = skl_bw_calc_min_cdclk,
> .modeset_calc_cdclk = bxt_modeset_calc_cdclk,
> .calc_voltage_level = tgl_calc_voltage_level,
> };
> @@ -3077,7 +3069,6 @@ static const struct intel_cdclk_funcs tgl_cdclk_funcs = {
> static const struct intel_cdclk_funcs ehl_cdclk_funcs = {
> .get_cdclk = bxt_get_cdclk,
> .set_cdclk = bxt_set_cdclk,
> - .bw_calc_min_cdclk = skl_bw_calc_min_cdclk,
> .modeset_calc_cdclk = bxt_modeset_calc_cdclk,
> .calc_voltage_level = ehl_calc_voltage_level,
> };
> @@ -3085,7 +3076,6 @@ static const struct intel_cdclk_funcs ehl_cdclk_funcs = {
> static const struct intel_cdclk_funcs icl_cdclk_funcs = {
> .get_cdclk = bxt_get_cdclk,
> .set_cdclk = bxt_set_cdclk,
> - .bw_calc_min_cdclk = skl_bw_calc_min_cdclk,
> .modeset_calc_cdclk = bxt_modeset_calc_cdclk,
> .calc_voltage_level = icl_calc_voltage_level,
> };
> @@ -3093,7 +3083,6 @@ static const struct intel_cdclk_funcs icl_cdclk_funcs = {
> static const struct intel_cdclk_funcs bxt_cdclk_funcs = {
> .get_cdclk = bxt_get_cdclk,
> .set_cdclk = bxt_set_cdclk,
> - .bw_calc_min_cdclk = skl_bw_calc_min_cdclk,
> .modeset_calc_cdclk = bxt_modeset_calc_cdclk,
> .calc_voltage_level = bxt_calc_voltage_level,
> };
> @@ -3101,53 +3090,45 @@ static const struct intel_cdclk_funcs bxt_cdclk_funcs = {
> static const struct intel_cdclk_funcs skl_cdclk_funcs = {
> .get_cdclk = skl_get_cdclk,
> .set_cdclk = skl_set_cdclk,
> - .bw_calc_min_cdclk = skl_bw_calc_min_cdclk,
> .modeset_calc_cdclk = skl_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs bdw_cdclk_funcs = {
> .get_cdclk = bdw_get_cdclk,
> .set_cdclk = bdw_set_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = bdw_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs chv_cdclk_funcs = {
> .get_cdclk = vlv_get_cdclk,
> .set_cdclk = chv_set_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = vlv_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs vlv_cdclk_funcs = {
> .get_cdclk = vlv_get_cdclk,
> .set_cdclk = vlv_set_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = vlv_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs hsw_cdclk_funcs = {
> .get_cdclk = hsw_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> /* SNB, IVB, 965G, 945G */
> static const struct intel_cdclk_funcs fixed_400mhz_cdclk_funcs = {
> .get_cdclk = fixed_400mhz_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs ilk_cdclk_funcs = {
> .get_cdclk = fixed_450mhz_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs gm45_cdclk_funcs = {
> .get_cdclk = gm45_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> @@ -3155,7 +3136,6 @@ static const struct intel_cdclk_funcs gm45_cdclk_funcs = {
>
> static const struct intel_cdclk_funcs i965gm_cdclk_funcs = {
> .get_cdclk = i965gm_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> @@ -3163,19 +3143,16 @@ static const struct intel_cdclk_funcs i965gm_cdclk_funcs = {
>
> static const struct intel_cdclk_funcs pnv_cdclk_funcs = {
> .get_cdclk = pnv_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs g33_cdclk_funcs = {
> .get_cdclk = g33_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs i945gm_cdclk_funcs = {
> .get_cdclk = i945gm_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> @@ -3183,37 +3160,31 @@ static const struct intel_cdclk_funcs i945gm_cdclk_funcs = {
>
> static const struct intel_cdclk_funcs i915gm_cdclk_funcs = {
> .get_cdclk = i915gm_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs i915g_cdclk_funcs = {
> .get_cdclk = fixed_333mhz_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs i865g_cdclk_funcs = {
> .get_cdclk = fixed_266mhz_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs i85x_cdclk_funcs = {
> .get_cdclk = i85x_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs i845g_cdclk_funcs = {
> .get_cdclk = fixed_200mhz_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> static const struct intel_cdclk_funcs i830_cdclk_funcs = {
> .get_cdclk = fixed_133mhz_get_cdclk,
> - .bw_calc_min_cdclk = intel_bw_calc_min_cdclk,
> .modeset_calc_cdclk = fixed_modeset_calc_cdclk,
> };
>
> --
> 2.32.0
>
next prev parent reply other threads:[~2022-02-01 8:52 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-01-18 9:23 [Intel-gfx] [PATCH 00/15] drm/i915: Fix bandwith related cdclk calculations Ville Syrjala
2022-01-18 9:23 ` [Intel-gfx] [PATCH 01/15] drm/i915: Drop pointless dev_priv argument Ville Syrjala
2022-01-27 8:15 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 02/15] drm/i915: Extract skl_ddb_entry_init() Ville Syrjala
2022-01-27 8:16 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 03/15] drm/i915: Fix plane relative_data_rate calculation Ville Syrjala
2022-01-27 8:21 ` Lisovskiy, Stanislav
2022-01-27 8:50 ` Ville Syrjälä
2022-01-18 9:23 ` [Intel-gfx] [PATCH 04/15] drm/i915: Introduce skl_plane_ddb_iter Ville Syrjala
2022-01-27 8:22 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 05/15] drm/i915: Extract skl_allocate_plane_ddb() Ville Syrjala
2022-01-27 8:24 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 06/15] drm/i915: Extract skl_crtc_calc_dbuf_bw() Ville Syrjala
2022-01-27 8:24 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 07/15] drm/i915: Tweak plane ddb allocation tracking Ville Syrjala
2022-02-01 8:06 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 08/15] drm/i915: Split plane data_rate into data_rate+data_rate_y Ville Syrjala
2022-02-01 8:08 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 09/15] drm/i915: Pre-calculate plane relative data rate Ville Syrjala
2022-02-01 8:11 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 10/15] drm/i915: Remove total[] and uv_total[] from ddb allocation Ville Syrjala
2022-02-01 8:26 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 11/15] drm/i915: Nuke intel_bw_calc_min_cdclk() Ville Syrjala
2022-02-01 8:52 ` Lisovskiy, Stanislav [this message]
2022-02-01 10:05 ` Ville Syrjälä
2022-02-01 11:18 ` Lisovskiy, Stanislav
2022-02-01 13:45 ` Ville Syrjälä
2022-02-01 14:38 ` Lisovskiy, Stanislav
2022-01-18 9:23 ` [Intel-gfx] [PATCH 12/15] drm/i915: Round up when calculating display bandwidth requirements Ville Syrjala
2022-01-18 9:23 ` [Intel-gfx] [PATCH 13/15] drm/i915: Properly write lock bw_state when it changes Ville Syrjala
2022-01-18 9:23 ` [Intel-gfx] [PATCH 14/15] drm/i915: Fix DBUF bandwidth vs. cdclk handling Ville Syrjala
2022-01-18 9:23 ` [Intel-gfx] [PATCH 15/15] drm/i915: Add "maximum pipe read bandwidth" checks Ville Syrjala
2022-01-18 9:46 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915: Fix bandwith related cdclk calculations Patchwork
2022-01-18 9:47 ` [Intel-gfx] ✗ Fi.CI.SPARSE: " Patchwork
2022-01-18 10:12 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2022-01-18 11:32 ` [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=20220201085239.GA9569@intel.com \
--to=stanislav.lisovskiy@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=ville.syrjala@linux.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.