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.lankho...@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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
