| Message ID | 1448360945-5723-4-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote: > Parallel modesets are still not allowed, but this will allow updating > a different crtc during a modeset if the clock is not changed. > > Additionally when all pipes are DPMS off the cdclk will be lowered > to the minimum allowed. > > Changes since v1: > - Add dev_priv->active_crtcs for tracking which crtcs are active. > - Rename min_cdclk to min_pixclk and move to dev_priv. > - Add a active_crtcs mask which is updated atomically. > - Add intel_atomic_state->modeset which is set on modesets. > - Commit new pixclk/active_crtcs right after state swap. > Changes since v2: > - Make the changes related to max_pixel_rate calculations more readable. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_drv.h | 5 ++ > drivers/gpu/drm/i915/intel_atomic.c | 2 +- > drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++---------- > drivers/gpu/drm/i915/intel_drv.h | 7 +- > 4 files changed, 112 insertions(+), 41 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index a32571f0d018..c7b76c2e8e09 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1821,8 +1821,13 @@ struct drm_i915_private { > struct intel_pipe_crc pipe_crc[I915_MAX_PIPES]; > #endif > > + /* dpll and cdclk state is protected by connection_mutex */ > int num_shared_dpll; > struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; > + > + unsigned int active_crtcs; > + unsigned int min_pixclk[I915_MAX_PIPES]; > + > int dpio_phy_iosf_port[I915_NUM_PHYS_VLV]; > > struct i915_workarounds workarounds; > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 643f342de33b..c4eadbc928b7 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s) > { > struct intel_atomic_state *state = to_intel_atomic_state(s); > drm_atomic_state_default_clear(&state->base); > - state->dpll_set = false; > + state->dpll_set = state->modeset = false; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3c46037b6e55..178a042f917e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > static int intel_mode_max_pixclk(struct drm_device *dev, > struct drm_atomic_state *state) > { > - struct intel_crtc *intel_crtc; > - struct intel_crtc_state *crtc_state; > - int max_pixclk = 0; > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + struct drm_i915_private *dev_priv = dev->dev_private; This is a nitpick but we should use nowadays to_i915() > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + unsigned max_pixclk = 0, i; > + enum pipe pipe; > > - for_each_intel_crtc(dev, intel_crtc) { > - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > - if (IS_ERR(crtc_state)) > - return PTR_ERR(crtc_state); > + memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, > + sizeof(intel_state->min_pixclk)); > > - if (!crtc_state->base.enable) > - continue; > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + int pixclk = 0; > + > + if (crtc_state->enable) > + pixclk = crtc_state->adjusted_mode.crtc_clock; > > - max_pixclk = max(max_pixclk, > - crtc_state->base.adjusted_mode.crtc_clock); > + intel_state->min_pixclk[i] = pixclk; > } > > + if (!intel_state->active_crtcs) > + return 0; > + > + for_each_pipe(dev_priv, pipe) > + max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk); > + > return max_pixclk; > } > > @@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) > for_each_power_domain(domain, domains) > intel_display_power_put(dev_priv, domain); > intel_crtc->enabled_power_domains = 0; > + > + dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe); > + dev_priv->min_pixclk[intel_crtc->pipe] = 0; > } > > /* > @@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) > /* compute the max rate for new configuration */ > static int ilk_max_pixel_rate(struct drm_atomic_state *state) > { > - struct intel_crtc *intel_crtc; > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + struct drm_i915_private *dev_priv = state->dev->dev_private; > + struct drm_crtc *crtc; > + struct drm_crtc_state *cstate; > struct intel_crtc_state *crtc_state; > - int max_pixel_rate = 0; > + unsigned max_pixel_rate = 0, i; > + enum pipe pipe; > > - for_each_intel_crtc(state->dev, intel_crtc) { > - int pixel_rate; > + memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, > + sizeof(intel_state->min_pixclk)); > > - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); > - if (IS_ERR(crtc_state)) > - return PTR_ERR(crtc_state); > + for_each_crtc_in_state(state, crtc, cstate, i) { > + int pixel_rate; > > - if (!crtc_state->base.enable) > + crtc_state = to_intel_crtc_state(cstate); > + if (!crtc_state->base.enable) { > + intel_state->min_pixclk[i] = 0; > continue; > + } > > pixel_rate = ilk_pipe_pixel_rate(crtc_state); > > /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ > - if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled) > + if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) > pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95); > > - max_pixel_rate = max(max_pixel_rate, pixel_rate); > + intel_state->min_pixclk[i] = pixel_rate; > } > > + if (!intel_state->active_crtcs) > + return 0; > + > + for_each_pipe(dev_priv, pipe) > + max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate); > + > return max_pixel_rate; > } > > @@ -13069,15 +13093,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state) > > static int intel_modeset_checks(struct drm_atomic_state *state) > { > - struct drm_device *dev = state->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - int ret; > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > + struct drm_i915_private *dev_priv = state->dev->dev_private; > + struct drm_crtc *crtc; > + struct drm_crtc_state *crtc_state; > + int ret = 0, i; > > if (!check_digital_port_conflicts(state)) { > DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); > return -EINVAL; > } > > + intel_state->modeset = true; > + intel_state->active_crtcs = dev_priv->active_crtcs; > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + if (crtc_state->active) > + intel_state->active_crtcs |= 1 << i; > + else > + intel_state->active_crtcs &= ~(1 << i); > + } > + > /* > * See if the config requires any additional preparation, e.g. > * to adjust global state with pipes off. We need to do this > @@ -13101,7 +13137,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) > > intel_modeset_clear_plls(state); > > - if (IS_HASWELL(dev)) > + if (IS_HASWELL(dev_priv)) > return haswell_mode_set_planes_workaround(state); > > return 0; > @@ -13318,12 +13354,12 @@ static int intel_atomic_commit(struct drm_device *dev, > struct drm_atomic_state *state, > bool async) > { > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc_state *crtc_state; > struct drm_crtc *crtc; > - int ret = 0; > - int i; > - bool any_ms = false; > + int ret = 0, i; > + bool hw_check = intel_state->modeset; > > ret = intel_atomic_prepare_commit(dev, state, async); > if (ret) { > @@ -13334,13 +13370,18 @@ static int intel_atomic_commit(struct drm_device *dev, > drm_atomic_helper_swap_state(dev, state); > dev_priv->wm.config = to_intel_atomic_state(state)->wm_config; > > + if (intel_state->modeset) { > + memcpy(dev_priv->min_pixclk, intel_state->min_pixclk, > + sizeof(intel_state->min_pixclk)); > + dev_priv->active_crtcs = intel_state->active_crtcs; > + } > + > for_each_crtc_in_state(state, crtc, crtc_state, i) { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > if (!needs_modeset(crtc->state)) > continue; > > - any_ms = true; > intel_pre_plane_update(intel_crtc); > > if (crtc_state->active) { > @@ -13355,7 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev, > * update the the output configuration. */ > intel_modeset_update_crtc_state(state); > > - if (any_ms) { > + if (intel_state->modeset) { > intel_shared_dpll_commit(state); > > drm_atomic_helper_update_legacy_modeset_state(state->dev, state); > @@ -13382,7 +13423,7 @@ static int intel_atomic_commit(struct drm_device *dev, > put_domains = modeset_get_crtc_power_domains(crtc); > > /* make sure intel_modeset_check_state runs */ > - any_ms = true; > + hw_check = true; > } > > if (!modeset) > @@ -13409,7 +13450,7 @@ static int intel_atomic_commit(struct drm_device *dev, > drm_atomic_helper_cleanup_planes(dev, state); > mutex_unlock(&dev->struct_mutex); > > - if (any_ms) > + if (hw_check) > intel_modeset_check_state(dev, state); > > drm_atomic_state_free(state); > @@ -15417,16 +15458,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > struct intel_connector *connector; > int i; > > + dev_priv->active_crtcs = 0; > + > for_each_intel_crtc(dev, crtc) { > - __drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state); > - memset(crtc->config, 0, sizeof(*crtc->config)); > - crtc->config->base.crtc = &crtc->base; > + struct intel_crtc_state *crtc_state = crtc->config; > + int pixclk = 0; > > - crtc->active = dev_priv->display.get_pipe_config(crtc, > - crtc->config); > + __drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base); > + memset(crtc_state, 0, sizeof(*crtc_state)); > + crtc_state->base.crtc = &crtc->base; > > - crtc->base.state->active = crtc->active; > - crtc->base.enabled = crtc->active; > + crtc_state->base.active = crtc_state->base.enable = > + dev_priv->display.get_pipe_config(crtc, crtc_state); > + > + crtc->base.enabled = crtc_state->base.enable; > + crtc->active = crtc_state->base.active; > + > + if (crtc_state->base.active) { > + dev_priv->active_crtcs |= 1 << crtc->pipe; > + > + if (IS_BROADWELL(dev_priv)) { > + pixclk = ilk_pipe_pixel_rate(crtc_state); > + > + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ > + if (crtc_state->ips_enabled) > + pixclk = DIV_ROUND_UP(pixclk * 100, 95); > + } else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv)) > + pixclk = crtc_state->base.adjusted_mode.crtc_clock; What happens with the other platforms? > + } > + > + dev_priv->min_pixclk[crtc->pipe] = pixclk; > > readout_plane_state(crtc); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 4dfca58d37bf..8cfc599fd413 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -246,7 +246,12 @@ struct intel_atomic_state { > struct drm_atomic_state base; > > unsigned int cdclk; > - bool dpll_set; > + > + bool dpll_set, modeset; > + > + unsigned int active_crtcs; > + unsigned int min_pixclk[I915_MAX_PIPES]; > + > struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS]; > struct intel_wm_config wm_config; > };
On Tue, Dec 15, 2015 at 12:22:40PM +0200, Mika Kahola wrote: > On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote: > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3c46037b6e55..178a042f917e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > > static int intel_mode_max_pixclk(struct drm_device *dev, > > struct drm_atomic_state *state) > > { > > - struct intel_crtc *intel_crtc; > > - struct intel_crtc_state *crtc_state; > > - int max_pixclk = 0; > > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > > + struct drm_i915_private *dev_priv = dev->dev_private; > This is a nitpick but we should use nowadays to_i915() If you're going to bring that up, we should be passing in the right pointer to begin with! -Chris
Op 15-12-15 om 11:22 schreef Mika Kahola: > On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote: >> Parallel modesets are still not allowed, but this will allow updating >> a different crtc during a modeset if the clock is not changed. >> >> Additionally when all pipes are DPMS off the cdclk will be lowered >> to the minimum allowed. >> >> Changes since v1: >> - Add dev_priv->active_crtcs for tracking which crtcs are active. >> - Rename min_cdclk to min_pixclk and move to dev_priv. >> - Add a active_crtcs mask which is updated atomically. >> - Add intel_atomic_state->modeset which is set on modesets. >> - Commit new pixclk/active_crtcs right after state swap. >> Changes since v2: >> - Make the changes related to max_pixel_rate calculations more readable. >> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/i915_drv.h | 5 ++ >> drivers/gpu/drm/i915/intel_atomic.c | 2 +- >> drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++---------- >> drivers/gpu/drm/i915/intel_drv.h | 7 +- >> 4 files changed, 112 insertions(+), 41 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h >> index a32571f0d018..c7b76c2e8e09 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -1821,8 +1821,13 @@ struct drm_i915_private { >> struct intel_pipe_crc pipe_crc[I915_MAX_PIPES]; >> #endif >> >> + /* dpll and cdclk state is protected by connection_mutex */ >> int num_shared_dpll; >> struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; >> + >> + unsigned int active_crtcs; >> + unsigned int min_pixclk[I915_MAX_PIPES]; >> + >> int dpio_phy_iosf_port[I915_NUM_PHYS_VLV]; >> >> struct i915_workarounds workarounds; >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >> index 643f342de33b..c4eadbc928b7 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s) >> { >> struct intel_atomic_state *state = to_intel_atomic_state(s); >> drm_atomic_state_default_clear(&state->base); >> - state->dpll_set = false; >> + state->dpll_set = state->modeset = false; >> } >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 3c46037b6e55..178a042f917e 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, >> static int intel_mode_max_pixclk(struct drm_device *dev, >> struct drm_atomic_state *state) >> { >> - struct intel_crtc *intel_crtc; >> - struct intel_crtc_state *crtc_state; >> - int max_pixclk = 0; >> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); >> + struct drm_i915_private *dev_priv = dev->dev_private; > This is a nitpick but we should use nowadays to_i915() > >> + struct drm_crtc *crtc; >> + struct drm_crtc_state *crtc_state; >> + unsigned max_pixclk = 0, i; >> + enum pipe pipe; >> >> - for_each_intel_crtc(dev, intel_crtc) { >> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >> - if (IS_ERR(crtc_state)) >> - return PTR_ERR(crtc_state); >> + memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, >> + sizeof(intel_state->min_pixclk)); >> >> - if (!crtc_state->base.enable) >> - continue; >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + int pixclk = 0; >> + >> + if (crtc_state->enable) >> + pixclk = crtc_state->adjusted_mode.crtc_clock; >> >> - max_pixclk = max(max_pixclk, >> - crtc_state->base.adjusted_mode.crtc_clock); >> + intel_state->min_pixclk[i] = pixclk; >> } >> >> + if (!intel_state->active_crtcs) >> + return 0; >> + >> + for_each_pipe(dev_priv, pipe) >> + max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk); >> + >> return max_pixclk; >> } >> >> @@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) >> for_each_power_domain(domain, domains) >> intel_display_power_put(dev_priv, domain); >> intel_crtc->enabled_power_domains = 0; >> + >> + dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe); >> + dev_priv->min_pixclk[intel_crtc->pipe] = 0; >> } >> >> /* >> @@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) >> /* compute the max rate for new configuration */ >> static int ilk_max_pixel_rate(struct drm_atomic_state *state) >> { >> - struct intel_crtc *intel_crtc; >> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); >> + struct drm_i915_private *dev_priv = state->dev->dev_private; >> + struct drm_crtc *crtc; >> + struct drm_crtc_state *cstate; >> struct intel_crtc_state *crtc_state; >> - int max_pixel_rate = 0; >> + unsigned max_pixel_rate = 0, i; >> + enum pipe pipe; >> >> - for_each_intel_crtc(state->dev, intel_crtc) { >> - int pixel_rate; >> + memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, >> + sizeof(intel_state->min_pixclk)); >> >> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); >> - if (IS_ERR(crtc_state)) >> - return PTR_ERR(crtc_state); >> + for_each_crtc_in_state(state, crtc, cstate, i) { >> + int pixel_rate; >> >> - if (!crtc_state->base.enable) >> + crtc_state = to_intel_crtc_state(cstate); >> + if (!crtc_state->base.enable) { >> + intel_state->min_pixclk[i] = 0; >> continue; >> + } >> >> pixel_rate = ilk_pipe_pixel_rate(crtc_state); >> >> /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ >> - if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled) >> + if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) >> pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95); >> >> - max_pixel_rate = max(max_pixel_rate, pixel_rate); >> + intel_state->min_pixclk[i] = pixel_rate; >> } >> >> + if (!intel_state->active_crtcs) >> + return 0; >> + >> + for_each_pipe(dev_priv, pipe) >> + max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate); >> + >> return max_pixel_rate; >> } >> >> @@ -13069,15 +13093,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state) >> >> static int intel_modeset_checks(struct drm_atomic_state *state) >> { >> - struct drm_device *dev = state->dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - int ret; >> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); >> + struct drm_i915_private *dev_priv = state->dev->dev_private; >> + struct drm_crtc *crtc; >> + struct drm_crtc_state *crtc_state; >> + int ret = 0, i; >> >> if (!check_digital_port_conflicts(state)) { >> DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); >> return -EINVAL; >> } >> >> + intel_state->modeset = true; >> + intel_state->active_crtcs = dev_priv->active_crtcs; >> + >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + if (crtc_state->active) >> + intel_state->active_crtcs |= 1 << i; >> + else >> + intel_state->active_crtcs &= ~(1 << i); >> + } >> + >> /* >> * See if the config requires any additional preparation, e.g. >> * to adjust global state with pipes off. We need to do this >> @@ -13101,7 +13137,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) >> >> intel_modeset_clear_plls(state); >> >> - if (IS_HASWELL(dev)) >> + if (IS_HASWELL(dev_priv)) >> return haswell_mode_set_planes_workaround(state); >> >> return 0; >> @@ -13318,12 +13354,12 @@ static int intel_atomic_commit(struct drm_device *dev, >> struct drm_atomic_state *state, >> bool async) >> { >> + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct drm_crtc_state *crtc_state; >> struct drm_crtc *crtc; >> - int ret = 0; >> - int i; >> - bool any_ms = false; >> + int ret = 0, i; >> + bool hw_check = intel_state->modeset; >> >> ret = intel_atomic_prepare_commit(dev, state, async); >> if (ret) { >> @@ -13334,13 +13370,18 @@ static int intel_atomic_commit(struct drm_device *dev, >> drm_atomic_helper_swap_state(dev, state); >> dev_priv->wm.config = to_intel_atomic_state(state)->wm_config; >> >> + if (intel_state->modeset) { >> + memcpy(dev_priv->min_pixclk, intel_state->min_pixclk, >> + sizeof(intel_state->min_pixclk)); >> + dev_priv->active_crtcs = intel_state->active_crtcs; >> + } >> + >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> >> if (!needs_modeset(crtc->state)) >> continue; >> >> - any_ms = true; >> intel_pre_plane_update(intel_crtc); >> >> if (crtc_state->active) { >> @@ -13355,7 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev, >> * update the the output configuration. */ >> intel_modeset_update_crtc_state(state); >> >> - if (any_ms) { >> + if (intel_state->modeset) { >> intel_shared_dpll_commit(state); >> >> drm_atomic_helper_update_legacy_modeset_state(state->dev, state); >> @@ -13382,7 +13423,7 @@ static int intel_atomic_commit(struct drm_device *dev, >> put_domains = modeset_get_crtc_power_domains(crtc); >> >> /* make sure intel_modeset_check_state runs */ >> - any_ms = true; >> + hw_check = true; >> } >> >> if (!modeset) >> @@ -13409,7 +13450,7 @@ static int intel_atomic_commit(struct drm_device *dev, >> drm_atomic_helper_cleanup_planes(dev, state); >> mutex_unlock(&dev->struct_mutex); >> >> - if (any_ms) >> + if (hw_check) >> intel_modeset_check_state(dev, state); >> >> drm_atomic_state_free(state); >> @@ -15417,16 +15458,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) >> struct intel_connector *connector; >> int i; >> >> + dev_priv->active_crtcs = 0; >> + >> for_each_intel_crtc(dev, crtc) { >> - __drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state); >> - memset(crtc->config, 0, sizeof(*crtc->config)); >> - crtc->config->base.crtc = &crtc->base; >> + struct intel_crtc_state *crtc_state = crtc->config; >> + int pixclk = 0; >> >> - crtc->active = dev_priv->display.get_pipe_config(crtc, >> - crtc->config); >> + __drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base); >> + memset(crtc_state, 0, sizeof(*crtc_state)); >> + crtc_state->base.crtc = &crtc->base; >> >> - crtc->base.state->active = crtc->active; >> - crtc->base.enabled = crtc->active; >> + crtc_state->base.active = crtc_state->base.enable = >> + dev_priv->display.get_pipe_config(crtc, crtc_state); >> + >> + crtc->base.enabled = crtc_state->base.enable; >> + crtc->active = crtc_state->base.active; >> + >> + if (crtc_state->base.active) { >> + dev_priv->active_crtcs |= 1 << crtc->pipe; >> + >> + if (IS_BROADWELL(dev_priv)) { >> + pixclk = ilk_pipe_pixel_rate(crtc_state); >> + >> + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ >> + if (crtc_state->ips_enabled) >> + pixclk = DIV_ROUND_UP(pixclk * 100, 95); >> + } else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv)) >> + pixclk = crtc_state->base.adjusted_mode.crtc_clock; > What happens with the other platforms? > Well the other platforms have no support for cdclk adjustment, but it seems I would need to add a IS_CHERRYVIEW() on that if branch, and perhaps a else WARN_ON(dev_priv->display.modeset_calc_cdclk); as well to make sure any future platform is covered.
On Tue, 2015-12-15 at 10:26 +0000, Chris Wilson wrote: > On Tue, Dec 15, 2015 at 12:22:40PM +0200, Mika Kahola wrote: > > On Tue, 2015-11-24 at 11:29 +0100, Maarten Lankhorst wrote: > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > > index 3c46037b6e55..178a042f917e 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, > > > static int intel_mode_max_pixclk(struct drm_device *dev, > > > struct drm_atomic_state *state) > > > { > > > - struct intel_crtc *intel_crtc; > > > - struct intel_crtc_state *crtc_state; > > > - int max_pixclk = 0; > > > + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > This is a nitpick but we should use nowadays to_i915() > > If you're going to bring that up, we should be passing in the right > pointer to begin with! > -Chris This was more meant be as a notification. I was advised that we should use 'to_i915()' in a new code. Personally, I don't have any preference one way or another. Both ways will work. Maybe this would be a topic for a follow up patch? Cheers, Mika
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index a32571f0d018..c7b76c2e8e09 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1821,8 +1821,13 @@ struct drm_i915_private { struct intel_pipe_crc pipe_crc[I915_MAX_PIPES]; #endif + /* dpll and cdclk state is protected by connection_mutex */ int num_shared_dpll; struct intel_shared_dpll shared_dplls[I915_NUM_PLLS]; + + unsigned int active_crtcs; + unsigned int min_pixclk[I915_MAX_PIPES]; + int dpio_phy_iosf_port[I915_NUM_PHYS_VLV]; struct i915_workarounds workarounds; diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 643f342de33b..c4eadbc928b7 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -306,5 +306,5 @@ void intel_atomic_state_clear(struct drm_atomic_state *s) { struct intel_atomic_state *state = to_intel_atomic_state(s); drm_atomic_state_default_clear(&state->base); - state->dpll_set = false; + state->dpll_set = state->modeset = false; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3c46037b6e55..178a042f917e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5991,22 +5991,31 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv, static int intel_mode_max_pixclk(struct drm_device *dev, struct drm_atomic_state *state) { - struct intel_crtc *intel_crtc; - struct intel_crtc_state *crtc_state; - int max_pixclk = 0; + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + struct drm_i915_private *dev_priv = dev->dev_private; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + unsigned max_pixclk = 0, i; + enum pipe pipe; - for_each_intel_crtc(dev, intel_crtc) { - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); + memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, + sizeof(intel_state->min_pixclk)); - if (!crtc_state->base.enable) - continue; + for_each_crtc_in_state(state, crtc, crtc_state, i) { + int pixclk = 0; + + if (crtc_state->enable) + pixclk = crtc_state->adjusted_mode.crtc_clock; - max_pixclk = max(max_pixclk, - crtc_state->base.adjusted_mode.crtc_clock); + intel_state->min_pixclk[i] = pixclk; } + if (!intel_state->active_crtcs) + return 0; + + for_each_pipe(dev_priv, pipe) + max_pixclk = max(intel_state->min_pixclk[pipe], max_pixclk); + return max_pixclk; } @@ -6310,6 +6319,9 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) for_each_power_domain(domain, domains) intel_display_power_put(dev_priv, domain); intel_crtc->enabled_power_domains = 0; + + dev_priv->active_crtcs &= ~(1 << intel_crtc->pipe); + dev_priv->min_pixclk[intel_crtc->pipe] = 0; } /* @@ -9555,29 +9567,41 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state) /* compute the max rate for new configuration */ static int ilk_max_pixel_rate(struct drm_atomic_state *state) { - struct intel_crtc *intel_crtc; + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + struct drm_i915_private *dev_priv = state->dev->dev_private; + struct drm_crtc *crtc; + struct drm_crtc_state *cstate; struct intel_crtc_state *crtc_state; - int max_pixel_rate = 0; + unsigned max_pixel_rate = 0, i; + enum pipe pipe; - for_each_intel_crtc(state->dev, intel_crtc) { - int pixel_rate; + memcpy(intel_state->min_pixclk, dev_priv->min_pixclk, + sizeof(intel_state->min_pixclk)); - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc); - if (IS_ERR(crtc_state)) - return PTR_ERR(crtc_state); + for_each_crtc_in_state(state, crtc, cstate, i) { + int pixel_rate; - if (!crtc_state->base.enable) + crtc_state = to_intel_crtc_state(cstate); + if (!crtc_state->base.enable) { + intel_state->min_pixclk[i] = 0; continue; + } pixel_rate = ilk_pipe_pixel_rate(crtc_state); /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ - if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled) + if (IS_BROADWELL(dev_priv) && crtc_state->ips_enabled) pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95); - max_pixel_rate = max(max_pixel_rate, pixel_rate); + intel_state->min_pixclk[i] = pixel_rate; } + if (!intel_state->active_crtcs) + return 0; + + for_each_pipe(dev_priv, pipe) + max_pixel_rate = max(intel_state->min_pixclk[pipe], max_pixel_rate); + return max_pixel_rate; } @@ -13069,15 +13093,27 @@ static int intel_modeset_all_pipes(struct drm_atomic_state *state) static int intel_modeset_checks(struct drm_atomic_state *state) { - struct drm_device *dev = state->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - int ret; + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); + struct drm_i915_private *dev_priv = state->dev->dev_private; + struct drm_crtc *crtc; + struct drm_crtc_state *crtc_state; + int ret = 0, i; if (!check_digital_port_conflicts(state)) { DRM_DEBUG_KMS("rejecting conflicting digital port configuration\n"); return -EINVAL; } + intel_state->modeset = true; + intel_state->active_crtcs = dev_priv->active_crtcs; + + for_each_crtc_in_state(state, crtc, crtc_state, i) { + if (crtc_state->active) + intel_state->active_crtcs |= 1 << i; + else + intel_state->active_crtcs &= ~(1 << i); + } + /* * See if the config requires any additional preparation, e.g. * to adjust global state with pipes off. We need to do this @@ -13101,7 +13137,7 @@ static int intel_modeset_checks(struct drm_atomic_state *state) intel_modeset_clear_plls(state); - if (IS_HASWELL(dev)) + if (IS_HASWELL(dev_priv)) return haswell_mode_set_planes_workaround(state); return 0; @@ -13318,12 +13354,12 @@ static int intel_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool async) { + struct intel_atomic_state *intel_state = to_intel_atomic_state(state); struct drm_i915_private *dev_priv = dev->dev_private; struct drm_crtc_state *crtc_state; struct drm_crtc *crtc; - int ret = 0; - int i; - bool any_ms = false; + int ret = 0, i; + bool hw_check = intel_state->modeset; ret = intel_atomic_prepare_commit(dev, state, async); if (ret) { @@ -13334,13 +13370,18 @@ static int intel_atomic_commit(struct drm_device *dev, drm_atomic_helper_swap_state(dev, state); dev_priv->wm.config = to_intel_atomic_state(state)->wm_config; + if (intel_state->modeset) { + memcpy(dev_priv->min_pixclk, intel_state->min_pixclk, + sizeof(intel_state->min_pixclk)); + dev_priv->active_crtcs = intel_state->active_crtcs; + } + for_each_crtc_in_state(state, crtc, crtc_state, i) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); if (!needs_modeset(crtc->state)) continue; - any_ms = true; intel_pre_plane_update(intel_crtc); if (crtc_state->active) { @@ -13355,7 +13396,7 @@ static int intel_atomic_commit(struct drm_device *dev, * update the the output configuration. */ intel_modeset_update_crtc_state(state); - if (any_ms) { + if (intel_state->modeset) { intel_shared_dpll_commit(state); drm_atomic_helper_update_legacy_modeset_state(state->dev, state); @@ -13382,7 +13423,7 @@ static int intel_atomic_commit(struct drm_device *dev, put_domains = modeset_get_crtc_power_domains(crtc); /* make sure intel_modeset_check_state runs */ - any_ms = true; + hw_check = true; } if (!modeset) @@ -13409,7 +13450,7 @@ static int intel_atomic_commit(struct drm_device *dev, drm_atomic_helper_cleanup_planes(dev, state); mutex_unlock(&dev->struct_mutex); - if (any_ms) + if (hw_check) intel_modeset_check_state(dev, state); drm_atomic_state_free(state); @@ -15417,16 +15458,36 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) struct intel_connector *connector; int i; + dev_priv->active_crtcs = 0; + for_each_intel_crtc(dev, crtc) { - __drm_atomic_helper_crtc_destroy_state(&crtc->base, crtc->base.state); - memset(crtc->config, 0, sizeof(*crtc->config)); - crtc->config->base.crtc = &crtc->base; + struct intel_crtc_state *crtc_state = crtc->config; + int pixclk = 0; - crtc->active = dev_priv->display.get_pipe_config(crtc, - crtc->config); + __drm_atomic_helper_crtc_destroy_state(&crtc->base, &crtc_state->base); + memset(crtc_state, 0, sizeof(*crtc_state)); + crtc_state->base.crtc = &crtc->base; - crtc->base.state->active = crtc->active; - crtc->base.enabled = crtc->active; + crtc_state->base.active = crtc_state->base.enable = + dev_priv->display.get_pipe_config(crtc, crtc_state); + + crtc->base.enabled = crtc_state->base.enable; + crtc->active = crtc_state->base.active; + + if (crtc_state->base.active) { + dev_priv->active_crtcs |= 1 << crtc->pipe; + + if (IS_BROADWELL(dev_priv)) { + pixclk = ilk_pipe_pixel_rate(crtc_state); + + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */ + if (crtc_state->ips_enabled) + pixclk = DIV_ROUND_UP(pixclk * 100, 95); + } else if (IS_BROXTON(dev_priv) || IS_VALLEYVIEW(dev_priv)) + pixclk = crtc_state->base.adjusted_mode.crtc_clock; + } + + dev_priv->min_pixclk[crtc->pipe] = pixclk; readout_plane_state(crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 4dfca58d37bf..8cfc599fd413 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -246,7 +246,12 @@ struct intel_atomic_state { struct drm_atomic_state base; unsigned int cdclk; - bool dpll_set; + + bool dpll_set, modeset; + + unsigned int active_crtcs; + unsigned int min_pixclk[I915_MAX_PIPES]; + struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS]; struct intel_wm_config wm_config; };
Parallel modesets are still not allowed, but this will allow updating a different crtc during a modeset if the clock is not changed. Additionally when all pipes are DPMS off the cdclk will be lowered to the minimum allowed. Changes since v1: - Add dev_priv->active_crtcs for tracking which crtcs are active. - Rename min_cdclk to min_pixclk and move to dev_priv. - Add a active_crtcs mask which is updated atomically. - Add intel_atomic_state->modeset which is set on modesets. - Commit new pixclk/active_crtcs right after state swap. Changes since v2: - Make the changes related to max_pixel_rate calculations more readable. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/i915_drv.h | 5 ++ drivers/gpu/drm/i915/intel_atomic.c | 2 +- drivers/gpu/drm/i915/intel_display.c | 139 +++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_drv.h | 7 +- 4 files changed, 112 insertions(+), 41 deletions(-)