From: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
DRI Development <dri-devel@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH 12/26] drm/atomic-helper: Massage swap_state signature somewhat
Date: Mon, 30 May 2016 17:09:07 +0200 [thread overview]
Message-ID: <20160530150907.GO27098@phenom.ffwll.local> (raw)
In-Reply-To: <61fb5391-5e19-f41b-8d31-a297a6910ab0@linux.intel.com>
On Mon, May 30, 2016 at 03:08:39PM +0200, Maarten Lankhorst wrote:
> Op 29-05-16 om 20:35 schreef Daniel Vetter:
> > - dev is redundant, we have state->atomic
> > - add stall parameter, which must be set when swapping needs to stall
> > for preceeding commits to stop looking at ->state pointers. Currently
> > all drivers need this to be, just prep work for a glorious future.
> I have to disagree, if you want to stall it should be done in a separate helper before swapping state.
> That function should also have the ability to fail, since we haven't called swap_state yet. :)
>
> Maybe with nonblock as parameter, so it could fail with -EBUSY if it would stall, and -EINTR if interrupted?
This is not the stalling you're thinking of. The EBUSY check is in
drm_atomic_helper_setup_commit (and as such entirely optional).
And if you don't ever use the nonblocking helpers, no struct
drm_crtc_commit will ever show up in crtc->commit_list, which means this
here also doesn't do anything.
This stall here is just to make sure that the previous commit doesn't get
confused because we've ripped out crtc/plane/connector->state from
underneath it. And as soon as we add previous_state pointers to
drm_atomic_state and wire it up through all the hooks we can set
stall=false here, since then pipelining with a queue depth > 1 is
possible.
-Daniel
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> > drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 2 +-
> > drivers/gpu/drm/drm_atomic_helper.c | 8 ++++----
> > drivers/gpu/drm/exynos/exynos_drm_drv.c | 2 +-
> > drivers/gpu/drm/i915/intel_display.c | 2 +-
> > drivers/gpu/drm/mediatek/mtk_drm_drv.c | 2 +-
> > drivers/gpu/drm/msm/msm_atomic.c | 2 +-
> > drivers/gpu/drm/omapdrm/omap_drv.c | 2 +-
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 +-
> > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 2 +-
> > drivers/gpu/drm/sti/sti_drv.c | 2 +-
> > drivers/gpu/drm/tegra/drm.c | 2 +-
> > drivers/gpu/drm/vc4/vc4_kms.c | 2 +-
> > include/drm/drm_atomic_helper.h | 4 ++--
> > 13 files changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > index 6485fa5bee8b..9ecf16c7911d 100644
> > --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
> > @@ -519,7 +519,7 @@ static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,
> > }
> >
> > /* Swap the state, this is the point of no return. */
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (async)
> > queue_work(dc->wq, &commit->work);
> > diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> > index 5298eb668ca7..fecbb52cbb85 100644
> > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > @@ -1160,7 +1160,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> > * the software side now.
> > */
> >
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > /*
> > * Everything below can be run asynchronously without the need to grab
> > @@ -1531,8 +1531,8 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
> >
> > /**
> > * drm_atomic_helper_swap_state - store atomic state into current sw state
> > - * @dev: DRM device
> > * @state: atomic state
> > + * @stall: stall for proceeding commits
> > *
> > * This function stores the atomic state into the current state pointers in all
> > * driver objects. It should be called after all failing steps have been done
> > @@ -1554,8 +1554,8 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
> > * 5. Call drm_atomic_helper_cleanup_planes() with @state, which since step 3
> > * contains the old state. Also do any other cleanup required with that state.
> > */
> > -void drm_atomic_helper_swap_state(struct drm_device *dev,
> > - struct drm_atomic_state *state)
> > +void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > + bool stall)
> > {
> > int i;
> > struct drm_connector *connector;
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > index cabc5fd0246d..deba76982358 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> > @@ -299,7 +299,7 @@ int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state,
> > priv->pending |= commit->crtcs;
> > spin_unlock(&priv->lock);
> >
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (nonblock)
> > schedule_work(&commit->work);
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9ccd76699f48..d32274071393 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13660,7 +13660,7 @@ static int intel_atomic_commit(struct drm_device *dev,
> > return ret;
> > }
> >
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> > dev_priv->wm.distrust_bios_wm = false;
> > dev_priv->wm.skl_results = intel_state->wm_results;
> > intel_shared_dpll_commit(state);
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > index 06a417b2f91e..c33bf98c5d5e 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> > @@ -91,7 +91,7 @@ static int mtk_atomic_commit(struct drm_device *drm,
> > mutex_lock(&private->commit.lock);
> > flush_work(&private->commit.work);
> >
> > - drm_atomic_helper_swap_state(drm, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (async)
> > mtk_atomic_schedule(private, state);
> > diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
> > index 9c0e4261dbba..d02bd6a50e90 100644
> > --- a/drivers/gpu/drm/msm/msm_atomic.c
> > +++ b/drivers/gpu/drm/msm/msm_atomic.c
> > @@ -238,7 +238,7 @@ int msm_atomic_commit(struct drm_device *dev,
> > * the software side now.
> > */
> >
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > /*
> > * Everything below can be run asynchronously without the need to grab
> > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
> > index d86f5479345b..5d8377aad639 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_drv.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c
> > @@ -175,7 +175,7 @@ static int omap_atomic_commit(struct drm_device *dev,
> > spin_unlock(&priv->commit.lock);
> >
> > /* Swap the state, this is the point of no return. */
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (nonblock)
> > schedule_work(&commit->work);
> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > index f315c55c1f65..3d04a506cf33 100644
> > --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> > @@ -327,7 +327,7 @@ static int rcar_du_atomic_commit(struct drm_device *dev,
> > }
> >
> > /* Swap the state, this is the point of no return. */
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (nonblock)
> > schedule_work(&commit->work);
> > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > index 03913b483506..5ea141dfae5b 100644
> > --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> > @@ -290,7 +290,7 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
> > mutex_lock(&commit->lock);
> > flush_work(&commit->work);
> >
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > commit->dev = dev;
> > commit->state = state;
> > diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
> > index b440617a7019..dd2c400c4a46 100644
> > --- a/drivers/gpu/drm/sti/sti_drv.c
> > +++ b/drivers/gpu/drm/sti/sti_drv.c
> > @@ -215,7 +215,7 @@ static int sti_atomic_commit(struct drm_device *drm,
> > * the software side now.
> > */
> >
> > - drm_atomic_helper_swap_state(drm, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (nonblock)
> > sti_atomic_schedule(private, state);
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index b59c3bf0df44..a177a42a9849 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -93,7 +93,7 @@ static int tegra_atomic_commit(struct drm_device *drm,
> > * the software side now.
> > */
> >
> > - drm_atomic_helper_swap_state(drm, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > if (nonblock)
> > tegra_atomic_schedule(tegra, state);
> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> > index 39c0b2048bfd..8f4d5ffc32be 100644
> > --- a/drivers/gpu/drm/vc4/vc4_kms.c
> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> > @@ -148,7 +148,7 @@ static int vc4_atomic_commit(struct drm_device *dev,
> > * the software side now.
> > */
> >
> > - drm_atomic_helper_swap_state(dev, state);
> > + drm_atomic_helper_swap_state(state, true);
> >
> > /*
> > * Everything below can be run asynchronously without the need to grab
> > diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> > index d473dcc91f54..0276447225ed 100644
> > --- a/include/drm/drm_atomic_helper.h
> > +++ b/include/drm/drm_atomic_helper.h
> > @@ -71,8 +71,8 @@ void drm_atomic_helper_commit_planes_on_crtc(struct drm_crtc_state *old_crtc_sta
> > void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc *crtc,
> > bool atomic);
> >
> > -void drm_atomic_helper_swap_state(struct drm_device *dev,
> > - struct drm_atomic_state *state);
> > +void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> > + bool stall);
> >
> > /* implementations for legacy interfaces */
> > int drm_atomic_helper_update_plane(struct drm_plane *plane,
>
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
next prev parent reply other threads:[~2016-05-30 15:09 UTC|newest]
Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-29 18:34 [PATCH 00/26] RFC: generic nonblocking support in the atomic helpers Daniel Vetter
2016-05-29 18:34 ` [PATCH 01/26] drm/atomic-helper: use for_each_*_in_state more Daniel Vetter
2016-05-30 12:17 ` Maarten Lankhorst
2016-05-30 15:02 ` Daniel Vetter
2016-05-29 18:34 ` [PATCH 02/26] drm/i915: Use drm_atomic_get_existing_plane_state Daniel Vetter
2016-05-30 13:10 ` [Intel-gfx] " Maarten Lankhorst
2016-05-29 18:35 ` [PATCH 03/26] drm/msm: Use for_each_*_in_state Daniel Vetter
2016-05-29 18:35 ` [PATCH 04/26] drm/rcar-du: " Daniel Vetter
2016-05-30 9:18 ` Laurent Pinchart
2016-05-30 9:58 ` Maarten Lankhorst
2016-05-30 14:54 ` [Intel-gfx] " Daniel Vetter
2016-06-02 22:54 ` Laurent Pinchart
2016-06-03 6:55 ` [Intel-gfx] " Daniel Vetter
2016-06-03 9:40 ` Laurent Pinchart
2016-06-03 9:45 ` Daniel Vetter
2016-06-03 9:50 ` Laurent Pinchart
2016-05-29 18:35 ` [PATCH 05/26] drm/vc4: Use for_each_plane_in_state Daniel Vetter
2016-05-29 18:35 ` [PATCH 06/26] drm/atomic: Add __drm_atomic_get_current_plane_state Daniel Vetter
2016-05-30 11:42 ` [Intel-gfx] " Maarten Lankhorst
2016-05-30 15:05 ` Daniel Vetter
2016-05-31 8:35 ` [Intel-gfx] " Maarten Lankhorst
2016-05-29 18:35 ` [PATCH 07/26] drm/exynos: Use for_each_crtc_in_state Daniel Vetter
2016-05-31 12:19 ` Inki Dae
2016-06-13 0:52 ` Inki Dae
2016-05-29 18:35 ` [PATCH 08/26] drm: Consolidate connector arrays in drm_atomic_state Daniel Vetter
2016-05-30 14:59 ` Ville Syrjälä
2016-05-30 15:13 ` Daniel Vetter
2016-05-30 15:25 ` [Intel-gfx] " Ville Syrjälä
2016-05-30 15:33 ` Daniel Vetter
2016-05-30 15:45 ` Ville Syrjälä
2016-05-30 15:48 ` [Intel-gfx] " Daniel Vetter
2016-05-29 18:35 ` [PATCH 09/26] drm: Consolidate plane " Daniel Vetter
2016-05-29 18:35 ` [PATCH 10/26] drm: Consolidate crtc " Daniel Vetter
2016-05-29 18:35 ` [PATCH 11/26] drm/fence: add fence to drm_pending_event Daniel Vetter
2016-05-29 18:35 ` [PATCH 12/26] drm/atomic-helper: Massage swap_state signature somewhat Daniel Vetter
2016-05-30 13:08 ` Maarten Lankhorst
2016-05-30 15:09 ` Daniel Vetter [this message]
2016-05-31 8:43 ` [Intel-gfx] " Maarten Lankhorst
2016-05-31 10:46 ` Daniel Vetter
2016-05-31 12:20 ` Maarten Lankhorst
2016-05-29 18:35 ` [PATCH 13/26] drm/arc: Nuke event_list Daniel Vetter
2016-05-29 18:35 ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 14/26] drm/arc: Actually bother with handling atomic events Daniel Vetter
2016-05-29 18:35 ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 15/26] drm/arc: Implement nonblocking commit correctly Daniel Vetter
2016-05-29 18:35 ` Daniel Vetter
2016-05-30 8:15 ` [Intel-gfx] " Maarten Lankhorst
2016-05-30 8:15 ` Maarten Lankhorst
2016-05-30 9:24 ` Daniel Vetter
2016-05-30 9:24 ` Daniel Vetter
2016-05-30 9:36 ` [Intel-gfx] " Maarten Lankhorst
2016-05-30 9:36 ` Maarten Lankhorst
2016-05-30 15:10 ` [Intel-gfx] " Daniel Vetter
2016-05-30 15:10 ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 16/26] drm/hdlcd: Use helper support for nonblocking commits Daniel Vetter
2016-05-31 11:02 ` Liviu Dudau
2016-05-31 11:07 ` Daniel Vetter
2016-05-31 13:13 ` Liviu Dudau
2016-05-29 18:35 ` [PATCH 17/26] drm/fsl-du: Implement some semblance of vblank event handling Daniel Vetter
2016-05-29 18:35 ` [PATCH 18/26] drm/hisilicon: " Daniel Vetter
2016-05-29 18:35 ` [PATCH 19/26] drm/sun4i: " Daniel Vetter
2016-06-01 16:18 ` Maxime Ripard
2016-06-01 22:02 ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 20/26] drm/atomic: kerneldoc for drm_atomic_crtc_needs_modeset Daniel Vetter
2016-05-29 18:35 ` [PATCH 21/26] drm/atomic-helper: nonblocking commit support Daniel Vetter
2016-05-30 8:01 ` [PATCH] " Daniel Vetter
2016-05-31 14:22 ` [Intel-gfx] " Maarten Lankhorst
2016-05-31 14:33 ` Daniel Vetter
2016-05-29 18:35 ` [PATCH 22/26] drm/i915: Signal drm events for atomic Daniel Vetter
2016-05-29 18:35 ` [PATCH 23/26] drm/i915: Roll out the helper nonblock tracking Daniel Vetter
2016-05-29 18:35 ` [PATCH 24/26] drm/rockchip: convert to helper nonblocking atomic commit Daniel Vetter
2016-05-29 18:35 ` [PATCH 25/26] drm/rockchip: Nuke pending event handling in preclose Daniel Vetter
2016-05-29 18:35 ` [PATCH 26/26] drm/virtio: Don't reinvent a flipping wheel Daniel Vetter
2016-05-31 13:40 ` ✗ Ro.CI.BAT: failure for RFC: generic nonblocking support in the atomic helpers Patchwork
2016-05-31 13:42 ` Patchwork
2016-05-31 14:29 ` Patchwork
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160530150907.GO27098@phenom.ffwll.local \
--to=daniel@ffwll.ch \
--cc=daniel.vetter@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=intel-gfx@lists.freedesktop.org \
--cc=maarten.lankhorst@linux.intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.