[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:55:55 PDT 2015
On Thu, Oct 22, 2015 at 03:42:58PM +0200, Maarten Lankhorst wrote:
> Op 22-10-15 om 15:08 schreef Daniel Vetter:
> > 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.
> Sorry this is the only case where a dependency on intel_crtc->active is valid.
> fbc and watermark users should stop looking at it. (intel_crtc_active)
>
> 2 crtc's, #1 requires clock at 100, #2 requires clock at 200.
> You do a DPMS mode off on #2, and if you don't track crtc_clock you would set the display clock to 100 and force a modeset on #1 too.
> If you immediately do dpms off on #1 afterwards it means that the user would see #1 being disabled, re-enabled and disabled again..
Well I assumed min_cdclk != 0 iff active. But the point here isn't to
outright remove the depency upon the active state (I agree we need that to
avoid needless flickering when only changing active state), but to remove
the specific locking restrictions on stuff in intel_crtc. Instead I think
it's better to have this stored in the global state, which naturally gives
us exactly the locking you want, without having to explicitly document it
(and look it up all the time).
> > 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.
> The tricks are needed to allow a modeset to run parallel to any page flips.
> Though perhaps it would be better to add a struct to dev_priv that contains
> min cdclk and a flag to see if the crtc is active or not.
Yup, I get the trick. I'm just not happy with the implementation, it looks
way too much like legacy-modesest-lets-track-stuff-in-objects instead of
proper a atomic state based design.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
More information about the Intel-gfx
mailing list