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?l? wrote:
> > On Fri, Jan 18, 2013 at 06:29:05PM -0200, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > >
> > > 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 <paulo.r.zanoni at intel.com>
> > > ---
> > > 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/i915_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/i915/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 initialization
> > > - * 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 *dev);
> > > 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/intel_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_device *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 = dev->dev_private;
> > > + bool is_enabled, enable_requested;
> > > + uint32_t tmp;
> > > +
> > > + tmp = I915_READ(HSW_PWR_WELL_DRIVER);
> > > + is_enabled = !!(tmp & HSW_PWR_WELL_STATE);
> > > + enable_requested = !!(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) == 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?l?
Intel OTC
