From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ander Conselvan de Oliveira Subject: Re: [PATCH 3/8] drm/i915: Add infrastructure for choosing DPLLs before disabling crtcs Date: Wed, 22 Oct 2014 14:33:30 +0300 Message-ID: <5447960A.1030302@gmail.com> References: <1413896529-32443-1-git-send-email-ander.conselvan.de.oliveira@intel.com> <1413896529-32443-4-git-send-email-ander.conselvan.de.oliveira@intel.com> <20141022083321.GV4284@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTP id 9F3EE6E2CF for ; Wed, 22 Oct 2014 04:33:40 -0700 (PDT) In-Reply-To: <20141022083321.GV4284@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: =?windows-1252?Q?Ville_Syrj=E4l=E4?= , Ander Conselvan de Oliveira Cc: intel-gfx@lists.freedesktop.org, shuang.he@linux.intel.com List-Id: intel-gfx@lists.freedesktop.org On 10/22/2014 11:33 AM, Ville Syrj=E4l=E4 wrote: > On Tue, Oct 21, 2014 at 04:02:04PM +0300, Ander Conselvan de Oliveira wro= te: >> 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. If that step is executed after disabling crtcs, in the >> failure case the hardware configuration is changed and needs to be >> restored. Doing those things early will allow the mode set to fail >> before actually touching the hardware. >> >> Follow up patches will convert different platforms to use the new >> infrastructure. >> >> Signed-off-by: Ander Conselvan de Oliveira >> --- >> drivers/gpu/drm/i915/i915_drv.h | 3 + >> drivers/gpu/drm/i915/intel_ddi.c | 2 + >> drivers/gpu/drm/i915/intel_display.c | 125 +++++++++++++++++++++++++++= -------- >> 3 files changed, 102 insertions(+), 28 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915= _drv.h >> index d7412d9..5b2464f 100644 >> --- a/drivers/gpu/drm/i915/i915_drv.h >> +++ b/drivers/gpu/drm/i915/i915_drv.h >> @@ -233,6 +233,8 @@ struct intel_shared_dpll_config { >> >> struct intel_shared_dpll { >> struct intel_shared_dpll_config config; >> + struct intel_shared_dpll_config *new_config; >> + >> int active; /* count of number of active CRTCs (i.e. DPMS on) */ >> bool on; /* is the PLL actually active? Disabled during modeset */ >> const char *name; >> @@ -480,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 intel_crtc *crtc); >> int (*crtc_mode_set)(struct intel_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/int= el_ddi.c >> index 5915a07..7b8c4b8 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1375,6 +1375,8 @@ static void hsw_shared_dplls_init(struct drm_i915_= private *dev_priv) >> dev_priv->num_shared_dpll =3D 2; >> >> for (i =3D 0; i < dev_priv->num_shared_dpll; i++) { >> + dev_priv->shared_dplls[i].new_config =3D >> + &dev_priv->shared_dplls[i].config; >> dev_priv->shared_dplls[i].id =3D i; >> dev_priv->shared_dplls[i].name =3D hsw_ddi_pll_names[i]; >> dev_priv->shared_dplls[i].disable =3D hsw_ddi_pll_disable; >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915= /intel_display.c >> index cdaf8ef..f2f7e8f 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3851,15 +3851,9 @@ void intel_put_shared_dpll(struct intel_crtc *crt= c) >> struct intel_shared_dpll *intel_get_shared_dpll(struct intel_crtc *crt= c) >> { >> struct drm_i915_private *dev_priv =3D crtc->base.dev->dev_private; >> - struct intel_shared_dpll *pll =3D 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 =3D (enum intel_dpll_id) crtc->pipe; >> @@ -3868,7 +3862,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(st= ruct intel_crtc *crtc) >> DRM_DEBUG_KMS("CRTC:%d using pre-allocated %s\n", >> crtc->base.base.id, pll->name); >> >> - WARN_ON(pll->config.crtc_mask); >> + WARN_ON(pll->new_config->crtc_mask); >> >> goto found; >> } >> @@ -3877,17 +3871,16 @@ struct intel_shared_dpll *intel_get_shared_dpll(= struct intel_crtc *crtc) >> pll =3D &dev_priv->shared_dplls[i]; >> >> /* Only want to check enabled timings first */ >> - if (pll->config.crtc_mask =3D=3D 0) >> + if (pll->new_config->crtc_mask =3D=3D 0) >> continue; >> >> - if (memcmp(&crtc->config.dpll_hw_state, >> - &pll->config.hw_state, >> - sizeof(pll->config.hw_state)) =3D=3D 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->new_config->hw_state, >> + sizeof(pll->new_config->hw_state)) =3D=3D 0) { >> + DRM_DEBUG_KMS("CRTC:%d sharing existing %s (crtc mask 0x%08x, ative = %d)\n", >> crtc->base.base.id, pll->name, >> - pll->config.crtc_mask, pll->active); >> - >> + pll->new_config->crtc_mask, >> + pll->active); >> goto found; >> } >> } >> @@ -3895,7 +3888,7 @@ struct intel_shared_dpll *intel_get_shared_dpll(st= ruct intel_crtc *crtc) >> /* Ok no matching timings, maybe there's a free one? */ >> for (i =3D 0; i < dev_priv->num_shared_dpll; i++) { >> pll =3D &dev_priv->shared_dplls[i]; >> - if (pll->config.crtc_mask =3D=3D 0) { >> + if (pll->new_config->crtc_mask =3D=3D 0) { >> DRM_DEBUG_KMS("CRTC:%d allocated %s\n", >> crtc->base.base.id, pll->name); >> goto found; >> @@ -3905,18 +3898,64 @@ struct intel_shared_dpll *intel_get_shared_dpll(= struct intel_crtc *crtc) >> return NULL; >> >> found: >> - if (pll->config.crtc_mask =3D=3D 0) >> - pll->config.hw_state =3D crtc->config.dpll_hw_state; >> + if (pll->new_config->crtc_mask =3D=3D 0) >> + pll->new_config->hw_state =3D crtc->new_config->dpll_hw_state; >> >> - crtc->config.shared_dpll =3D i; >> + crtc->new_config->shared_dpll =3D i; >> DRM_DEBUG_DRIVER("using %s for pipe %c\n", pll->name, >> pipe_name(crtc->pipe)); >> >> - pll->config.crtc_mask |=3D 1 << crtc->pipe; >> + pll->new_config->crtc_mask |=3D 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 int 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 =3D 0; i < dev_priv->num_shared_dpll; i++) { >> + pll =3D &dev_priv->shared_dplls[i]; >> + >> + pll->new_config =3D kmalloc(sizeof(*pll->new_config), >> + GFP_KERNEL); >> + if (!pll->new_config) >> + return -ENOMEM; > > Need to restore new_config =3D &config? For the crtc config we have > new_config=3D=3DNULL outside modeset operations to catch abusers, but you > seem to have new_config=3D=3D&config for the plls normally. I had the crtc > config that way too initially, but Daniel wanted to changed it. Not sure > how hard it would be to do the same thing for plls? That makes sense. It shouldn't be too difficult to do that, since the = only function that needs to work on both old and new config is = intel_get_shared_dpll(), and that is only called during mode set. > My original idea for the new_config=3D=3D&config approach in crtcs was th= at > we might be able to share some functions that get called both during > and after modesets and they would automagically consult the correct > pipe config. But the NULL approach certainly seems to work so far and > it would result in a neat explosion if it didn't. > > Also what happens with the already allocated pll configs if we fail > midway through the loop here? Or if we fail later in the > .compute_clock() or some other place before the pll config is commited? Yeah, oops, we end up with corrupted state. Will fix. > Could also use kmemdup() here I suppose. > >> + >> + pll->new_config->crtc_mask =3D >> + pll->config.crtc_mask & ~clear_pipes; >> + pll->new_config->hw_state =3D pll->config.hw_state; >> + } >> + >> + return 0; >> +} >> + [...] >> @@ -12586,6 +12651,7 @@ static void intel_init_display(struct drm_device= *dev) >> if (HAS_DDI(dev)) { >> dev_priv->display.get_pipe_config =3D haswell_get_pipe_config; >> dev_priv->display.get_plane_config =3D ironlake_get_plane_config; >> + dev_priv->display.crtc_compute_clock =3D NULL; > > dev_priv is kzalloc()ed so no need for NULL initialization. Ok. I'll resend with the above fixed. Thanks, Ander