From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx Date: Wed, 27 Nov 2013 20:30:00 +0200 Message-ID: <20131127183000.GY10036@intel.com> References: <1385575744-7743-1-git-send-email-imre.deak@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id 73859FB769 for ; Wed, 27 Nov 2013 10:30:04 -0800 (PST) Content-Disposition: inline In-Reply-To: <1385575744-7743-1-git-send-email-imre.deak@intel.com> 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: Imre Deak Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, Nov 27, 2013 at 08:09:04PM +0200, Imre Deak wrote: > Atm we call intel_display_power_enabled() from > i915_capture_error_state() in IRQ context and then take a mutex. To fix > this add a new intel_display_power_enabled_sw() which returns the domain > state based on software tracking as opposed to reading the actual HW > state. > = > Since we use domain_use_count for this without locking on the reader > side make sure we increase the counter only after enabling all required > power wells and decrease it before disabling any of these power wells. > = > Regression introduced in > commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c > Author: Imre Deak > Date: Tue Sep 24 16:17:09 2013 +0300 > = > drm/i915: support for multiple power wells > = > Reported-by: Chris Wilson > Signed-off-by: Imre Deak > --- > drivers/gpu/drm/i915/i915_drv.h | 2 -- > drivers/gpu/drm/i915/intel_display.c | 4 ++-- > drivers/gpu/drm/i915/intel_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_pm.c | 38 ++++++++++++++++++++++++++++--= ------ > 4 files changed, 34 insertions(+), 12 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_= drv.h > index 47b8fd1..d17a62a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -966,9 +966,7 @@ struct i915_power_domains { > int power_well_count; > = > struct mutex lock; > -#if IS_ENABLED(CONFIG_DEBUG_FS) > int domain_use_count[POWER_DOMAIN_NUM]; > -#endif > struct i915_power_well *power_wells; > }; > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index fab7d35..5b5d831 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11374,7 +11374,7 @@ intel_display_capture_error_state(struct drm_devi= ce *dev) > error->power_well_driver =3D I915_READ(HSW_PWR_WELL_DRIVER); > = > for_each_pipe(i) { > - if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i))) > + if (!intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i))) > continue; > = > if (INTEL_INFO(dev)->gen <=3D 6 || IS_VALLEYVIEW(dev)) { > @@ -11410,7 +11410,7 @@ intel_display_capture_error_state(struct drm_devi= ce *dev) > for (i =3D 0; i < error->num_transcoders; i++) { > enum transcoder cpu_transcoder =3D transcoders[i]; > = > - if (!intel_display_power_enabled(dev, > + if (!intel_display_power_enabled_sw(dev, > POWER_DOMAIN_TRANSCODER(cpu_transcoder))) > continue; > = > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/inte= l_drv.h > index b2d2cc1..fb3cfc5 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -830,6 +830,8 @@ int intel_power_domains_init(struct drm_device *dev); > void intel_power_domains_remove(struct drm_device *dev); > bool intel_display_power_enabled(struct drm_device *dev, > enum intel_display_power_domain domain); > +bool intel_display_power_enabled_sw(struct drm_device *dev, > + enum intel_display_power_domain domain); > void intel_display_power_get(struct drm_device *dev, > enum intel_display_power_domain domain); > void intel_display_power_put(struct drm_device *dev, > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel= _pm.c > index a1a54ab..b9a900d 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5633,6 +5633,21 @@ static bool hsw_power_well_enabled(struct drm_devi= ce *dev, > (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED); > } > = > +bool intel_display_power_enabled_sw(struct drm_device *dev, > + enum intel_display_power_domain domain) > +{ > + struct drm_i915_private *dev_priv =3D dev->dev_private; > + struct i915_power_domains *power_domains; > + bool enabled; > + > + power_domains =3D &dev_priv->power_domains; > + > + enabled =3D power_domains->domain_use_count[domain]; > + smp_rmb(); I'm not sure the barriers really provide extra value. Some other thread could still come along just after this and disable the power wells. > + > + return enabled; > +} > + > bool intel_display_power_enabled(struct drm_device *dev, > enum intel_display_power_domain domain) > { > @@ -5753,12 +5768,16 @@ void intel_display_power_get(struct drm_device *d= ev, > = > mutex_lock(&power_domains->lock); > = > -#if IS_ENABLED(CONFIG_DEBUG_FS) > - power_domains->domain_use_count[domain]++; > -#endif > for_each_power_well(i, power_well, BIT(domain), power_domains) > __intel_power_well_get(dev, power_well); > = > + /* > + * Don't reorder the adjustment of domain_use_count with whatever > + * __intel_power_well_get does. > + */ > + smp_wmb(); > + power_domains->domain_use_count[domain]++; > + > mutex_unlock(&power_domains->lock); > } > = > @@ -5774,13 +5793,16 @@ void intel_display_power_put(struct drm_device *d= ev, > = > mutex_lock(&power_domains->lock); > = > - for_each_power_well_rev(i, power_well, BIT(domain), power_domains) > - __intel_power_well_put(dev, power_well); > - > -#if IS_ENABLED(CONFIG_DEBUG_FS) > WARN_ON(!power_domains->domain_use_count[domain]); > power_domains->domain_use_count[domain]--; > -#endif > + /* > + * Don't reorder the adjustment of domain_use_count with whatever > + * __intel_power_well_put does. > + */ > + smp_wmb(); > + > + for_each_power_well_rev(i, power_well, BIT(domain), power_domains) > + __intel_power_well_put(dev, power_well); > = > mutex_unlock(&power_domains->lock); > } > -- = > 1.8.4 > = > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- = Ville Syrj=E4l=E4 Intel OTC