[Intel-gfx] [PATCH 1/3] drm/i915: unify the x_modeset_calc_cdclk() functions
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Mar 2 20:28:05 UTC 2017
On Mon, Feb 20, 2017 at 05:00:40PM -0300, Paulo Zanoni wrote:
> There's a lot of duplicated platform-independent logic in the current
> modeset_calc_cdclk() functions. Adding cdclk support for more
> platforms will only add more copies of this code.
>
> To solve this problem, in this patch we create a new function called
> intel_modeset_calc_cdclk() which unifies the platform-independent
> logic, and we also create a new vfunc called calc_cdclk_state(), which
> is responsible to contain only the platform-dependent code.
>
> The code is now smaller and support for new platforms should be easier
> and not require duplicated platform-independent code.
>
> v2: Previously I had 2 different patches addressing these problems,
> but wiht Ville's suggestion I think it makes more sense to keep
> everything in a single patch (Ville).
>
> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 4 +-
> drivers/gpu/drm/i915/intel_cdclk.c | 187 ++++++++++-------------------------
> drivers/gpu/drm/i915/intel_display.c | 6 +-
> drivers/gpu/drm/i915/intel_drv.h | 1 +
> 4 files changed, 60 insertions(+), 138 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4e7046d..f1c35c7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -644,7 +644,9 @@ struct drm_i915_display_funcs {
> struct intel_crtc_state *cstate);
> int (*compute_global_watermarks)(struct drm_atomic_state *state);
> void (*update_wm)(struct intel_crtc *crtc);
> - int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
> + void (*calc_cdclk_state)(struct drm_i915_private *dev_priv,
> + int max_pixclk,
> + struct intel_cdclk_state *cdclk_state);
> /* Returns the active state of the crtc, and if the crtc is active,
> * fills out the pipe-config with the hw state. */
> bool (*get_pipe_config)(struct intel_crtc *,
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c b/drivers/gpu/drm/i915/intel_cdclk.c
> index d643c0c..dd6fe25 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -1496,148 +1496,69 @@ static int intel_max_pixel_rate(struct drm_atomic_state *state)
> return max_pixel_rate;
> }
>
> -static int vlv_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void vlv_calc_cdclk_state(struct drm_i915_private *dev_priv,
> + int max_pixclk,
> + struct intel_cdclk_state *cdclk_state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->dev);
> - 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.logical.cdclk = cdclk;
> -
> - 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;
> + cdclk_state->cdclk = vlv_calc_cdclk(dev_priv, max_pixclk);
> }
>
> -static int bdw_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void bdw_calc_cdclk_state(struct drm_i915_private *dev_priv,
> + int max_pixclk,
> + struct intel_cdclk_state *cdclk_state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->dev);
> - struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> - int max_pixclk = intel_max_pixel_rate(state);
> - int cdclk;
> -
> - /*
> - * FIXME should also account for plane ratio
> - * once 64bpp pixel formats are supported.
> - */
> - cdclk = bdw_calc_cdclk(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.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;
> + cdclk_state->cdclk = bdw_calc_cdclk(max_pixclk);
> }
>
> -static int skl_modeset_calc_cdclk(struct drm_atomic_state *state)
> +static void skl_calc_cdclk_state(struct drm_i915_private *dev_priv,
> + int max_pixclk,
> + struct intel_cdclk_state *cdclk_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 cdclk, vco;
> + if (!cdclk_state->vco)
> + cdclk_state->vco = dev_priv->skl_preferred_vco_freq;
Hmm. To maintain the behaviour of the old code we would need to
explicitly pluck the vco from the logicial cdclk state. Otherwise
when we're computing the actual cdclk state we might choose the
wrong vco.
Well, I must admit I'm not 100% sure that we could get the wrong
answer. I'd have to go through the entire thing again, thinking
about all the cases when the vco might change. But since I'm feeling
a little lazy I don't really want to do that. So I think it would
be safer to keep to the current way of doing things.
An alternative would be to always do the actual=logical assignment
in intel_modeset_calc_cdclk() so that the actual.vco==logical.vco
by the time we compute the actual cdclk state. This feels a little
cleaner perhaps, but could probably use a comment to avoid someone
optimizing the copy away again.
>
> - vco = intel_state->cdclk.logical.vco;
> - if (!vco)
> - vco = dev_priv->skl_preferred_vco_freq;
> -
> - /*
> - * FIXME should also account for plane ratio
> - * once 64bpp pixel formats are supported.
> - */
> - cdclk = skl_calc_cdclk(max_pixclk, vco);
> -
> - 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.logical.vco = vco;
> - intel_state->cdclk.logical.cdclk = cdclk;
> -
> - if (!intel_state->active_crtcs) {
> - cdclk = skl_calc_cdclk(0, vco);
> + cdclk_state->cdclk = skl_calc_cdclk(max_pixclk, cdclk_state->vco);
> +}
>
> - intel_state->cdclk.actual.vco = vco;
> - intel_state->cdclk.actual.cdclk = cdclk;
> - } else {
> - intel_state->cdclk.actual =
> - intel_state->cdclk.logical;
> - }
> +static void bxt_calc_cdclk_state(struct drm_i915_private *dev_priv,
> + int max_pixclk,
> + struct intel_cdclk_state *cdclk_state)
> +{
> + cdclk_state->cdclk = bxt_calc_cdclk(max_pixclk);
> + cdclk_state->vco = bxt_de_pll_vco(dev_priv, cdclk_state->cdclk);
> +}
>
> - return 0;
> +static void glk_calc_cdclk_state(struct drm_i915_private *dev_priv,
> + int max_pixclk,
> + struct intel_cdclk_state *cdclk_state)
> +{
> + cdclk_state->cdclk = glk_calc_cdclk(max_pixclk);
> + cdclk_state->vco = glk_de_pll_vco(dev_priv, cdclk_state->cdclk);
> }
>
> -static int bxt_modeset_calc_cdclk(struct drm_atomic_state *state)
> +int intel_modeset_calc_cdclk(struct intel_atomic_state *state)
> {
> - struct drm_i915_private *dev_priv = to_i915(state->dev);
> - int max_pixclk = intel_max_pixel_rate(state);
> - struct intel_atomic_state *intel_state =
> - to_intel_atomic_state(state);
> - int cdclk, vco;
> + struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> + struct intel_cdclk_state *logical = &state->cdclk.logical;
> + struct intel_cdclk_state *actual = &state->cdclk.actual;
> + int max_pixclk = intel_max_pixel_rate(&state->base);
>
> - if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(max_pixclk);
> - vco = glk_de_pll_vco(dev_priv, cdclk);
> - } else {
> - cdclk = bxt_calc_cdclk(max_pixclk);
> - vco = bxt_de_pll_vco(dev_priv, cdclk);
> - }
> + /*
> + * FIXME: should also account for plane ratio once 64bpp pixel formats
> + * are supported.
> + */
> + dev_priv->display.calc_cdclk_state(dev_priv, max_pixclk, logical);
>
> - if (cdclk > dev_priv->max_cdclk_freq) {
> + if (logical->cdclk > dev_priv->max_cdclk_freq) {
> DRM_DEBUG_KMS("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> - cdclk, dev_priv->max_cdclk_freq);
> + logical->cdclk, dev_priv->max_cdclk_freq);
> return -EINVAL;
> }
>
> - intel_state->cdclk.logical.vco = vco;
> - intel_state->cdclk.logical.cdclk = cdclk;
> -
> - if (!intel_state->active_crtcs) {
> - if (IS_GEMINILAKE(dev_priv)) {
> - cdclk = glk_calc_cdclk(0);
> - vco = glk_de_pll_vco(dev_priv, cdclk);
> - } else {
> - cdclk = bxt_calc_cdclk(0);
> - vco = bxt_de_pll_vco(dev_priv, cdclk);
> - }
> -
> - intel_state->cdclk.actual.vco = vco;
> - intel_state->cdclk.actual.cdclk = cdclk;
> - } else {
> - intel_state->cdclk.actual =
> - intel_state->cdclk.logical;
> - }
> + if (!state->active_crtcs)
> + dev_priv->display.calc_cdclk_state(dev_priv, 0, actual);
> + else
> + *actual = *logical;
>
> return 0;
> }
> @@ -1823,24 +1744,22 @@ void intel_init_cdclk_hooks(struct drm_i915_private *dev_priv)
> {
> if (IS_CHERRYVIEW(dev_priv)) {
> dev_priv->display.set_cdclk = chv_set_cdclk;
> - dev_priv->display.modeset_calc_cdclk =
> - vlv_modeset_calc_cdclk;
> + dev_priv->display.calc_cdclk_state = vlv_calc_cdclk_state;
> } else if (IS_VALLEYVIEW(dev_priv)) {
> dev_priv->display.set_cdclk = vlv_set_cdclk;
> - dev_priv->display.modeset_calc_cdclk =
> - vlv_modeset_calc_cdclk;
> + dev_priv->display.calc_cdclk_state = vlv_calc_cdclk_state;
> } else if (IS_BROADWELL(dev_priv)) {
> dev_priv->display.set_cdclk = bdw_set_cdclk;
> - dev_priv->display.modeset_calc_cdclk =
> - bdw_modeset_calc_cdclk;
> - } else if (IS_GEN9_LP(dev_priv)) {
> + dev_priv->display.calc_cdclk_state = bdw_calc_cdclk_state;
> + } else if (IS_BROXTON(dev_priv)) {
> + dev_priv->display.set_cdclk = bxt_set_cdclk;
> + dev_priv->display.calc_cdclk_state = bxt_calc_cdclk_state;
> + } else if (IS_GEMINILAKE(dev_priv)) {
> dev_priv->display.set_cdclk = bxt_set_cdclk;
> - dev_priv->display.modeset_calc_cdclk =
> - bxt_modeset_calc_cdclk;
> + dev_priv->display.calc_cdclk_state = glk_calc_cdclk_state;
> } else if (IS_GEN9_BC(dev_priv)) {
> dev_priv->display.set_cdclk = skl_set_cdclk;
> - dev_priv->display.modeset_calc_cdclk =
> - skl_modeset_calc_cdclk;
> + dev_priv->display.calc_cdclk_state = skl_calc_cdclk_state;
> }
>
> if (IS_GEN9_BC(dev_priv))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f6feb93..1d2cb491 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12414,8 +12414,8 @@ static int intel_modeset_checks(struct drm_atomic_state *state)
> * mode set on this crtc. For other crtcs we need to use the
> * adjusted_mode bits in the crtc directly.
> */
> - if (dev_priv->display.modeset_calc_cdclk) {
> - ret = dev_priv->display.modeset_calc_cdclk(state);
> + if (dev_priv->display.calc_cdclk_state) {
> + ret = intel_modeset_calc_cdclk(intel_state);
> if (ret < 0)
> return ret;
>
> @@ -15468,7 +15468,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> pixclk = crtc_state->pixel_rate;
> else
> - WARN_ON(dev_priv->display.modeset_calc_cdclk);
> + WARN_ON(dev_priv->display.calc_cdclk_state);
>
> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 821c57c..3e112fe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1260,6 +1260,7 @@ bool intel_cdclk_state_compare(const struct intel_cdclk_state *a,
> const struct intel_cdclk_state *b);
> void intel_set_cdclk(struct drm_i915_private *dev_priv,
> const struct intel_cdclk_state *cdclk_state);
> +int intel_modeset_calc_cdclk(struct intel_atomic_state *state);
>
> /* intel_display.c */
> enum transcoder intel_crtc_pch_transcoder(struct intel_crtc *crtc);
> --
> 2.7.4
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list