| Message ID | 1464546923-13439-13-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? > 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,
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
Op 30-05-16 om 17:09 schreef Daniel Vetter: > 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. If a driver supports depth > 1 it could skip the extra help waiter and just call swap_state. The helper should return -EINTR when needed. if (nonblock) { ret = drm_atomic_helper_wait_for_completion(state); if (ret) return ret; } drm_atomic_helper_swap_state..
On Tue, May 31, 2016 at 10:43:56AM +0200, Maarten Lankhorst wrote: > Op 30-05-16 om 17:09 schreef Daniel Vetter: > > 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. > If a driver supports depth > 1 it could skip the extra help waiter and just call swap_state. > The helper should return -EINTR when needed. > > if (nonblock) { > ret = drm_atomic_helper_wait_for_completion(state); > if (ret) > return ret; > } > > drm_atomic_helper_swap_state.. That's why there's this stall parameter ... Also, wait_for_completion(state) is not enough, there's 3 different completions. -Daniel
Op 31-05-16 om 12:46 schreef Daniel Vetter: > On Tue, May 31, 2016 at 10:43:56AM +0200, Maarten Lankhorst wrote: >> Op 30-05-16 om 17:09 schreef Daniel Vetter: >>> 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. >> If a driver supports depth > 1 it could skip the extra help waiter and just call swap_state. >> The helper should return -EINTR when needed. >> >> if (nonblock) { >> ret = drm_atomic_helper_wait_for_completion(state); >> if (ret) >> return ret; >> } >> >> drm_atomic_helper_swap_state.. > That's why there's this stall parameter ... Also, > wait_for_completion(state) is not enough, there's 3 different completions. Yeah but this is pseudocode. I don't think waiting belongs to swap_state, should be done before so we can still back out if wait fails.. ~Maarten
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,
- 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. 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(-)