On Mon, 2017-02-20 at 17:00 -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@xxxxxxxxxxxxxxx>
> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
> ---
> 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;
>
> - 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);
I was looking at DK's patch adding minimum cdclk restrictions for glk [1] and
that led me to think the code before the patch should look like
cdclk = calc_cdclk()
if (cdclk < min_cdclk)
cdclk = min_cdclk;
vco = de_pll_vco(cdclk);
Now that patch will conflict with this and I think we either will have to
replicate that min_cdclk check in every calc_cdclk_state() implementation or
then split that into two hooks: one for the calc_cdclk() part and another for
the vco part. Or something else?
[1] https://patchwork.freedesktop.org/patch/141371/
Ander
>
> - 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);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
