On Thu, 09 Oct 2014, Daniel Vetter <daniel@xxxxxxxx> 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@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.
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@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
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
