On Tue, Mar 10, 2015 at 10:01:51AM -0700, Matt Roper wrote:
> On Tue, Mar 10, 2015 at 01:15:24PM +0200, ville.syrjala@xxxxxxxxxxxxxxx wrote:
> > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > Cc: Sonika Jindal <sonika.jindal@xxxxxxxxx>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > ---
> > 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.
I can think of one way to make it fail. That is, first set a smaller
mode with the primary plane (and fb) configured to cover that fully, and
then switch to a larger mode without reconfiguring the primary plane. If
the hardware requires the primary plane to be fullscreen it'll fail. But
that should actaully not be possible using the legacy modeset API as it
always reconfigures the primary, so we'd only have to worry about that
with full atomic modeset, and for that we anyway need to change the code
to do the check stuff up front.
So yeah, with the way things are this should not be able to fail. I'll
respin with the WARN.
> > + 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).
We should already be holding all the modeset locks if we're this deep
into the modeset code.
>
>
> 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
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
