| Message ID | 1434364436-6366-16-git-send-email-maarten.lankhorst@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote: > Now that all planes are added during a modeset we can use the > calculated changes before disabling a plane, and then either commit > or force disable a plane before disabling the crtc. > > The code is shared with atomic_begin/flush, except watermark updating > and vblank evasion are not used. > > This is needed for proper atomic suspend/resume support. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- > drivers/gpu/drm/i915/intel_sprite.c | 4 +- > 2 files changed, 23 insertions(+), 84 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cc4ca4970716..beb69281f45c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) > intel_wait_for_pipe_off(crtc); > } > > -/** > - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe > - * @plane: plane to be enabled > - * @crtc: crtc for the plane > - * > - * Enable @plane on @crtc, making sure that the pipe is running first. > - */ > -static void intel_enable_primary_hw_plane(struct drm_plane *plane, > - struct drm_crtc *crtc) > -{ > - struct drm_device *dev = plane->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - > - /* If the pipe isn't enabled, we can't pump pixels and may hang */ > - assert_pipe_enabled(dev_priv, intel_crtc->pipe); > - to_intel_plane_state(plane->state)->visible = true; > - > - dev_priv->display.update_primary_plane(crtc, plane->fb, > - crtc->x, crtc->y); > -} > - > static bool need_vtd_wa(struct drm_device *dev) > { > #ifdef CONFIG_INTEL_IOMMU > @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) > } > } > > -static void intel_enable_sprite_planes(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - enum pipe pipe = to_intel_crtc(crtc)->pipe; > - struct drm_plane *plane; > - struct intel_plane *intel_plane; > - > - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { > - intel_plane = to_intel_plane(plane); > - if (intel_plane->pipe == pipe) > - intel_plane_restore(&intel_plane->base); > - } > -} > - > void hsw_enable_ips(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) > intel_pre_disable_primary(&crtc->base); > } > > -static void intel_crtc_enable_planes(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int pipe = intel_crtc->pipe; > - > - intel_enable_primary_hw_plane(crtc->primary, crtc); > - intel_enable_sprite_planes(crtc); > - if (to_intel_plane_state(crtc->cursor->state)->visible) > - intel_crtc_update_cursor(crtc, true); > - > - intel_post_enable_primary(crtc); > - > - /* > - * FIXME: Once we grow proper nuclear flip support out of this we need > - * to compute the mask of flip planes precisely. For the time being > - * consider this a flip to a NULL plane. > - */ > - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); > -} > - > static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) > { > struct drm_device *dev = crtc->dev; > @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask > struct drm_plane *p; > int pipe = intel_crtc->pipe; > > - intel_crtc_wait_for_pending_flips(crtc); > - > - intel_pre_disable_primary(crtc); > - > intel_crtc_dpms_overlay_disable(intel_crtc); > > drm_for_each_plane_mask(p, dev, plane_mask) > @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) > if (!intel_crtc->active) > return; > > + if (to_intel_plane_state(crtc->primary->state)->visible) { > + intel_crtc_wait_for_pending_flips(crtc); > + intel_pre_disable_primary(crtc); > + } > + > intel_crtc_disable_planes(crtc, crtc->state->plane_mask); > dev_priv->display.crtc_disable(crtc); > > @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > if (old_plane_state->base.fb && !fb) > intel_crtc->atomic.disabled_planes |= 1 << i; > > - /* don't run rest during modeset yet */ > - if (!intel_crtc->active || mode_changed) > - return 0; > - > was_visible = old_plane_state->visible; > visible = to_intel_plane_state(plane_state)->visible; > > @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) > drm_atomic_helper_swap_state(dev, state); > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > if (!needs_modeset(crtc->state)) > continue; > > any_ms = true; > - if (!crtc_state->active) > - continue; > + intel_pre_plane_update(intel_crtc); > > - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > - dev_priv->display.crtc_disable(crtc); > + if (crtc_state->active) { > + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > + dev_priv->display.crtc_disable(crtc); > + } > } > > /* Only after disabling all output pipelines that will be changed can we > @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > - drm_atomic_helper_commit_planes_on_crtc(crtc_state); > - > - if (!needs_modeset(crtc->state) || !crtc->state->active) > - continue; > - > - update_scanline_offset(to_intel_crtc(crtc)); > + if (needs_modeset(crtc->state) && crtc->state->active) { > + update_scanline_offset(to_intel_crtc(crtc)); > + dev_priv->display.crtc_enable(crtc); > + } > > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > + drm_atomic_helper_commit_planes_on_crtc(crtc_state); > } > > /* FIXME: add subpixel order */ > @@ -13858,7 +13798,7 @@ intel_commit_primary_plane(struct drm_plane *plane, > crtc->x = src->x1 >> 16; > crtc->y = src->y1 >> 16; > > - if (!intel_crtc->active) > + if (!crtc->state->active) > return; > > if (state->visible) > @@ -13885,7 +13825,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > - intel_pre_plane_update(intel_crtc); > + if (!needs_modeset(crtc->state)) > + intel_pre_plane_update(intel_crtc); To make sure I'm understanding...we make this change because __intel_set_mode() has already handled the intel_pre_plane_update() for the modeset case, so we're only worried about legacy setmode's that don't result in a full modeset (so we never called this function), or entry via the atomic ioctl (which doesn't support full modesets yet), right? And presumably once we do allow full modesetting via the atomic ioctl it will also be calling intel_pre_plane_update() before getting to this point? Matt > > if (intel_crtc->atomic.update_wm) > intel_update_watermarks(crtc); > @@ -13893,7 +13834,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > intel_runtime_pm_get(dev_priv); > > /* Perform vblank evasion around commit operation */ > - if (crtc->state->active && !needs_modeset(crtc->state)) > + if (crtc->state->active) > intel_crtc->atomic.evade = > intel_pipe_update_start(intel_crtc, > &intel_crtc->atomic.start_vbl_count); > @@ -14102,7 +14043,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, > intel_crtc->cursor_bo = obj; > > update: > - if (intel_crtc->active) > + if (crtc->state->active) > intel_crtc_update_cursor(crtc, state->visible); > } > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index f7eb3bd0a7c7..168f90f346c2 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -921,16 +921,14 @@ intel_commit_sprite_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > struct drm_crtc *crtc = state->base.crtc; > - struct intel_crtc *intel_crtc; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_framebuffer *fb = state->base.fb; > > crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > > plane->fb = fb; > > - if (!intel_crtc->active) > + if (!crtc->state->active) > return; > > if (state->visible) { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote: > Now that all planes are added during a modeset we can use the > calculated changes before disabling a plane, and then either commit > or force disable a plane before disabling the crtc. > > The code is shared with atomic_begin/flush, except watermark updating > and vblank evasion are not used. > > This is needed for proper atomic suspend/resume support. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- > drivers/gpu/drm/i915/intel_sprite.c | 4 +- > 2 files changed, 23 insertions(+), 84 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index cc4ca4970716..beb69281f45c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) > intel_wait_for_pipe_off(crtc); > } > > -/** > - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe > - * @plane: plane to be enabled > - * @crtc: crtc for the plane > - * > - * Enable @plane on @crtc, making sure that the pipe is running first. > - */ > -static void intel_enable_primary_hw_plane(struct drm_plane *plane, > - struct drm_crtc *crtc) > -{ > - struct drm_device *dev = plane->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - > - /* If the pipe isn't enabled, we can't pump pixels and may hang */ > - assert_pipe_enabled(dev_priv, intel_crtc->pipe); > - to_intel_plane_state(plane->state)->visible = true; > - > - dev_priv->display.update_primary_plane(crtc, plane->fb, > - crtc->x, crtc->y); > -} > - > static bool need_vtd_wa(struct drm_device *dev) > { > #ifdef CONFIG_INTEL_IOMMU > @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) > } > } > > -static void intel_enable_sprite_planes(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - enum pipe pipe = to_intel_crtc(crtc)->pipe; > - struct drm_plane *plane; > - struct intel_plane *intel_plane; > - > - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { > - intel_plane = to_intel_plane(plane); > - if (intel_plane->pipe == pipe) > - intel_plane_restore(&intel_plane->base); > - } > -} > - > void hsw_enable_ips(struct intel_crtc *crtc) > { > struct drm_device *dev = crtc->base.dev; > @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) > intel_pre_disable_primary(&crtc->base); > } > > -static void intel_crtc_enable_planes(struct drm_crtc *crtc) > -{ > - struct drm_device *dev = crtc->dev; > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - int pipe = intel_crtc->pipe; > - > - intel_enable_primary_hw_plane(crtc->primary, crtc); > - intel_enable_sprite_planes(crtc); > - if (to_intel_plane_state(crtc->cursor->state)->visible) > - intel_crtc_update_cursor(crtc, true); > - > - intel_post_enable_primary(crtc); > - > - /* > - * FIXME: Once we grow proper nuclear flip support out of this we need > - * to compute the mask of flip planes precisely. For the time being > - * consider this a flip to a NULL plane. > - */ > - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); > -} > - > static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) > { > struct drm_device *dev = crtc->dev; > @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask > struct drm_plane *p; > int pipe = intel_crtc->pipe; > > - intel_crtc_wait_for_pending_flips(crtc); > - > - intel_pre_disable_primary(crtc); > - > intel_crtc_dpms_overlay_disable(intel_crtc); > > drm_for_each_plane_mask(p, dev, plane_mask) > @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) > if (!intel_crtc->active) > return; > > + if (to_intel_plane_state(crtc->primary->state)->visible) { > + intel_crtc_wait_for_pending_flips(crtc); > + intel_pre_disable_primary(crtc); > + } > + > intel_crtc_disable_planes(crtc, crtc->state->plane_mask); > dev_priv->display.crtc_disable(crtc); > > @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > if (old_plane_state->base.fb && !fb) > intel_crtc->atomic.disabled_planes |= 1 << i; > > - /* don't run rest during modeset yet */ > - if (!intel_crtc->active || mode_changed) > - return 0; > - > was_visible = old_plane_state->visible; > visible = to_intel_plane_state(plane_state)->visible; > > @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) > drm_atomic_helper_swap_state(dev, state); > > for_each_crtc_in_state(state, crtc, crtc_state, i) { > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > if (!needs_modeset(crtc->state)) > continue; > > any_ms = true; > - if (!crtc_state->active) > - continue; > + intel_pre_plane_update(intel_crtc); > > - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > - dev_priv->display.crtc_disable(crtc); > + if (crtc_state->active) { > + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > + dev_priv->display.crtc_disable(crtc); > + } > } > > /* Only after disabling all output pipelines that will be changed can we > @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) > > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > - drm_atomic_helper_commit_planes_on_crtc(crtc_state); > - > - if (!needs_modeset(crtc->state) || !crtc->state->active) > - continue; > - > - update_scanline_offset(to_intel_crtc(crtc)); > + if (needs_modeset(crtc->state) && crtc->state->active) { > + update_scanline_offset(to_intel_crtc(crtc)); > + dev_priv->display.crtc_enable(crtc); > + } > > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > + drm_atomic_helper_commit_planes_on_crtc(crtc_state); Why are we still commiting planes on a disabled pipes? The way I envisioned the modeset path looking is something like this: 1. atomically disable all planes for pipes getting disabled 2. disable all pipes that need disabling 3. enable all pipes that need enabling 4. atomically update all planes that require it on now active pipes > } > > /* FIXME: add subpixel order */ > @@ -13858,7 +13798,7 @@ intel_commit_primary_plane(struct drm_plane *plane, > crtc->x = src->x1 >> 16; > crtc->y = src->y1 >> 16; > > - if (!intel_crtc->active) > + if (!crtc->state->active) > return; > > if (state->visible) > @@ -13885,7 +13825,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > struct drm_i915_private *dev_priv = dev->dev_private; > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > - intel_pre_plane_update(intel_crtc); > + if (!needs_modeset(crtc->state)) > + intel_pre_plane_update(intel_crtc); > > if (intel_crtc->atomic.update_wm) > intel_update_watermarks(crtc); > @@ -13893,7 +13834,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, > intel_runtime_pm_get(dev_priv); > > /* Perform vblank evasion around commit operation */ > - if (crtc->state->active && !needs_modeset(crtc->state)) > + if (crtc->state->active) > intel_crtc->atomic.evade = > intel_pipe_update_start(intel_crtc, > &intel_crtc->atomic.start_vbl_count); > @@ -14102,7 +14043,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, > intel_crtc->cursor_bo = obj; > > update: > - if (intel_crtc->active) > + if (crtc->state->active) > intel_crtc_update_cursor(crtc, state->visible); > } > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index f7eb3bd0a7c7..168f90f346c2 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -921,16 +921,14 @@ intel_commit_sprite_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > struct drm_crtc *crtc = state->base.crtc; > - struct intel_crtc *intel_crtc; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct drm_framebuffer *fb = state->base.fb; > > crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > > plane->fb = fb; > > - if (!intel_crtc->active) > + if (!crtc->state->active) > return; > > if (state->visible) { > -- > 2.1.0 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Op 18-06-15 om 16:21 schreef Matt Roper: > On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote: >> Now that all planes are added during a modeset we can use the >> calculated changes before disabling a plane, and then either commit >> or force disable a plane before disabling the crtc. >> >> The code is shared with atomic_begin/flush, except watermark updating >> and vblank evasion are not used. >> >> This is needed for proper atomic suspend/resume support. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- >> drivers/gpu/drm/i915/intel_sprite.c | 4 +- >> 2 files changed, 23 insertions(+), 84 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index cc4ca4970716..beb69281f45c 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) >> intel_wait_for_pipe_off(crtc); >> } >> >> -/** >> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe >> - * @plane: plane to be enabled >> - * @crtc: crtc for the plane >> - * >> - * Enable @plane on @crtc, making sure that the pipe is running first. >> - */ >> -static void intel_enable_primary_hw_plane(struct drm_plane *plane, >> - struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = plane->dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - >> - /* If the pipe isn't enabled, we can't pump pixels and may hang */ >> - assert_pipe_enabled(dev_priv, intel_crtc->pipe); >> - to_intel_plane_state(plane->state)->visible = true; >> - >> - dev_priv->display.update_primary_plane(crtc, plane->fb, >> - crtc->x, crtc->y); >> -} >> - >> static bool need_vtd_wa(struct drm_device *dev) >> { >> #ifdef CONFIG_INTEL_IOMMU >> @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) >> } >> } >> >> -static void intel_enable_sprite_planes(struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = crtc->dev; >> - enum pipe pipe = to_intel_crtc(crtc)->pipe; >> - struct drm_plane *plane; >> - struct intel_plane *intel_plane; >> - >> - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { >> - intel_plane = to_intel_plane(plane); >> - if (intel_plane->pipe == pipe) >> - intel_plane_restore(&intel_plane->base); >> - } >> -} >> - >> void hsw_enable_ips(struct intel_crtc *crtc) >> { >> struct drm_device *dev = crtc->base.dev; >> @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) >> intel_pre_disable_primary(&crtc->base); >> } >> >> -static void intel_crtc_enable_planes(struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = crtc->dev; >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - int pipe = intel_crtc->pipe; >> - >> - intel_enable_primary_hw_plane(crtc->primary, crtc); >> - intel_enable_sprite_planes(crtc); >> - if (to_intel_plane_state(crtc->cursor->state)->visible) >> - intel_crtc_update_cursor(crtc, true); >> - >> - intel_post_enable_primary(crtc); >> - >> - /* >> - * FIXME: Once we grow proper nuclear flip support out of this we need >> - * to compute the mask of flip planes precisely. For the time being >> - * consider this a flip to a NULL plane. >> - */ >> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); >> -} >> - >> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) >> { >> struct drm_device *dev = crtc->dev; >> @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask >> struct drm_plane *p; >> int pipe = intel_crtc->pipe; >> >> - intel_crtc_wait_for_pending_flips(crtc); >> - >> - intel_pre_disable_primary(crtc); >> - >> intel_crtc_dpms_overlay_disable(intel_crtc); >> >> drm_for_each_plane_mask(p, dev, plane_mask) >> @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) >> if (!intel_crtc->active) >> return; >> >> + if (to_intel_plane_state(crtc->primary->state)->visible) { >> + intel_crtc_wait_for_pending_flips(crtc); >> + intel_pre_disable_primary(crtc); >> + } >> + >> intel_crtc_disable_planes(crtc, crtc->state->plane_mask); >> dev_priv->display.crtc_disable(crtc); >> >> @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, >> if (old_plane_state->base.fb && !fb) >> intel_crtc->atomic.disabled_planes |= 1 << i; >> >> - /* don't run rest during modeset yet */ >> - if (!intel_crtc->active || mode_changed) >> - return 0; >> - >> was_visible = old_plane_state->visible; >> visible = to_intel_plane_state(plane_state)->visible; >> >> @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) >> drm_atomic_helper_swap_state(dev, state); >> >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + >> if (!needs_modeset(crtc->state)) >> continue; >> >> any_ms = true; >> - if (!crtc_state->active) >> - continue; >> + intel_pre_plane_update(intel_crtc); >> >> - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); >> - dev_priv->display.crtc_disable(crtc); >> + if (crtc_state->active) { >> + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); >> + dev_priv->display.crtc_disable(crtc); >> + } >> } >> >> /* Only after disabling all output pipelines that will be changed can we >> @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) >> >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> - drm_atomic_helper_commit_planes_on_crtc(crtc_state); >> - >> - if (!needs_modeset(crtc->state) || !crtc->state->active) >> - continue; >> - >> - update_scanline_offset(to_intel_crtc(crtc)); >> + if (needs_modeset(crtc->state) && crtc->state->active) { >> + update_scanline_offset(to_intel_crtc(crtc)); >> + dev_priv->display.crtc_enable(crtc); >> + } >> >> - dev_priv->display.crtc_enable(crtc); >> - intel_crtc_enable_planes(crtc); >> + drm_atomic_helper_commit_planes_on_crtc(crtc_state); >> } >> >> /* FIXME: add subpixel order */ >> @@ -13858,7 +13798,7 @@ intel_commit_primary_plane(struct drm_plane *plane, >> crtc->x = src->x1 >> 16; >> crtc->y = src->y1 >> 16; >> >> - if (!intel_crtc->active) >> + if (!crtc->state->active) >> return; >> >> if (state->visible) >> @@ -13885,7 +13825,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, >> struct drm_i915_private *dev_priv = dev->dev_private; >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> >> - intel_pre_plane_update(intel_crtc); >> + if (!needs_modeset(crtc->state)) >> + intel_pre_plane_update(intel_crtc); > To make sure I'm understanding...we make this change because > __intel_set_mode() has already handled the intel_pre_plane_update() for > the modeset case, so we're only worried about legacy setmode's that > don't result in a full modeset (so we never called this function), or > entry via the atomic ioctl (which doesn't support full modesets yet), > right? And presumably once we do allow full modesetting via the atomic > ioctl it will also be calling intel_pre_plane_update() before getting to > this point? > Correct, pre_plane_update does all the scafffolding for disabling planes. ~Maarten
Op 18-06-15 om 17:28 schreef Ville Syrjälä: > On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote: >> Now that all planes are added during a modeset we can use the >> calculated changes before disabling a plane, and then either commit >> or force disable a plane before disabling the crtc. >> >> The code is shared with atomic_begin/flush, except watermark updating >> and vblank evasion are not used. >> >> This is needed for proper atomic suspend/resume support. >> >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- >> drivers/gpu/drm/i915/intel_sprite.c | 4 +- >> 2 files changed, 23 insertions(+), 84 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index cc4ca4970716..beb69281f45c 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) >> intel_wait_for_pipe_off(crtc); >> } >> >> -/** >> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe >> - * @plane: plane to be enabled >> - * @crtc: crtc for the plane >> - * >> - * Enable @plane on @crtc, making sure that the pipe is running first. >> - */ >> -static void intel_enable_primary_hw_plane(struct drm_plane *plane, >> - struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = plane->dev; >> - struct drm_i915_private *dev_priv = dev->dev_private; >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - >> - /* If the pipe isn't enabled, we can't pump pixels and may hang */ >> - assert_pipe_enabled(dev_priv, intel_crtc->pipe); >> - to_intel_plane_state(plane->state)->visible = true; >> - >> - dev_priv->display.update_primary_plane(crtc, plane->fb, >> - crtc->x, crtc->y); >> -} >> - >> static bool need_vtd_wa(struct drm_device *dev) >> { >> #ifdef CONFIG_INTEL_IOMMU >> @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) >> } >> } >> >> -static void intel_enable_sprite_planes(struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = crtc->dev; >> - enum pipe pipe = to_intel_crtc(crtc)->pipe; >> - struct drm_plane *plane; >> - struct intel_plane *intel_plane; >> - >> - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { >> - intel_plane = to_intel_plane(plane); >> - if (intel_plane->pipe == pipe) >> - intel_plane_restore(&intel_plane->base); >> - } >> -} >> - >> void hsw_enable_ips(struct intel_crtc *crtc) >> { >> struct drm_device *dev = crtc->base.dev; >> @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) >> intel_pre_disable_primary(&crtc->base); >> } >> >> -static void intel_crtc_enable_planes(struct drm_crtc *crtc) >> -{ >> - struct drm_device *dev = crtc->dev; >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> - int pipe = intel_crtc->pipe; >> - >> - intel_enable_primary_hw_plane(crtc->primary, crtc); >> - intel_enable_sprite_planes(crtc); >> - if (to_intel_plane_state(crtc->cursor->state)->visible) >> - intel_crtc_update_cursor(crtc, true); >> - >> - intel_post_enable_primary(crtc); >> - >> - /* >> - * FIXME: Once we grow proper nuclear flip support out of this we need >> - * to compute the mask of flip planes precisely. For the time being >> - * consider this a flip to a NULL plane. >> - */ >> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); >> -} >> - >> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) >> { >> struct drm_device *dev = crtc->dev; >> @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask >> struct drm_plane *p; >> int pipe = intel_crtc->pipe; >> >> - intel_crtc_wait_for_pending_flips(crtc); >> - >> - intel_pre_disable_primary(crtc); >> - >> intel_crtc_dpms_overlay_disable(intel_crtc); >> >> drm_for_each_plane_mask(p, dev, plane_mask) >> @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) >> if (!intel_crtc->active) >> return; >> >> + if (to_intel_plane_state(crtc->primary->state)->visible) { >> + intel_crtc_wait_for_pending_flips(crtc); >> + intel_pre_disable_primary(crtc); >> + } >> + >> intel_crtc_disable_planes(crtc, crtc->state->plane_mask); >> dev_priv->display.crtc_disable(crtc); >> >> @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, >> if (old_plane_state->base.fb && !fb) >> intel_crtc->atomic.disabled_planes |= 1 << i; >> >> - /* don't run rest during modeset yet */ >> - if (!intel_crtc->active || mode_changed) >> - return 0; >> - >> was_visible = old_plane_state->visible; >> visible = to_intel_plane_state(plane_state)->visible; >> >> @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) >> drm_atomic_helper_swap_state(dev, state); >> >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + >> if (!needs_modeset(crtc->state)) >> continue; >> >> any_ms = true; >> - if (!crtc_state->active) >> - continue; >> + intel_pre_plane_update(intel_crtc); >> >> - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); >> - dev_priv->display.crtc_disable(crtc); >> + if (crtc_state->active) { >> + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); >> + dev_priv->display.crtc_disable(crtc); >> + } >> } >> >> /* Only after disabling all output pipelines that will be changed can we >> @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) >> >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >> for_each_crtc_in_state(state, crtc, crtc_state, i) { >> - drm_atomic_helper_commit_planes_on_crtc(crtc_state); >> - >> - if (!needs_modeset(crtc->state) || !crtc->state->active) >> - continue; >> - >> - update_scanline_offset(to_intel_crtc(crtc)); >> + if (needs_modeset(crtc->state) && crtc->state->active) { >> + update_scanline_offset(to_intel_crtc(crtc)); >> + dev_priv->display.crtc_enable(crtc); >> + } >> >> - dev_priv->display.crtc_enable(crtc); >> - intel_crtc_enable_planes(crtc); >> + drm_atomic_helper_commit_planes_on_crtc(crtc_state); > Why are we still commiting planes on a disabled pipes? > > The way I envisioned the modeset path looking is something like this: > > 1. atomically disable all planes for pipes getting disabled > 2. disable all pipes that need disabling > 3. enable all pipes that need enabling > 4. atomically update all planes that require it on now active pipes > > I looked for other ways to do this, but they were ugly. Only updating sw state in commit_planes for disabled crtc's made things a lot cleaner without worrying about special cases. plane_disable doesn't update sw state, so it performs 1. The commit function is still called because update plane->fb needs to be updated, even for disabled crtc's. Conditionally calling plane_commit caused more issues than it solved. ~Maarten
On Fri, Jun 19, 2015 at 06:01:07AM +0200, Maarten Lankhorst wrote: > Op 18-06-15 om 17:28 schreef Ville Syrjälä: > > On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote: > >> Now that all planes are added during a modeset we can use the > >> calculated changes before disabling a plane, and then either commit > >> or force disable a plane before disabling the crtc. > >> > >> The code is shared with atomic_begin/flush, except watermark updating > >> and vblank evasion are not used. > >> > >> This is needed for proper atomic suspend/resume support. > >> > >> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 > >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- > >> drivers/gpu/drm/i915/intel_sprite.c | 4 +- > >> 2 files changed, 23 insertions(+), 84 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index cc4ca4970716..beb69281f45c 100644 > >> --- a/drivers/gpu/drm/i915/intel_display.c > >> +++ b/drivers/gpu/drm/i915/intel_display.c > >> @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) > >> intel_wait_for_pipe_off(crtc); > >> } > >> > >> -/** > >> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe > >> - * @plane: plane to be enabled > >> - * @crtc: crtc for the plane > >> - * > >> - * Enable @plane on @crtc, making sure that the pipe is running first. > >> - */ > >> -static void intel_enable_primary_hw_plane(struct drm_plane *plane, > >> - struct drm_crtc *crtc) > >> -{ > >> - struct drm_device *dev = plane->dev; > >> - struct drm_i915_private *dev_priv = dev->dev_private; > >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> - > >> - /* If the pipe isn't enabled, we can't pump pixels and may hang */ > >> - assert_pipe_enabled(dev_priv, intel_crtc->pipe); > >> - to_intel_plane_state(plane->state)->visible = true; > >> - > >> - dev_priv->display.update_primary_plane(crtc, plane->fb, > >> - crtc->x, crtc->y); > >> -} > >> - > >> static bool need_vtd_wa(struct drm_device *dev) > >> { > >> #ifdef CONFIG_INTEL_IOMMU > >> @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) > >> } > >> } > >> > >> -static void intel_enable_sprite_planes(struct drm_crtc *crtc) > >> -{ > >> - struct drm_device *dev = crtc->dev; > >> - enum pipe pipe = to_intel_crtc(crtc)->pipe; > >> - struct drm_plane *plane; > >> - struct intel_plane *intel_plane; > >> - > >> - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { > >> - intel_plane = to_intel_plane(plane); > >> - if (intel_plane->pipe == pipe) > >> - intel_plane_restore(&intel_plane->base); > >> - } > >> -} > >> - > >> void hsw_enable_ips(struct intel_crtc *crtc) > >> { > >> struct drm_device *dev = crtc->base.dev; > >> @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) > >> intel_pre_disable_primary(&crtc->base); > >> } > >> > >> -static void intel_crtc_enable_planes(struct drm_crtc *crtc) > >> -{ > >> - struct drm_device *dev = crtc->dev; > >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> - int pipe = intel_crtc->pipe; > >> - > >> - intel_enable_primary_hw_plane(crtc->primary, crtc); > >> - intel_enable_sprite_planes(crtc); > >> - if (to_intel_plane_state(crtc->cursor->state)->visible) > >> - intel_crtc_update_cursor(crtc, true); > >> - > >> - intel_post_enable_primary(crtc); > >> - > >> - /* > >> - * FIXME: Once we grow proper nuclear flip support out of this we need > >> - * to compute the mask of flip planes precisely. For the time being > >> - * consider this a flip to a NULL plane. > >> - */ > >> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); > >> -} > >> - > >> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) > >> { > >> struct drm_device *dev = crtc->dev; > >> @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask > >> struct drm_plane *p; > >> int pipe = intel_crtc->pipe; > >> > >> - intel_crtc_wait_for_pending_flips(crtc); > >> - > >> - intel_pre_disable_primary(crtc); > >> - > >> intel_crtc_dpms_overlay_disable(intel_crtc); > >> > >> drm_for_each_plane_mask(p, dev, plane_mask) > >> @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) > >> if (!intel_crtc->active) > >> return; > >> > >> + if (to_intel_plane_state(crtc->primary->state)->visible) { > >> + intel_crtc_wait_for_pending_flips(crtc); > >> + intel_pre_disable_primary(crtc); > >> + } > >> + > >> intel_crtc_disable_planes(crtc, crtc->state->plane_mask); > >> dev_priv->display.crtc_disable(crtc); > >> > >> @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, > >> if (old_plane_state->base.fb && !fb) > >> intel_crtc->atomic.disabled_planes |= 1 << i; > >> > >> - /* don't run rest during modeset yet */ > >> - if (!intel_crtc->active || mode_changed) > >> - return 0; > >> - > >> was_visible = old_plane_state->visible; > >> visible = to_intel_plane_state(plane_state)->visible; > >> > >> @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) > >> drm_atomic_helper_swap_state(dev, state); > >> > >> for_each_crtc_in_state(state, crtc, crtc_state, i) { > >> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >> + > >> if (!needs_modeset(crtc->state)) > >> continue; > >> > >> any_ms = true; > >> - if (!crtc_state->active) > >> - continue; > >> + intel_pre_plane_update(intel_crtc); > >> > >> - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > >> - dev_priv->display.crtc_disable(crtc); > >> + if (crtc_state->active) { > >> + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); > >> + dev_priv->display.crtc_disable(crtc); > >> + } > >> } > >> > >> /* Only after disabling all output pipelines that will be changed can we > >> @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) > >> > >> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > >> for_each_crtc_in_state(state, crtc, crtc_state, i) { > >> - drm_atomic_helper_commit_planes_on_crtc(crtc_state); > >> - > >> - if (!needs_modeset(crtc->state) || !crtc->state->active) > >> - continue; > >> - > >> - update_scanline_offset(to_intel_crtc(crtc)); > >> + if (needs_modeset(crtc->state) && crtc->state->active) { > >> + update_scanline_offset(to_intel_crtc(crtc)); > >> + dev_priv->display.crtc_enable(crtc); > >> + } > >> > >> - dev_priv->display.crtc_enable(crtc); > >> - intel_crtc_enable_planes(crtc); > >> + drm_atomic_helper_commit_planes_on_crtc(crtc_state); > > Why are we still commiting planes on a disabled pipes? > > > > The way I envisioned the modeset path looking is something like this: > > > > 1. atomically disable all planes for pipes getting disabled > > 2. disable all pipes that need disabling > > 3. enable all pipes that need enabling > > 4. atomically update all planes that require it on now active pipes > > > > > I looked for other ways to do this, but they were ugly. Only updating sw state in commit_planes for disabled crtc's made things a lot cleaner without worrying about special cases. The commit hooks shouldn't update any sw state. As for special cases, the current code has one special case after another, and it's giving me a headache every time I look at it. > > plane_disable doesn't update sw state, so it performs 1. The commit function is still called because update plane->fb > needs to be updated, even for disabled crtc's. Conditionally calling plane_commit caused more issues than it solved. plane->fb should be clearly moved out of the commit hooks to where the rest of the state gets swapped (assuming we can't just get rid of it).
Hey, Op 22-06-15 om 11:49 schreef Ville Syrjälä: > On Fri, Jun 19, 2015 at 06:01:07AM +0200, Maarten Lankhorst wrote: >> Op 18-06-15 om 17:28 schreef Ville Syrjälä: >>> On Mon, Jun 15, 2015 at 12:33:52PM +0200, Maarten Lankhorst wrote: >>>> Now that all planes are added during a modeset we can use the >>>> calculated changes before disabling a plane, and then either commit >>>> or force disable a plane before disabling the crtc. >>>> >>>> The code is shared with atomic_begin/flush, except watermark updating >>>> and vblank evasion are not used. >>>> >>>> This is needed for proper atomic suspend/resume support. >>>> >>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- >>>> drivers/gpu/drm/i915/intel_sprite.c | 4 +- >>>> 2 files changed, 23 insertions(+), 84 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index cc4ca4970716..beb69281f45c 100644 >>>> --- a/drivers/gpu/drm/i915/intel_display.c >>>> +++ b/drivers/gpu/drm/i915/intel_display.c >>>> @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) >>>> intel_wait_for_pipe_off(crtc); >>>> } >>>> >>>> -/** >>>> - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe >>>> - * @plane: plane to be enabled >>>> - * @crtc: crtc for the plane >>>> - * >>>> - * Enable @plane on @crtc, making sure that the pipe is running first. >>>> - */ >>>> -static void intel_enable_primary_hw_plane(struct drm_plane *plane, >>>> - struct drm_crtc *crtc) >>>> -{ >>>> - struct drm_device *dev = plane->dev; >>>> - struct drm_i915_private *dev_priv = dev->dev_private; >>>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> - >>>> - /* If the pipe isn't enabled, we can't pump pixels and may hang */ >>>> - assert_pipe_enabled(dev_priv, intel_crtc->pipe); >>>> - to_intel_plane_state(plane->state)->visible = true; >>>> - >>>> - dev_priv->display.update_primary_plane(crtc, plane->fb, >>>> - crtc->x, crtc->y); >>>> -} >>>> - >>>> static bool need_vtd_wa(struct drm_device *dev) >>>> { >>>> #ifdef CONFIG_INTEL_IOMMU >>>> @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) >>>> } >>>> } >>>> >>>> -static void intel_enable_sprite_planes(struct drm_crtc *crtc) >>>> -{ >>>> - struct drm_device *dev = crtc->dev; >>>> - enum pipe pipe = to_intel_crtc(crtc)->pipe; >>>> - struct drm_plane *plane; >>>> - struct intel_plane *intel_plane; >>>> - >>>> - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { >>>> - intel_plane = to_intel_plane(plane); >>>> - if (intel_plane->pipe == pipe) >>>> - intel_plane_restore(&intel_plane->base); >>>> - } >>>> -} >>>> - >>>> void hsw_enable_ips(struct intel_crtc *crtc) >>>> { >>>> struct drm_device *dev = crtc->base.dev; >>>> @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) >>>> intel_pre_disable_primary(&crtc->base); >>>> } >>>> >>>> -static void intel_crtc_enable_planes(struct drm_crtc *crtc) >>>> -{ >>>> - struct drm_device *dev = crtc->dev; >>>> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> - int pipe = intel_crtc->pipe; >>>> - >>>> - intel_enable_primary_hw_plane(crtc->primary, crtc); >>>> - intel_enable_sprite_planes(crtc); >>>> - if (to_intel_plane_state(crtc->cursor->state)->visible) >>>> - intel_crtc_update_cursor(crtc, true); >>>> - >>>> - intel_post_enable_primary(crtc); >>>> - >>>> - /* >>>> - * FIXME: Once we grow proper nuclear flip support out of this we need >>>> - * to compute the mask of flip planes precisely. For the time being >>>> - * consider this a flip to a NULL plane. >>>> - */ >>>> - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); >>>> -} >>>> - >>>> static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) >>>> { >>>> struct drm_device *dev = crtc->dev; >>>> @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask >>>> struct drm_plane *p; >>>> int pipe = intel_crtc->pipe; >>>> >>>> - intel_crtc_wait_for_pending_flips(crtc); >>>> - >>>> - intel_pre_disable_primary(crtc); >>>> - >>>> intel_crtc_dpms_overlay_disable(intel_crtc); >>>> >>>> drm_for_each_plane_mask(p, dev, plane_mask) >>>> @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) >>>> if (!intel_crtc->active) >>>> return; >>>> >>>> + if (to_intel_plane_state(crtc->primary->state)->visible) { >>>> + intel_crtc_wait_for_pending_flips(crtc); >>>> + intel_pre_disable_primary(crtc); >>>> + } >>>> + >>>> intel_crtc_disable_planes(crtc, crtc->state->plane_mask); >>>> dev_priv->display.crtc_disable(crtc); >>>> >>>> @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, >>>> if (old_plane_state->base.fb && !fb) >>>> intel_crtc->atomic.disabled_planes |= 1 << i; >>>> >>>> - /* don't run rest during modeset yet */ >>>> - if (!intel_crtc->active || mode_changed) >>>> - return 0; >>>> - >>>> was_visible = old_plane_state->visible; >>>> visible = to_intel_plane_state(plane_state)->visible; >>>> >>>> @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) >>>> drm_atomic_helper_swap_state(dev, state); >>>> >>>> for_each_crtc_in_state(state, crtc, crtc_state, i) { >>>> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >>>> + >>>> if (!needs_modeset(crtc->state)) >>>> continue; >>>> >>>> any_ms = true; >>>> - if (!crtc_state->active) >>>> - continue; >>>> + intel_pre_plane_update(intel_crtc); >>>> >>>> - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); >>>> - dev_priv->display.crtc_disable(crtc); >>>> + if (crtc_state->active) { >>>> + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); >>>> + dev_priv->display.crtc_disable(crtc); >>>> + } >>>> } >>>> >>>> /* Only after disabling all output pipelines that will be changed can we >>>> @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) >>>> >>>> /* Now enable the clocks, plane, pipe, and connectors that we set up. */ >>>> for_each_crtc_in_state(state, crtc, crtc_state, i) { >>>> - drm_atomic_helper_commit_planes_on_crtc(crtc_state); >>>> - >>>> - if (!needs_modeset(crtc->state) || !crtc->state->active) >>>> - continue; >>>> - >>>> - update_scanline_offset(to_intel_crtc(crtc)); >>>> + if (needs_modeset(crtc->state) && crtc->state->active) { >>>> + update_scanline_offset(to_intel_crtc(crtc)); >>>> + dev_priv->display.crtc_enable(crtc); >>>> + } >>>> >>>> - dev_priv->display.crtc_enable(crtc); >>>> - intel_crtc_enable_planes(crtc); >>>> + drm_atomic_helper_commit_planes_on_crtc(crtc_state); >>> Why are we still commiting planes on a disabled pipes? >>> >>> The way I envisioned the modeset path looking is something like this: >>> >>> 1. atomically disable all planes for pipes getting disabled >>> 2. disable all pipes that need disabling >>> 3. enable all pipes that need enabling >>> 4. atomically update all planes that require it on now active pipes >>> >>> >> I looked for other ways to do this, but they were ugly. Only updating sw state in commit_planes for disabled crtc's made things a lot cleaner without worrying about special cases. > The commit hooks shouldn't update any sw state. > > As for special cases, the current code has one special case after another, > and it's giving me a headache every time I look at it. Yes, but this will simplify with a unified atomic commit path. >> plane_disable doesn't update sw state, so it performs 1. The commit function is still called because update plane->fb >> needs to be updated, even for disabled crtc's. Conditionally calling plane_commit caused more issues than it solved. > plane->fb should be clearly moved out of the commit hooks to where the > rest of the state gets swapped (assuming we can't just get rid of it I completely agree, but this becomes a lot easier when all transitional helpers are gone. ~Maarten
On Mon, Jun 22, 2015 at 12:08:36PM +0200, Maarten Lankhorst wrote: > >>> Why are we still commiting planes on a disabled pipes? > >>> > >>> The way I envisioned the modeset path looking is something like this: > >>> > >>> 1. atomically disable all planes for pipes getting disabled > >>> 2. disable all pipes that need disabling > >>> 3. enable all pipes that need enabling > >>> 4. atomically update all planes that require it on now active pipes > >>> > >>> > >> I looked for other ways to do this, but they were ugly. Only updating sw state in commit_planes for disabled crtc's made things a lot cleaner without worrying about special cases. > > The commit hooks shouldn't update any sw state. I looked at that a bit and the plane disabling (i.e. step 2 above) doesn't touch state objects. plane->fb is still a bit a sorry state since apparently page_flip won't update this properly. There's even a TODO about this in drm_atomic_helper_page_flip, I guess I should finally address this one asap. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index cc4ca4970716..beb69281f45c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2217,28 +2217,6 @@ static void intel_disable_pipe(struct intel_crtc *crtc) intel_wait_for_pipe_off(crtc); } -/** - * intel_enable_primary_hw_plane - enable the primary plane on a given pipe - * @plane: plane to be enabled - * @crtc: crtc for the plane - * - * Enable @plane on @crtc, making sure that the pipe is running first. - */ -static void intel_enable_primary_hw_plane(struct drm_plane *plane, - struct drm_crtc *crtc) -{ - struct drm_device *dev = plane->dev; - struct drm_i915_private *dev_priv = dev->dev_private; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - - /* If the pipe isn't enabled, we can't pump pixels and may hang */ - assert_pipe_enabled(dev_priv, intel_crtc->pipe); - to_intel_plane_state(plane->state)->visible = true; - - dev_priv->display.update_primary_plane(crtc, plane->fb, - crtc->x, crtc->y); -} - static bool need_vtd_wa(struct drm_device *dev) { #ifdef CONFIG_INTEL_IOMMU @@ -4508,20 +4486,6 @@ static void ironlake_pfit_enable(struct intel_crtc *crtc) } } -static void intel_enable_sprite_planes(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - enum pipe pipe = to_intel_crtc(crtc)->pipe; - struct drm_plane *plane; - struct intel_plane *intel_plane; - - drm_for_each_legacy_plane(plane, &dev->mode_config.plane_list) { - intel_plane = to_intel_plane(plane); - if (intel_plane->pipe == pipe) - intel_plane_restore(&intel_plane->base); - } -} - void hsw_enable_ips(struct intel_crtc *crtc) { struct drm_device *dev = crtc->base.dev; @@ -4817,27 +4781,6 @@ static void intel_pre_plane_update(struct intel_crtc *crtc) intel_pre_disable_primary(&crtc->base); } -static void intel_crtc_enable_planes(struct drm_crtc *crtc) -{ - struct drm_device *dev = crtc->dev; - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int pipe = intel_crtc->pipe; - - intel_enable_primary_hw_plane(crtc->primary, crtc); - intel_enable_sprite_planes(crtc); - if (to_intel_plane_state(crtc->cursor->state)->visible) - intel_crtc_update_cursor(crtc, true); - - intel_post_enable_primary(crtc); - - /* - * FIXME: Once we grow proper nuclear flip support out of this we need - * to compute the mask of flip planes precisely. For the time being - * consider this a flip to a NULL plane. - */ - intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe)); -} - static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask) { struct drm_device *dev = crtc->dev; @@ -4845,10 +4788,6 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask struct drm_plane *p; int pipe = intel_crtc->pipe; - intel_crtc_wait_for_pending_flips(crtc); - - intel_pre_disable_primary(crtc); - intel_crtc_dpms_overlay_disable(intel_crtc); drm_for_each_plane_mask(p, dev, plane_mask) @@ -6270,6 +6209,11 @@ static void intel_crtc_disable_noatomic(struct drm_crtc *crtc) if (!intel_crtc->active) return; + if (to_intel_plane_state(crtc->primary->state)->visible) { + intel_crtc_wait_for_pending_flips(crtc); + intel_pre_disable_primary(crtc); + } + intel_crtc_disable_planes(crtc, crtc->state->plane_mask); dev_priv->display.crtc_disable(crtc); @@ -11783,10 +11727,6 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state, if (old_plane_state->base.fb && !fb) intel_crtc->atomic.disabled_planes |= 1 << i; - /* don't run rest during modeset yet */ - if (!intel_crtc->active || mode_changed) - return 0; - was_visible = old_plane_state->visible; visible = to_intel_plane_state(plane_state)->visible; @@ -13255,15 +13195,18 @@ static int __intel_set_mode(struct drm_atomic_state *state) drm_atomic_helper_swap_state(dev, state); for_each_crtc_in_state(state, crtc, crtc_state, i) { + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + if (!needs_modeset(crtc->state)) continue; any_ms = true; - if (!crtc_state->active) - continue; + intel_pre_plane_update(intel_crtc); - intel_crtc_disable_planes(crtc, crtc_state->plane_mask); - dev_priv->display.crtc_disable(crtc); + if (crtc_state->active) { + intel_crtc_disable_planes(crtc, crtc_state->plane_mask); + dev_priv->display.crtc_disable(crtc); + } } /* Only after disabling all output pipelines that will be changed can we @@ -13277,15 +13220,12 @@ static int __intel_set_mode(struct drm_atomic_state *state) /* Now enable the clocks, plane, pipe, and connectors that we set up. */ for_each_crtc_in_state(state, crtc, crtc_state, i) { - drm_atomic_helper_commit_planes_on_crtc(crtc_state); - - if (!needs_modeset(crtc->state) || !crtc->state->active) - continue; - - update_scanline_offset(to_intel_crtc(crtc)); + if (needs_modeset(crtc->state) && crtc->state->active) { + update_scanline_offset(to_intel_crtc(crtc)); + dev_priv->display.crtc_enable(crtc); + } - dev_priv->display.crtc_enable(crtc); - intel_crtc_enable_planes(crtc); + drm_atomic_helper_commit_planes_on_crtc(crtc_state); } /* FIXME: add subpixel order */ @@ -13858,7 +13798,7 @@ intel_commit_primary_plane(struct drm_plane *plane, crtc->x = src->x1 >> 16; crtc->y = src->y1 >> 16; - if (!intel_crtc->active) + if (!crtc->state->active) return; if (state->visible) @@ -13885,7 +13825,8 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - intel_pre_plane_update(intel_crtc); + if (!needs_modeset(crtc->state)) + intel_pre_plane_update(intel_crtc); if (intel_crtc->atomic.update_wm) intel_update_watermarks(crtc); @@ -13893,7 +13834,7 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc, intel_runtime_pm_get(dev_priv); /* Perform vblank evasion around commit operation */ - if (crtc->state->active && !needs_modeset(crtc->state)) + if (crtc->state->active) intel_crtc->atomic.evade = intel_pipe_update_start(intel_crtc, &intel_crtc->atomic.start_vbl_count); @@ -14102,7 +14043,7 @@ intel_commit_cursor_plane(struct drm_plane *plane, intel_crtc->cursor_bo = obj; update: - if (intel_crtc->active) + if (crtc->state->active) intel_crtc_update_cursor(crtc, state->visible); } diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index f7eb3bd0a7c7..168f90f346c2 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -921,16 +921,14 @@ intel_commit_sprite_plane(struct drm_plane *plane, struct intel_plane_state *state) { struct drm_crtc *crtc = state->base.crtc; - struct intel_crtc *intel_crtc; struct intel_plane *intel_plane = to_intel_plane(plane); struct drm_framebuffer *fb = state->base.fb; crtc = crtc ? crtc : plane->crtc; - intel_crtc = to_intel_crtc(crtc); plane->fb = fb; - if (!intel_crtc->active) + if (!crtc->state->active) return; if (state->visible) {
Now that all planes are added during a modeset we can use the calculated changes before disabling a plane, and then either commit or force disable a plane before disabling the crtc. The code is shared with atomic_begin/flush, except watermark updating and vblank evasion are not used. This is needed for proper atomic suspend/resume support. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=90868 Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> --- drivers/gpu/drm/i915/intel_display.c | 103 ++++++++--------------------------- drivers/gpu/drm/i915/intel_sprite.c | 4 +- 2 files changed, 23 insertions(+), 84 deletions(-)