On Wed, Oct 08, 2014 at 06:32:23PM +0300, Ander Conselvan de Oliveira wrote:
> From: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
>
> 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@xxxxxxxxx>
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.
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@xxxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
