From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH] drm/i915: cache hw power well enabled state Date: Thu, 05 Jun 2014 20:44:25 +0300 Message-ID: <1401990265.29558.6.camel@intelbox> References: <20140528111708.253e2578@jbarnes-desktop> <1401989507-9290-1-git-send-email-imre.deak@intel.com> <20140605103515.5adb3e52@jbarnes-desktop> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2001451586==" Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C3EF6E293 for ; Thu, 5 Jun 2014 10:45:20 -0700 (PDT) In-Reply-To: <20140605103515.5adb3e52@jbarnes-desktop> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jesse Barnes Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============2001451586== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-loYzWqJcQv6QQsU3lFz9" --=-loYzWqJcQv6QQsU3lFz9 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Thu, 2014-06-05 at 10:35 -0700, Jesse Barnes wrote: > On Thu, 5 Jun 2014 20:31:47 +0300 > Imre Deak wrote: >=20 > > Jesse noticed that the punit communication needed to query the VLV powe= r > > well status can cause substantial delays. Since we can query the state > > frequently, for example during I2C transfers, maintain a cached version > > of the HW state to get rid of this delay. > >=20 > > This fixes at least one reported regression where boot time increased b= y > > ~4 seconds due to frequent power well state queries on VLV during eDP > > EDID read. > >=20 > > Reported-by: Jesse Barnes > > Signed-off-by: Imre Deak > > --- > > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > > drivers/gpu/drm/i915/intel_display.c | 6 +++--- > > drivers/gpu/drm/i915/intel_drv.h | 4 ++-- > > drivers/gpu/drm/i915/intel_pm.c | 37 +++++++++++++++-------------= -------- > > 4 files changed, 22 insertions(+), 27 deletions(-) > >=20 > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i91= 5_drv.h > > index 8631fb3..000a6ce 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -976,6 +976,8 @@ struct i915_power_well { > > bool always_on; > > /* power well enable/disable usage count */ > > int count; > > + /* cached hw enabled state */ > > + bool hw_enabled; > > unsigned long domains; > > unsigned long data; > > const struct i915_power_well_ops *ops; > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i91= 5/intel_display.c > > index b5cbb28..882d4f5 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -12692,8 +12692,8 @@ intel_display_capture_error_state(struct drm_de= vice *dev) > > =20 > > for_each_pipe(i) { > > error->pipe[i].power_domain_on =3D > > - intel_display_power_enabled_sw(dev_priv, > > - POWER_DOMAIN_PIPE(i)); > > + intel_display_power_enabled_unlocked(dev_priv, > > + POWER_DOMAIN_PIPE(i)); > > if (!error->pipe[i].power_domain_on) > > continue; > > =20 > > @@ -12728,7 +12728,7 @@ intel_display_capture_error_state(struct drm_de= vice *dev) > > enum transcoder cpu_transcoder =3D transcoders[i]; > > =20 > > error->transcoder[i].power_domain_on =3D > > - intel_display_power_enabled_sw(dev_priv, > > + intel_display_power_enabled_unlocked(dev_priv, > > POWER_DOMAIN_TRANSCODER(cpu_transcoder)); > > if (!error->transcoder[i].power_domain_on) > > continue; > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/in= tel_drv.h > > index 78d4124..1455ddb 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -948,8 +948,8 @@ int intel_power_domains_init(struct drm_i915_privat= e *); > > void intel_power_domains_remove(struct drm_i915_private *); > > bool intel_display_power_enabled(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain domain); > > -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv, > > - enum intel_display_power_domain domain); > > +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev= _priv, > > + enum intel_display_power_domain domain); > > void intel_display_power_get(struct drm_i915_private *dev_priv, > > enum intel_display_power_domain domain); > > void intel_display_power_put(struct drm_i915_private *dev_priv, > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/int= el_pm.c > > index ee27d74..96a2d31 100644 > > --- a/drivers/gpu/drm/i915/intel_pm.c > > +++ b/drivers/gpu/drm/i915/intel_pm.c > > @@ -5801,8 +5801,8 @@ static bool hsw_power_well_enabled(struct drm_i91= 5_private *dev_priv, > > (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED); > > } > > =20 > > -bool intel_display_power_enabled_sw(struct drm_i915_private *dev_priv, > > - enum intel_display_power_domain domain) > > +bool intel_display_power_enabled_unlocked(struct drm_i915_private *dev= _priv, > > + enum intel_display_power_domain domain) > > { > > struct i915_power_domains *power_domains; > > struct i915_power_well *power_well; > > @@ -5813,16 +5813,19 @@ bool intel_display_power_enabled_sw(struct drm_= i915_private *dev_priv, > > return false; > > =20 > > power_domains =3D &dev_priv->power_domains; > > + > > is_enabled =3D true; > > + > > for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { > > if (power_well->always_on) > > continue; > > =20 > > - if (!power_well->count) { > > + if (!power_well->hw_enabled) { > > is_enabled =3D false; > > break; > > } > > } > > + > > return is_enabled; > > } > > =20 > > @@ -5830,30 +5833,15 @@ bool intel_display_power_enabled(struct drm_i91= 5_private *dev_priv, > > enum intel_display_power_domain domain) > > { > > struct i915_power_domains *power_domains; > > - struct i915_power_well *power_well; > > - bool is_enabled; > > - int i; > > - > > - if (dev_priv->pm.suspended) > > - return false; > > + bool ret; > > =20 > > power_domains =3D &dev_priv->power_domains; > > =20 > > - is_enabled =3D true; > > - > > mutex_lock(&power_domains->lock); > > - for_each_power_well_rev(i, power_well, BIT(domain), power_domains) { > > - if (power_well->always_on) > > - continue; > > - > > - if (!power_well->ops->is_enabled(dev_priv, power_well)) { > > - is_enabled =3D false; > > - break; > > - } > > - } > > + ret =3D intel_display_power_enabled_unlocked(dev_priv, domain); > > mutex_unlock(&power_domains->lock); > > =20 > > - return is_enabled; > > + return ret; > > } > > =20 > > /* > > @@ -6174,6 +6162,7 @@ void intel_display_power_get(struct drm_i915_priv= ate *dev_priv, > > if (!power_well->count++) { > > DRM_DEBUG_KMS("enabling %s\n", power_well->name); > > power_well->ops->enable(dev_priv, power_well); > > + power_well->hw_enabled =3D true; > > } > > =20 > > check_power_well_state(dev_priv, power_well); > > @@ -6203,6 +6192,7 @@ void intel_display_power_put(struct drm_i915_priv= ate *dev_priv, > > =20 > > if (!--power_well->count && i915.disable_power_well) { > > DRM_DEBUG_KMS("disabling %s\n", power_well->name); > > + power_well->hw_enabled =3D false; > > power_well->ops->disable(dev_priv, power_well); > > } > > =20 > > @@ -6463,8 +6453,11 @@ static void intel_power_domains_resume(struct dr= m_i915_private *dev_priv) > > int i; > > =20 > > mutex_lock(&power_domains->lock); > > - for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) > > + for_each_power_well(i, power_well, POWER_DOMAIN_MASK, power_domains) = { > > power_well->ops->sync_hw(dev_priv, power_well); > > + power_well->hw_enabled =3D power_well->ops->is_enabled(dev_priv, > > + power_well); > > + } > > mutex_unlock(&power_domains->lock); > > } > > =20 >=20 > I guess we have check_power_well_state for the cross checking, so: Yea, that still reads the real HW state, and it's done only during 0->1 1->0 transitions, so (hopefully) not a big overhead. I also tested this lightly on HSW and VLV. --Imre > Reviewed-by: Jesse Barnes >=20 --=-loYzWqJcQv6QQsU3lFz9 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) iQEcBAABAgAGBQJTkKx5AAoJEORIIAnNuWDFZUwH/RZJdFHanFOokehA3sTjT9+H zj9sH5h7pLM/E1WRgCfGw2KQPLhw224exzb7E14WAN3nFMnT+TswJ0pf2iXoT6Mo NwIKUTStquIefQcUdxwQ5e8oFuCLHDLdV6mJZPJppKEotf0+NAJJFrPxfFnEH0Qo s+HLE++YuDC0f+axl2jaBWoLhtkRpHhNnRSuQwhOn9ZcaekK1D5UZQ7SLfBxtPe7 NLCRRGbKwkXdOZL7Z+JjFKcxwzayz3SlUvblSF9FDOXdCfnoyDYI3UveaUW8eM+H bubozuZ7IF7jUkV1Yg/VlLPSMLWmzlGbz7xq3GnHRnucLjqaBBNNOUOOS5gaBe0= =wOGb -----END PGP SIGNATURE----- --=-loYzWqJcQv6QQsU3lFz9-- --===============2001451586== 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 --===============2001451586==--