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