Re: [PATCH 32/42] drm/i915: Calculate haswell plane workaround.

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

Op 12-05-15 om 11:43 schreef Daniel Vetter:
> On Mon, May 11, 2015 at 04:25:08PM +0200, Maarten Lankhorst wrote:
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 109 +++++++++++++++++++++++++----------
>>  drivers/gpu/drm/i915/intel_drv.h     |   2 +
>>  2 files changed, 81 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 7bc78b49f9f4..7a79659dca00 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4848,35 +4848,6 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
>>  	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>>  }
>>  
>> -/*
>> - * This implements the workaround described in the "notes" section of the mode
>> - * set sequence documentation. When going from no pipes or single pipe to
>> - * multiple pipes, and planes are enabled after the pipe, we need to wait at
>> - * least 2 vblanks on the first pipe before enabling planes on the second pipe.
>> - */
>> -static void haswell_mode_set_planes_workaround(struct intel_crtc *crtc)
>> -{
>> -	struct drm_device *dev = crtc->base.dev;
>> -	struct intel_crtc *crtc_it, *other_active_crtc = NULL;
>> -
>> -	/* We want to get the other_active_crtc only if there's only 1 other
>> -	 * active crtc. */
>> -	for_each_intel_crtc(dev, crtc_it) {
>> -		if (!crtc_it->active || crtc_it == crtc)
>> -			continue;
>> -
>> -		if (other_active_crtc)
>> -			return;
>> -
>> -		other_active_crtc = crtc_it;
>> -	}
>> -	if (!other_active_crtc)
>> -		return;
>> -
>> -	intel_wait_for_vblank(dev, other_active_crtc->pipe);
>> -	intel_wait_for_vblank(dev, other_active_crtc->pipe);
>> -}
>> -
>>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>>  {
>>  	struct drm_device *dev = crtc->dev;
>> @@ -4967,7 +4938,10 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>>  
>>  	/* If we change the relative order between pipe/planes enabling, we need
>>  	 * to change the workaround. */
>> -	haswell_mode_set_planes_workaround(intel_crtc);
>> +	if (pipe_config->hsw_workaround_pipe != INVALID_PIPE) {
>> +		intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
>> +		intel_wait_for_vblank(dev, pipe_config->hsw_workaround_pipe);
>> +	}
>>  }
>>  
>>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
>> @@ -12147,6 +12121,74 @@ done:
>>  	return ret;
>>  }
>>  
>> +/*
>> + * This implements the workaround described in the "notes" section of the mode
>> + * set sequence documentation. When going from no pipes or single pipe to
>> + * multiple pipes, and planes are enabled after the pipe, we need to wait at
>> + * least 2 vblanks on the first pipe before enabling planes on the second pipe.
>> + */
>> +static int haswell_mode_set_planes_workaround(struct drm_atomic_state *state)
>> +{
>> +	struct drm_crtc *crtc;
>> +	struct drm_crtc_state *crtc_state;
>> +	struct intel_crtc_state *first_crtc_state = NULL, *other_crtc_state = NULL;
>> +	struct intel_crtc *intel_crtc, *first_crtc = NULL, *enabled_crtc;
>> +	int enabled_crtcs = 0, ret, i;
>> +
>> +	for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +		struct intel_crtc_state *pipe_config =
>> +			to_intel_crtc_state(crtc_state);
>> +
>> +		pipe_config->hsw_workaround_pipe = INVALID_PIPE;
> This kind of state resetting should be done in duplicate_state.
The v2 of this patch uses intel_crtc->atomic.hsw_workaround pipe, which would work better. :)
>> +
>> +		if (!crtc_state->active)
>> +			continue;
>> +
>> +		if (!needs_modeset(crtc_state)) {
>> +			enabled_crtcs++;
>> +			enabled_crtc = to_intel_crtc(crtc);
>> +			continue;
>> +		}
>> +
>> +		if (first_crtc) {
>> +			other_crtc_state = pipe_config;
>> +			break;
>> +		}
>> +		first_crtc = to_intel_crtc(crtc);
>> +		first_crtc_state = pipe_config;
>> +	}
>> +
>> +	/* No workaround needed? */
>> +	if (!first_crtc || enabled_crtcs > 1)
>> +		return 0;
>> +
>> +	for_each_intel_crtc(state->dev, intel_crtc) {
>> +		if (state->crtcs[drm_crtc_index(&intel_crtc->base)])
>> +			continue;
>> +
>> +		ret = drm_modeset_lock(&intel_crtc->base.mutex,
>> +				       state->acquire_ctx);
>> +		if (ret)
>> +			return ret;
>> +
>> +		if (!intel_crtc->base.state->active)
>> +			continue;
> Imo just unconditionally acquire the crtc state, there' shouldn't be any
> harm in that (as long as we leave mode/active/planes_changed untouched).
Probably.
>> +
>> +		/* 2 enabled crtcs means no need for w/a */
>> +		if (++enabled_crtcs >= 2)
>> +			return 0;
>> +
>> +		enabled_crtc = intel_crtc;
>> +	}
>> +
>> +	if (enabled_crtcs == 1)
>> +		first_crtc_state->hsw_workaround_pipe = enabled_crtc->pipe;
> first_crtc_state could be miscomputed if 1 crtc is already on and you add
> another one. Then first_crtc will point at the other crtc since that's the
> only one at first in the atomic set. Imo a simpler algo would be:
No, in that case enabled_crtcs != 0 and enabled_crtc is set to the correct crtc.


> 1. Check whether your activating any crtc at all.
> 2. If so add all crtc states.
> 3. Same loop as the old one, just using for_each_crtc_in_state.
>
> There's other cases with shared resources where we need to grab all crtc
> locks already anyway (shared dpll), trying to be clever doesn't seem
> beneficial. And if this indeed becomes a problem then we need a global
> state ww mutex and use that (instead of crtc locks) for this book-keeping:
> First enable crtc would set global_state->hsw_wa_pipe, 2nd and later would
> clear it. We might need this eventually (I've heard rumours about people
> not liking stalls when unplugging external screens that much), but let's
> not overcomplicate things while we do this conversion.
You would still need it when disabling crtc's then. To do it right you would need to keep crtc->active,
or use a crtc_mask, but that would be abused so I'd rather not. The crtc lock is also needed so the
other crtc's cannot be disabled or enabled while modesetting, but I guess always taking the
connector_mutex for that could work too.

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