On Thu, 2015-04-23 at 08:19 +0200, Maarten Lankhorst wrote: > This kills off most of the transitional sers and uses atomic plane updates > in the modeset path to update everything. > > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx> > --- > Changes since v1: > - Add atomic and sprite planes during a modeset too so they > will be restored. > - Drop a WARN_ON(!crtc_state->enabled) in atomic_plane_check for > cursor and sprite planes. Keep it for primary as this probably > indicates we messed up somewhere. > > drivers/gpu/drm/i915/intel_atomic_plane.c | 58 ++-- > drivers/gpu/drm/i915/intel_display.c | 485 +++++++++++++++++++++--------- > drivers/gpu/drm/i915/intel_sprite.c | 33 +- > 3 files changed, 366 insertions(+), 210 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c > index f7bd3b8fa245..ba4ab392b6b0 100644 > --- a/drivers/gpu/drm/i915/intel_atomic_plane.c > +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c > @@ -110,32 +110,40 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > struct drm_plane_state *state) > { > struct drm_crtc *crtc = state->crtc; > - struct intel_crtc *intel_crtc; > - struct intel_crtc_state *crtc_state; > + struct drm_crtc_state *crtc_state; > struct intel_plane *intel_plane = to_intel_plane(plane); > struct intel_plane_state *intel_state = to_intel_plane_state(state); > > - crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > + intel_state->visible = false; > /* > * Both crtc and plane->crtc could be NULL if we're updating a > * property while the plane is disabled. We don't actually have > * anything driver-specific we need to test in that case, so > * just return success. > */ > - if (!crtc) > + if (!crtc) { > + DRM_DEBUG_ATOMIC("Invisible: no crtc\n"); > return 0; > + } > + > + crtc_state = state->state->crtc_states[drm_crtc_index(crtc)]; > + if (WARN_ON(!crtc_state)) > + return 0; > + > + if (!crtc_state->enable) { > + DRM_DEBUG_ATOMIC("Invisible: crtc off\n"); > > - /* FIXME: temporary hack necessary while we still use the plane update > - * helper. */ > - if (state->state) { > - crtc_state = > - intel_atomic_get_crtc_state(state->state, intel_crtc); > - if (IS_ERR(crtc_state)) > - return PTR_ERR(crtc_state); > - } else { > - crtc_state = intel_crtc->config; > + /* > + * Probably allowed after converting to atomic. Right > + * now it probably means we have the state confused. > + */ > + I915_STATE_WARN_ON(plane->type == DRM_PLANE_TYPE_PRIMARY); > + return 0; > + } > + > + if (!crtc_state->active) { > + DRM_DEBUG_ATOMIC("Invisible: dpms off\n"); > + return 0; > } > > /* > @@ -155,24 +163,8 @@ static int intel_plane_atomic_check(struct drm_plane *plane, > /* Clip all planes to CRTC size, or 0x0 if CRTC is disabled */ > intel_state->clip.x1 = 0; > intel_state->clip.y1 = 0; > - intel_state->clip.x2 = > - crtc_state->base.active ? crtc_state->pipe_src_w : 0; > - intel_state->clip.y2 = > - crtc_state->base.active ? crtc_state->pipe_src_h : 0; > - > - /* > - * Disabling a plane is always okay; we just need to update > - * fb tracking in a special way since cleanup_fb() won't > - * get called by the plane helpers. > - */ > - if (state->fb == NULL && plane->state->fb != NULL) { > - /* > - * 'prepare' is never called when plane is being disabled, so > - * we need to handle frontbuffer tracking as a special case > - */ > - intel_crtc->atomic.disabled_planes |= > - (1 << drm_plane_index(plane)); > - } > + intel_state->clip.x2 = to_intel_crtc_state(crtc_state)->pipe_src_w; > + intel_state->clip.y2 = to_intel_crtc_state(crtc_state)->pipe_src_h; > > if (state->fb && intel_rotation_90_or_270(state->rotation)) { > if (!(state->fb->modifier[0] == I915_FORMAT_MOD_Y_TILED || > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 2ffacb4c3a12..acb5c5bea428 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -100,12 +100,16 @@ static void vlv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > static void chv_prepare_pll(struct intel_crtc *crtc, > const struct intel_crtc_state *pipe_config); > +static int intel_atomic_check_crtc(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state); > static void intel_begin_crtc_commit(struct drm_crtc *crtc); > static void intel_finish_crtc_commit(struct drm_crtc *crtc); > static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_crtc, > struct intel_crtc_state *crtc_state); > static void intel_crtc_enable_planes(struct drm_crtc *crtc); > static void intel_crtc_disable_planes(struct drm_crtc *crtc); > +static void intel_pre_disable_primary(struct drm_crtc *crtc); > +static void intel_post_enable_primary(struct drm_crtc *crtc); > > static struct intel_encoder *intel_find_encoder(struct intel_connector *connector, int pipe) > { > @@ -3056,11 +3060,20 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb, > { > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_plane_state *plane_state = > + to_intel_plane_state(crtc->primary->state); > + bool was_visible = plane_state->visible; > > - if (dev_priv->display.disable_fbc) > + /* Not supported right now by the helper, but lets be thorough. */ > + if (was_visible && !fb) > + intel_pre_disable_primary(crtc); > + else if (was_visible && dev_priv->display.disable_fbc) > dev_priv->display.disable_fbc(dev); > > + plane_state->visible = !!fb; > dev_priv->display.update_primary_plane(crtc, fb, x, y); > + if (!was_visible && fb) > + intel_post_enable_primary(crtc); > > return 0; > } > @@ -3087,16 +3100,17 @@ static void intel_update_primary_planes(struct drm_device *dev) > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > drm_modeset_lock(&crtc->mutex, NULL); > - /* > - * FIXME: Once we have proper support for primary planes (and > - * disabling them without disabling the entire crtc) allow again > - * a NULL crtc->primary->fb. > - */ > - if (intel_crtc->active && crtc->primary->fb) > + > + if (intel_crtc->active) { > + const struct intel_plane_state *state = > + to_intel_plane_state(crtc->primary->state); > + > dev_priv->display.update_primary_plane(crtc, > - crtc->primary->fb, > - crtc->x, > - crtc->y); > + state->base.fb, > + state->src.x1 >> 16, > + state->src.y1 >> 16); > + } > + > drm_modeset_unlock(&crtc->mutex); > } > } > @@ -10659,6 +10673,7 @@ static struct drm_crtc_helper_funcs intel_helper_funcs = { > .load_lut = intel_crtc_load_lut, > .atomic_begin = intel_begin_crtc_commit, > .atomic_flush = intel_finish_crtc_commit, > + .atomic_check = intel_atomic_check_crtc, > }; > > /** > @@ -10713,7 +10728,6 @@ static void intel_modeset_update_connector_atomic_state(struct drm_device *dev) > */ > static void intel_modeset_fixup_state(struct drm_atomic_state *state) > { > - struct intel_crtc *crtc; > struct intel_encoder *encoder; > struct intel_connector *connector; > > @@ -10736,11 +10750,6 @@ static void intel_modeset_fixup_state(struct drm_atomic_state *state) > encoder->base.crtc = NULL; > } > > - for_each_intel_crtc(state->dev, crtc) { > - crtc->base.enabled = crtc->base.state->enable; > - crtc->config = to_intel_crtc_state(crtc->base.state); > - } > - > /* Copy the new configuration to the staged state, to keep the few > * pieces of code that haven't been converted yet happy */ > intel_modeset_update_staged_output_state(state->dev); > @@ -11170,6 +11179,8 @@ intel_modeset_update_state(struct drm_atomic_state *state) > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > struct drm_connector *connector; > + struct drm_connector_state *connector_state; > + int i; > > intel_shared_dpll_commit(dev_priv); > > @@ -11185,7 +11196,26 @@ intel_modeset_update_state(struct drm_atomic_state *state) > intel_encoder->connectors_active = false; > } > > - drm_atomic_helper_swap_state(state->dev, state); > + /* > + * swap crtc and connector state, plane state is already swapped in > + * __intel_set_mode_update_planes. Once .crtc_disable is fixed > + * all state should be swapped before disabling crtc's. > + */ Can't we just fix .crtc_disable() first? > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + crtc->enabled = crtc_state->enable; > + to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc_state); > + > + crtc->state->state = state; > + swap(state->crtc_states[i], crtc->state); > + crtc->state->state = NULL; > + } > + > + for_each_connector_in_state(state, connector, connector_state, i) { > + connector->state->state = state; > + swap(state->connector_states[i], connector->state); > + connector->state->state = NULL; > + } > + > intel_modeset_fixup_state(state); > > /* Double check state. */ > @@ -11740,6 +11770,30 @@ static void update_scanline_offset(struct intel_crtc *crtc) > crtc->scanline_offset = 1; > } > > +static int intel_modeset_compute_planes(struct drm_atomic_state *state, > + struct drm_crtc *crtc) > +{ > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + int pipe = intel_crtc->pipe; > + struct drm_plane_state *plane_state; > + struct drm_plane *plane; > + > + plane_state = drm_atomic_get_plane_state(state, crtc->cursor); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > + > + /* Add all overlay planes */ > + drm_for_each_legacy_plane(plane, &crtc->dev->mode_config.plane_list) { > + if (to_intel_plane(plane)->pipe == pipe) { > + plane_state = drm_atomic_get_plane_state(state, plane); > + if (IS_ERR(plane_state)) > + return PTR_ERR(plane_state); > + } > + } > + > + return 0; > +} > + > static struct intel_crtc_state * > intel_modeset_compute_config(struct drm_crtc *crtc, > struct drm_atomic_state *state) > @@ -11765,8 +11819,14 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > if (IS_ERR(pipe_config)) > return pipe_config; > > + if (needs_modeset(&pipe_config->base)) { > + ret = intel_modeset_compute_planes(state, crtc); > + if (ret) > + return ERR_PTR(ret); > + } > + > if (!pipe_config->base.enable) > - return pipe_config; > + goto done; > > ret = intel_modeset_pipe_config(crtc, state, pipe_config); > if (ret) > @@ -11784,8 +11844,8 @@ intel_modeset_compute_config(struct drm_crtc *crtc, > * required changes and forcing a mode set. > */ > > - intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config,"[modeset]"); > - > + intel_dump_pipe_config(to_intel_crtc(crtc), pipe_config, "[modeset]"); > +done: > ret = drm_atomic_helper_check_planes(state->dev, state); > if (ret) > return ERR_PTR(ret); > @@ -11869,6 +11929,113 @@ static int __intel_set_mode_checks(struct drm_atomic_state *state) > return 0; > } > > +static void __intel_set_mode_update_planes(struct drm_device *dev, > + struct drm_atomic_state *state) > +{ > + int i; > + struct drm_plane_state *old_plane_state; > + struct drm_crtc_state *crtc_state; > + struct drm_plane *plane; > + struct drm_crtc *crtc; > + > + /* > + * For now only swap plane state, should be replaced with a > + * call to drm_atomic_helper_swap_state > + */ > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + struct drm_plane *plane = state->planes[i]; > + > + if (!plane) > + continue; > + > + plane->state->state = state; > + swap(state->plane_states[i], plane->state); > + plane->state->state = NULL; > + } > + > + for_each_crtc_in_state(state, crtc, crtc_state, i) { > + const struct drm_crtc_helper_funcs *funcs; > + > + funcs = crtc->helper_private; > + > + if (!funcs || !funcs->atomic_begin) > + continue; > + > + /* XXX: Hack because crtc state is not swapped */ > + crtc->state->mode_changed = crtc_state->mode_changed; > + crtc->state->active_changed = crtc_state->active_changed; > + > + DRM_DEBUG_ATOMIC("Calling atomic_begin on crtc %i\n", i); > + funcs->atomic_begin(crtc); > + } > + > + for_each_plane_in_state(state, plane, old_plane_state, i) { > + bool visible = to_intel_plane_state(plane->state)->visible; > + struct intel_plane *intel_plane = to_intel_plane(plane); > + const struct drm_plane_helper_funcs *funcs; > + > + crtc = plane->state->crtc; > + funcs = plane->helper_private; > + > + if (!funcs) > + continue; > + > + DRM_DEBUG_ATOMIC("Plane %i is visible: %i\n", i, visible); > + > + if (!visible) > + funcs->atomic_update(plane, old_plane_state); I'm getting a NULL pointer dereference in intel_commit_primary_plane() because of a NULL crtc when an already disabled plane is disabled again. Should we just skip the update here? > + else if (crtc->state->mode_changed) > + intel_plane->disable_plane(plane, crtc, true); > + } > +} > + > +static void __intel_set_mode_cleanup_planes(struct drm_device *dev, > + struct drm_atomic_state *old_state) > +{ > + int nplanes = dev->mode_config.num_total_plane; > + int ncrtcs = dev->mode_config.num_crtc; > + int i; > + > + for (i = 0; i < nplanes; i++) { > + const struct drm_plane_helper_funcs *funcs; > + struct drm_plane *plane = old_state->planes[i]; > + struct drm_plane_state *old_plane_state; > + > + if (!plane) > + continue; > + > + funcs = plane->helper_private; > + > + if (!funcs) > + continue; > + > + old_plane_state = old_state->plane_states[i]; > + > + if (to_intel_plane_state(plane->state)->visible) { > + DRM_DEBUG_ATOMIC("Plane %i is updated\n", i); > + funcs->atomic_update(plane, old_plane_state); > + } else > + DRM_DEBUG_ATOMIC("Plane %i is left alone\n", i); > + } > + > + for (i = 0; i < ncrtcs; i++) { > + const struct drm_crtc_helper_funcs *funcs; > + struct drm_crtc *crtc = old_state->crtcs[i]; > + > + if (!crtc) > + continue; > + > + funcs = crtc->helper_private; > + > + if (!funcs || !funcs->atomic_flush) > + continue; > + > + DRM_DEBUG_ATOMIC("Calling atomic_flush on crtc %i\n", i); > + funcs->atomic_flush(crtc); > + } These loops could use for_each_{crtc,plane}_in_state macros. > + drm_atomic_helper_cleanup_planes(dev, old_state); > +} > + > static int __intel_set_mode(struct drm_crtc *modeset_crtc, > struct intel_crtc_state *pipe_config) > { > @@ -11877,7 +12044,7 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > struct drm_atomic_state *state = pipe_config->base.state; > struct drm_crtc *crtc; > struct drm_crtc_state *crtc_state; > - int ret = 0; > + int ret; > int i; > > ret = __intel_set_mode_checks(state); > @@ -11888,14 +12055,14 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > if (ret) > return ret; > > + __intel_set_mode_update_planes(dev, state); > + > for_each_crtc_in_state(state, crtc, crtc_state, i) { > if (!needs_modeset(crtc_state)) > continue; > > - intel_crtc_disable_planes(crtc); > + intel_crtc_dpms_overlay_disable(to_intel_crtc(crtc)); > dev_priv->display.crtc_disable(crtc); > - if (!crtc_state->enable) > - drm_plane_helper_disable(crtc->primary); > } > > /* crtc->mode is already used by the ->mode_set callbacks, hence we need > @@ -11926,8 +12093,6 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > > modeset_update_crtc_power_domains(state); > > - drm_atomic_helper_commit_planes(dev, state); > - > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > for_each_crtc_in_state(state, crtc, crtc_state, i) { > if (!crtc->state->enable) > @@ -11935,13 +12100,13 @@ static int __intel_set_mode(struct drm_crtc *modeset_crtc, > > update_scanline_offset(to_intel_crtc(crtc)); > > - dev_priv->display.crtc_enable(crtc); > - intel_crtc_enable_planes(crtc); > + if (needs_modeset(crtc->state)) > + dev_priv->display.crtc_enable(crtc); In v2 of the patch that removes modeset_pipes and friends, I added a check for needs_modeset() before update_scanline_offset(). > } > > - /* FIXME: add subpixel order */ > + __intel_set_mode_cleanup_planes(dev, state); > > - drm_atomic_helper_cleanup_planes(dev, state); > + /* FIXME: add subpixel order */ > > drm_atomic_state_free(state); > > @@ -12170,6 +12335,7 @@ intel_modeset_stage_output_state(struct drm_device *dev, > return ret; > > crtc_state->enable = drm_atomic_connectors_for_crtc(state, crtc); > + crtc_state->active = crtc_state->enable; Should this be a separate fix? > } > > ret = intel_modeset_setup_plane_state(state, set->crtc, set->mode, > @@ -12190,20 +12356,11 @@ intel_modeset_stage_output_state(struct drm_device *dev, > return 0; > } > > -static bool primary_plane_visible(struct drm_crtc *crtc) > -{ > - struct intel_plane_state *plane_state = > - to_intel_plane_state(crtc->primary->state); > - > - return plane_state->visible; > -} > - > static int intel_crtc_set_config(struct drm_mode_set *set) > { > struct drm_device *dev; > struct drm_atomic_state *state = NULL; > struct intel_crtc_state *pipe_config; > - bool primary_plane_was_visible; > int ret; > > BUG_ON(!set); > @@ -12242,38 +12399,7 @@ static int intel_crtc_set_config(struct drm_mode_set *set) > > intel_update_pipe_size(to_intel_crtc(set->crtc)); > > - primary_plane_was_visible = primary_plane_visible(set->crtc); > - > ret = intel_set_mode_with_config(set->crtc, pipe_config); > - > - if (ret == 0 && > - pipe_config->base.enable && > - pipe_config->base.planes_changed && > - !needs_modeset(&pipe_config->base)) { > - struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc); > - > - /* > - * We need to make sure the primary plane is re-enabled if it > - * has previously been turned off. > - */ > - if (ret == 0 && !primary_plane_was_visible && > - primary_plane_visible(set->crtc)) { > - WARN_ON(!intel_crtc->active); > - intel_post_enable_primary(set->crtc); > - } > - > - /* > - * In the fastboot case this may be our only check of the > - * state after boot. It would be better to only do it on > - * the first update, but we don't have a nice way of doing that > - * (and really, set_config isn't used much for high freq page > - * flipping, so increasing its cost here shouldn't be a big > - * deal). > - */ > - if (i915.fastboot && ret == 0) > - intel_modeset_check_state(set->crtc->dev); > - } > - Maybe this could be a separate patch? > if (ret) { > DRM_DEBUG_KMS("failed to set mode on [CRTC:%d], err = %d\n", > set->crtc->base.id, ret); > @@ -12413,6 +12539,9 @@ bool intel_wm_need_update(struct drm_plane *plane, > plane->state->rotation != state->rotation) > return true; > > + if (plane->state->crtc_w != state->crtc_w) > + return true; > + > return false; > } > > @@ -12507,74 +12636,22 @@ intel_check_primary_plane(struct drm_plane *plane, > struct intel_plane_state *state) > { > struct drm_device *dev = plane->dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_crtc *crtc = state->base.crtc; > - struct intel_crtc *intel_crtc; > struct drm_framebuffer *fb = state->base.fb; > struct drm_rect *dest = &state->dst; > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > bool can_position = false; > - int ret; > - > - crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > > if (INTEL_INFO(dev)->gen >= 9) > can_position = true; > > - ret = drm_plane_helper_check_update(plane, crtc, fb, > - src, dest, clip, > - DRM_PLANE_HELPER_NO_SCALING, > - DRM_PLANE_HELPER_NO_SCALING, > - can_position, true, > - &state->visible); > - if (ret) > - return ret; > - > - if (intel_crtc->active) { > - struct intel_plane_state *old_state = > - to_intel_plane_state(plane->state); > - > - intel_crtc->atomic.wait_for_flips = true; > - > - /* > - * FBC does not work on some platforms for rotated > - * planes, so disable it when rotation is not 0 and > - * update it when rotation is set back to 0. > - * > - * FIXME: This is redundant with the fbc update done in > - * the primary plane enable function except that that > - * one is done too late. We eventually need to unify > - * this. > - */ > - if (state->visible && > - INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > - dev_priv->fbc.crtc == intel_crtc && > - state->base.rotation != BIT(DRM_ROTATE_0)) { > - intel_crtc->atomic.disable_fbc = true; > - } > - > - if (state->visible && !old_state->visible) { > - /* > - * 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 (IS_BROADWELL(dev)) > - intel_crtc->atomic.wait_vblank = true; > - } > - > - intel_crtc->atomic.fb_bits |= > - INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > - > - intel_crtc->atomic.update_fbc = true; > - > - if (intel_wm_need_update(plane, &state->base)) > - intel_crtc->atomic.update_wm = true; > - } > - > - return 0; > + return drm_plane_helper_check_update(plane, crtc, fb, > + src, dest, clip, > + DRM_PLANE_HELPER_NO_SCALING, > + DRM_PLANE_HELPER_NO_SCALING, > + can_position, true, > + &state->visible); > } > > static void > @@ -12600,8 +12677,10 @@ intel_commit_primary_plane(struct drm_plane *plane, > /* FIXME: kill this fastboot hack */ > intel_update_pipe_size(intel_crtc); > > - dev_priv->display.update_primary_plane(crtc, plane->fb, > - crtc->x, crtc->y); > + dev_priv->display.update_primary_plane(crtc, > + state->base.fb, > + state->src.x1 >> 16, > + state->src.y1 >> 16); > } > } > > @@ -12616,6 +12695,123 @@ intel_disable_primary_plane(struct drm_plane *plane, > dev_priv->display.update_primary_plane(crtc, NULL, 0, 0); > } > > +/* Transitional checking here, mostly for plane updates */ > +static int intel_atomic_check_crtc(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state) > +{ > + struct intel_crtc_state *pipe_config = to_intel_crtc_state(crtc_state); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_atomic_state *state = crtc_state->state; > + struct drm_device *dev = crtc->dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + unsigned plane_mask; > + int i, nplanes = dev->mode_config.num_total_plane, idx; > + bool mode_changed = needs_modeset(crtc_state); > + bool is_crtc_enabled = crtc_state->active; > + bool was_crtc_enabled = crtc->state->active && intel_crtc->active; > + > + memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); > + intel_crtc->atomic.update_wm = mode_changed; > + > + idx = drm_crtc_index(crtc); > + DRM_DEBUG_ATOMIC("Crtc %i was enabled %i now enabled: %i\n", > + idx, was_crtc_enabled, is_crtc_enabled); > + > + plane_mask = crtc_state->plane_mask | crtc->state->plane_mask; > + for (i = 0; i < nplanes; i++) { > + struct intel_plane_state *plane_state, *old_plane_state; > + struct intel_plane *plane = to_intel_plane(state->planes[i]); > + bool turn_off, turn_on, visible, was_visible; > + struct drm_framebuffer *fb; > + > + if (!plane) > + continue; > + > + plane_state = to_intel_plane_state(state->plane_states[i]); > + old_plane_state = to_intel_plane_state(plane->base.state); > + > + was_visible = old_plane_state->visible && was_crtc_enabled; > + visible = plane_state->visible && is_crtc_enabled; > + > + turn_off = was_visible && (!visible || mode_changed); > + turn_on = visible && (!was_visible || mode_changed); > + fb = plane_state->base.fb; > + > + DRM_DEBUG_ATOMIC("Crtc %i has plane %i with fb %i\n", idx, > + drm_plane_index(&plane->base), fb ? fb->base.id : -1); > + DRM_DEBUG_ATOMIC("\tvisible %i -> %i, off %i, on %i, ms %i\n", > + was_visible, visible, turn_off, turn_on, mode_changed); > + > + /* plane being turned off as part of modeset or changes? */ > + if (intel_wm_need_update(&plane->base, &plane_state->base)) > + intel_crtc->atomic.update_wm = true; > + > + /* > + * 'prepare' is never called when plane is being disabled, so > + * we need to handle frontbuffer tracking as a special case > + */ > + if (old_plane_state->base.fb && !plane_state->base.fb) > + intel_crtc->atomic.disabled_planes |= > + (1 << drm_plane_index(&plane->base)); > + > + switch (plane->base.type) { > + case DRM_PLANE_TYPE_PRIMARY: > + intel_crtc->atomic.fb_bits |= > + INTEL_FRONTBUFFER_PRIMARY(intel_crtc->pipe); > + > + intel_crtc->atomic.wait_for_flips = true; > + intel_crtc->atomic.pre_disable_primary = turn_off; > + intel_crtc->atomic.post_enable_primary = turn_on; > + > + if (turn_off) > + intel_crtc->atomic.disable_fbc = true; > + > + /* > + * FBC does not work on some platforms for rotated > + * planes, so disable it when rotation is not 0 and > + * update it when rotation is set back to 0. > + * > + * FIXME: This is redundant with the fbc update done in > + * the primary plane enable function except that that > + * one is done too late. We eventually need to unify > + * this. > + */ > + > + if (visible && > + INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev) && > + dev_priv->fbc.crtc == intel_crtc && > + plane_state->base.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 = true; > + break; > + case DRM_PLANE_TYPE_CURSOR: > + intel_crtc->atomic.fb_bits |= > + INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); > + break; > + case DRM_PLANE_TYPE_OVERLAY: > + intel_crtc->atomic.fb_bits |= > + INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); > + > + if (turn_off) { > + intel_crtc->atomic.wait_vblank = true; > + intel_crtc->atomic.update_sprite_watermarks |= > + (1 << drm_plane_index(&plane->base)); > + } > + break; > + } > + } > + return 0; > +} > + Is there a specific reason why this needs to be in check_crtc vs check_plane? IMO, it would make review easier if this move was a separate patch. > static void intel_begin_crtc_commit(struct drm_crtc *crtc) > { > struct drm_device *dev = crtc->dev; > @@ -12664,10 +12860,13 @@ static void intel_begin_crtc_commit(struct drm_crtc *crtc) > intel_runtime_pm_get(dev_priv); > > /* Perform vblank evasion around commit operation */ > - if (intel_crtc->active) > + if (intel_crtc->active && !needs_modeset(crtc->state) && > + !dev_priv->power_domains.init_power_on) > intel_crtc->atomic.evade = > intel_pipe_update_start(intel_crtc, > &intel_crtc->atomic.start_vbl_count); > + else > + intel_crtc->atomic.evade = false; > } > > static void intel_finish_crtc_commit(struct drm_crtc *crtc) > @@ -12703,6 +12902,8 @@ static void intel_finish_crtc_commit(struct drm_crtc *crtc) > false, false); > > memset(&intel_crtc->atomic, 0, sizeof(intel_crtc->atomic)); > + crtc->state->mode_changed = false; > + crtc->state->active_changed = false; > } > > /** > @@ -12812,13 +13013,9 @@ intel_check_cursor_plane(struct drm_plane *plane, > struct drm_rect *src = &state->src; > const struct drm_rect *clip = &state->clip; > struct drm_i915_gem_object *obj = intel_fb_obj(fb); > - struct intel_crtc *intel_crtc; > unsigned stride; > int ret; > > - crtc = crtc ? crtc : plane->crtc; > - intel_crtc = to_intel_crtc(crtc); > - > ret = drm_plane_helper_check_update(plane, crtc, fb, > src, dest, clip, > DRM_PLANE_HELPER_NO_SCALING, > @@ -12830,7 +13027,7 @@ intel_check_cursor_plane(struct drm_plane *plane, > > /* if we want to turn off the cursor ignore width and height */ > if (!obj) > - goto finish; > + return 0; > > /* Check for which cursor types we support */ > if (!cursor_size_ok(dev, state->base.crtc_w, state->base.crtc_h)) { > @@ -12847,19 +13044,10 @@ intel_check_cursor_plane(struct drm_plane *plane, > > if (fb->modifier[0] != DRM_FORMAT_MOD_NONE) { > DRM_DEBUG_KMS("cursor cannot be tiled\n"); > - ret = -EINVAL; > - } > - > -finish: > - if (intel_crtc->active) { > - if (plane->state->crtc_w != state->base.crtc_w) > - intel_crtc->atomic.update_wm = true; > - > - intel_crtc->atomic.fb_bits |= > - INTEL_FRONTBUFFER_CURSOR(intel_crtc->pipe); > + return -EINVAL; > } > > - return ret; > + return 0; > } > > static void > @@ -14089,6 +14277,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > > WARN_ON(crtc->active); > crtc->base.state->enable = false; > + crtc->base.state->active = false; > crtc->base.enabled = false; > } > > @@ -14117,6 +14306,7 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc) > crtc->active ? "enabled" : "disabled"); > > crtc->base.state->enable = crtc->active; > + crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > > /* Because we only establish the connector -> encoder -> > @@ -14255,6 +14445,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev) > crtc->config); > > crtc->base.state->enable = crtc->active; > + crtc->base.state->active = crtc->active; > crtc->base.enabled = crtc->active; > Should this be a separate bug fix? Ander > plane_state = to_intel_plane_state(primary->state); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index 7419e04b113f..5a277757ac2d 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -811,12 +811,8 @@ intel_check_sprite_plane(struct drm_plane *plane, > int max_scale, min_scale; > int pixel_size; > > - intel_crtc = intel_crtc ? intel_crtc : to_intel_crtc(plane->crtc); > - > - if (!fb) { > - state->visible = false; > - goto finish; > - } > + if (!fb) > + return 0; > > /* Don't modify another pipe's plane */ > if (intel_plane->pipe != intel_crtc->pipe) { > @@ -847,7 +843,7 @@ intel_check_sprite_plane(struct drm_plane *plane, > vscale = drm_rect_calc_vscale_relaxed(src, dst, min_scale, max_scale); > BUG_ON(vscale < 0); > > - state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); > + state->visible = drm_rect_clip_scaled(src, dst, clip, hscale, vscale); > > crtc_x = dst->x1; > crtc_y = dst->y1; > @@ -952,29 +948,6 @@ intel_check_sprite_plane(struct drm_plane *plane, > dst->y1 = crtc_y; > dst->y2 = crtc_y + crtc_h; > > -finish: > - /* > - * If the sprite is completely covering the primary plane, > - * we can disable the primary and save power. > - */ > - if (intel_crtc->active) { > - intel_crtc->atomic.fb_bits |= > - INTEL_FRONTBUFFER_SPRITE(intel_crtc->pipe); > - > - if (intel_wm_need_update(plane, &state->base)) > - intel_crtc->atomic.update_wm = true; > - > - if (!state->visible) { > - /* > - * Avoid underruns when disabling the sprite. > - * FIXME remove once watermark updates are done properly. > - */ > - intel_crtc->atomic.wait_vblank = true; > - intel_crtc->atomic.update_sprite_watermarks |= > - (1 << drm_plane_index(plane)); > - } > - } > - > return 0; > } > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx