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@xxxxxxxxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
