[Intel-gfx] [PATCH 4/9] drm/i915: Make derived plane state correct after crtc_enable
Matt Roper
matthew.d.roper at intel.com
Tue Mar 10 10:01:51 PDT 2015
On Tue, Mar 10, 2015 at 01:15:24PM +0200, ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
>
> We need to call the plane .atomic_check() hook when enabling the crtc
> to make sure all the derived state (eg. visible, clipped src/dst
> coordinates) are up to date when we enable the plane. This allows us to
> trust this derived state everywhere.
>
> We also take the opportunity to rewrite the plane enable sequence to
> perform it as a single atomic update, which is a bit closer to where we
> want to end up. Obviously this is a bit of a hack as we can't deal with
> errors from .atomic_check(), so we just paper over that by making sure
> visible=false so that we don't try to enable the plane with potentially
> garbage coordinates and whatnot.
>
> We also abuse the atomic code a bit by not making another copy of the
> plane state and just frobbing directly with the plane->state.
>
> Cc: Matt Roper <matthew.d.roper at intel.com>
> Cc: Sonika Jindal <sonika.jindal at intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 3 +
> drivers/gpu/drm/i915/intel_display.c | 222 ++++++++++++++---------------------
> 2 files changed, 88 insertions(+), 137 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 5462cbf..b16c0a7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -238,6 +238,9 @@ enum hpd_pin {
> #define for_each_intel_crtc(dev, intel_crtc) \
> list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
>
> +#define for_each_intel_plane(dev, intel_plane) \
> + list_for_each_entry(intel_plane, &dev->mode_config.plane_list, base.head)
> +
> #define for_each_intel_encoder(dev, intel_encoder) \
> list_for_each_entry(intel_encoder, \
> &(dev)->mode_config.encoder_list, \
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0da3abf..fdc83f1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2124,66 +2124,6 @@ void intel_flush_primary_plane(struct drm_i915_private *dev_priv,
> POSTING_READ(reg);
> }
>
> -/**
> - * 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);
> -
> - if (intel_crtc->primary_enabled)
> - return;
> -
> - intel_crtc->primary_enabled = true;
> -
> - dev_priv->display.update_primary_plane(crtc, plane->fb,
> - crtc->x, crtc->y);
> -
> - /*
> - * 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_wait_for_vblank(dev, intel_crtc->pipe);
> -}
> -
> -/**
> - * intel_disable_primary_hw_plane - disable the primary hardware plane
> - * @plane: plane to be disabled
> - * @crtc: crtc for the plane
> - *
> - * Disable @plane on @crtc, making sure that the pipe is running first.
> - */
> -static void intel_disable_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 (WARN_ON(!intel_crtc->active))
> - return;
> -
> - if (!intel_crtc->primary_enabled)
> - return;
> -
> - intel_crtc->primary_enabled = false;
> -
> - 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
> @@ -2895,7 +2835,10 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
> POSTING_READ(PLANE_SURF(pipe, 0));
> }
>
> -/* Assume fb object is pinned & idle & fenced and just update base pointers */
> +/*
> + * Assume fb object is big enough & pinned & idle & fenced,
> + * and just update base pointers
> + */
> static int
> intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> int x, int y, enum mode_set_atomic state)
> @@ -2906,6 +2849,7 @@ intel_pipe_set_base_atomic(struct drm_crtc *crtc, struct drm_framebuffer *fb,
> if (dev_priv->display.disable_fbc)
> dev_priv->display.disable_fbc(dev);
>
> + to_intel_crtc(crtc)->primary_enabled = true;
> dev_priv->display.update_primary_plane(crtc, fb, x, y);
>
> return 0;
> @@ -2933,16 +2877,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);
> }
> }
> @@ -4183,50 +4128,76 @@ 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);
> - }
> -}
> -
> /*
> - * Disable a plane internally without actually modifying the plane's state.
> - * This will allow us to easily restore the plane later by just reprogramming
> - * its state.
> + * Ugly hack to make sure we update the planes correctly
> */
> -static void disable_plane_internal(struct drm_plane *plane)
> +static void _intel_crtc_enable_planes(struct intel_crtc *crtc)
> {
> - struct intel_plane *intel_plane = to_intel_plane(plane);
> - struct drm_plane_state *state =
> - plane->funcs->atomic_duplicate_state(plane);
> - struct intel_plane_state *intel_state = to_intel_plane_state(state);
> + struct drm_device *dev = crtc->base.dev;
> + enum pipe pipe = crtc->pipe;
> + struct intel_plane *plane;
> + const struct drm_crtc_helper_funcs *crtc_funcs =
> + crtc->base.helper_private;
>
> - intel_state->visible = false;
> - intel_plane->commit_plane(plane, intel_state);
> + for_each_intel_plane(dev, plane) {
> + const struct drm_plane_helper_funcs *funcs =
> + plane->base.helper_private;
> + struct intel_plane_state *state =
> + to_intel_plane_state(plane->base.state);
>
> - intel_plane_destroy_state(plane, state);
> + if (plane->pipe != pipe)
> + continue;
> +
> + if (funcs->atomic_check(&plane->base, &state->base))
Maybe add a WARN_ON() here? I'm assuming that this shouldn't really be
possible since if this fails it means we've already previously done a
commit of invalid state on a previous atomic transaction. But if it
does somehow happen, the WARN will give us a clue why the plane contents
simply didn't show up.
> + state->visible = false;
> + }
> +
> + crtc_funcs->atomic_begin(&crtc->base);
> +
> + for_each_intel_plane(dev, plane) {
> + const struct drm_plane_helper_funcs *funcs =
> + plane->base.helper_private;
> +
> + if (plane->pipe != pipe)
> + continue;
> +
> + funcs->atomic_update(&plane->base, plane->base.state);
> + }
> +
> + crtc_funcs->atomic_flush(&crtc->base);
> }
>
> -static void intel_disable_sprite_planes(struct drm_crtc *crtc)
> +static void _intel_crtc_disable_planes(struct intel_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 (plane->fb && intel_plane->pipe == pipe)
> - disable_plane_internal(plane);
> + struct drm_device *dev = crtc->base.dev;
> + enum pipe pipe = crtc->pipe;
> + struct intel_plane *plane;
> + const struct drm_crtc_helper_funcs *crtc_funcs =
> + crtc->base.helper_private;
> +
> + for_each_intel_plane(dev, plane) {
> + struct intel_plane_state *state =
> + to_intel_plane_state(plane->base.state);
> +
> + if (plane->pipe != pipe)
> + continue;
> +
> + state->visible = false;
> }
I'm not super familiar with the current locking situation of our legacy
modeset paths; is it possible for us to race this with another plane
update or are we already holding locks from somewhere higher in the
callstack? If it's possible for another thread to swap in a new
plane->state right here then we're not really committing what we thought
we were (and plane->state->visible may be true).
Matt
> +
> + crtc_funcs->atomic_begin(&crtc->base);
> +
> + for_each_intel_plane(dev, plane) {
> + const struct drm_plane_helper_funcs *funcs =
> + plane->base.helper_private;
> +
> + if (plane->pipe != pipe)
> + continue;
> +
> + funcs->atomic_update(&plane->base, plane->base.state);
> + }
> +
> + crtc_funcs->atomic_flush(&crtc->base);
> }
>
> void hsw_enable_ips(struct intel_crtc *crtc)
> @@ -4358,9 +4329,7 @@ static void intel_crtc_enable_planes(struct drm_crtc *crtc)
> 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);
> - intel_crtc_update_cursor(crtc, true);
> + _intel_crtc_enable_planes(intel_crtc);
> intel_crtc_dpms_overlay(intel_crtc, true);
>
> hsw_enable_ips(intel_crtc);
> @@ -4392,9 +4361,7 @@ static void intel_crtc_disable_planes(struct drm_crtc *crtc)
> hsw_disable_ips(intel_crtc);
>
> intel_crtc_dpms_overlay(intel_crtc, false);
> - intel_crtc_update_cursor(crtc, false);
> - intel_disable_sprite_planes(crtc);
> - intel_disable_primary_hw_plane(crtc->primary, crtc);
> + _intel_crtc_disable_planes(intel_crtc);
>
> /*
> * FIXME: Once we grow proper nuclear flip support out of this we need
> @@ -11708,7 +11675,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> modeset_pipes, prepare_pipes,
> disable_pipes);
> } else if (config->fb_changed) {
> - struct intel_crtc *intel_crtc = to_intel_crtc(set->crtc);
> struct drm_plane *primary = set->crtc->primary;
> int vdisplay, hdisplay;
>
> @@ -11719,15 +11685,6 @@ static int intel_crtc_set_config(struct drm_mode_set *set)
> hdisplay << 16, vdisplay << 16);
>
> /*
> - * We need to make sure the primary plane is re-enabled if it
> - * has previously been turned off.
> - */
> - if (!intel_crtc->primary_enabled && ret == 0) {
> - WARN_ON(!intel_crtc->active);
> - intel_enable_primary_hw_plane(set->crtc->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
> @@ -12054,24 +12011,15 @@ intel_commit_primary_plane(struct drm_plane *plane,
> intel_plane->obj = obj;
>
> if (intel_crtc->active) {
> - if (state->visible) {
> - /* FIXME: kill this fastboot hack */
> + /* FIXME: kill this fastboot hack */
> + if (state->visible)
> intel_update_pipe_size(intel_crtc);
>
> - intel_crtc->primary_enabled = true;
> -
> - dev_priv->display.update_primary_plane(crtc, plane->fb,
> - crtc->x, crtc->y);
> - } else {
> - /*
> - * If clipping results in a non-visible primary plane,
> - * we'll disable the primary plane. Note that this is
> - * a bit different than what happens if userspace
> - * explicitly disables the plane by passing fb=0
> - * because plane->fb still gets set and pinned.
> - */
> - intel_disable_primary_hw_plane(plane, crtc);
> - }
> + intel_crtc->primary_enabled = state->visible;
> + dev_priv->display.update_primary_plane(crtc,
> + state->base.fb,
> + state->src.x1 >> 16,
> + state->src.y1 >> 16);
> }
> }
>
> --
> 2.0.5
>
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
More information about the Intel-gfx
mailing list