On Mon, 2016-12-19 at 19:28 +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> The current dev_cdclk vs. cdclk vs. atomic_cdclk_freq is quite a mess.
> So here I'm introducing the "actual" and "logical" naming for our
> cdclk state. "actual" is what we'll bash into the hardware and "logical"
> is what everyone should use for state computaion/checking and whatnot.
> We'll track both using the intel_cdclk_state as both will need other
> differing parameters than just the actual cdclk frequency.
>
> While doing that we can at the same time unify the appearance of the
> .modeset_calc_cdclk() implementations a little bit.
>
> v2: Commit dev_priv->cdclk.actual since that already has the
> new state by the time .modeset_commit_cdclk() is called.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 13 ++--
> drivers/gpu/drm/i915/intel_cdclk.c | 123 ++++++++++++++++++++++-------------
> drivers/gpu/drm/i915/intel_display.c | 39 ++++++-----
> drivers/gpu/drm/i915/intel_dp.c | 2 +-
> drivers/gpu/drm/i915/intel_drv.h | 24 ++++---
> drivers/gpu/drm/i915/intel_pm.c | 4 +-
> 6 files changed, 121 insertions(+), 84 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a4f1231ff8ca..b5a8d0f4cfbd 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2237,18 +2237,19 @@ struct drm_i915_private {
> unsigned int skl_preferred_vco_freq;
> unsigned int max_cdclk_freq;
>
> - /*
> - * For reading holding any crtc lock is sufficient,
> - * for writing must hold all of them.
> - */
> - unsigned int atomic_cdclk_freq;
> -
> unsigned int max_dotclk_freq;
> unsigned int rawclk_freq;
> unsigned int hpll_freq;
> unsigned int czclk_freq;
>
> struct {
> + /*
> + * The current logical cdclk state.
> + * For reading holding any crtc lock is sufficient,
> + * for writing must hold all of them.
> + */
> + struct intel_cdclk_state logical;
> + struct intel_cdclk_state actual;
> struct intel_cdclk_state hw;
It would be great if all three fields were documented, including an example
configuration where the logical and actual fields differ and what which fields
hold, to make the distinction clear.
> } cdclk;
>
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index 13c3b5555473..08ae3b78b8ed 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1368,12 +1368,26 @@ static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> int max_pixclk = intel_max_pixel_rate(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> + int cdclk;
> +
> + cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> +
> + if (cdclk > dev_priv->max_cdclk_freq) {
> + DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> + cdclk, dev_priv->max_cdclk_freq);
> + return -EINVAL;
> + }
>
> - intel_state->cdclk = intel_state->dev_cdclk =
> - vlv_calc_cdclk(dev_priv, max_pixclk);
> + intel_state->cdclk.logical.cdclk = cdclk;
>
> - if (!intel_state->active_crtcs)
> - intel_state->dev_cdclk = vlv_calc_cdclk(dev_priv, 0);
> + if (!intel_state->active_crtcs) {
> + cdclk = vlv_calc_cdclk(dev_priv, 0);
> +
> + intel_state->cdclk.actual.cdclk = cdclk;
> + } else {
> + intel_state->cdclk.actual =
> + intel_state->cdclk.logical;
> + }
>
> return 0;
> }
> @@ -1382,9 +1396,7 @@ static void vlv_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> {
> struct drm_device *dev = old_state->dev;
> struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_atomic_state *old_intel_state =
> - to_intel_atomic_state(old_state);
> - unsigned int req_cdclk = old_intel_state->dev_cdclk;
> + unsigned int req_cdclk = dev_priv->cdclk.actual.cdclk;
>
> /*
> * FIXME: We can end up here with all power domains off, yet
> @@ -1426,9 +1438,16 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> return -EINVAL;
> }
>
> - intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> - if (!intel_state->active_crtcs)
> - intel_state->dev_cdclk = bdw_calc_cdclk(0);
> + intel_state->cdclk.logical.cdclk = cdclk;
> +
> + if (!intel_state->active_crtcs) {
> + cdclk = bdw_calc_cdclk(0);
> +
> + intel_state->cdclk.actual.cdclk = cdclk;
> + } else {
> + intel_state->cdclk.actual =
> + intel_state->cdclk.logical;
> + }
>
> return 0;
> }
> @@ -1436,9 +1455,7 @@ static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> static void bdw_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> {
> struct drm_device *dev = old_state->dev;
> - struct intel_atomic_state *old_intel_state =
> - to_intel_atomic_state(old_state);
> - unsigned int req_cdclk = old_intel_state->dev_cdclk;
> + unsigned int req_cdclk = to_i915(dev)->cdclk.actual.cdclk;
>
> bdw_set_cdclk(dev, req_cdclk);
> }
> @@ -1448,8 +1465,11 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> struct drm_i915_private *dev_priv = to_i915(state->dev);
> const int max_pixclk = intel_max_pixel_rate(state);
> - int vco = intel_state->cdclk_pll_vco;
> - int cdclk;
> + int cdclk, vco;
> +
> + vco = intel_state->cdclk.logical.vco;
> + if (!vco)
> + vco = dev_priv->skl_preferred_vco_freq;
>
> /*
> * FIXME should also account for plane ratio
> @@ -1457,19 +1477,24 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> */
> cdclk = skl_calc_cdclk(max_pixclk, vco);
>
> - /*
> - * FIXME move the cdclk caclulation to
> - * compute_config() so we can fail gracegully.
> - */
> if (cdclk > dev_priv->max_cdclk_freq) {
> - DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> - cdclk = dev_priv->max_cdclk_freq;
> + DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> + cdclk, dev_priv->max_cdclk_freq);
> + return -EINVAL;
> }
>
> - intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> - if (!intel_state->active_crtcs)
> - intel_state->dev_cdclk = skl_calc_cdclk(0, vco);
> + intel_state->cdclk.logical.vco = vco;
> + intel_state->cdclk.logical.cdclk = cdclk;
> +
> + if (!intel_state->active_crtcs) {
> + cdclk = skl_calc_cdclk(0, vco);
> +
> + intel_state->cdclk.actual.vco = vco;
> + intel_state->cdclk.actual.cdclk = cdclk;
> + } else {
> + intel_state->cdclk.actual =
> + intel_state->cdclk.logical;
> + }
>
> return 0;
> }
> @@ -1477,10 +1502,8 @@ static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> static void skl_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> {
> struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(old_state);
> - unsigned int req_cdclk = intel_state->dev_cdclk;
> - unsigned int req_vco = intel_state->cdclk_pll_vco;
> + unsigned int req_cdclk = dev_priv->cdclk.actual.cdclk;
> + unsigned int req_vco = dev_priv->cdclk.actual.vco;
>
> skl_set_cdclk(dev_priv, req_cdclk, req_vco);
> }
> @@ -1491,22 +1514,39 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> int max_pixclk = intel_max_pixel_rate(state);
> struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> - int cdclk;
> + int cdclk, vco;
>
> - if (IS_GEMINILAKE(dev_priv))
> + if (IS_GEMINILAKE(dev_priv)) {
> cdclk = glk_calc_cdclk(max_pixclk);
> - else
> + vco = glk_de_pll_vco(dev_priv, cdclk);
> + } else {
> cdclk = bxt_calc_cdclk(max_pixclk);
> + vco = bxt_de_pll_vco(dev_priv, cdclk);
> + }
> +
> + if (cdclk > dev_priv->max_cdclk_freq) {
> + DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> + cdclk, dev_priv->max_cdclk_freq);
> + return -EINVAL;
> + }
>
> - intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> + intel_state->cdclk.logical.vco = vco;
> + intel_state->cdclk.logical.cdclk = cdclk;
>
> if (!intel_state->active_crtcs) {
> - if (IS_GEMINILAKE(dev_priv))
> + if (IS_GEMINILAKE(dev_priv)) {
> cdclk = glk_calc_cdclk(0);
> - else
> + vco = glk_de_pll_vco(dev_priv, cdclk);
> + } else {
> cdclk = bxt_calc_cdclk(0);
> + vco = bxt_de_pll_vco(dev_priv, cdclk);
> + }
>
> - intel_state->dev_cdclk = cdclk;
> + intel_state->cdclk.actual.vco = vco;
> + intel_state->cdclk.actual.cdclk = cdclk;
> + } else {
> + intel_state->cdclk.actual =
> + intel_state->cdclk.logical;
> }
>
> return 0;
> @@ -1515,15 +1555,8 @@ static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> static void bxt_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> {
> struct drm_i915_private *dev_priv = to_i915(old_state->dev);
> - struct intel_atomic_state *old_intel_state =
> - to_intel_atomic_state(old_state);
> - unsigned int req_cdclk = old_intel_state->dev_cdclk;
> - unsigned int req_vco;
> -
> - if (IS_GEMINILAKE(dev_priv))
> - req_vco = glk_de_pll_vco(dev_priv, req_cdclk);
> - else
> - req_vco = bxt_de_pll_vco(dev_priv, req_cdclk);
> + unsigned int req_cdclk = dev_priv->cdclk.actual.cdclk;
> + unsigned int req_vco = dev_priv->cdclk.actual.vco;
>
> bxt_set_cdclk(dev_priv, req_cdclk, req_vco);
> }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6c38ab299506..fb3abb58b6ca 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12416,6 +12416,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
>
> intel_state->modeset = true;
> intel_state->active_crtcs = dev_priv->active_crtcs;
> + intel_state->cdclk.logical = dev_priv->cdclk.logical;
> + intel_state->cdclk.actual = dev_priv->cdclk.actual;
Not an issue with this patch specifically, but shouldn't this be part of a
duplicate_cdclk_state() to be inline with other atomic stuff?
>
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> if (crtc_state->active)
> @@ -12435,38 +12437,35 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> * adjusted_mode bits in the crtc directly.
> */
> if (dev_priv->display.modeset_calc_cdclk) {
> - if (!intel_state->cdclk_pll_vco)
> - intel_state->cdclk_pll_vco = dev_priv->cdclk.hw.vco;
> - if (!intel_state->cdclk_pll_vco)
> - intel_state->cdclk_pll_vco = dev_priv->skl_preferred_vco_freq;
> -
> ret = dev_priv->display.modeset_calc_cdclk(state);
> if (ret < 0)
> return ret;
>
> /*
> - * Writes to dev_priv->atomic_cdclk_freq must protected by
> + * Writes to dev_priv->cdclk_locical must protected by
logical
> * holding all the crtc locks, even if we don't end up
> * touching the hardware
> */
> - if (intel_state->cdclk != dev_priv->atomic_cdclk_freq) {
> + if (!intel_cdclk_state_compare(&dev_priv->cdclk.logical,
> + &intel_state->cdclk.logical)) {
> ret = intel_lock_all_pipes(state);
> if (ret < 0)
> return ret;
> }
>
> /* All pipes must be switched off while we change the cdclk. */
> - if (intel_state->dev_cdclk != dev_priv->cdclk.hw.cdclk ||
> - intel_state->cdclk_pll_vco != dev_priv->cdclk.hw.vco) {
> + if (!intel_cdclk_state_compare(&dev_priv->cdclk.actual,
> + &intel_state->cdclk.actual)) {
> ret = intel_modeset_all_pipes(state);
> if (ret < 0)
> return ret;
> }
>
> - DRM_DEBUG_KMS("New cdclk calculated to be atomic %u, actual %u\n",
> - intel_state->cdclk, intel_state->dev_cdclk);
> + DRM_DEBUG_KMS("New cdclk calculated to be logical %u kHz, actual %u kHz\n",
> + intel_state->cdclk.logical.cdclk,
> + intel_state->cdclk.actual.cdclk);
> } else {
> - to_intel_atomic_state(state)->cdclk = dev_priv->atomic_cdclk_freq;
> + to_intel_atomic_state(state)->cdclk.logical = dev_priv->cdclk.logical;
> }
>
> intel_modeset_clear_plls(state);
> @@ -12569,7 +12568,7 @@ static int intel_atomic_check(struct drm_device *dev,
> if (ret)
> return ret;
> } else {
> - intel_state->cdclk = dev_priv->atomic_cdclk_freq;
> + intel_state->cdclk.logical = dev_priv->cdclk.logical;
> }
>
> ret = drm_atomic_helper_check_planes(dev, state);
> @@ -12874,8 +12873,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
>
> if (dev_priv->display.modeset_commit_cdclk &&
> - (intel_state->dev_cdclk != dev_priv->cdclk.hw.cdclk ||
> - intel_state->cdclk_pll_vco != dev_priv->cdclk.hw.vco))
> + !intel_cdclk_state_compare(&dev_priv->cdclk.hw,
> + &dev_priv->cdclk.actual))
> dev_priv->display.modeset_commit_cdclk(state);
>
> /*
> @@ -13056,7 +13055,8 @@ static int intel_atomic_commit(struct drm_device *dev,
> memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> sizeof(intel_state->min_pixclk));
> dev_priv->active_crtcs = intel_state->active_crtcs;
> - dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> + dev_priv->cdclk.logical = intel_state->cdclk.logical;
> + dev_priv->cdclk.actual = intel_state->cdclk.actual;
> }
>
> drm_atomic_state_get(state);
> @@ -13298,7 +13298,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
> return DRM_PLANE_HELPER_NO_SCALING;
>
> crtc_clock = crtc_state->base.adjusted_mode.crtc_clock;
> - cdclk = to_intel_atomic_state(crtc_state->base.state)->cdclk;
> + cdclk = to_intel_atomic_state(crtc_state->base.state)->cdclk.logical.cdclk;
>
> if (WARN_ON_ONCE(!crtc_clock || cdclk < crtc_clock))
> return DRM_PLANE_HELPER_NO_SCALING;
> @@ -14723,8 +14723,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
> struct drm_i915_private *dev_priv = to_i915(dev);
>
> intel_update_cdclk(dev_priv);
> -
> - dev_priv->atomic_cdclk_freq = dev_priv->cdclk.hw.cdclk;
> + dev_priv->cdclk.logical = dev_priv->cdclk.actual = dev_priv->cdclk.hw;
>
> intel_init_clock_gating(dev_priv);
> }
> @@ -14897,7 +14896,7 @@ int intel_modeset_init(struct drm_device *dev)
>
> intel_update_czclk(dev_priv);
> intel_update_cdclk(dev_priv);
> - dev_priv->atomic_cdclk_freq = dev_priv->cdclk.hw.cdclk;
> + dev_priv->cdclk.logical = dev_priv->cdclk.actual = dev_priv->cdclk.hw;
>
> intel_shared_dpll_init(dev);
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 73a708b8d887..4309509e8a34 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1734,7 +1734,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> break;
> }
>
> - to_intel_atomic_state(pipe_config->base.state)->cdclk_pll_vco = vco;
> + to_intel_atomic_state(pipe_config->base.state)->cdclk.logical.vco = vco;
> }
>
> if (!HAS_DDI(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index cc0122282a7b..d167ee458d63 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -333,13 +333,20 @@ struct dpll {
> struct intel_atomic_state {
> struct drm_atomic_state base;
>
> - unsigned int cdclk;
> -
> - /*
> - * Calculated device cdclk, can be different from cdclk
> - * only when all crtc's are DPMS off.
> - */
> - unsigned int dev_cdclk;
> + struct {
> + /*
> + * Logical state of cdclk (used for all scaling, watermark,
> + * etc. calculations and checks). This is computed as if all
> + * enabled crtcs were active.
> + */
> + struct intel_cdclk_state logical;
> +
> + /*
> + * Actual state of cdclk, can be different from the logical
> + * state only when all crtc's are DPMS off.
> + */
> + struct intel_cdclk_state actual;
> + } cdclk;
Ah, here's the field descriptions. Maybe the fields in dev_priv should point at
here. Compared to the other atomic states, the cdclk one is a bit different,
since there's only once cdclk per device but we have multiple states. Maybe the
state struct should have both logical and actual, and dev_priv would include
only one struct intel_cdclk_state? Then there would be only one place for
documentation and it would be more inline with other atomic stuff.
I agree that the patch does make things less confusing, though, so
Reviewed-by: Ander Conselvan de Oliveira <conselvan2@xxxxxxxxx>
>
> bool dpll_set, modeset;
>
> @@ -356,9 +363,6 @@ struct intel_atomic_state {
> unsigned int active_crtcs;
> unsigned int min_pixclk[I915_MAX_PIPES];
>
> - /* SKL/KBL Only */
> - unsigned int cdclk_pll_vco;
> -
> struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
>
> /*
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 79e2be4216e4..fe522ec21502 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2095,7 +2095,7 @@ hsw_compute_linetime_wm(const struct intel_crtc_state *cstate)
> return 0;
> if (WARN_ON(adjusted_mode->crtc_clock == 0))
> return 0;
> - if (WARN_ON(intel_state->cdclk == 0))
> + if (WARN_ON(intel_state->cdclk.logical.cdclk == 0))
> return 0;
>
> /* The WM are computed with base on how long it takes to fill a single
> @@ -2104,7 +2104,7 @@ hsw_compute_linetime_wm(const struct intel_crtc_state *cstate)
> linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> adjusted_mode->crtc_clock);
> ips_linetime = DIV_ROUND_CLOSEST(adjusted_mode->crtc_htotal * 1000 * 8,
> - intel_state->cdclk);
> + intel_state->cdclk.logical.cdclk);
>
> return PIPE_WM_LINETIME_IPS_LINETIME(ips_linetime) |
> PIPE_WM_LINETIME_TIME(linetime);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
