| Message ID | 1487620842-22893-2-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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
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@linux.intel.com> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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; > > - 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);
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); - 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);
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@linux.intel.com> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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(-)