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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 12, 2015 at 04:05:24PM +0200, Maarten Lankhorst wrote:
> 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.

Hm right I mixed up first_crtc_state and enabled_crtc. Looks correct on
second pass.

> > 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.

Hm yeah connector_mutex is the general "global modeset state" lock. I
guess we could instead acquire that one and make sure that
crtc_state->active is also protected by it. I.e. have a

	for_each_crtc_in_state(crtc_state)
		if (crtc_state->active_changed)
			acquire_connection_mutex();

Maybe even do this in the helper check_modeset function? Well looking at
that one it's kinda guaranteed already, so a WARN_ON(!ww_mutex_locked)
should do the trick too. Plus a comment that crtc_state->active is
protected by the connection_mutex thanks to the helper's logic.

Going with that is fine with me too, I just feel like the current
open-coded locking logic is a bit too clever/complicated for what we want
to do.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx