[Intel-gfx] [PATCH 3/4] drm/i915: Do not acquire crtc state to check clock during modeset, v3.
Maarten Lankhorst
maarten.lankhorst at linux.intel.com
Wed Dec 16 02:10:28 PST 2015
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 at 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.
More information about the Intel-gfx
mailing list