On Thu, 2015-11-26 at 15:31 +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > On skylake when calculating plane visibility with the crtc in
> > dpms off mode the real cdclk may be different from what it would be
> > if the crtc was active. This may result in a WARN_ON(cdclk < crtc_clock)
> > from skl_max_scale. The fix is to keep a atomic_cdclk that would be true
> > if all crtc's were active.
> >
> > This is required to get the same calculations done correctly regardless
> > of dpms mode.
> >
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 2 +-
> > drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++---------
> > --
> > -
> > drivers/gpu/drm/i915/intel_drv.h | 6 ++++
> > 3 files changed, 44 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3caecf896f17..f3ad72abeea7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1776,7 +1776,7 @@ struct drm_i915_private {
> >
> > unsigned int fsb_freq, mem_freq, is_ddr3;
> > unsigned int skl_boot_cdclk;
> > - unsigned int cdclk_freq, max_cdclk_freq;
> > + unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> > unsigned int max_dotclk_freq;
> > unsigned int hpll_freq;
> > unsigned int czclk_freq;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 7f69f98d8b23..b2bf92a3b701 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5312,6 +5312,7 @@ static void modeset_put_power_domains(struct
> > drm_i915_private *dev_priv,
> >
> > static void modeset_update_crtc_power_domains(struct drm_atomic_state
> > *state)
> > {
> > + struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> > struct drm_device *dev = state->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > unsigned long put_domains[I915_MAX_PIPES] = {};
> > @@ -5325,13 +5326,9 @@ static void modeset_update_crtc_power_domains(struct
> > drm_atomic_state *state)
> > modeset_get_crtc_power_domains(crtc);
> > }
> >
> > - if (dev_priv->display.modeset_commit_cdclk) {
> > - unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
> > -
> > - if (cdclk != dev_priv->cdclk_freq &&
> > - !WARN_ON(!state->allow_modeset))
> > - dev_priv->display.modeset_commit_cdclk(state);
> > - }
> > + if (dev_priv->display.modeset_commit_cdclk &&
> > + intel_state->dev_cdclk != dev_priv->cdclk_freq)
> > + dev_priv->display.modeset_commit_cdclk(state);
> >
> > for (i = 0; i < I915_MAX_PIPES; i++)
> > if (put_domains[i])
> > @@ -6039,13 +6036,18 @@ static int valleyview_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > struct drm_device *dev = state->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > int max_pixclk = intel_mode_max_pixclk(dev, state);
> > + struct intel_atomic_state *intel_state =
> > + to_intel_atomic_state(state);
> >
> > if (max_pixclk < 0)
> > return max_pixclk;
> >
> > - to_intel_atomic_state(state)->cdclk =
> > + intel_state->cdclk = intel_state->dev_cdclk =
> > valleyview_calc_cdclk(dev_priv, max_pixclk);
> >
> > + if (!intel_state->active_crtcs)
> > + intel_state->dev_cdclk = valleyview_calc_cdclk(dev_priv,
> > 0);
> > +
> > return 0;
> > }
> >
> > @@ -6054,13 +6056,18 @@ static int broxton_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > struct drm_device *dev = state->dev;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > int max_pixclk = intel_mode_max_pixclk(dev, state);
> > + struct intel_atomic_state *intel_state =
> > + to_intel_atomic_state(state);
> >
> > if (max_pixclk < 0)
> > return max_pixclk;
> >
> > - to_intel_atomic_state(state)->cdclk =
> > + intel_state->cdclk = intel_state->dev_cdclk =
> > broxton_calc_cdclk(dev_priv, max_pixclk);
> >
> > + if (!intel_state->active_crtcs)
> > + intel_state->dev_cdclk = broxton_calc_cdclk(dev_priv, 0);
> > +
> > return 0;
> > }
> >
> > @@ -6103,8 +6110,10 @@ static void vlv_program_pfi_credits(struct
> > drm_i915_private *dev_priv)
> > static void valleyview_modeset_commit_cdclk(struct drm_atomic_state
> > *old_state)
> > {
> > struct drm_device *dev = old_state->dev;
> > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct intel_atomic_state *old_intel_state =
> > + to_intel_atomic_state(old_state);
> > + unsigned req_cdclk = old_intel_state->dev_cdclk;
> >
> > /*
> > * FIXME: We can end up here with all power domains off, yet
> > @@ -9574,7 +9583,9 @@ void hsw_disable_pc8(struct drm_i915_private
> > *dev_priv)
> > static void broxton_modeset_commit_cdclk(struct drm_atomic_state
> > *old_state)
> > {
> > struct drm_device *dev = old_state->dev;
> > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> > + struct intel_atomic_state *old_intel_state =
> > + to_intel_atomic_state(old_state);
> > + unsigned int req_cdclk = old_intel_state->dev_cdclk;
> >
> > broxton_set_cdclk(dev, req_cdclk);
> > }
> > @@ -9700,6 +9711,7 @@ static void broadwell_set_cdclk(struct drm_device
> > *dev,
> > int cdclk)
> > static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *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 = ilk_max_pixel_rate(state);
> > int cdclk;
> >
> > @@ -9722,7 +9734,9 @@ static int broadwell_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > return -EINVAL;
> > }
> >
> > - to_intel_atomic_state(state)->cdclk = cdclk;
> > + intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> > + if (!intel_state->active_crtcs)
> > + intel_state->dev_cdclk = 337500;
> >
> > return 0;
> > }
> > @@ -9730,7 +9744,9 @@ static int broadwell_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> > static void broadwell_modeset_commit_cdclk(struct drm_atomic_state
> > *old_state)
> > {
> > struct drm_device *dev = old_state->dev;
> > - unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> > + struct intel_atomic_state *old_intel_state =
> > + to_intel_atomic_state(old_state);
> > + unsigned req_cdclk = old_intel_state->dev_cdclk;
> >
> > broadwell_set_cdclk(dev, req_cdclk);
> > }
> > @@ -13066,18 +13082,15 @@ static int intel_modeset_checks(struct
> > drm_atomic_state *state)
> > * adjusted_mode bits in the crtc directly.
> > */
> > if (dev_priv->display.modeset_calc_cdclk) {
> > - unsigned int cdclk;
> > -
> > ret = dev_priv->display.modeset_calc_cdclk(state);
> >
> > - cdclk = to_intel_atomic_state(state)->cdclk;
> > - if (!ret && cdclk != dev_priv->cdclk_freq)
> > + if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)
> > ret = intel_modeset_all_pipes(state);
> >
> > if (ret < 0)
> > return ret;
> > } else
> > - to_intel_atomic_state(state)->cdclk = dev_priv->cdclk_freq;
> > + to_intel_atomic_state(state)->cdclk = dev_priv
> > ->atomic_cdclk_freq;
> >
> > intel_modeset_clear_plls(state);
> >
> > @@ -13358,6 +13371,7 @@ 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;
> > }
> >
> > for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > @@ -15026,7 +15040,12 @@ static void i915_disable_vga(struct drm_device
> > *dev)
> >
> > void intel_modeset_init_hw(struct drm_device *dev)
> > {
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > intel_update_cdclk(dev);
> > +
> > + dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
> > +
> > intel_prepare_ddi(dev);
> > intel_init_clock_gating(dev);
> > intel_enable_gt_powersave(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d2e1dc698fca..7fd025f64f4c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -247,6 +247,12 @@ struct intel_atomic_state {
> >
> > unsigned int cdclk;
> >
> > + /*
> > + * Calculated device cdclk, can be different from cdclk
> > + * only when all crtc's are DPMS off.
> > + */
> > + unsigned int dev_cdclk;
> > +
> > bool dpll_set, modeset;
> >
> > unsigned int active_crtcs;
>
> So state->cdclk and dev_priv->atomic_cdclk_freq hold the value for when all
> CRTCs are enabled and state->dev_cdclkand dev_priv->cdclk_freq hold the actual
s/enabled/active/
> cdclk value (which changes for DPMS off). In intel_atomic_check(), there is
> the
> following assignment:
>
> if (any_ms) {
> ...
> } else
> intel_state->cdclk = to_i915(state->dev)->cdclk_freq;
>
> Can an update during DPMS off cause the value of cdclk for when all CRTCs are
> enabled to be overwritten with the DPMS off value?
>
> Patch looks fine other than that, but maybe as a followup we could rename
> those
> fields so the mapping between them is a bit clearer. Maybe current_cdclk for
> both state and dev_priv for the value currently programmed to the hardware.
> Not
> sure about the atomic_cdclk. How about dpms_on_cdclk or active_cdclk?
>
> Ander
>
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
