| Message ID | 1431548627-2527-16-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <maarten.lankhorst@linux.intel.com> > --- > 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 > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi, On 18 May 2015 at 16:47, Daniel Vetter <daniel@ffwll.ch> wrote: > 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. What makes me slightly nervous is the number of places where we duplicate the state, meaning that we could easily bleed those if we're not extremely careful about how we do it. I argued for this one to go into intel_crtc->atomic as it was much more suited than the static state, but if we're implementing a transient state section of intel_crtc_state (ideally under its own sub-struct, so it can easily be zeroed), then I guess that wfm. Cheers, Daniel
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 {
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. 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 <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 100 ++++++++++++++++++++++++----------- drivers/gpu/drm/i915/intel_drv.h | 3 ++ 2 files changed, 72 insertions(+), 31 deletions(-)