From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 03/10] drm/i915: fix intel_init_power_wells Date: Tue, 22 Jan 2013 15:47:27 +0200 Message-ID: <20130122134727.GF9135@intel.com> References: <1358540953-3979-1-git-send-email-przanoni@gmail.com> <1358540953-3979-4-git-send-email-przanoni@gmail.com> <20130121133742.GB9135@intel.com> <20130122130226.GJ31870@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 4E046E5C85 for ; Tue, 22 Jan 2013 05:47:32 -0800 (PST) Content-Disposition: inline In-Reply-To: <20130122130226.GJ31870@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Tue, Jan 22, 2013 at 02:02:26PM +0100, Daniel Vetter wrote: > On Mon, Jan 21, 2013 at 03:37:42PM +0200, Ville Syrj=E4l=E4 wrote: > > On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote: > > > From: Paulo Zanoni > > > = > > > The current code was wrong in many different ways, so this is a full > > > rewrite. We don't have "different power wells for different parts of > > > the GPU", we have a single power well, but we have multiple registers > > > that can be used to request enabling/disabling the power well. So > > > let's be a good citizen and only use the register we're supposed to > > > use, except when we're loading the driver, where we clear the request > > > made by the BIOS. > > > = > > > If any of the registers is requesting the power well to be enabled, it > > > will be enabled. If none of the registers is requesting the power well > > > to be enabled, it will be disabled. > > > = > > > For now we're just forcing the power well to be enabled, but in the > > > next commits we'll change this. > > > = > > > Signed-off-by: Paulo Zanoni > > > --- > > > drivers/gpu/drm/i915/i915_reg.h | 8 ++-- > > > drivers/gpu/drm/i915/intel_display.c | 5 +-- > > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > > drivers/gpu/drm/i915/intel_pm.c | 70 ++++++++++++++++++++++++= +--------- > > > 4 files changed, 59 insertions(+), 26 deletions(-) > > > = > > > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i= 915_reg.h > > > index 2521617..f054554 100644 > > > --- a/drivers/gpu/drm/i915/i915_reg.h > > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > > @@ -4414,10 +4414,10 @@ > > > #define AUDIO_CP_READY_C (1<<9) > > > = > > > /* HSW Power Wells */ > > > -#define HSW_PWR_WELL_CTL1 0x45400 /* BIOS */ > > > -#define HSW_PWR_WELL_CTL2 0x45404 /* Driver */ > > > -#define HSW_PWR_WELL_CTL3 0x45408 /* KVMR */ > > > -#define HSW_PWR_WELL_CTL4 0x4540C /* Debug */ > > > +#define HSW_PWR_WELL_BIOS 0x45400 /* CTL1 */ > > > +#define HSW_PWR_WELL_DRIVER 0x45404 /* CTL2 */ > > > +#define HSW_PWR_WELL_KVMR 0x45408 /* CTL3 */ > > > +#define HSW_PWR_WELL_DEBUG 0x4540C /* CTL4 */ > > > #define HSW_PWR_WELL_ENABLE (1<<31) > > > #define HSW_PWR_WELL_STATE (1<<30) > > > #define HSW_PWR_WELL_CTL5 0x45410 > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i= 915/intel_display.c > > > index b35902e..4a9f048 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -8647,10 +8647,7 @@ static void i915_disable_vga(struct drm_device= *dev) > > > = > > > void intel_modeset_init_hw(struct drm_device *dev) > > > { > > > - /* We attempt to init the necessary power wells early in the initia= lization > > > - * time, so the subsystems that expect power to be enabled can work. > > > - */ > > > - intel_init_power_wells(dev); > > > + intel_init_power_well(dev); > > > = > > > intel_prepare_ddi(dev); > > > = > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/= intel_drv.h > > > index aeff0d1..8cfad75 100644 > > > --- a/drivers/gpu/drm/i915/intel_drv.h > > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > > @@ -666,7 +666,7 @@ extern void intel_update_fbc(struct drm_device *d= ev); > > > extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv); > > > extern void intel_gpu_ips_teardown(void); > > > = > > > -extern void intel_init_power_wells(struct drm_device *dev); > > > +extern void intel_init_power_well(struct drm_device *dev); > > > extern void intel_enable_gt_powersave(struct drm_device *dev); > > > extern void intel_disable_gt_powersave(struct drm_device *dev); > > > extern void gen6_gt_check_fifodbg(struct drm_i915_private *dev_priv); > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/i= ntel_pm.c > > > index 5a8a72c..2273b9c 100644 > > > --- a/drivers/gpu/drm/i915/intel_pm.c > > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > > @@ -4043,33 +4043,69 @@ void intel_init_clock_gating(struct drm_devic= e *dev) > > > dev_priv->display.init_clock_gating(dev); > > > } > > > = > > > -/* Starting with Haswell, we have different power wells for > > > - * different parts of the GPU. This attempts to enable them all. > > > +static void intel_set_power_well(struct drm_device *dev, bool enable) > > > +{ > > > + struct drm_i915_private *dev_priv =3D dev->dev_private; > > > + bool is_enabled, enable_requested; > > > + uint32_t tmp; > > > + > > > + tmp =3D I915_READ(HSW_PWR_WELL_DRIVER); > > > + is_enabled =3D !!(tmp & HSW_PWR_WELL_STATE); > > > + enable_requested =3D !!(tmp & HSW_PWR_WELL_ENABLE); > > > + > > > + if (enable) { > > > + if (!enable_requested) > > > + I915_WRITE(HSW_PWR_WELL_DRIVER, HSW_PWR_WELL_ENABLE); > > > + > > > + if (!is_enabled) { > > > + DRM_DEBUG_KMS("Enabling power well\n"); > > > + if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) & > > > + HSW_PWR_WELL_STATE), 20)) > > > + DRM_ERROR("Timeout enabling power well\n"); > > > + } > > > + } else { > > > + if (enable_requested) > > > + I915_WRITE(HSW_PWR_WELL_DRIVER, 0); > > > + > > > + if (is_enabled) { > > > + if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE) > > > + DRM_DEBUG_KMS("Not disabling power well: requested by BIOS\n"); > > > + else if (I915_READ(HSW_PWR_WELL_KVMR) & HSW_PWR_WELL_ENABLE) > > > + DRM_DEBUG_KMS("Not disabling power well: requested by KVMR\n"); > > > + else if (I915_READ(HSW_PWR_WELL_DEBUG) & HSW_PWR_WELL_ENABLE) > > > + DRM_DEBUG_KMS("Not disabling power well: requested by DEBUG\n"); > > > + else { > > > + DRM_DEBUG_KMS("Disabling power well\n"); > > > + if (wait_for((I915_READ(HSW_PWR_WELL_DRIVER) & > > > + HSW_PWR_WELL_STATE) =3D=3D 0, 20)) > > > + DRM_ERROR("Timeout disabling power well\n"); > > > + } > > > + } > > > + } > > = > > The documentation says not to touch the enable bits if there's a state > > transition already in progress. I have no idea how we're supposed to do > > that without all kinds of race conditions due multiple entities making > > requests simultaneosly. Do you know something about this that's not in > > the documentation? > > = > > Apart from that the patch looks good to me. > = > We should be protected by dev->mode_config.mutex in all callsite (safe for > maybe setup/resume, but that's just cosmetics). The dev->struct_mutex > locking we already have seems to be cargo-culting afaict. Paulo, can you > add a follow-up patch to unconfuse matters here a bit? I'm not concerned about out driver, but mainly KVMR. It can enable/disable the power well at any time. Unless of course KVMR is never used on Linux machines, in which case my worries are pointless. -- = Ville Syrj=E4l=E4 Intel OTC