[Intel-gfx] [PATCH 02/11] drm/i915: Do not acquire crtc state to check clock during modeset.
Daniel Vetter
daniel at ffwll.ch
Thu Oct 22 06:08:25 PDT 2015
On Thu, Oct 22, 2015 at 01:56:27PM +0200, 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.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> drivers/gpu/drm/i915/intel_display.c | 90 ++++++++++++++++++++++--------------
> drivers/gpu/drm/i915/intel_drv.h | 9 ++++
> 2 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 022e628b8520..051a1e2b1c55 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5237,6 +5237,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] = {};
> @@ -5245,13 +5246,20 @@ static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
> int i;
>
> for_each_crtc_in_state(state, crtc, crtc_state, i) {
> - if (needs_modeset(crtc->state))
> - put_domains[to_intel_crtc(crtc)->pipe] =
> - modeset_get_crtc_power_domains(crtc);
> + struct intel_crtc *intel_crtc =
> + to_intel_crtc(crtc);
> + enum pipe pipe = intel_crtc->pipe;
> +
> + if (!needs_modeset(crtc->state))
> + continue;
> +
> + intel_crtc->min_cdclk = intel_state->pipe_cdclk[pipe];
> +
> + put_domains[pipe] = modeset_get_crtc_power_domains(crtc);
> }
>
> if (dev_priv->display.modeset_commit_cdclk) {
> - unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
> + unsigned int cdclk = intel_state->cdclk;
>
> if (cdclk != dev_priv->cdclk_freq &&
> !WARN_ON(!state->allow_modeset))
> @@ -5919,7 +5927,6 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> /*
> * FIXME:
> * - remove the guardband, it's not needed on BXT
> - * - set 19.2MHz bypass frequency if there are no active pipes
> */
> if (max_pixclk > 576000*9/10)
> return 624000;
> @@ -5929,8 +5936,10 @@ static int broxton_calc_cdclk(struct drm_i915_private *dev_priv,
> return 384000;
> else if (max_pixclk > 144000*9/10)
> return 288000;
> - else
> + else if (max_pixclk)
> return 144000;
> + else
> + return 19200;
> }
>
> /* Compute the max pixel clock for new configuration. Uses atomic state if
> @@ -5939,22 +5948,31 @@ 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;
> + int max_pixel_rate = 0;
> + bool any_active = false;
>
> - 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);
> + for_each_intel_crtc(state->dev, intel_crtc) {
> + struct drm_crtc_state *drm_crtc_state;
> + struct drm_crtc *crtc = &intel_crtc->base;
> + int pixclk = 0;
>
> - if (!crtc_state->base.enable)
> - continue;
> + drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> + if (!drm_crtc_state) {
> + any_active |= intel_crtc->active;
> + pixclk = intel_crtc->min_cdclk;
> + } else if (drm_crtc_state->enable) {
> + struct intel_crtc_state *crtc_state =
> + to_intel_crtc_state(drm_crtc_state);
> +
> + any_active |= drm_crtc_state->active;
> + pixclk = crtc_state->base.adjusted_mode.crtc_clock;
> + }
>
> - max_pixclk = max(max_pixclk,
> - crtc_state->base.adjusted_mode.crtc_clock);
> + to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixclk;
> + max_pixel_rate = max(max_pixel_rate, pixclk);
> }
>
> - return max_pixclk;
> + return any_active ? max_pixel_rate : 0;
> }
>
> static int valleyview_modeset_calc_cdclk(struct drm_atomic_state *state)
> @@ -9491,29 +9509,35 @@ static void broxton_modeset_commit_cdclk(struct drm_atomic_state *old_state)
> static int ilk_max_pixel_rate(struct drm_atomic_state *state)
> {
> struct intel_crtc *intel_crtc;
> - struct intel_crtc_state *crtc_state;
> int max_pixel_rate = 0;
> + bool any_active = false;
>
> for_each_intel_crtc(state->dev, intel_crtc) {
> - int pixel_rate;
> -
> - crtc_state = intel_atomic_get_crtc_state(state, intel_crtc);
> - if (IS_ERR(crtc_state))
> - return PTR_ERR(crtc_state);
> + struct drm_crtc_state *drm_crtc_state;
> + struct drm_crtc *crtc = &intel_crtc->base;
> + int pixel_rate = 0;
>
> - if (!crtc_state->base.enable)
> - continue;
> + drm_crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
> + if (!drm_crtc_state) {
> + any_active |= intel_crtc->active;
> + pixel_rate = intel_crtc->min_cdclk;
> + } else if (drm_crtc_state->enable) {
> + struct intel_crtc_state *crtc_state =
> + to_intel_crtc_state(drm_crtc_state);
>
> - pixel_rate = ilk_pipe_pixel_rate(crtc_state);
> + any_active |= drm_crtc_state->active;
> + 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)
> - pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> + /* pixel rate mustn't exceed 95% of cdclk with IPS on BDW */
> + if (IS_BROADWELL(state->dev) && crtc_state->ips_enabled)
> + pixel_rate = DIV_ROUND_UP(pixel_rate * 100, 95);
> + }
>
> + to_intel_atomic_state(state)->pipe_cdclk[intel_crtc->pipe] = pixel_rate;
> max_pixel_rate = max(max_pixel_rate, pixel_rate);
> }
>
> - return max_pixel_rate;
> + return any_active ? max_pixel_rate : 0;
> }
>
> static void broadwell_set_cdclk(struct drm_device *dev, int cdclk)
> @@ -9612,14 +9636,10 @@ static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> else
> cdclk = 337500;
>
> - /*
> - * FIXME move the cdclk caclulation to
> - * compute_config() so we can fail gracegully.
> - */
> if (cdclk > dev_priv->max_cdclk_freq) {
> DRM_ERROR("requested cdclk (%d kHz) exceeds max (%d kHz)\n",
> cdclk, dev_priv->max_cdclk_freq);
> - cdclk = dev_priv->max_cdclk_freq;
> + return -EINVAL;
> }
>
> to_intel_atomic_state(state)->cdclk = cdclk;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 51722e657b91..54a2c0da2ece 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -250,6 +250,7 @@ struct intel_atomic_state {
> unsigned int cdclk;
> bool dpll_set;
> struct intel_shared_dpll_config shared_dpll[I915_NUM_PLLS];
> + unsigned int pipe_cdclk[I915_MAX_PIPES];
> };
>
> struct intel_plane_state {
> @@ -536,8 +537,16 @@ struct intel_crtc {
> * Whether the crtc and the connected output pipeline is active. Implies
> * that crtc->enabled is set, i.e. the current mode configuration has
> * some outputs connected to this crtc.
> + *
> + * Protected by crtc.mutex and connection_mutex
This needs to be clarified to mean: read-protected by connection_mutex,
writers additionally must hold crtc->mutex. Same below.
Thinking about this a bit more the usual design we do is put the state
copies not into the parent object with funky rules, but instead into the
state of the higher-level objects. So for example the read-only
plane_active state is in crtc_state->plane_mask.
Similarly in this case here I think it would be cleaner design to have a
min_cdclk array in intel_state, setting it to 0 if the pipe is totally
off. That way locking rules are 100% clear: If you need to change it, you
need to copy the global state and acquire connection_mutex. That
automatically gives you read-access to all crtc's min_cdclk.
That way we don't play tricks with get_existing_state, don't add more
depencies on the legacy-encumbered intel_crtc->active and it's just
generally more awesome.
Thoughts?
-Daniel
> */
> bool active;
> +
> + /* Minimum cdclk required for this pipe,
> + * protected by crtc.mutex and connection_mutex.
> + */
> + unsigned int min_cdclk;
> +
> unsigned long enabled_power_domains;
> bool lowfreq_avail;
> struct intel_overlay *overlay;
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list