Re: [PATCH 03/42] drm/i915: Only update required power domains.

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 11-05-15 om 19:00 schreef Daniel Vetter:
> On Mon, May 11, 2015 at 04:24:39PM +0200, Maarten Lankhorst wrote:
>> This prevents unnecessarily updating power domains, while still
>> enabling all power domains on initial setup.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 52 ++++++++++++++++++++++++++++--------
>>  1 file changed, 41 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index af96d686aae2..42d0cc329b37 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5298,36 +5298,72 @@ static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
>>  	return mask;
>>  }
>>  
>> +static bool
>> +needs_modeset(struct drm_crtc_state *state)
> I think we should extract this from drm_atomic_helper.c. But that can be
> done as a follow-up - if you track it ;-)
>> +{
>> +	return state->mode_changed || state->active_changed;
>> +}
>> +
>>  static void modeset_update_crtc_power_domains(struct drm_atomic_state *state)
>>  {
>>  	struct drm_device *dev = state->dev;
>>  	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	unsigned long pipe_domains[I915_MAX_PIPES] = { 0, };
>>  	struct intel_crtc *crtc;
>> +	bool init_power = dev_priv->power_domains.init_power_on;
>> +	bool any_power = init_power, any_modeset = false;
>> +	unsigned long domains;
>>  
>>  	/*
>>  	 * First get all needed power domains, then put all unneeded, to avoid
>>  	 * any unnecessary toggling of the power wells.
>>  	 */
>>  	for_each_intel_crtc(dev, crtc) {
>> +		int idx = drm_crtc_index(&crtc->base);
>> +		struct drm_crtc_state *crtc_state = state->crtc_states[idx];
>>  		enum intel_display_power_domain domain;
>>  
>> -		if (!crtc->base.state->enable)
>> +		if (!init_power && !crtc_state)
>> +			continue;
> 		if (!needs_modeset)
> 			continue;
>
> while you're optimizing this?
>
>> +
>> +		if (needs_modeset(crtc->base.state))
>> +			any_modeset = true;
>> +
>> +		if (crtc->base.state->enable)
>> +			pipe_domains[crtc->pipe] =
>> +				get_crtc_power_domains(&crtc->base);
>> +
>> +		if (pipe_domains[crtc->pipe] == crtc->enabled_power_domains)
>>  			continue;
>>  
>> -		pipe_domains[crtc->pipe] = get_crtc_power_domains(&crtc->base);
>> +		WARN_ON(!init_power && !needs_modeset(crtc->base.state));
>> +
>> +		any_power = true;
>> +		domains = pipe_domains[crtc->pipe] &
>> +			  ~crtc->enabled_power_domains;
>>  
>> -		for_each_power_domain(domain, pipe_domains[crtc->pipe])
>> +		for_each_power_domain(domain, domains)
>>  			intel_display_power_get(dev_priv, domain);
>>  	}
>>  
>> -	if (dev_priv->display.modeset_global_resources)
>> +	if (any_modeset && dev_priv->display.modeset_global_resources)
>>  		dev_priv->display.modeset_global_resources(state);
>>  
>> +	if (!any_power)
>> +		return;
>> +
>>  	for_each_intel_crtc(dev, crtc) {
>> +		int idx = drm_crtc_index(&crtc->base);
>> +		struct drm_crtc_state *crtc_state = state->crtc_states[idx];
>>  		enum intel_display_power_domain domain;
>>  
>> -		for_each_power_domain(domain, crtc->enabled_power_domains)
>> +		if (!init_power && !crtc_state)
>> +			continue;
> Same shortcut here?
It's not an optimization, the same path can be used for modeset and updating planes.
If it's a plane update the code might run with irqs disabled, in which case we can't call power_get or power_put because that requires a mutex.

~Maarten

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx