From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH 05/19] drm/i915: power domains: add power well ops Date: Mon, 24 Feb 2014 13:42:37 +0200 Message-ID: <1393242157.13131.34.camel@intelbox> References: <1392674540-10915-1-git-send-email-imre.deak@intel.com> <1392674540-10915-6-git-send-email-imre.deak@intel.com> <20140220112633.6c0361c0@jbarnes-desktop> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1836764935==" Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id B27FAFA29A for ; Mon, 24 Feb 2014 03:42:39 -0800 (PST) In-Reply-To: <20140220112633.6c0361c0@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1836764935== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-ry5QjIhkG6mf/W0EY3/b" --=-ry5QjIhkG6mf/W0EY3/b Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2014-02-20 at 11:26 -0800, Jesse Barnes wrote: > On Tue, 18 Feb 2014 00:02:06 +0200 > Imre Deak wrote: >=20 > > Split the 'set' power well handler into an 'enable', 'disable' and > > 'sync_hw' handler. This maps more conveniently to higher level > > operations, for example it allows us to push the hsw package c8 handlin= g > > into the corresponding hsw/bdw enable/disable handlers and the hsw BIOS > > hand-over setting into the hsw/bdw sync_hw handler. > >=20 > > No functional change. > >=20 > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/i915_drv.h | 35 +++++++++++++++++--- > > drivers/gpu/drm/i915/intel_pm.c | 73 +++++++++++++++++++++++++++------= -------- > > 2 files changed, 80 insertions(+), 28 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i91= 5_drv.h > > index de0c0e0..8c8635e 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1006,6 +1006,36 @@ struct intel_ilk_power_mgmt { > > struct drm_i915_gem_object *renderctx; > > }; > > =20 > > +struct drm_i915_private; > > +struct i915_power_well; > > + > > +struct i915_power_well_ops { > > + /* > > + * Synchronize the well's hw state to match the current sw state, for > > + * example enable/disable it based on the current refcount. Called > > + * during driver init and resume time, possibly after first calling > > + * the enable/disable handlers. > > + */ > > + void (*sync_hw)(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well); > > + /* > > + * Enable the well and resources that depend on it (for example > > + * interrupts located on the well). Called after the 0->1 refcount > > + * transition. > > + */ > > + void (*enable)(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well); > > + /* > > + * Disable the well and resources that depend on it. Called after > > + * the 1->0 refcount transition. > > + */ > > + void (*disable)(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well); > > + /* Returns the hw enabled state. */ > > + bool (*is_enabled)(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well); > > +}; > > + > > /* Power well structure for haswell */ > > struct i915_power_well { > > const char *name; > > @@ -1014,10 +1044,7 @@ struct i915_power_well { > > int count; > > unsigned long domains; > > void *data; > > - void (*set)(struct drm_i915_private *dev_priv, struct i915_power_well= *power_well, > > - bool enable); > > - bool (*is_enabled)(struct drm_i915_private *dev_priv, > > - struct i915_power_well *power_well); > > + const struct i915_power_well_ops *ops; > > }; > > =20 > > struct i915_power_domains { > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index 9a608f1..7866426 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5203,7 +5203,7 @@ bool intel_display_power_enabled(struct drm_i915_= private *dev_priv, > > if (power_well->always_on) > > continue; > > =20 > > - if (!power_well->is_enabled(dev_priv, power_well)) { > > + if (!power_well->ops->is_enabled(dev_priv, power_well)) { > > is_enabled =3D false; > > break; > > } > > @@ -5305,6 +5305,33 @@ static void hsw_set_power_well(struct drm_i915_p= rivate *dev_priv, > > } > > } > > =20 > > +static void hsw_power_well_sync_hw(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + hsw_set_power_well(dev_priv, power_well, power_well->count > 0); > > + > > + /* > > + * We're taking over the BIOS, so clear any requests made by it since > > + * the driver is in charge now. > > + */ > > + if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST) > > + I915_WRITE(HSW_PWR_WELL_BIOS, 0); > > +} > > + > > +static void hsw_power_well_enable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + hsw_disable_package_c8(dev_priv); > > + hsw_set_power_well(dev_priv, power_well, true); > > +} > > + > > +static void hsw_power_well_disable(struct drm_i915_private *dev_priv, > > + struct i915_power_well *power_well) > > +{ > > + hsw_set_power_well(dev_priv, power_well, false); > > + hsw_enable_package_c8(dev_priv); > > +} > > + > > void intel_display_power_get(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain domain) > > { > > @@ -5317,10 +5344,8 @@ void intel_display_power_get(struct drm_i915_pri= vate *dev_priv, > > mutex_lock(&power_domains->lock); > > =20 > > for_each_power_well(i, power_well, BIT(domain), power_domains) > > - if (!power_well->count++ && power_well->set) { > > - hsw_disable_package_c8(dev_priv); > > - power_well->set(dev_priv, power_well, true); > > - } > > + if (!power_well->count++ && power_well->ops->enable) > > + power_well->ops->enable(dev_priv, power_well); > > =20 > > power_domains->domain_use_count[domain]++; > > =20 > > @@ -5344,11 +5369,9 @@ void intel_display_power_put(struct drm_i915_pri= vate *dev_priv, > > for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { > > WARN_ON(!power_well->count); > > =20 > > - if (!--power_well->count && power_well->set && > > - i915.disable_power_well) { > > - power_well->set(dev_priv, power_well, false); > > - hsw_enable_package_c8(dev_priv); > > - } > > + if (!--power_well->count && power_well->ops->disable && > > + i915.disable_power_well) > > + power_well->ops->disable(dev_priv, power_well); > > } > > =20 > > mutex_unlock(&power_domains->lock); > > @@ -5401,25 +5424,35 @@ EXPORT_SYMBOL_GPL(i915_release_power_well); > > (POWER_DOMAIN_MASK & ~BDW_ALWAYS_ON_POWER_DOMAINS) | \ > > BIT(POWER_DOMAIN_INIT)) > > =20 > > +static const struct i915_power_well_ops i9xx_always_on_power_well_ops = =3D { }; > > + > > static struct i915_power_well i9xx_always_on_power_well[] =3D { > > { > > .name =3D "always-on", > > .always_on =3D 1, > > .domains =3D POWER_DOMAIN_MASK, > > + .ops =3D &i9xx_always_on_power_well_ops, > > }, > > }; > > =20 > > +static const struct i915_power_well_ops hsw_power_well_ops =3D { > > + .sync_hw =3D hsw_power_well_sync_hw, > > + .enable =3D hsw_power_well_enable, > > + .disable =3D hsw_power_well_disable, > > + .is_enabled =3D hsw_power_well_enabled, > > +}; > > + > > static struct i915_power_well hsw_power_wells[] =3D { > > { > > .name =3D "always-on", > > .always_on =3D 1, > > .domains =3D HSW_ALWAYS_ON_POWER_DOMAINS, > > + .ops =3D &i9xx_always_on_power_well_ops, > > }, > > { > > .name =3D "display", > > .domains =3D HSW_DISPLAY_POWER_DOMAINS, > > - .is_enabled =3D hsw_power_well_enabled, > > - .set =3D hsw_set_power_well, > > + .ops =3D &hsw_power_well_ops, > > }, > > }; > > =20 > > @@ -5428,12 +5461,12 @@ static struct i915_power_well bdw_power_wells[]= =3D { > > .name =3D "always-on", > > .always_on =3D 1, > > .domains =3D BDW_ALWAYS_ON_POWER_DOMAINS, > > + .ops =3D &i9xx_always_on_power_well_ops, > > }, > > { > > .name =3D "display", > > .domains =3D BDW_DISPLAY_POWER_DOMAINS, > > - .is_enabled =3D hsw_power_well_enabled, > > - .set =3D hsw_set_power_well, > > + .ops =3D &hsw_power_well_ops, > > }, > > }; > > =20 > > @@ -5478,8 +5511,8 @@ static void intel_power_domains_resume(struct drm= _i915_private *dev_priv) > > =20 > > mutex_lock(&power_domains->lock); > > for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) = { > > - if (power_well->set) > > - power_well->set(dev_priv, power_well, power_well->count > 0); > > + if (power_well->ops->sync_hw) > > + power_well->ops->sync_hw(dev_priv, power_well); > > } > > mutex_unlock(&power_domains->lock); > > } > > @@ -5495,14 +5528,6 @@ void intel_power_domains_init_hw(struct drm_i915= _private *dev_priv) > > /* For now, we need the power well to be always enabled. */ > > intel_display_set_init_power(dev_priv, true); > > intel_power_domains_resume(dev_priv); > > - > > - if (!(IS_HASWELL(dev_priv->dev) || IS_BROADWELL(dev_priv->dev))) > > - return; > > - > > - /* We're taking over the BIOS, so clear any requests made by it since > > - * the driver is in charge now. */ > > - if (I915_READ(HSW_PWR_WELL_BIOS) & HSW_PWR_WELL_ENABLE_REQUEST) > > - I915_WRITE(HSW_PWR_WELL_BIOS, 0); > > } > > =20 > > /* Disables PC8 so we can use the GMBUS and DP AUX interrupts. */ >=20 > Looks good. Only nitpick might be to have no-op ops for the 9xx case > and get rid of the checks for whether the ops function exists. Going > forward that might force us to think about things a bit at least. But > that's really just a style issue, the rest looks like a good change and > pulls some of the HSW bits out of the generic routines. Yea, I was going back and forth between allowing a NULL handler or not, but I guess with less special casing in the generic path, it's easier to read the code which is a big plus. And because we have to enable/disable runtime PM even for always-on wells we'll have to anyway add a proper handlers eventually. So I'll update this adding the empty stubs for now. --Imre >=20 > Reviewed-by: Jesse Barnes >=20 --=-ry5QjIhkG6mf/W0EY3/b Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJTCzAtAAoJEORIIAnNuWDF4HAH/0vSlmYFl3ZrQ7fx/xkD3cNs C2iR40xOLwlCu99PCUHTrfiS2fqw538crmaH8jMlAbM3ze7r2HM4gTJgovM9lazE kw5+9Im/KRLguUqi80ZD/3yo4LdH1nIV41wo1ZeX3IyWzVScakVnFdXs3VJozX6/ 6RYvzVEDk/ooI0boFvkFXJXkqKUWJMcF5iEqMymbb7qjtHVFVCFHY4myrG+TZ1jB KKpPbP8SdEg/g3l+ZB1GG5nNgGAxhM0TlAwL+Krybyrm6SzCxTAyogzu8KCisCOS KDggxR9f8is3ZEPvd4HWTX9frz5FsOYm/vBY5u0KPuUu0kj1ZENrwp0vAjnm1J8= =uCs0 -----END PGP SIGNATURE----- --=-ry5QjIhkG6mf/W0EY3/b-- --===============1836764935== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1836764935==--