| Message ID | 1447945645-32005-6-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: > Currently we perform our own wait in post_plane_update, > but the atomic core performs another one in wait_for_vblanks. > This means that 2 vblanks are done when a fb is changed, > which is a bit overkill. > > Merge them by creating a helper function that takes a crtc mask > for the planes to wait on. > > The broadwell vblank workaround may look gone entirely but this is > not the case. pipe_config->wm_changed is set to true > when any plane is turned on, which forces a vblank wait. > > Changes since v1: > - Removing the double vblank wait on broadwell moved to its own commit. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 1 + > drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++--------- > - > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 65 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > b/drivers/gpu/drm/i915/intel_atomic.c > index 4625f8a9ba12..8e579a8505ac 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > crtc_state->disable_lp_wm = false; > crtc_state->disable_cxsr = false; > crtc_state->wm_changed = false; > + crtc_state->fb_changed = false; > > return &crtc_state->base; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 60f17bc5f0ce..299edbf6f99e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc > *crtc) > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > - if (atomic->wait_vblank) > - intel_wait_for_vblank(dev, crtc->pipe); > - > intel_frontbuffer_flip(dev, atomic->fb_bits); > > crtc->wm.cxsr_allowed = true; > @@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc > *crtc) > if (atomic->post_enable_primary) > intel_post_enable_primary(&crtc->base); > > + if (needs_modeset(&pipe_config->base)) > + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > + > memset(atomic, 0, sizeof(*atomic)); > } > > @@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > if (!was_visible && !visible) > return 0; > > + if (fb != old_plane_state->base.fb) > + pipe_config->fb_changed = true; > + > turn_off = was_visible && (!visible || mode_changed); > turn_on = visible && (!was_visible || mode_changed); > > @@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > > /* must disable cxsr around plane enable/disable */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > - if (is_crtc_enabled) > - intel_crtc->atomic.wait_vblank = true; > pipe_config->disable_cxsr = true; > } > } else if ((was_visible || visible) && > @@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > plane_state->rotation != BIT(DRM_ROTATE_0)) > intel_crtc->atomic.disable_fbc = true; > > - /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (turn_on && IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - > intel_crtc->atomic.update_fbc |= visible || mode_changed; > break; > case DRM_PLANE_TYPE_CURSOR: > @@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > if (IS_IVYBRIDGE(dev) && > needs_scaling(to_intel_plane_state(plane_state)) && > !needs_scaling(old_plane_state)) { > - to_intel_crtc_state(crtc_state)->disable_lp_wm = > true; > - } else if (turn_off && !mode_changed) { > - intel_crtc->atomic.wait_vblank = true; > + pipe_config->disable_lp_wm = true; > + } else if (turn_off && !mode_changed) > intel_crtc->atomic.update_sprite_watermarks |= > 1 << i; > - } > > break; > } > @@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct > drm_device *dev, > return ret; > } > > +static void intel_atomic_wait_for_vblanks(struct drm_device *dev, > + struct drm_i915_private *dev_priv, > + unsigned crtc_mask) > +{ > + unsigned last_vblank_count[I915_MAX_PIPES]; > + enum pipe pipe; > + int ret; > + > + if (!crtc_mask) > + return; > + > + for_each_pipe(dev_priv, pipe) { > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + > + if (!((1 << pipe) & crtc_mask)) > + continue; > + > + ret = drm_crtc_vblank_get(crtc); > + if (ret != 0) { > + crtc_mask &= ~(1 << pipe); > + continue; > + } > + > + last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); > + } > + > + for_each_pipe(dev_priv, pipe) { > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + > + if (!((1 << pipe) & crtc_mask)) > + continue; > + > + wait_event_timeout(dev->vblank[pipe].queue, > + last_vblank_count[pipe] != > + drm_crtc_vblank_count(crtc), > + msecs_to_jiffies(50)); > + > + drm_crtc_vblank_put(crtc); > + } > +} > + > + > /** > * intel_atomic_commit - commit validated state object > * @dev: DRM device > @@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev, > struct drm_crtc *crtc; > int ret = 0, i; > bool hw_check = intel_state->modeset; > + unsigned crtc_vblank_mask = 0; > > ret = intel_atomic_prepare_commit(dev, state, async); > if (ret) { > @@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev, > for_each_crtc_in_state(state, crtc, crtc_state, i) { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > bool modeset = needs_modeset(crtc->state); > - bool update_pipe = !modeset && > - to_intel_crtc_state(crtc->state)->update_pipe; > + struct intel_crtc_state *pipe_config = > + to_intel_crtc_state(crtc->state); > + bool update_pipe = !modeset && pipe_config->update_pipe; > unsigned long put_domains = 0; > > if (modeset) > @@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device > *dev, > (crtc->state->planes_changed || update_pipe)) > drm_atomic_helper_commit_planes_on_crtc(crtc_state); > > + if (pipe_config->base.active && > + (pipe_config->wm_changed || pipe_config->disable_cxsr || > + pipe_config->fb_changed)) > + crtc_vblank_mask |= 1 << i; > + > if (put_domains) > modeset_put_power_domains(dev_priv, put_domains); > - > - intel_post_plane_update(intel_crtc); > - > - if (modeset) > - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); Would it be too evil to move the power domain get/put out of the loop? If I understand correctly, intel_state->modeset already tells us if there is any crtc that needs a modeset, so we could just protect the calls with that. I'm not sure what is the impact, but at least we could avoid having the get in intel_atomic_commit() and the put in intel_post_plane_enable(). Ander > } > > /* FIXME: add subpixel order */ > > - drm_atomic_helper_wait_for_vblanks(dev, state); > + intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) > + intel_post_plane_update(to_intel_crtc(crtc)); > > mutex_lock(&dev->struct_mutex); > drm_atomic_helper_cleanup_planes(dev, state); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index db61c37dbf09..66f66740ccaa 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -373,6 +373,7 @@ struct intel_crtc_state { > bool update_pipe; /* can a fast modeset be performed? */ > bool disable_cxsr; > bool wm_changed; /* watermarks are updated */ > + bool fb_changed; /* fb on any of the planes is changed */ > > /* Pipe source size (ie. panel fitter input size) > * All planes will be positioned inside this space, > @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit { > > /* Sleepable operations to perform after commit */ > unsigned fb_bits; > - bool wait_vblank; > bool update_fbc; > bool post_enable_primary; > unsigned update_sprite_watermarks;
On ke, 2015-11-25 at 14:21 +0200, Ander Conselvan De Oliveira wrote: > On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: > > Currently we perform our own wait in post_plane_update, > > but the atomic core performs another one in wait_for_vblanks. > > This means that 2 vblanks are done when a fb is changed, > > which is a bit overkill. > > > > Merge them by creating a helper function that takes a crtc mask > > for the planes to wait on. > > > > The broadwell vblank workaround may look gone entirely but this is > > not the case. pipe_config->wm_changed is set to true > > when any plane is turned on, which forces a vblank wait. > > > > Changes since v1: > > - Removing the double vblank wait on broadwell moved to its own commit. > > > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_atomic.c | 1 + > > drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++--------- > > - > > drivers/gpu/drm/i915/intel_drv.h | 2 +- > > 3 files changed, 65 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > > b/drivers/gpu/drm/i915/intel_atomic.c > > index 4625f8a9ba12..8e579a8505ac 100644 > > --- a/drivers/gpu/drm/i915/intel_atomic.c > > +++ b/drivers/gpu/drm/i915/intel_atomic.c > > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > crtc_state->disable_lp_wm = false; > > crtc_state->disable_cxsr = false; > > crtc_state->wm_changed = false; > > + crtc_state->fb_changed = false; > > > > return &crtc_state->base; > > } > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 60f17bc5f0ce..299edbf6f99e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc > > *crtc) > > struct drm_device *dev = crtc->base.dev; > > struct drm_i915_private *dev_priv = dev->dev_private; > > > > - if (atomic->wait_vblank) > > - intel_wait_for_vblank(dev, crtc->pipe); > > - > > intel_frontbuffer_flip(dev, atomic->fb_bits); > > > > crtc->wm.cxsr_allowed = true; > > @@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc > > *crtc) > > if (atomic->post_enable_primary) > > intel_post_enable_primary(&crtc->base); > > > > + if (needs_modeset(&pipe_config->base)) > > + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > + > > memset(atomic, 0, sizeof(*atomic)); > > } > > > > @@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct > > drm_crtc_state *crtc_state, > > if (!was_visible && !visible) > > return 0; > > > > + if (fb != old_plane_state->base.fb) > > + pipe_config->fb_changed = true; > > + > > turn_off = was_visible && (!visible || mode_changed); > > turn_on = visible && (!was_visible || mode_changed); > > > > @@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct > > drm_crtc_state *crtc_state, > > > > /* must disable cxsr around plane enable/disable */ > > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > > - if (is_crtc_enabled) > > - intel_crtc->atomic.wait_vblank = true; > > pipe_config->disable_cxsr = true; > > } > > } else if ((was_visible || visible) && > > @@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct > > drm_crtc_state *crtc_state, > > plane_state->rotation != BIT(DRM_ROTATE_0)) > > intel_crtc->atomic.disable_fbc = true; > > > > - /* > > - * BDW signals flip done immediately if the plane > > - * is disabled, even if the plane enable is already > > - * armed to occur at the next vblank :( > > - */ > > - if (turn_on && IS_BROADWELL(dev)) > > - intel_crtc->atomic.wait_vblank = true; > > - > > intel_crtc->atomic.update_fbc |= visible || mode_changed; > > break; > > case DRM_PLANE_TYPE_CURSOR: > > @@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct > > drm_crtc_state *crtc_state, > > if (IS_IVYBRIDGE(dev) && > > needs_scaling(to_intel_plane_state(plane_state)) && > > !needs_scaling(old_plane_state)) { > > - to_intel_crtc_state(crtc_state)->disable_lp_wm = > > true; > > - } else if (turn_off && !mode_changed) { > > - intel_crtc->atomic.wait_vblank = true; > > + pipe_config->disable_lp_wm = true; > > + } else if (turn_off && !mode_changed) > > intel_crtc->atomic.update_sprite_watermarks |= > > 1 << i; > > - } > > > > break; > > } > > @@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct > > drm_device *dev, > > return ret; > > } > > > > +static void intel_atomic_wait_for_vblanks(struct drm_device *dev, > > + struct drm_i915_private *dev_priv, > > + unsigned crtc_mask) > > +{ > > + unsigned last_vblank_count[I915_MAX_PIPES]; > > + enum pipe pipe; > > + int ret; > > + > > + if (!crtc_mask) > > + return; > > + > > + for_each_pipe(dev_priv, pipe) { > > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > > + > > + if (!((1 << pipe) & crtc_mask)) > > + continue; > > + > > + ret = drm_crtc_vblank_get(crtc); > > + if (ret != 0) { > > + crtc_mask &= ~(1 << pipe); > > + continue; > > + } > > + > > + last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); > > + } > > + > > + for_each_pipe(dev_priv, pipe) { > > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > > + > > + if (!((1 << pipe) & crtc_mask)) > > + continue; > > + > > + wait_event_timeout(dev->vblank[pipe].queue, > > + last_vblank_count[pipe] != > > + drm_crtc_vblank_count(crtc), > > + msecs_to_jiffies(50)); > > + > > + drm_crtc_vblank_put(crtc); > > + } > > +} > > + > > + > > /** > > * intel_atomic_commit - commit validated state object > > * @dev: DRM device > > @@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev, > > struct drm_crtc *crtc; > > int ret = 0, i; > > bool hw_check = intel_state->modeset; > > + unsigned crtc_vblank_mask = 0; > > > > ret = intel_atomic_prepare_commit(dev, state, async); > > if (ret) { > > @@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev, > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > bool modeset = needs_modeset(crtc->state); > > - bool update_pipe = !modeset && > > - to_intel_crtc_state(crtc->state)->update_pipe; > > + struct intel_crtc_state *pipe_config = > > + to_intel_crtc_state(crtc->state); > > + bool update_pipe = !modeset && pipe_config->update_pipe; > > unsigned long put_domains = 0; > > > > if (modeset) > > @@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device > > *dev, > > (crtc->state->planes_changed || update_pipe)) > > drm_atomic_helper_commit_planes_on_crtc(crtc_state); > > > > + if (pipe_config->base.active && > > + (pipe_config->wm_changed || pipe_config->disable_cxsr || > > + pipe_config->fb_changed)) > > + crtc_vblank_mask |= 1 << i; > > + > > if (put_domains) > > modeset_put_power_domains(dev_priv, put_domains); > > - > > - intel_post_plane_update(intel_crtc); > > - > > - if (modeset) > > - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > Would it be too evil to move the power domain get/put out of the loop? If I > understand correctly, intel_state->modeset already tells us if there is any crtc > that needs a modeset, so we could just protect the calls with that. I'm not sure > what is the impact, but at least we could avoid having the get in > intel_atomic_commit() and the put in intel_post_plane_enable(). Yes, that's cleaner, and possibly avoids some unnecessary on/off toggling of power wells. We could've done this already earlier using the existing any_ms flag.. Btw, not the problem in this patch, but I just noticed we should take the modeset power domain already earlier. We are disabling at least CRTCs/PLLs already earlier which is in the set of operations we need to protect with this power domain. That could be fixed as a follow-up. --Imre > > Ander > > > } > > > > /* FIXME: add subpixel order */ > > > > - drm_atomic_helper_wait_for_vblanks(dev, state); > > + intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); > > + > > + for_each_crtc_in_state(state, crtc, crtc_state, i) > > + intel_post_plane_update(to_intel_crtc(crtc)); > > > > mutex_lock(&dev->struct_mutex); > > drm_atomic_helper_cleanup_planes(dev, state); > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > > b/drivers/gpu/drm/i915/intel_drv.h > > index db61c37dbf09..66f66740ccaa 100644 > > --- a/drivers/gpu/drm/i915/intel_drv.h > > +++ b/drivers/gpu/drm/i915/intel_drv.h > > @@ -373,6 +373,7 @@ struct intel_crtc_state { > > bool update_pipe; /* can a fast modeset be performed? */ > > bool disable_cxsr; > > bool wm_changed; /* watermarks are updated */ > > + bool fb_changed; /* fb on any of the planes is changed */ > > > > /* Pipe source size (ie. panel fitter input size) > > * All planes will be positioned inside this space, > > @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit { > > > > /* Sleepable operations to perform after commit */ > > unsigned fb_bits; > > - bool wait_vblank; > > bool update_fbc; > > bool post_enable_primary; > > unsigned update_sprite_watermarks;
On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: > Currently we perform our own wait in post_plane_update, > but the atomic core performs another one in wait_for_vblanks. > This means that 2 vblanks are done when a fb is changed, > which is a bit overkill. > > Merge them by creating a helper function that takes a crtc mask > for the planes to wait on. > > The broadwell vblank workaround may look gone entirely but this is > not the case. pipe_config->wm_changed is set to true > when any plane is turned on, which forces a vblank wait. > > Changes since v1: > - Removing the double vblank wait on broadwell moved to its own commit. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 1 + > drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++--------- > - > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 3 files changed, 65 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c > b/drivers/gpu/drm/i915/intel_atomic.c > index 4625f8a9ba12..8e579a8505ac 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > crtc_state->disable_lp_wm = false; > crtc_state->disable_cxsr = false; > crtc_state->wm_changed = false; > + crtc_state->fb_changed = false; > > return &crtc_state->base; > } > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 60f17bc5f0ce..299edbf6f99e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc > *crtc) > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > > - if (atomic->wait_vblank) > - intel_wait_for_vblank(dev, crtc->pipe); > - > intel_frontbuffer_flip(dev, atomic->fb_bits); > > crtc->wm.cxsr_allowed = true; > @@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc > *crtc) > if (atomic->post_enable_primary) > intel_post_enable_primary(&crtc->base); > > + if (needs_modeset(&pipe_config->base)) > + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > + > memset(atomic, 0, sizeof(*atomic)); > } > > @@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > if (!was_visible && !visible) > return 0; > > + if (fb != old_plane_state->base.fb) > + pipe_config->fb_changed = true; > + > turn_off = was_visible && (!visible || mode_changed); > turn_on = visible && (!was_visible || mode_changed); > > @@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > > /* must disable cxsr around plane enable/disable */ > if (plane->type != DRM_PLANE_TYPE_CURSOR) { > - if (is_crtc_enabled) > - intel_crtc->atomic.wait_vblank = true; > pipe_config->disable_cxsr = true; > } > } else if ((was_visible || visible) && > @@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > plane_state->rotation != BIT(DRM_ROTATE_0)) > intel_crtc->atomic.disable_fbc = true; > > - /* > - * BDW signals flip done immediately if the plane > - * is disabled, even if the plane enable is already > - * armed to occur at the next vblank :( > - */ > - if (turn_on && IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - > intel_crtc->atomic.update_fbc |= visible || mode_changed; > break; > case DRM_PLANE_TYPE_CURSOR: > @@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct > drm_crtc_state *crtc_state, > if (IS_IVYBRIDGE(dev) && > needs_scaling(to_intel_plane_state(plane_state)) && > !needs_scaling(old_plane_state)) { > - to_intel_crtc_state(crtc_state)->disable_lp_wm = > true; > - } else if (turn_off && !mode_changed) { > - intel_crtc->atomic.wait_vblank = true; > + pipe_config->disable_lp_wm = true; > + } else if (turn_off && !mode_changed) > intel_crtc->atomic.update_sprite_watermarks |= > 1 << i; > - } Did you intend to delete the braces on both sides of the if? Sorry, OCD. :) Other than this and the comment on the other thread were I added Patrik and Imre, this looks good. Ander > > break; > } > @@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct > drm_device *dev, > return ret; > } > > +static void intel_atomic_wait_for_vblanks(struct drm_device *dev, > + struct drm_i915_private *dev_priv, > + unsigned crtc_mask) > +{ > + unsigned last_vblank_count[I915_MAX_PIPES]; > + enum pipe pipe; > + int ret; > + > + if (!crtc_mask) > + return; > + > + for_each_pipe(dev_priv, pipe) { > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + > + if (!((1 << pipe) & crtc_mask)) > + continue; > + > + ret = drm_crtc_vblank_get(crtc); > + if (ret != 0) { > + crtc_mask &= ~(1 << pipe); > + continue; > + } > + > + last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); > + } > + > + for_each_pipe(dev_priv, pipe) { > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; > + > + if (!((1 << pipe) & crtc_mask)) > + continue; > + > + wait_event_timeout(dev->vblank[pipe].queue, > + last_vblank_count[pipe] != > + drm_crtc_vblank_count(crtc), > + msecs_to_jiffies(50)); > + > + drm_crtc_vblank_put(crtc); > + } > +} > + > + > /** > * intel_atomic_commit - commit validated state object > * @dev: DRM device > @@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev, > struct drm_crtc *crtc; > int ret = 0, i; > bool hw_check = intel_state->modeset; > + unsigned crtc_vblank_mask = 0; > > ret = intel_atomic_prepare_commit(dev, state, async); > if (ret) { > @@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev, > for_each_crtc_in_state(state, crtc, crtc_state, i) { > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > bool modeset = needs_modeset(crtc->state); > - bool update_pipe = !modeset && > - to_intel_crtc_state(crtc->state)->update_pipe; > + struct intel_crtc_state *pipe_config = > + to_intel_crtc_state(crtc->state); > + bool update_pipe = !modeset && pipe_config->update_pipe; > unsigned long put_domains = 0; > > if (modeset) > @@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device > *dev, > (crtc->state->planes_changed || update_pipe)) > drm_atomic_helper_commit_planes_on_crtc(crtc_state); > > + if (pipe_config->base.active && > + (pipe_config->wm_changed || pipe_config->disable_cxsr || > + pipe_config->fb_changed)) > + crtc_vblank_mask |= 1 << i; > + > if (put_domains) > modeset_put_power_domains(dev_priv, put_domains); > - > - intel_post_plane_update(intel_crtc); > - > - if (modeset) > - intel_display_power_put(dev_priv, > POWER_DOMAIN_MODESET); > } > > /* FIXME: add subpixel order */ > > - drm_atomic_helper_wait_for_vblanks(dev, state); > + intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) > + intel_post_plane_update(to_intel_crtc(crtc)); > > mutex_lock(&dev->struct_mutex); > drm_atomic_helper_cleanup_planes(dev, state); > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index db61c37dbf09..66f66740ccaa 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -373,6 +373,7 @@ struct intel_crtc_state { > bool update_pipe; /* can a fast modeset be performed? */ > bool disable_cxsr; > bool wm_changed; /* watermarks are updated */ > + bool fb_changed; /* fb on any of the planes is changed */ > > /* Pipe source size (ie. panel fitter input size) > * All planes will be positioned inside this space, > @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit { > > /* Sleepable operations to perform after commit */ > unsigned fb_bits; > - bool wait_vblank; > bool update_fbc; > bool post_enable_primary; > unsigned update_sprite_watermarks;
Hi, On 25 November 2015 at 12:21, Ander Conselvan De Oliveira <conselvan2@gmail.com> wrote: > On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote: >> @@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc >> *crtc) >> if (atomic->post_enable_primary) >> intel_post_enable_primary(&crtc->base); >> >> + if (needs_modeset(&pipe_config->base)) >> + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); >> + >> memset(atomic, 0, sizeof(*atomic)); >> } >> >> @@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev, >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> bool modeset = needs_modeset(crtc->state); >> - bool update_pipe = !modeset && >> - to_intel_crtc_state(crtc->state)->update_pipe; >> + struct intel_crtc_state *pipe_config = >> + to_intel_crtc_state(crtc->state); >> + bool update_pipe = !modeset && pipe_config->update_pipe; >> unsigned long put_domains = 0; >> >> if (modeset) >> @@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device >> *dev, >> (crtc->state->planes_changed || update_pipe)) >> drm_atomic_helper_commit_planes_on_crtc(crtc_state); >> >> + if (pipe_config->base.active && >> + (pipe_config->wm_changed || pipe_config->disable_cxsr || >> + pipe_config->fb_changed)) >> + crtc_vblank_mask |= 1 << i; >> + >> if (put_domains) >> modeset_put_power_domains(dev_priv, put_domains); >> - >> - intel_post_plane_update(intel_crtc); >> - >> - if (modeset) >> - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > > Would it be too evil to move the power domain get/put out of the loop? If I > understand correctly, intel_state->modeset already tells us if there is any crtc > that needs a modeset, so we could just protect the calls with that. I'm not sure > what is the impact, but at least we could avoid having the get in > intel_atomic_commit() and the put in intel_post_plane_enable(). Even better would be if POWER_DOMAIN_MODESET was a part of put_domains, and all domains were put after completion. I suggested this during review of Patrik's series, but that seems to have got lost. :( Cheers, Daniel
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 4625f8a9ba12..8e579a8505ac 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) crtc_state->disable_lp_wm = false; crtc_state->disable_cxsr = false; crtc_state->wm_changed = false; + crtc_state->fb_changed = false; return &crtc_state->base; } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 60f17bc5f0ce..299edbf6f99e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4738,9 +4738,6 @@ static void intel_post_plane_update(struct intel_crtc *crtc) struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - if (atomic->wait_vblank) - intel_wait_for_vblank(dev, crtc->pipe); - intel_frontbuffer_flip(dev, atomic->fb_bits); crtc->wm.cxsr_allowed = true; @@ -4754,6 +4751,9 @@ static void intel_post_plane_update(struct intel_crtc *crtc) if (atomic->post_enable_primary) intel_post_enable_primary(&crtc->base); + if (needs_modeset(&pipe_config->base)) + intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); + memset(atomic, 0, sizeof(*atomic)); } @@ -11693,6 +11693,9 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, if (!was_visible && !visible) return 0; + if (fb != old_plane_state->base.fb) + pipe_config->fb_changed = true; + turn_off = was_visible && (!visible || mode_changed); turn_on = visible && (!was_visible || mode_changed); @@ -11708,8 +11711,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, /* must disable cxsr around plane enable/disable */ if (plane->type != DRM_PLANE_TYPE_CURSOR) { - if (is_crtc_enabled) - intel_crtc->atomic.wait_vblank = true; pipe_config->disable_cxsr = true; } } else if ((was_visible || visible) && @@ -11757,14 +11758,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, plane_state->rotation != BIT(DRM_ROTATE_0)) intel_crtc->atomic.disable_fbc = true; - /* - * BDW signals flip done immediately if the plane - * is disabled, even if the plane enable is already - * armed to occur at the next vblank :( - */ - if (turn_on && IS_BROADWELL(dev)) - intel_crtc->atomic.wait_vblank = true; - intel_crtc->atomic.update_fbc |= visible || mode_changed; break; case DRM_PLANE_TYPE_CURSOR: @@ -11779,12 +11772,10 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, if (IS_IVYBRIDGE(dev) && needs_scaling(to_intel_plane_state(plane_state)) && !needs_scaling(old_plane_state)) { - to_intel_crtc_state(crtc_state)->disable_lp_wm = true; - } else if (turn_off && !mode_changed) { - intel_crtc->atomic.wait_vblank = true; + pipe_config->disable_lp_wm = true; + } else if (turn_off && !mode_changed) intel_crtc->atomic.update_sprite_watermarks |= 1 << i; - } break; } @@ -13299,6 +13290,48 @@ static int intel_atomic_prepare_commit(struct drm_device *dev, return ret; } +static void intel_atomic_wait_for_vblanks(struct drm_device *dev, + struct drm_i915_private *dev_priv, + unsigned crtc_mask) +{ + unsigned last_vblank_count[I915_MAX_PIPES]; + enum pipe pipe; + int ret; + + if (!crtc_mask) + return; + + for_each_pipe(dev_priv, pipe) { + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + + if (!((1 << pipe) & crtc_mask)) + continue; + + ret = drm_crtc_vblank_get(crtc); + if (ret != 0) { + crtc_mask &= ~(1 << pipe); + continue; + } + + last_vblank_count[pipe] = drm_crtc_vblank_count(crtc); + } + + for_each_pipe(dev_priv, pipe) { + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe]; + + if (!((1 << pipe) & crtc_mask)) + continue; + + wait_event_timeout(dev->vblank[pipe].queue, + last_vblank_count[pipe] != + drm_crtc_vblank_count(crtc), + msecs_to_jiffies(50)); + + drm_crtc_vblank_put(crtc); + } +} + + /** * intel_atomic_commit - commit validated state object * @dev: DRM device @@ -13325,6 +13358,7 @@ static int intel_atomic_commit(struct drm_device *dev, struct drm_crtc *crtc; int ret = 0, i; bool hw_check = intel_state->modeset; + unsigned crtc_vblank_mask = 0; ret = intel_atomic_prepare_commit(dev, state, async); if (ret) { @@ -13375,8 +13409,9 @@ static int intel_atomic_commit(struct drm_device *dev, for_each_crtc_in_state(state, crtc, crtc_state, i) { struct intel_crtc *intel_crtc = to_intel_crtc(crtc); bool modeset = needs_modeset(crtc->state); - bool update_pipe = !modeset && - to_intel_crtc_state(crtc->state)->update_pipe; + struct intel_crtc_state *pipe_config = + to_intel_crtc_state(crtc->state); + bool update_pipe = !modeset && pipe_config->update_pipe; unsigned long put_domains = 0; if (modeset) @@ -13401,18 +13436,21 @@ static int intel_atomic_commit(struct drm_device *dev, (crtc->state->planes_changed || update_pipe)) drm_atomic_helper_commit_planes_on_crtc(crtc_state); + if (pipe_config->base.active && + (pipe_config->wm_changed || pipe_config->disable_cxsr || + pipe_config->fb_changed)) + crtc_vblank_mask |= 1 << i; + if (put_domains) modeset_put_power_domains(dev_priv, put_domains); - - intel_post_plane_update(intel_crtc); - - if (modeset) - intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); } /* FIXME: add subpixel order */ - drm_atomic_helper_wait_for_vblanks(dev, state); + intel_atomic_wait_for_vblanks(dev, dev_priv, crtc_vblank_mask); + + for_each_crtc_in_state(state, crtc, crtc_state, i) + intel_post_plane_update(to_intel_crtc(crtc)); mutex_lock(&dev->struct_mutex); drm_atomic_helper_cleanup_planes(dev, state); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index db61c37dbf09..66f66740ccaa 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -373,6 +373,7 @@ struct intel_crtc_state { bool update_pipe; /* can a fast modeset be performed? */ bool disable_cxsr; bool wm_changed; /* watermarks are updated */ + bool fb_changed; /* fb on any of the planes is changed */ /* Pipe source size (ie. panel fitter input size) * All planes will be positioned inside this space, @@ -539,7 +540,6 @@ struct intel_crtc_atomic_commit { /* Sleepable operations to perform after commit */ unsigned fb_bits; - bool wait_vblank; bool update_fbc; bool post_enable_primary; unsigned update_sprite_watermarks;
Currently we perform our own wait in post_plane_update, but the atomic core performs another one in wait_for_vblanks. This means that 2 vblanks are done when a fb is changed, which is a bit overkill. Merge them by creating a helper function that takes a crtc mask for the planes to wait on. The broadwell vblank workaround may look gone entirely but this is not the case. pipe_config->wm_changed is set to true when any plane is turned on, which forces a vblank wait. Changes since v1: - Removing the double vblank wait on broadwell moved to its own commit. Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_atomic.c | 1 + drivers/gpu/drm/i915/intel_display.c | 88 ++++++++++++++++++++++++++---------- drivers/gpu/drm/i915/intel_drv.h | 2 +- 3 files changed, 65 insertions(+), 26 deletions(-)