Re: [PATCH 4/4] drm/i915: Compute clocks and choose DPLLs before disabling crtcs

[Jani Nikula <jani.nikula@linux.intel.com>]

On Thu, 09 Oct 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Oct 08, 2014 at 06:32:23PM +0300, Ander Conselvan de Oliveira wrote:
>> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>>
>> It is possible for a mode set to fail if there aren't shared DPLLS that
>> match the new configuration requirement or other errors in clock
>> computation. Since that step was executed after disabling crtcs, in the
>> failure case the hardware configuration is changed and needs to be
>> restored. Doing those things early allows the mode set to fail before
>> actually touching the hardware.
>>
>> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
>
> Looks nifty and gets us to the shiny new world without too much fuzz. I
> think for paranoia it would be good to split this patch here per platform
> (i.e. i9xx, ilk+ and hsw+). And if we make the crtc_mode_set hook optional
> we can also get rid of all the dummy functions here. And remove the caller
> (yay!) at the end of the series.

Should we add WARN_ON(!encoder->new_crtc) all around?

Jani.

>
> Longer-term we might still want to shuffle quite a few of the
> encoder-specific pll computation code into encoder->compute_config. But
> that can be done as leisure if someone gets bored ;-)
>
> Cheers, Daniel
>
>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h      |   7 +++
>>  drivers/gpu/drm/i915/intel_ddi.c     |   2 -
>>  drivers/gpu/drm/i915/intel_display.c | 113 ++++++++++++++++++++++++++---------
>>  3 files changed, 92 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index cb6df07..0bb3b54 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -245,6 +245,12 @@ struct intel_shared_dpll {
>>   bool (*get_hw_state)(struct drm_i915_private *dev_priv,
>>       struct intel_shared_dpll *pll,
>>       struct intel_dpll_hw_state *hw_state);
>> +
>> + /* Holds the new configuration of the shared DPLLS during mode set */
>> + struct {
>> + unsigned crtc_mask;
>> + struct intel_dpll_hw_state hw_state;
>> + } staged_config;
>>  };
>>
>>  /* Used by dp and fdi links */
>> @@ -476,6 +482,7 @@ struct drm_i915_display_funcs {
>>   struct intel_crtc_config *);
>>   void (*get_plane_config)(struct intel_crtc *,
>>   struct intel_plane_config *);
>> + int (*crtc_compute_clock)(struct drm_crtc *crtc);
>>   int (*crtc_mode_set)(struct drm_crtc *crtc,
>>       int x, int y,
>>       struct drm_framebuffer *old_fb);
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 619723d..fc6f988 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -821,8 +821,6 @@ bool intel_ddi_pll_select(struct intel_crtc *intel_crtc)
>>   struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc);
>>   int clock = intel_crtc->config.port_clock;
>>
>> - intel_put_shared_dpll(intel_crtc);
>> -
>>   return hsw_ddi_pll_select(intel_crtc, intel_encoder, clock);
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 46df2db..4a8aff7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -3852,15 +3852,9 @@ void intel_put_shared_dpll(struct intel_crtc *crtc)
>>  struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>  {
>>   struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> - struct intel_shared_dpll *pll = intel_crtc_to_shared_dpll(crtc);
>> + struct intel_shared_dpll *pll;
>>   enum intel_dpll_id i;
>>
>> - if (pll) {
>> - DRM_DEBUG_KMS("CRTC:%d dropping existing %s\n",
>> -      crtc->base.base.id, pll->name);
>> - intel_put_shared_dpll(crtc);
>> - }
>> -
>>   if (HAS_PCH_IBX(dev_priv->dev)) {
>>   /* Ironlake PCH has a fixed PLL->PCH pipe mapping. */
>>   i = (enum intel_dpll_id) crtc->pipe;
>> @@ -3869,7 +3863,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n",
>>        crtc->base.base.id, pll->name);
>>
>> - WARN_ON(pll->crtc_mask);
>> + WARN_ON(pll->staged_config.crtc_mask);
>>
>>   goto found;
>>   }
>> @@ -3878,16 +3872,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   pll = &dev_priv->shared_dplls[i];
>>
>>   /* Only want to check enabled timings first */
>> - if (pll->crtc_mask == 0)
>> + if (pll->staged_config.crtc_mask == 0)
>>   continue;
>>
>> - if (memcmp(&crtc->config.dpll_hw_state, &pll->hw_state,
>> -   sizeof(pll->hw_state)) == 0) {
>> - DRM_DEBUG_KMS("CRTC:%d sharing existing %s "
>> -      "(crtc_mask 0x%08x, active %d)\n",
>> + if (memcmp(&crtc->new_config->dpll_hw_state,
>> +   &pll->staged_config.hw_state,
>> +   sizeof(pll->staged_config.hw_state)) == 0) {
>> + DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative %d)\n",
>>        crtc->base.base.id, pll->name,
>> -      pll->crtc_mask, pll->active);
>> -
>> +      pll->staged_config.crtc_mask,
>> +      pll->active);
>>   goto found;
>>   }
>>   }
>> @@ -3895,7 +3889,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   /* Ok no matching timings, maybe there's a free one? */
>>   for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>>   pll = &dev_priv->shared_dplls[i];
>> - if (pll->crtc_mask == 0) {
>> + if (pll->staged_config.crtc_mask == 0) {
>>   DRM_DEBUG_KMS("CRTC:%d allocated %s\n",
>>        crtc->base.base.id, pll->name);
>>   goto found;
>> @@ -3905,18 +3899,53 @@ struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crtc)
>>   return NULL;
>>
>>  found:
>> - if (pll->crtc_mask == 0)
>> - pll->hw_state = crtc->config.dpll_hw_state;
>> + if (pll->staged_config.crtc_mask == 0)
>> + pll->staged_config.hw_state = crtc->new_config->dpll_hw_state;
>>
>> - crtc->config.shared_dpll = i;
>> + crtc->new_config->shared_dpll = i;
>>   DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name,
>>   pipe_name(crtc->pipe));
>>
>> - pll->crtc_mask |= 1 << crtc->pipe;
>> + pll->staged_config.crtc_mask |= 1 << crtc->pipe;
>>
>>   return pll;
>>  }
>>
>> +/**
>> + * intel_shared_dpll_start_config - start a new PLL staged config
>> + * @dev_priv: DRM device
>> + * @clear_pipes: mask of pipes that will have their PLLs freed
>> + *
>> + * Starts a new PLL staged config, copying the current config but
>> + * releasing the references of pipes specified in clear_pipes.
>> + */
>> +static void intel_shared_dpll_start_config(struct drm_i915_private *dev_priv,
>> +   unsigned clear_pipes)
>> +{
>> + struct intel_shared_dpll *pll;
>> + enum intel_dpll_id i;
>> +
>> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> + pll = &dev_priv->shared_dplls[i];
>> +
>> + pll->staged_config.crtc_mask = pll->crtc_mask & ~clear_pipes;
>> + pll->staged_config.hw_state = pll->hw_state;
>> + }
>> +}
>> +
>> +static void intel_shared_dpll_commit(struct drm_i915_private *dev_priv)
>> +{
>> + struct intel_shared_dpll *pll;
>> + enum intel_dpll_id i;
>> +
>> + for (i = 0; i < dev_priv->num_shared_dpll; i++) {
>> + pll = &dev_priv->shared_dplls[i];
>> +
>> + pll->hw_state = pll->staged_config.hw_state;
>> + pll->crtc_mask = pll->staged_config.crtc_mask;
>> + }
>> +}
>> +
>>  static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>>  {
>>   struct drm_i915_private *dev_priv = dev->dev_private;
>> @@ -6272,6 +6301,11 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>>        int x, int y,
>>        struct drm_framebuffer *fb)
>>  {
>> + return 0;
>> +}
>> +
>> +static int i9xx_crtc_compute_clock(struct drm_crtc *crtc)
>> +{
>>   struct drm_device *dev = crtc->dev;
>>   struct drm_i915_private *dev_priv = dev->dev_private;
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -7281,9 +7315,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>>   return dpll | DPLL_VCO_ENABLE;
>>  }
>>
>> -static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>> -  int x, int y,
>> -  struct drm_framebuffer *fb)
>> +static int ironlake_crtc_compute_clock(struct drm_crtc *crtc)
>>  {
>>   struct drm_device *dev = crtc->dev;
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>> @@ -7336,8 +7368,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>   pipe_name(intel_crtc->pipe));
>>   return -EINVAL;
>>   }
>> - } else
>> - intel_put_shared_dpll(intel_crtc);
>> + }
>>
>>   if (is_lvds && has_reduced_clock && i915.powersave)
>>   intel_crtc->lowfreq_avail = true;
>> @@ -7347,6 +7378,13 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>   return 0;
>>  }
>>
>> +static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>> +  int x, int y,
>> +  struct drm_framebuffer *fb)
>> +{
>> + return 0;
>> +}
>> +
>>  static void intel_pch_transcoder_get_m_n(struct intel_crtc *crtc,
>>   struct intel_link_m_n *m_n)
>>  {
>> @@ -7836,9 +7874,7 @@ static void haswell_modeset_global_resources(struct drm_device *dev)
>>   modeset_update_crtc_power_domains(dev);
>>  }
>>
>> -static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> - int x, int y,
>> - struct drm_framebuffer *fb)
>> +static int haswell_crtc_compute_clock(struct drm_crtc *crtc)
>>  {
>>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>
>> @@ -7850,6 +7886,13 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>>   return 0;
>>  }
>>
>> +static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>> + int x, int y,
>> + struct drm_framebuffer *fb)
>> +{
>> + return 0;
>> +}
>> +
>>  static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv,
>>   enum port port,
>>   struct intel_crtc_config *pipe_config)
>> @@ -10988,6 +11031,14 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>   prepare_pipes &= ~disable_pipes;
>>   }
>>
>> + intel_shared_dpll_start_config(dev_priv, modeset_pipes | disable_pipes);
>> +
>> + for_each_intel_crtc_masked(dev, modeset_pipes, intel_crtc) {
>> + ret = dev_priv->display.crtc_compute_clock(&intel_crtc->base);
>> + if (ret)
>> + goto done;
>> + }
>> +
>>   for_each_intel_crtc_masked(dev, disable_pipes, intel_crtc)
>>   intel_crtc_disable(&intel_crtc->base);
>>
>> @@ -11015,6 +11066,8 @@ static int __intel_set_mode(struct drm_crtc *crtc,
>>   &pipe_config->adjusted_mode);
>>   }
>>
>> + intel_shared_dpll_commit(dev_priv);
>> +
>>   /* Only after disabling all output pipelines that will be changed can we
>>   * update the the output configuration. */
>>   intel_modeset_update_state(dev, prepare_pipes);
>> @@ -12580,6 +12633,7 @@ static void intel_init_display(struct drm_device *dev)
>>   if (HAS_DDI(dev)) {
>>   dev_priv->display.get_pipe_config = haswell_get_pipe_config;
>>   dev_priv->display.get_plane_config = ironlake_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = haswell_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
>>   dev_priv->display.crtc_enable = haswell_crtc_enable;
>>   dev_priv->display.crtc_disable = haswell_crtc_disable;
>> @@ -12593,6 +12647,7 @@ static void intel_init_display(struct drm_device *dev)
>>   } else if (HAS_PCH_SPLIT(dev)) {
>>   dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
>>   dev_priv->display.get_plane_config = ironlake_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = ironlake_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = ironlake_crtc_mode_set;
>>   dev_priv->display.crtc_enable = ironlake_crtc_enable;
>>   dev_priv->display.crtc_disable = ironlake_crtc_disable;
>> @@ -12602,6 +12657,7 @@ static void intel_init_display(struct drm_device *dev)
>>   } else if (IS_VALLEYVIEW(dev)) {
>>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>>   dev_priv->display.get_plane_config = i9xx_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>>   dev_priv->display.crtc_enable = valleyview_crtc_enable;
>>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
>> @@ -12611,6 +12667,7 @@ static void intel_init_display(struct drm_device *dev)
>>   } else {
>>   dev_priv->display.get_pipe_config = i9xx_get_pipe_config;
>>   dev_priv->display.get_plane_config = i9xx_get_plane_config;
>> + dev_priv->display.crtc_compute_clock = i9xx_crtc_compute_clock;
>>   dev_priv->display.crtc_mode_set = i9xx_crtc_mode_set;
>>   dev_priv->display.crtc_enable = i9xx_crtc_enable;
>>   dev_priv->display.crtc_disable = i9xx_crtc_disable;
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center