On Wed, May 13, 2015 at 10:23:45PM +0200, Maarten Lankhorst wrote: > This needs to be a global check because at the time of crtc checking > not all modesets have to be calculated yet. Use intel_crtc->atomic > because there's no reason to keep it in state.
Hm, intel_crtc->atomic needs to be moved into intel_crtc_state eventually, so I don't really understand your reasoning here. Yes it's not really a static state, but we carry all the other state transition hints like ->need_modeset or ->active_changed around in state objects too. Imo it's the right place for this stuff, the only tricky part is that we must clear them all in the relevant duplicate_state hooks. Anyway nothing that blocks this one, just something to keep in mind for the road ahead. -Daniel > > Changes since v1: > - Use intel_crtc->atomic as a place to put hsw_workaround_pipe. > - Make sure quirk only applies to haswell. > - Use first loop to iterate over newly enabled crtc's only. > This increases readability. > > Signed-off-by: Maarten Lankhorst <[email protected]> > --- > drivers/gpu/drm/i915/intel_display.c | 100 > ++++++++++++++++++++++++----------- > drivers/gpu/drm/i915/intel_drv.h | 3 ++ > 2 files changed, 72 insertions(+), 31 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 1d3b0233f070..9316b0be4f5b 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4897,42 +4897,13 @@ 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; > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > struct intel_encoder *encoder; > - int pipe = intel_crtc->pipe; > + int pipe = intel_crtc->pipe, hsw_workaround_pipe; > > if (WARN_ON(intel_crtc->active)) > return; > @@ -5009,7 +4980,11 @@ 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); > + hsw_workaround_pipe = intel_crtc->atomic.hsw_workaround_pipe; > + if (IS_HASWELL(dev) && hsw_workaround_pipe != INVALID_PIPE) { > + intel_wait_for_vblank(dev, hsw_workaround_pipe); > + intel_wait_for_vblank(dev, hsw_workaround_pipe); > + } > } > > static void ironlake_pfit_disable(struct intel_crtc *crtc) > @@ -12099,6 +12074,66 @@ static int __intel_set_mode_setup_plls(struct > drm_atomic_state *state) > 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 *intel_crtc, *enabled_crtc; > + struct intel_crtc *first_crtc = NULL, *other_crtc = NULL; > + int enabled_crtcs = 0, i; > + > + /* look at all crtc's that are going to be enabled in during modeset */ > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + intel_crtc = to_intel_crtc(crtc); > + > + if (!crtc_state->active || !needs_modeset(crtc_state)) > + continue; > + > + if (first_crtc) { > + other_crtc = intel_crtc; > + break; > + } else { > + first_crtc = intel_crtc; > + } > + } > + > + /* No workaround needed? */ > + if (!first_crtc) > + return 0; > + > + /* w/a possibly needed, check how many crtc's are already enabled. */ > + for_each_intel_crtc(state->dev, intel_crtc) { > + intel_crtc->atomic.hsw_workaround_pipe = INVALID_PIPE; > + > + crtc_state = drm_atomic_get_crtc_state(state, > + &intel_crtc->base); > + if (IS_ERR(crtc_state)) > + return PTR_ERR(crtc_state); > + > + if (!crtc_state->active || needs_modeset(crtc_state)) > + continue; > + > + /* 2 or more enabled crtcs means no need for w/a */ > + if (++enabled_crtcs >= 2) > + return 0; > + > + enabled_crtc = intel_crtc; > + } > + > + if (enabled_crtcs == 1) > + first_crtc->atomic.hsw_workaround_pipe = enabled_crtc->pipe; > + else if (other_crtc) > + other_crtc->atomic.hsw_workaround_pipe = first_crtc->pipe; > + > + return 0; > +} > + > /* Code that should eventually be part of atomic_check() */ > static int __intel_set_mode_checks(struct drm_atomic_state *state) > { > @@ -12122,6 +12157,9 @@ static int __intel_set_mode_checks(struct > drm_atomic_state *state) > if (ret) > return ret; > > + if (IS_HASWELL(dev)) > + haswell_mode_set_planes_workaround(state); > + > return 0; > } > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 65f5f3d41b5a..1aa10a9445de 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -490,6 +490,9 @@ struct intel_crtc_atomic_commit { > bool update_fbc; > bool post_enable_primary; > unsigned update_sprite_watermarks; > + > + /* Operations to perform during modeset */ > + enum pipe hsw_workaround_pipe; > }; > > struct intel_crtc { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > [email protected] > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
