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