Re: [PATCH 4/5] drm/i915: Introduce a non-blocking power domain for vblank interrupts

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Op 08-01-18 om 20:59 schreef Pandiyan, Dhinakaran:
> On Thu, 2018-01-04 at 18:08 -0800, Rodrigo Vivi wrote:
>> On Wed, Jan 03, 2018 at 08:40:00PM +0000, Dhinakaran Pandiyan wrote:
>>> When DC states are enabled and PSR is active, the hardware enters DC5/DC6
>>> states resulting in the frame counter resetting. The frame counter reset
>>> mess up the vblank counting logic as the diff between the new frame
>>> counter value and the previous is negative, and this negative diff gets
>>> applied as an unsigned value to the vblank count. We cannot reject negative
>>> diffs altogether because they can arise from legitimate frame counter
>>> overflows when there is a long period with vblank disabled. So, this
>>> approach allows for the driver to notice a DC state toggle between a vblank
>>> disable and enable and fill in the missed vblanks.
>>>
>>> But, in order to disable DC states when vblank interrupts are required,
>>> the DC_OFF power well has to be disabled in an atomic context. So,
>>> introduce a new VBLANK power domain that can be acquired and released in
>>> atomic contexts with these changes -
>>> 1)  _vblank_get() and _vblank_put() methods skip the power_domain mutex
>>> and use a spin lock for the DC power well.
>>> 2) power_domains->domain_use_count is converted to an atomic_t array so
>>> that it can be updated outside of the power domain mutex.
>>>
>>> v3: Squash domain_use_count atomic_t conversion (Maarten)
>>> v2: Fix deadlock by switching irqsave spinlock.
>>>     Implement atomic version of get_if_enabled.
>>>     Modify power_domain_verify_state to check power well use count and
>>> enabled status atomically.
>>>     Rewrite of intel_power_well_{get,put}
>>>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
>>> ---
>>>  drivers/gpu/drm/i915/i915_debugfs.c     |   2 +-
>>>  drivers/gpu/drm/i915/i915_drv.h         |  19 +++-
>>>  drivers/gpu/drm/i915/intel_display.h    |   1 +
>>>  drivers/gpu/drm/i915/intel_drv.h        |   3 +
>>>  drivers/gpu/drm/i915/intel_runtime_pm.c | 195 ++++++++++++++++++++++++++++----
>>>  5 files changed, 199 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>>> index d81cb2513069..5a7ce734de02 100644
>>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>>> @@ -2746,7 +2746,7 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
>>>  		for_each_power_domain(power_domain, power_well->domains)
>>>  			seq_printf(m, "  %-23s %d\n",
>>>  				 intel_display_power_domain_str(power_domain),
>>> -				 power_domains->domain_use_count[power_domain]);
>>> +				 atomic_read(&power_domains->domain_use_count[power_domain]));
>>>  	}
>>>  
>>>  	mutex_unlock(&power_domains->lock);
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index caebd5825279..61a635f03af7 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -1032,6 +1032,23 @@ struct i915_power_well {
>>>  			bool has_fuses:1;
>>>  		} hsw;
>>>  	};
>>> +
>>> +	/* Lock to serialize access to count, hw_enabled and ops, used for
>>> +	 * power wells that have supports_atomix_ctx set to True.
>>> +	 */
>>> +	spinlock_t lock;
>>> +
>>> +	/* Indicates that the get/put methods for this power well can be called
>>> +	 * in atomic contexts, requires .ops to not sleep. This is valid
>>> +	 * only for the DC_OFF power well currently.
>>> +	 */
>>> +	bool supports_atomic_ctx;
>>> +
>>> +	/* DC_OFF power well was disabled since the last time vblanks were
>>> +	 * disabled.
>>> +	 */
>>> +	bool dc_off_disabled;
>>> +
>>>  	const struct i915_power_well_ops *ops;
>>>  };
>>>  
>>> @@ -1045,7 +1062,7 @@ struct i915_power_domains {
>>>  	int power_well_count;
>>>  
>>>  	struct mutex lock;
>>> -	int domain_use_count[POWER_DOMAIN_NUM];
>>> +	atomic_t domain_use_count[POWER_DOMAIN_NUM];
>>>  	struct i915_power_well *power_wells;
>>>  };
>>>  
>>> diff --git a/drivers/gpu/drm/i915/intel_display.h b/drivers/gpu/drm/i915/intel_display.h
>>> index a0d2b6169361..3e9671ff6f79 100644
>>> --- a/drivers/gpu/drm/i915/intel_display.h
>>> +++ b/drivers/gpu/drm/i915/intel_display.h
>>> @@ -172,6 +172,7 @@ enum intel_display_power_domain {
>>>  	POWER_DOMAIN_AUX_C,
>>>  	POWER_DOMAIN_AUX_D,
>>>  	POWER_DOMAIN_GMBUS,
>>> +	POWER_DOMAIN_VBLANK,
>> Maybe we could start a new category of atomic domains and on interations that makes sense go over both
>> or making the domains in an array with is_atomic bool in case we can transform the atomic get,put to
>> be really generic or into .enable .disable function pointers.
>
> The generic interfaces we've discussed until now to get/put vblank power
> domain atomically are still not generic enough. We might as well accept
> DC_OFF to be a special case and deal with it as such - there is no other
> power well that we need to enable/disable atomically. We can always
> rewrite the code in the future if a better idea comes up.
>
>>>  	POWER_DOMAIN_MODESET,
>>>  	POWER_DOMAIN_GT_IRQ,
>>>  	POWER_DOMAIN_INIT,
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 30f791f89d64..164e62cb047b 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -1797,6 +1797,9 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>>>  					enum intel_display_power_domain domain);
>>>  void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>  			     enum intel_display_power_domain domain);
>>> +void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
>>> +				    bool *needs_restore);
>> can we dump the needs_restore and always restore the counter?
> I checked this, seems possible.
>
>> if so, we could use regular intel_power_domain_{get,put} functions and built the generic atomic inside it.
>>
>>> +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv);
>>>  
>>>  static inline void
>>>  assert_rpm_device_not_suspended(struct drm_i915_private *dev_priv)
>>> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> index d758da6156a8..93b324dcc55d 100644
>>> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> @@ -56,6 +56,19 @@ static struct i915_power_well *
>>>  lookup_power_well(struct drm_i915_private *dev_priv,
>>>  		  enum i915_power_well_id power_well_id);
>>>  
>>> +/* Optimize for the case when this is called from atomic contexts,
>>> + * although the case is unlikely.
>>> + */
>>> +#define power_well_lock(power_well, flags) do {			\
>>> +	if (likely(power_well->supports_atomic_ctx))		\
>>> +		spin_lock_irqsave(&power_well->lock, flags);	\
>>> +	} while (0)
>>> +
>>> +#define power_well_unlock(power_well, flags) do {			\
>>> +	if (likely(power_well->supports_atomic_ctx))			\
>>> +		spin_unlock_irqrestore(&power_well->lock, flags);	\
>>> +	} while (0)
>>> +
>>>  const char *
>>>  intel_display_power_domain_str(enum intel_display_power_domain domain)
>>>  {
>>> @@ -126,6 +139,8 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>>>  		return "AUX_D";
>>>  	case POWER_DOMAIN_GMBUS:
>>>  		return "GMBUS";
>>> +	case POWER_DOMAIN_VBLANK:
>>> +		return "VBLANK";
>>>  	case POWER_DOMAIN_INIT:
>>>  		return "INIT";
>>>  	case POWER_DOMAIN_MODESET:
>>> @@ -141,6 +156,9 @@ intel_display_power_domain_str(enum intel_display_power_domain domain)
>>>  static void intel_power_well_enable(struct drm_i915_private *dev_priv,
>>>  				    struct i915_power_well *power_well)
>>>  {
>>> +	if (power_well->supports_atomic_ctx)
>>> +		assert_spin_locked(&power_well->lock);
>>> +
>>>  	DRM_DEBUG_KMS("enabling %s\n", power_well->name);
>>>  	power_well->ops->enable(dev_priv, power_well);
>>>  	power_well->hw_enabled = true;
>>> @@ -149,26 +167,43 @@ static void intel_power_well_enable(struct drm_i915_private *dev_priv,
>>>  static void intel_power_well_disable(struct drm_i915_private *dev_priv,
>>>  				     struct i915_power_well *power_well)
>>>  {
>>> +	if (power_well->supports_atomic_ctx)
>>> +		assert_spin_locked(&power_well->lock);
>>> +
>>>  	DRM_DEBUG_KMS("disabling %s\n", power_well->name);
>>>  	power_well->hw_enabled = false;
>>>  	power_well->ops->disable(dev_priv, power_well);
>>>  }
>>>  
>>> -static void intel_power_well_get(struct drm_i915_private *dev_priv,
>>> +static void __intel_power_well_get(struct drm_i915_private *dev_priv,
>>>  				 struct i915_power_well *power_well)
>>>  {
>>>  	if (!power_well->count++)
>>>  		intel_power_well_enable(dev_priv, power_well);
>>>  }
>>>  
>>> +static void intel_power_well_get(struct drm_i915_private *dev_priv,
>>> +				 struct i915_power_well *power_well)
>>> +{
>>> +	unsigned long flags = 0;
>>> +
>>> +	power_well_lock(power_well, flags);
>>> +	__intel_power_well_get(dev_priv, power_well);
>>> +	power_well_unlock(power_well, flags);
>>> +}
>>> +
>>>  static void intel_power_well_put(struct drm_i915_private *dev_priv,
>>>  				 struct i915_power_well *power_well)
>>>  {
>>> +	unsigned long flags = 0;
>>> +
>>> +	power_well_lock(power_well, flags);
>>>  	WARN(!power_well->count, "Use count on power well %s is already zero",
>>>  	     power_well->name);
>>>  
>>>  	if (!--power_well->count)
>>>  		intel_power_well_disable(dev_priv, power_well);
>>> +	power_well_unlock(power_well, flags);
>>>  }
>>>  
>>>  /**
>>> @@ -736,6 +771,7 @@ static void gen9_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
>>>  		skl_enable_dc6(dev_priv);
>>>  	else if (dev_priv->csr.allowed_dc_mask & DC_STATE_EN_UPTO_DC5)
>>>  		gen9_enable_dc5(dev_priv);
>>> +	power_well->dc_off_disabled = true;
>>>  }
>>>  
>>>  static void i9xx_power_well_sync_hw_noop(struct drm_i915_private *dev_priv,
>>> @@ -1453,6 +1489,58 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv,
>>>  	chv_set_pipe_power_well(dev_priv, power_well, false);
>>>  }
>>>  
>>> +void intel_display_power_vblank_get(struct drm_i915_private *dev_priv,
>>> +				    bool *needs_restore)
>>> +{
>>> +	struct i915_power_domains *power_domains  = &dev_priv->power_domains;
>>> +	struct i915_power_well *power_well;
>>> +	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
>>> +
>>> +	*needs_restore = false;
>>> +
>>> +	if (!HAS_CSR(dev_priv))
>>> +		return;
>>> +
>>> +	if (!(HAS_PSR(dev_priv) && dev_priv->psr.sink_support))
>>> +		return;
>>> +
>>> +	intel_runtime_pm_get_noresume(dev_priv);
>>> +
>>> +	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) {
>>> +		unsigned long flags = 0;
>>> +
>>> +		power_well_lock(power_well, flags);
>>> +		__intel_power_well_get(dev_priv, power_well);
>>> +		*needs_restore = power_well->dc_off_disabled;
>>> +		power_well->dc_off_disabled = false;
>> it seem also that by always restoring we don't need to add this specific dc_off_disabled
>> variable inside the generic pw struct.
>>
>>> +		power_well_unlock(power_well, flags);
>>> +	}
>>> +
>>> +	atomic_inc(&power_domains->domain_use_count[domain]);
>>> +}
>>> +
>>> +void intel_display_power_vblank_put(struct drm_i915_private *dev_priv)
>>> +{
>>> +	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>>> +	struct i915_power_well *power_well;
>>> +	enum intel_display_power_domain domain = POWER_DOMAIN_VBLANK;
>>> +
>>> +	if (!HAS_CSR(dev_priv))
>>> +		return;
>>> +
>>> +	if (!(HAS_PSR(dev_priv) && dev_priv->psr.sink_support))
>>> +		return;
>> Can we remove these checks and do this for any vblank domain?
>> I believe this kind of association should be only on the domain<->well association
>> and not check for feature here.
> Do you recommend moving it up to the caller? I want to avoid acquiring
> locks, updating ref counts etc, when it is not needed.
>
> Related to your question, if my understanding is correct, the hardware
> does not really enter DC5/6 without PSR when a pipe is enabled. So
> instead of allowing DC5/6, we might as well disable it when there is no
> PSR.
>
>
>>> +
>>> +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
>>> +	     "Use count on domain %s was already zero\n",
>>> +	     intel_display_power_domain_str(domain));
>> is the atomic safe enough here? or we need a spinlock?
> I believe it is safe, domain_use_count[x] does not affect any subsequent
> operation. We can get away with modifying it outside the power well spin
> lock.
>>> +
>>> +	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
>>> +		intel_power_well_put(dev_priv, power_well);
>>> +
>>> +	intel_runtime_pm_put(dev_priv);
>>> +}
>>> +
>>>  static void
>>>  __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>>>  				 enum intel_display_power_domain domain)
>>> @@ -1463,7 +1551,7 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv,
>>>  	for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain))
>>>  		intel_power_well_get(dev_priv, power_well);
>>>  
>>> -	power_domains->domain_use_count[domain]++;
>>> +	atomic_inc(&power_domains->domain_use_count[domain]);
>>>  }
>>>  
>>>  /**
>>> @@ -1492,6 +1580,38 @@ void intel_display_power_get(struct drm_i915_private *dev_priv,
>>>  	mutex_unlock(&power_domains->lock);
>>>  }
>>>  
>>> +static bool dc_off_get_if_enabled(struct drm_i915_private *dev_priv,
>>> +				  enum intel_display_power_domain domain)
>>> +{
>>> +	struct i915_power_well *power_well;
>>> +	bool is_enabled;
>>> +	unsigned long flags = 0;
>>> +
>>> +	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
>>> +	if (!power_well || !(power_well->domains & domain))
>>> +		return true;
>>> +
>>> +	power_well_lock(power_well, flags);
>>> +	is_enabled = power_well->hw_enabled;
>>> +	if (is_enabled)
>>> +		__intel_power_well_get(dev_priv, power_well);
>>> +	power_well_unlock(power_well, flags);
>>> +
>>> +	return is_enabled;
>>> +}
>>> +
>>> +static void dc_off_put(struct drm_i915_private *dev_priv,
>>> +		       enum intel_display_power_domain domain)
>>> +{
>>> +	struct i915_power_well *power_well;
>>> +
>>> +	power_well = lookup_power_well(dev_priv, SKL_DISP_PW_DC_OFF);
>>> +	if (!power_well || !(power_well->domains & domain))
>>> +		return;
>>> +
>>> +	intel_power_well_put(dev_priv, power_well);
>>> +}
>>> +
>>>  /**
>>>   * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain
>>>   * @dev_priv: i915 device instance
>>> @@ -1508,20 +1628,24 @@ bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv,
>>>  					enum intel_display_power_domain domain)
>>>  {
>>>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>>> -	bool is_enabled;
>>> +	bool is_enabled = false;
>>>  
>>>  	if (!intel_runtime_pm_get_if_in_use(dev_priv))
>>>  		return false;
>>>  
>>>  	mutex_lock(&power_domains->lock);
>>>  
>>> +	if (!dc_off_get_if_enabled(dev_priv, domain))
>>> +		goto out;
>>> +
>>>  	if (__intel_display_power_is_enabled(dev_priv, domain)) {
>>>  		__intel_display_power_get_domain(dev_priv, domain);
>>>  		is_enabled = true;
>>> -	} else {
>>> -		is_enabled = false;
>>>  	}
>>>  
>>> +	dc_off_put(dev_priv, domain);
>>> +
>>> +out:
>>>  	mutex_unlock(&power_domains->lock);
>>>  
>>>  	if (!is_enabled)
>>> @@ -1549,10 +1673,9 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>  
>>>  	mutex_lock(&power_domains->lock);
>>>  
>>> -	WARN(!power_domains->domain_use_count[domain],
>>> -	     "Use count on domain %s is already zero\n",
>>> +	WARN(atomic_dec_return(&power_domains->domain_use_count[domain]) < 0,
>>> +	     "Use count on domain %s was already zero\n",
>>>  	     intel_display_power_domain_str(domain));
>>> -	power_domains->domain_use_count[domain]--;
>>>  
>>>  	for_each_power_domain_well_rev(dev_priv, power_well, BIT_ULL(domain))
>>>  		intel_power_well_put(dev_priv, power_well);
>>> @@ -1720,6 +1843,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>  	BIT_ULL(POWER_DOMAIN_GT_IRQ) |			\
>>>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>>>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>>> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>>>  	BIT_ULL(POWER_DOMAIN_INIT))
>>>  
>>>  #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
>>> @@ -1743,6 +1867,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>>>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>>>  	BIT_ULL(POWER_DOMAIN_GMBUS) |			\
>>> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>>>  	BIT_ULL(POWER_DOMAIN_INIT))
>>>  #define BXT_DPIO_CMN_A_POWER_DOMAINS (			\
>>>  	BIT_ULL(POWER_DOMAIN_PORT_DDI_A_LANES) |		\
>>> @@ -1803,6 +1928,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>>>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>>>  	BIT_ULL(POWER_DOMAIN_GMBUS) |			\
>>> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>>>  	BIT_ULL(POWER_DOMAIN_INIT))
>>>  
>>>  #define CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS (		\
>>> @@ -1850,6 +1976,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
>>>  	CNL_DISPLAY_POWERWELL_2_POWER_DOMAINS |		\
>>>  	BIT_ULL(POWER_DOMAIN_MODESET) |			\
>>>  	BIT_ULL(POWER_DOMAIN_AUX_A) |			\
>>> +	BIT_ULL(POWER_DOMAIN_VBLANK) |			\
>>>  	BIT_ULL(POWER_DOMAIN_INIT))
>>>  
>>>  static const struct i915_power_well_ops i9xx_always_on_power_well_ops = {
>>> @@ -2083,9 +2210,12 @@ bool intel_display_power_well_is_enabled(struct drm_i915_private *dev_priv,
>>>  {
>>>  	struct i915_power_well *power_well;
>>>  	bool ret;
>>> +	unsigned long flags = 0;
>>>  
>>>  	power_well = lookup_power_well(dev_priv, power_well_id);
>>> +	power_well_lock(power_well, flags);
>>>  	ret = power_well->ops->is_enabled(dev_priv, power_well);
>>> +	power_well_unlock(power_well, flags);
>>>  
>>>  	return ret;
>>>  }
>>> @@ -2120,6 +2250,8 @@ static struct i915_power_well skl_power_wells[] = {
>>>  		.domains = SKL_DISPLAY_DC_OFF_POWER_DOMAINS,
>>>  		.ops = &gen9_dc_off_power_well_ops,
>>>  		.id = SKL_DISP_PW_DC_OFF,
>>> +		.supports_atomic_ctx = true,
>>> +		.dc_off_disabled = false,
>>>  	},
>>>  	{
>>>  		.name = "power well 2",
>>> @@ -2180,6 +2312,8 @@ static struct i915_power_well bxt_power_wells[] = {
>>>  		.domains = BXT_DISPLAY_DC_OFF_POWER_DOMAINS,
>>>  		.ops = &gen9_dc_off_power_well_ops,
>>>  		.id = SKL_DISP_PW_DC_OFF,
>>> +		.supports_atomic_ctx = true,
>>> +		.dc_off_disabled = false,
>>>  	},
>>>  	{
>>>  		.name = "power well 2",
>>> @@ -2235,6 +2369,8 @@ static struct i915_power_well glk_power_wells[] = {
>>>  		.domains = GLK_DISPLAY_DC_OFF_POWER_DOMAINS,
>>>  		.ops = &gen9_dc_off_power_well_ops,
>>>  		.id = SKL_DISP_PW_DC_OFF,
>>> +		.supports_atomic_ctx = true,
>>> +		.dc_off_disabled = false,
>>>  	},
>>>  	{
>>>  		.name = "power well 2",
>>> @@ -2359,6 +2495,8 @@ static struct i915_power_well cnl_power_wells[] = {
>>>  		.domains = CNL_DISPLAY_DC_OFF_POWER_DOMAINS,
>>>  		.ops = &gen9_dc_off_power_well_ops,
>>>  		.id = SKL_DISP_PW_DC_OFF,
>>> +		.supports_atomic_ctx = true,
>>> +		.dc_off_disabled = false,
>>>  	},
>>>  	{
>>>  		.name = "power well 2",
>>> @@ -2487,6 +2625,7 @@ static void assert_power_well_ids_unique(struct drm_i915_private *dev_priv)
>>>  int intel_power_domains_init(struct drm_i915_private *dev_priv)
>>>  {
>>>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>>> +	struct i915_power_well *power_well;
>>>  
>>>  	i915_modparams.disable_power_well =
>>>  		sanitize_disable_power_well_option(dev_priv,
>>> @@ -2524,6 +2663,10 @@ int intel_power_domains_init(struct drm_i915_private *dev_priv)
>>>  		set_power_wells(power_domains, i9xx_always_on_power_well);
>>>  	}
>>>  
>>> +	for_each_power_well(dev_priv, power_well)
>>> +		if (power_well->supports_atomic_ctx)
>>> +			spin_lock_init(&power_well->lock);
>>> +
>>>  	assert_power_well_ids_unique(dev_priv);
>>>  
>>>  	return 0;
>>> @@ -2571,9 +2714,13 @@ static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
>>>  
>>>  	mutex_lock(&power_domains->lock);
>>>  	for_each_power_well(dev_priv, power_well) {
>>> +		unsigned long flags = 0;
>>> +
>>> +		power_well_lock(power_well, flags);
>>>  		power_well->ops->sync_hw(dev_priv, power_well);
>>>  		power_well->hw_enabled = power_well->ops->is_enabled(dev_priv,
>>>  								     power_well);
>>> +		power_well_unlock(power_well, flags);
>>>  	}
>>>  	mutex_unlock(&power_domains->lock);
>>>  }
>>> @@ -3046,21 +3193,23 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
>>>  		bxt_display_core_uninit(dev_priv);
>>>  }
>>>  
>>> -static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv)
>>> +static void intel_power_domains_dump_info(struct drm_i915_private *dev_priv,
>>> +					  const int *power_well_use)
>>>  {
>>>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>>>  	struct i915_power_well *power_well;
>>> +	int i = 0;
>>>  
>>>  	for_each_power_well(dev_priv, power_well) {
>>>  		enum intel_display_power_domain domain;
>>>  
>>>  		DRM_DEBUG_DRIVER("%-25s %d\n",
>>> -				 power_well->name, power_well->count);
>>> +				 power_well->name, power_well_use[i++]);
>>>  
>>>  		for_each_power_domain(domain, power_well->domains)
>>>  			DRM_DEBUG_DRIVER("  %-23s %d\n",
>>>  					 intel_display_power_domain_str(domain),
>>> -					 power_domains->domain_use_count[domain]);
>>> +					 atomic_read(&power_domains->domain_use_count[domain]));
>>>  	}
>>>  }
>>>  
>>> @@ -3079,6 +3228,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>>>  	struct i915_power_domains *power_domains = &dev_priv->power_domains;
>>>  	struct i915_power_well *power_well;
>>>  	bool dump_domain_info;
>>> +	int power_well_use[dev_priv->power_domains.power_well_count];
>>>  
>>>  	mutex_lock(&power_domains->lock);
>>>  
>>> @@ -3087,6 +3237,14 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>>>  		enum intel_display_power_domain domain;
>>>  		int domains_count;
>>>  		bool enabled;
>>> +		int well_count, i = 0;
>>> +		unsigned long flags = 0;
>>> +
>>> +		power_well_lock(power_well, flags);
>>> +		well_count = power_well->count;
>>> +		enabled = power_well->ops->is_enabled(dev_priv, power_well);
>>> +		power_well_unlock(power_well, flags);
>>> +		power_well_use[i++] = well_count;
>>>  
>>>  		/*
>>>  		 * Power wells not belonging to any domain (like the MISC_IO
>>> @@ -3096,20 +3254,19 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>>>  		if (!power_well->domains)
>>>  			continue;
>>>  
>>> -		enabled = power_well->ops->is_enabled(dev_priv, power_well);
>>> -		if ((power_well->count || power_well->always_on) != enabled)
>>> +
>>> +		if ((well_count || power_well->always_on) != enabled)
>>>  			DRM_ERROR("power well %s state mismatch (refcount %d/enabled %d)",
>>> -				  power_well->name, power_well->count, enabled);
>>> +				  power_well->name, well_count, enabled);
>>>  
>>>  		domains_count = 0;
>>>  		for_each_power_domain(domain, power_well->domains)
>>> -			domains_count += power_domains->domain_use_count[domain];
>>> +			domains_count += atomic_read(&power_domains->domain_use_count[domain]);
>>>  
>>> -		if (power_well->count != domains_count) {
>>> +		if (well_count != domains_count) {
>>>  			DRM_ERROR("power well %s refcount/domain refcount mismatch "
>>>  				  "(refcount %d/domains refcount %d)\n",
>>> -				  power_well->name, power_well->count,
>>> -				  domains_count);
>>> +				  power_well->name, well_count, domains_count);
>>>  			dump_domain_info = true;
>>>  		}
>>>  	}
>>> @@ -3118,7 +3275,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv)
>>>  		static bool dumped;
>>>  
>>>  		if (!dumped) {
>>> -			intel_power_domains_dump_info(dev_priv);
>>> +			intel_power_domains_dump_info(dev_priv, power_well_use);
>>>  			dumped = true;
>>>  		}
>>>  	}
>>> -- 
>>> 2.11.0
>>>
>> I will probably have more comments later, but just doing a brain dump now
>> since I end up forgetting to write yesterday...
>>
>> The approach here in general is good and much better than that pre,post hooks.
>> But I just believe we can do this here in a more generic approach than deviating
>> the initial power well and domains.
> I would have liked a generic approach (for display_power_{get,put}), but
> I think this case is special enough that making it stand out is better.
Agreed, I can think of no other way myself without making the generic case too complicated. The whole runtime power management became way too complex for a special case.

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux