[Intel-gfx] [PATCH 2/3] drm/i915: Enable IPS for sprite plane

[Maarten Lankhorst]

Op 17-11-17 om 16:55 schreef Ville Syrjälä:
> On Fri, Nov 17, 2017 at 04:37:55PM +0100, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++-------
>>  1 file changed, 6 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 8283e80597bd..38a1cdb3dbb2 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -5044,7 +5044,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
>>  		intel_fbc_post_update(crtc);
>>  
>>  		if (primary_state->base.visible &&
>> -		    (needs_modeset(&pipe_config->base) ||
>> +		    (pipe_config->disable_cxsr ||
>>  		     !old_primary_state->base.visible))
>>  			intel_post_enable_primary(&crtc->base, pipe_config);
>>  	}
>> @@ -5064,7 +5064,7 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
>>  	struct intel_atomic_state *old_intel_state =
>>  		to_intel_atomic_state(old_state);
>>  
>> -	if (needs_modeset(&pipe_config->base) || !pipe_config->ips_enabled)
>> +	if (pipe_config->disable_cxsr || !pipe_config->ips_enabled)
> What does IPS have to do with cxsr?
Nothing, laziness. disable cxsr is set when planes get disabled.
>>  		hsw_disable_ips(old_crtc_state);
>>  
>>  	if (old_pri_state) {
>> @@ -6224,12 +6224,11 @@ static bool pipe_config_supports_ips(struct drm_i915_private *dev_priv,
>>  	visible_planes = pipe_config->active_planes & ~BIT(PLANE_CURSOR);
>>  
>>  	/*
>> -	 * FIXME IPS should be fine as long as one plane is
>> -	 * enabled, but in practice it seems to have problems
>> -	 * when going from primary only to sprite only and vice
>> -	 * versa.
>> +	 * IPS should be fine as long as one plane is enabled, but
>> +	 * temporarily disable it when when going from primary only
>> +	 * to sprite only and vice versa.
>>  	 */
>> -	if (visible_planes != BIT(PLANE_PRIMARY))
>> +	if (hweight32(visible_planes) != 1)
>>  		return false;
> That should just be
> if (active_planes == 0)
> 	return false;
>
> assuming we have no problems with the toggling between
> primary only and sprite only.
>
> I can't recall how the cursor affecrs IPS. But I think IPS should 
> work as long as any plane (including the cursor) is enabled.
But cursor visibility can change without the full atomic commit, so it's too unreliable to take into
account with the calculations.

What happens when it doesn't work well? Would it be caught by any tests?

~Maarten