The Allwinner backend has a commit bit in order to push the new
configuration to the actual hardware. We've always been using that bit.
However, we also should poll for that bit to clear, which we don't.
Accessing any register while a commit is pending is forbidden, and will for
example show a symptom of reading another, random, register.
If you get this during a read/modify/write cycle, this will result in
random register corruption, which are pretty bad.
This can be shown using the following program (while the backend is
active):
http://code.bulix.org/gdl44p-161437?raw
Fortunately for us, this is not really likely to happen. The window where
it can happen is quite thin, and it only happens during a modeset, since
it's the only time we commit some new configuration.
Unfortunately for us, QT does a ridiculous amount of modeset, and will just
hit that window after a while, creating a distorded (since the register we
read/modify/write also has scaling attributes) or with weird colors (since
it also has a invertion of red and blue components). And might fix itself
later on by reading another random register with proper values for these
fields.
Let me know what you think,
Maxime
Maxime Ripard (4):
drm/atomic: implement drm_atomic_helper_commit_tail for runtime_pm users
drm/sun4i: Use the runtime_pm commit_tail variant
drm/sun4i: engine: Add commit_poll function
drm/sun4i: make sure we don't have a commit pending
drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +----------
drivers/gpu/drm/sun4i/sun4i_backend.c | 14 +++++++-
drivers/gpu/drm/sun4i/sun4i_crtc.c | 3 +-
drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 6 +++-
drivers/gpu/drm/sun4i/sunxi_engine.h | 14 +++++++-
include/drm/drm_atomic_helper.h | 1 +-
9 files changed, 73 insertions(+), 78 deletions(-)
base-commit: a70e9c77d0f09e7d00b62a8d618a61b2dfc5d889
--
git-series 0.9.1
The current drm_atomic_helper_commit_tail helper works only if the CRTC is
accessible, and documents an alternative implementation that is supposed to
be used if that happens.
That implementation is then duplicated by some drivers. Instead of
documenting it, let's implement an helper that all the relevant users can
use directly.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +----------
include/drm/drm_atomic_helper.h | 1 +-
5 files changed, 36 insertions(+), 78 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 86d3093c6c9b..a288805078f9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1245,23 +1245,11 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
* @old_state: atomic state object with old state structures
*
* This is the default implementation for the
- * &drm_mode_config_helper_funcs.atomic_commit_tail hook.
+ * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers
+ * that do not support runtime_pm.
*
* Note that the default ordering of how the various stages are called is to
- * match the legacy modeset helper library closest. One peculiarity of that is
- * that it doesn't mesh well with runtime PM at all.
- *
- * For drivers supporting runtime PM the recommended sequence is instead ::
- *
- * drm_atomic_helper_commit_modeset_disables(dev, old_state);
- *
- * drm_atomic_helper_commit_modeset_enables(dev, old_state);
- *
- * drm_atomic_helper_commit_planes(dev, old_state,
- * DRM_PLANE_COMMIT_ACTIVE_ONLY);
- *
- * for committing the atomic update to hardware. See the kerneldoc entries for
- * these three functions for more details.
+ * match the legacy modeset helper library closest.
*/
void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
{
@@ -1281,6 +1269,35 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
}
EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
+/**
+ * drm_atomic_helper_commit_tail_runtime_pm - commit atomic update to hardware
+ * @old_state: new modeset state to be committed
+ *
+ * This is a variant of drm_atomic_helper_commit_tail suited for
+ * drivers that implement runtime_pm.
+ *
+ * Note that the default ordering of how the various stages are called is to
+ * match the legacy modeset helper library closest.
+ */
+void drm_atomic_helper_commit_tail_runtime_pm(struct drm_atomic_state *old_state)
+{
+ struct drm_device *dev = old_state->dev;
+
+ drm_atomic_helper_commit_modeset_disables(dev, old_state);
+
+ drm_atomic_helper_commit_modeset_enables(dev, old_state);
+
+ drm_atomic_helper_commit_planes(dev, old_state,
+ DRM_PLANE_COMMIT_ACTIVE_ONLY);
+
+ drm_atomic_helper_commit_hw_done(old_state);
+
+ drm_atomic_helper_wait_for_vblanks(dev, old_state);
+
+ drm_atomic_helper_cleanup_planes(dev, old_state);
+}
+EXPORT_SYMBOL(drm_atomic_helper_commit_tail_runtime_pm);
+
static void commit_tail(struct drm_atomic_state *old_state)
{
struct drm_device *dev = old_state->dev;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
index d48fd7c918f8..71f6873255f1 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
@@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
return exynos_fb->dma_addr[index];
}
-static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state)
-{
- struct drm_device *dev = state->dev;
-
- drm_atomic_helper_commit_modeset_disables(dev, state);
-
- drm_atomic_helper_commit_modeset_enables(dev, state);
-
- /*
- * Exynos can't update planes with CRTCs and encoders disabled,
- * its updates routines, specially for FIMD, requires the clocks
- * to be enabled. So it is necessary to handle the modeset operations
- * *before* the commit_planes() step, this way it will always
- * have the relevant clocks enabled to perform the update.
- */
- drm_atomic_helper_commit_planes(dev, state,
- DRM_PLANE_COMMIT_ACTIVE_ONLY);
-
- drm_atomic_helper_commit_hw_done(state);
-
- drm_atomic_helper_wait_for_vblanks(dev, state);
-
- drm_atomic_helper_cleanup_planes(dev, state);
-}
-
static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
- .atomic_commit_tail = exynos_drm_atomic_commit_tail,
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
};
static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index f4125c8ca902..cb0f6266fbae 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -249,28 +249,12 @@ static int rcar_du_atomic_check(struct drm_device *dev,
return rcar_du_atomic_check_planes(dev, state);
}
-static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
-{
- struct drm_device *dev = old_state->dev;
-
- /* Apply the atomic update. */
- drm_atomic_helper_commit_modeset_disables(dev, old_state);
- drm_atomic_helper_commit_modeset_enables(dev, old_state);
- drm_atomic_helper_commit_planes(dev, old_state,
- DRM_PLANE_COMMIT_ACTIVE_ONLY);
-
- drm_atomic_helper_commit_hw_done(old_state);
- drm_atomic_helper_wait_for_vblanks(dev, old_state);
-
- drm_atomic_helper_cleanup_planes(dev, old_state);
-}
-
/* -----------------------------------------------------------------------------
* Initialization
*/
static const struct drm_mode_config_helper_funcs rcar_du_mode_config_helper = {
- .atomic_commit_tail = rcar_du_atomic_commit_tail,
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
};
static const struct drm_mode_config_funcs rcar_du_mode_config_funcs = {
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 81f9548672b0..647745479c39 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev)
drm_fb_helper_hotplug_event(fb_helper);
}
-static void
-rockchip_atomic_commit_tail(struct drm_atomic_state *state)
-{
- struct drm_device *dev = state->dev;
-
- drm_atomic_helper_commit_modeset_disables(dev, state);
-
- drm_atomic_helper_commit_modeset_enables(dev, state);
-
- drm_atomic_helper_commit_planes(dev, state,
- DRM_PLANE_COMMIT_ACTIVE_ONLY);
-
- drm_atomic_helper_commit_hw_done(state);
-
- drm_atomic_helper_wait_for_vblanks(dev, state);
-
- drm_atomic_helper_cleanup_planes(dev, state);
-}
-
static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
- .atomic_commit_tail = rockchip_atomic_commit_tail,
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
};
static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
index f0a8678ae98e..9ff64c6d3043 100644
--- a/include/drm/drm_atomic_helper.h
+++ b/include/drm/drm_atomic_helper.h
@@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev,
int drm_atomic_helper_check(struct drm_device *dev,
struct drm_atomic_state *state);
void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
+void drm_atomic_helper_commit_tail_runtime_pm(struct drm_atomic_state *state);
int drm_atomic_helper_commit(struct drm_device *dev,
struct drm_atomic_state *state,
bool nonblock);
--
git-series 0.9.1
The backend (planes) commit can only happen when the TCON (CRTC) is
enabled, which is not guaranteed with the default commit_tail helper.
Let's use the runtime_pm version that is designed specifically to deal with
that case.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_framebuffer.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
index 9872e0fc03b0..189048d33a1d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_framebuffer.c
@@ -10,6 +10,7 @@
* the License, or (at your option) any later version.
*/
+#include <drm/drm_atomic.h>
#include <drm/drm_atomic_helper.h>
#include <drm/drm_fb_cma_helper.h>
#include <drm/drmP.h>
@@ -31,6 +32,10 @@ static const struct drm_mode_config_funcs sun4i_de_mode_config_funcs = {
.fb_create = drm_fb_cma_create,
};
+static struct drm_mode_config_helper_funcs sun4i_de_mode_config_helpers = {
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
+};
+
struct drm_fbdev_cma *sun4i_framebuffer_init(struct drm_device *drm)
{
drm_mode_config_reset(drm);
@@ -39,6 +44,7 @@ struct drm_fbdev_cma *sun4i_framebuffer_init(struct drm_device *drm)
drm->mode_config.max_height = 8192;
drm->mode_config.funcs = &sun4i_de_mode_config_funcs;
+ drm->mode_config.helper_private = &sun4i_de_mode_config_helpers;
return drm_fbdev_cma_init(drm, 32, drm->mode_config.num_connector);
}
--
git-series 0.9.1
In the earlier display engine designs, any register access while a commit
is pending is forbidden.
One of the symptoms is that reading a register will return another, random,
register value which can lead to register corruptions if we ever do a
read/modify/write cycle.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_backend.c | 14 ++++++++++++++
drivers/gpu/drm/sun4i/sun4i_crtc.c | 1 +
2 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index cf480218daa5..ce1f40f7511e 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -67,6 +67,19 @@ static void sun4i_backend_commit(struct sunxi_engine *engine)
SUN4I_BACKEND_REGBUFFCTL_LOADCTL);
}
+static int sun4i_backend_commit_poll(struct sunxi_engine *engine)
+{
+ u32 val;
+
+ DRM_DEBUG_DRIVER("Polling for the commit to end\n");
+
+ return regmap_read_poll_timeout(engine->regs,
+ SUN4I_BACKEND_REGBUFFCTL_REG,
+ val,
+ !(val & SUN4I_BACKEND_REGBUFFCTL_LOADCTL),
+ 100, 50000);
+}
+
void sun4i_backend_layer_enable(struct sun4i_backend *backend,
int layer, bool enable)
{
@@ -330,6 +343,7 @@ static int sun4i_backend_of_get_id(struct device_node *node)
static const struct sunxi_engine_ops sun4i_backend_engine_ops = {
.commit = sun4i_backend_commit,
+ .commit_poll = sun4i_backend_commit_poll,
.layers_init = sun4i_layers_init,
.apply_color_correction = sun4i_backend_apply_color_correction,
.disable_color_correction = sun4i_backend_disable_color_correction,
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index 2eba1d8639d8..31550493fa1d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -34,6 +34,7 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
struct drm_crtc_state *old_state)
{
struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc);
+ struct sunxi_engine *engine = scrtc->engine;
struct drm_device *dev = crtc->dev;
unsigned long flags;
--
git-series 0.9.1
On the earlier Allwinner chips, with the first iteration of the display
engine, the backend commit bit needs to be polled before making any
register access to the backend.
Add an operation for that, that will be called in atomic_begin in order to
be sure to have that bit cleared before we do any modifications.
Signed-off-by: Maxime Ripard <[email protected]>
---
drivers/gpu/drm/sun4i/sun4i_crtc.c | 2 ++
drivers/gpu/drm/sun4i/sunxi_engine.h | 14 ++++++++++++++
2 files changed, 16 insertions(+)
diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
index f8c70439d1e2..2eba1d8639d8 100644
--- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
+++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
@@ -45,6 +45,8 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
spin_unlock_irqrestore(&dev->event_lock, flags);
crtc->state->event = NULL;
}
+
+ WARN_ON(sunxi_engine_commit_poll(engine));
}
static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/sun4i/sunxi_engine.h b/drivers/gpu/drm/sun4i/sunxi_engine.h
index 4cb70ae65c79..6618e182613c 100644
--- a/drivers/gpu/drm/sun4i/sunxi_engine.h
+++ b/drivers/gpu/drm/sun4i/sunxi_engine.h
@@ -17,6 +17,7 @@ struct sunxi_engine;
struct sunxi_engine_ops {
void (*commit)(struct sunxi_engine *engine);
+ int (*commit_poll)(struct sunxi_engine *engine);
struct drm_plane **(*layers_init)(struct drm_device *drm,
struct sunxi_engine *engine);
@@ -55,6 +56,19 @@ sunxi_engine_commit(struct sunxi_engine *engine)
}
/**
+ * sunxi_engine_commit_poll() - wait for all changes to be committed
+ * @engine: pointer to the engine
+ */
+static inline int
+sunxi_engine_commit_poll(struct sunxi_engine *engine)
+{
+ if (engine->ops && engine->ops->commit_poll)
+ return engine->ops->commit_poll(engine);
+
+ return 0;
+}
+
+/**
* sunxi_engine_layers_init() - Create planes (layers) for the engine
* @drm: pointer to the drm_device for which planes will be created
* @engine: pointer to the engine
--
git-series 0.9.1
On Thu, Jul 13, 2017 at 04:41:13PM +0200, Maxime Ripard wrote:
> The current drm_atomic_helper_commit_tail helper works only if the CRTC is
> accessible, and documents an alternative implementation that is supposed to
> be used if that happens.
>
> That implementation is then duplicated by some drivers. Instead of
> documenting it, let's implement an helper that all the relevant users can
> use directly.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +----------
> include/drm/drm_atomic_helper.h | 1 +-
> 5 files changed, 36 insertions(+), 78 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 86d3093c6c9b..a288805078f9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1245,23 +1245,11 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_vblanks);
> * @old_state: atomic state object with old state structures
> *
> * This is the default implementation for the
> - * &drm_mode_config_helper_funcs.atomic_commit_tail hook.
> + * &drm_mode_config_helper_funcs.atomic_commit_tail hook, for drivers
> + * that do not support runtime_pm.
> *
> * Note that the default ordering of how the various stages are called is to
> - * match the legacy modeset helper library closest. One peculiarity of that is
> - * that it doesn't mesh well with runtime PM at all.
> - *
> - * For drivers supporting runtime PM the recommended sequence is instead ::
> - *
> - * drm_atomic_helper_commit_modeset_disables(dev, old_state);
> - *
> - * drm_atomic_helper_commit_modeset_enables(dev, old_state);
> - *
> - * drm_atomic_helper_commit_planes(dev, old_state,
> - * DRM_PLANE_COMMIT_ACTIVE_ONLY);
> - *
> - * for committing the atomic update to hardware. See the kerneldoc entries for
> - * these three functions for more details.
> + * match the legacy modeset helper library closest.
Please sprinkle links into both functions (and everywhere the old one was
referenced) to make this more discoverable, and explain when to use the
other variant.
> */
> void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> {
> @@ -1281,6 +1269,35 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
> }
> EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
>
> +/**
> + * drm_atomic_helper_commit_tail_runtime_pm - commit atomic update to hardware
> + * @old_state: new modeset state to be committed
> + *
> + * This is a variant of drm_atomic_helper_commit_tail suited for
> + * drivers that implement runtime_pm.
> + *
> + * Note that the default ordering of how the various stages are called is to
> + * match the legacy modeset helper library closest.
> + */
> +void drm_atomic_helper_commit_tail_runtime_pm(struct drm_atomic_state *old_state)
Bikeshed: I'd go with _rpm since this thing is already super long. But fee
free to ignore that.
With the docs polished I think this looks good.
-Daniel
> +{
> + struct drm_device *dev = old_state->dev;
> +
> + drm_atomic_helper_commit_modeset_disables(dev, old_state);
> +
> + drm_atomic_helper_commit_modeset_enables(dev, old_state);
> +
> + drm_atomic_helper_commit_planes(dev, old_state,
> + DRM_PLANE_COMMIT_ACTIVE_ONLY);
> +
> + drm_atomic_helper_commit_hw_done(old_state);
> +
> + drm_atomic_helper_wait_for_vblanks(dev, old_state);
> +
> + drm_atomic_helper_cleanup_planes(dev, old_state);
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_tail_runtime_pm);
> +
> static void commit_tail(struct drm_atomic_state *old_state)
> {
> struct drm_device *dev = old_state->dev;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> index d48fd7c918f8..71f6873255f1 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c
> @@ -187,33 +187,8 @@ dma_addr_t exynos_drm_fb_dma_addr(struct drm_framebuffer *fb, int index)
> return exynos_fb->dma_addr[index];
> }
>
> -static void exynos_drm_atomic_commit_tail(struct drm_atomic_state *state)
> -{
> - struct drm_device *dev = state->dev;
> -
> - drm_atomic_helper_commit_modeset_disables(dev, state);
> -
> - drm_atomic_helper_commit_modeset_enables(dev, state);
> -
> - /*
> - * Exynos can't update planes with CRTCs and encoders disabled,
> - * its updates routines, specially for FIMD, requires the clocks
> - * to be enabled. So it is necessary to handle the modeset operations
> - * *before* the commit_planes() step, this way it will always
> - * have the relevant clocks enabled to perform the update.
> - */
> - drm_atomic_helper_commit_planes(dev, state,
> - DRM_PLANE_COMMIT_ACTIVE_ONLY);
> -
> - drm_atomic_helper_commit_hw_done(state);
> -
> - drm_atomic_helper_wait_for_vblanks(dev, state);
> -
> - drm_atomic_helper_cleanup_planes(dev, state);
> -}
> -
> static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = {
> - .atomic_commit_tail = exynos_drm_atomic_commit_tail,
> + .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
> };
>
> static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index f4125c8ca902..cb0f6266fbae 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -249,28 +249,12 @@ static int rcar_du_atomic_check(struct drm_device *dev,
> return rcar_du_atomic_check_planes(dev, state);
> }
>
> -static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
> -{
> - struct drm_device *dev = old_state->dev;
> -
> - /* Apply the atomic update. */
> - drm_atomic_helper_commit_modeset_disables(dev, old_state);
> - drm_atomic_helper_commit_modeset_enables(dev, old_state);
> - drm_atomic_helper_commit_planes(dev, old_state,
> - DRM_PLANE_COMMIT_ACTIVE_ONLY);
> -
> - drm_atomic_helper_commit_hw_done(old_state);
> - drm_atomic_helper_wait_for_vblanks(dev, old_state);
> -
> - drm_atomic_helper_cleanup_planes(dev, old_state);
> -}
> -
> /* -----------------------------------------------------------------------------
> * Initialization
> */
>
> static const struct drm_mode_config_helper_funcs rcar_du_mode_config_helper = {
> - .atomic_commit_tail = rcar_du_atomic_commit_tail,
> + .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
> };
>
> static const struct drm_mode_config_funcs rcar_du_mode_config_funcs = {
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 81f9548672b0..647745479c39 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -174,27 +174,8 @@ static void rockchip_drm_output_poll_changed(struct drm_device *dev)
> drm_fb_helper_hotplug_event(fb_helper);
> }
>
> -static void
> -rockchip_atomic_commit_tail(struct drm_atomic_state *state)
> -{
> - struct drm_device *dev = state->dev;
> -
> - drm_atomic_helper_commit_modeset_disables(dev, state);
> -
> - drm_atomic_helper_commit_modeset_enables(dev, state);
> -
> - drm_atomic_helper_commit_planes(dev, state,
> - DRM_PLANE_COMMIT_ACTIVE_ONLY);
> -
> - drm_atomic_helper_commit_hw_done(state);
> -
> - drm_atomic_helper_wait_for_vblanks(dev, state);
> -
> - drm_atomic_helper_cleanup_planes(dev, state);
> -}
> -
> static const struct drm_mode_config_helper_funcs rockchip_mode_config_helpers = {
> - .atomic_commit_tail = rockchip_atomic_commit_tail,
> + .atomic_commit_tail = drm_atomic_helper_commit_tail_runtime_pm,
> };
>
> static const struct drm_mode_config_funcs rockchip_drm_mode_config_funcs = {
> diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h
> index f0a8678ae98e..9ff64c6d3043 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -41,6 +41,7 @@ int drm_atomic_helper_check_planes(struct drm_device *dev,
> int drm_atomic_helper_check(struct drm_device *dev,
> struct drm_atomic_state *state);
> void drm_atomic_helper_commit_tail(struct drm_atomic_state *state);
> +void drm_atomic_helper_commit_tail_runtime_pm(struct drm_atomic_state *state);
> int drm_atomic_helper_commit(struct drm_device *dev,
> struct drm_atomic_state *state,
> bool nonblock);
> --
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Maxime,
Thank you for the patch.
On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
> The current drm_atomic_helper_commit_tail helper works only if the CRTC is
> accessible, and documents an alternative implementation that is supposed to
> be used if that happens.
>
> That implementation is then duplicated by some drivers. Instead of
> documenting it, let's implement an helper that all the relevant users can
> use directly.
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to
avoid flicker" that changes the rcar-du implementation to the standard
disable/update planes/enable order, so I'd appreciate if you could drop the
rcar-du part of this patch to avoid conflicts.
This being said, the reason why I switched back from the "runtime PM" to the
"standard" order is probably of interest to you. Quoting the commit message,
> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> start to CRTC resume") changed the order of the plane commit and CRTC
> enable operations to accommodate the runtime PM requirements. However,
> this introduced corruption in the first displayed frame, as the CRTC is
> now enabled without any plane configured. On Gen2 hardware the first
> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> starting the display before the VSP compositor, which is more
> noticeable.
>
> To fix this, revert the order of the commit operations back, and handle
> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> helper operation handlers.
I believe that the "runtime PM" order is problematic in most drivers. The
problem usually goes unnoticed as most monitors will not even display the
first frame, and I assume many devices will just output it black, but it's an
issue nonetheless.
Note that my driver hasn't lost the "runtime PM" requirements, so I had to
support them with the "standard" order. The best way I've found was to runtime
resume in the one of .atomic_begin() and .enable() that is run first. Not very
neat, as similar code would be needed in most drivers. I wonder whether it
wouldn't be useful to add resume/suspend helper callbacks for the CRTC.
> drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 21 +----------
> include/drm/drm_atomic_helper.h | 1 +-
> 5 files changed, 36 insertions(+), 78 deletions(-)
--
Regards,
Laurent Pinchart
On Fri, Jul 14, 2017 at 1:43 AM, Laurent Pinchart
<[email protected]> wrote:
>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
>> start to CRTC resume") changed the order of the plane commit and CRTC
>> enable operations to accommodate the runtime PM requirements. However,
>> this introduced corruption in the first displayed frame, as the CRTC is
>> now enabled without any plane configured. On Gen2 hardware the first
>> frame will be black and likely unnoticed, but on Gen3 hardware we end up
>> starting the display before the VSP compositor, which is more
>> noticeable.
>>
>> To fix this, revert the order of the commit operations back, and handle
>> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
>> helper operation handlers.
>
> I believe that the "runtime PM" order is problematic in most drivers. The
> problem usually goes unnoticed as most monitors will not even display the
> first frame, and I assume many devices will just output it black, but it's an
> issue nonetheless.
>
> Note that my driver hasn't lost the "runtime PM" requirements, so I had to
> support them with the "standard" order. The best way I've found was to runtime
> resume in the one of .atomic_begin() and .enable() that is run first. Not very
> neat, as similar code would be needed in most drivers. I wonder whether it
> wouldn't be useful to add resume/suspend helper callbacks for the CRTC.
I discussed this with Laurent and explained that "first black frame"
is exactly what i915 does. I'd say given special customer requests we
don't yet have to bother with this in general ...
Wrt adding more hooks for rpm: I honestly don't like that, because
then someone else wants to add a hook for clocks, or for the sideband
and then we have a horror show of hooks where every driver uses just a
very small subset. The point of atomic helpers is to make them
modular, if one part doesn't fit, just redo in your driver. Goal
should be that shared parts are good for about 90% of
drivers/use-cases (maybe even less, there's sooooo many special
cases).
tldr; I still think the _runtime_pm variant is the recommended way to do this.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi,
On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
<[email protected]> wrote:
> In the earlier display engine designs, any register access while a commit
> is pending is forbidden.
>
> One of the symptoms is that reading a register will return another, random,
> register value which can lead to register corruptions if we ever do a
> read/modify/write cycle.
Alternatively, if changes to the backend (layers) are guaranteed to happen
while the CRTC is disabled (which seems to be the case after looking at
drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
could just turn on register auto-commit all the time and not deal with
this.
ChenYu
>
> Signed-off-by: Maxime Ripard <[email protected]>
> ---
> drivers/gpu/drm/sun4i/sun4i_backend.c | 14 ++++++++++++++
> drivers/gpu/drm/sun4i/sun4i_crtc.c | 1 +
> 2 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
> index cf480218daa5..ce1f40f7511e 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_backend.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
> @@ -67,6 +67,19 @@ static void sun4i_backend_commit(struct sunxi_engine *engine)
> SUN4I_BACKEND_REGBUFFCTL_LOADCTL);
> }
>
> +static int sun4i_backend_commit_poll(struct sunxi_engine *engine)
> +{
> + u32 val;
> +
> + DRM_DEBUG_DRIVER("Polling for the commit to end\n");
> +
> + return regmap_read_poll_timeout(engine->regs,
> + SUN4I_BACKEND_REGBUFFCTL_REG,
> + val,
> + !(val & SUN4I_BACKEND_REGBUFFCTL_LOADCTL),
> + 100, 50000);
> +}
> +
> void sun4i_backend_layer_enable(struct sun4i_backend *backend,
> int layer, bool enable)
> {
> @@ -330,6 +343,7 @@ static int sun4i_backend_of_get_id(struct device_node *node)
>
> static const struct sunxi_engine_ops sun4i_backend_engine_ops = {
> .commit = sun4i_backend_commit,
> + .commit_poll = sun4i_backend_commit_poll,
> .layers_init = sun4i_layers_init,
> .apply_color_correction = sun4i_backend_apply_color_correction,
> .disable_color_correction = sun4i_backend_disable_color_correction,
> diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> index 2eba1d8639d8..31550493fa1d 100644
> --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c
> +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c
> @@ -34,6 +34,7 @@ static void sun4i_crtc_atomic_begin(struct drm_crtc *crtc,
> struct drm_crtc_state *old_state)
> {
> struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc);
> + struct sunxi_engine *engine = scrtc->engine;
> struct drm_device *dev = crtc->dev;
> unsigned long flags;
>
> --
> git-series 0.9.1
On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
> Hi,
>
> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
> <[email protected]> wrote:
> > In the earlier display engine designs, any register access while a commit
> > is pending is forbidden.
> >
> > One of the symptoms is that reading a register will return another, random,
> > register value which can lead to register corruptions if we ever do a
> > read/modify/write cycle.
>
> Alternatively, if changes to the backend (layers) are guaranteed to happen
> while the CRTC is disabled (which seems to be the case after looking at
> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
> could just turn on register auto-commit all the time and not deal with
> this.
As far as I understand, it will only be the case if we need a new
modeset or we changed the active CRTC or connectors. But if you change
only the format, buffers or properties it won't be the case, and we'll
need to commit.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
<[email protected]> wrote:
> On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
>> Hi,
>>
>> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
>> <[email protected]> wrote:
>> > In the earlier display engine designs, any register access while a commit
>> > is pending is forbidden.
>> >
>> > One of the symptoms is that reading a register will return another, random,
>> > register value which can lead to register corruptions if we ever do a
>> > read/modify/write cycle.
>>
>> Alternatively, if changes to the backend (layers) are guaranteed to happen
>> while the CRTC is disabled (which seems to be the case after looking at
>> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
>> could just turn on register auto-commit all the time and not deal with
>> this.
>
> As far as I understand, it will only be the case if we need a new
> modeset or we changed the active CRTC or connectors. But if you change
> only the format, buffers or properties it won't be the case, and we'll
> need to commit.
So in other words, if someone were to use it for actual compositing and
moved the upper composited layer around, we would need commit support to be
safe.
Sounds more or less like something a video player would do.
Thanks
ChenYu
Hi Laurent,
On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> Thank you for the patch.
>
> On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
> > The current drm_atomic_helper_commit_tail helper works only if the CRTC is
> > accessible, and documents an alternative implementation that is supposed to
> > be used if that happens.
> >
> > That implementation is then duplicated by some drivers. Instead of
> > documenting it, let's implement an helper that all the relevant users can
> > use directly.
> >
> > Signed-off-by: Maxime Ripard <[email protected]>
> > ---
> > drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
> > drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
> > drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
>
> I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to
> avoid flicker" that changes the rcar-du implementation to the standard
> disable/update planes/enable order, so I'd appreciate if you could drop the
> rcar-du part of this patch to avoid conflicts.
I will.
> This being said, the reason why I switched back from the "runtime PM" to the
> "standard" order is probably of interest to you. Quoting the commit message,
>
> > Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > start to CRTC resume") changed the order of the plane commit and CRTC
> > enable operations to accommodate the runtime PM requirements. However,
> > this introduced corruption in the first displayed frame, as the CRTC is
> > now enabled without any plane configured. On Gen2 hardware the first
> > frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > starting the display before the VSP compositor, which is more
> > noticeable.
> >
> > To fix this, revert the order of the commit operations back, and handle
> > runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > helper operation handlers.
>
> I believe that the "runtime PM" order is problematic in most drivers. The
> problem usually goes unnoticed as most monitors will not even display the
> first frame, and I assume many devices will just output it black, but it's an
> issue nonetheless.
>
> Note that my driver hasn't lost the "runtime PM" requirements, so I had to
> support them with the "standard" order. The best way I've found was to runtime
> resume in the one of .atomic_begin() and .enable() that is run first. Not very
> neat, as similar code would be needed in most drivers. I wonder whether it
> wouldn't be useful to add resume/suspend helper callbacks for the CRTC.
I'm not sure it would apply. Our driver doesn't use runtime_pm at all,
but in order for the commits to happen, we need to have the CRTC
active, but it will remain powered up the whole time. I'm not sure if
we'll ever see such a frame.
But since this seems to be a pretty generic, maybe we should address
it in the helper itself?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote:
> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
> <[email protected]> wrote:
> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
> >> Hi,
> >>
> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
> >> <[email protected]> wrote:
> >> > In the earlier display engine designs, any register access while a commit
> >> > is pending is forbidden.
> >> >
> >> > One of the symptoms is that reading a register will return another, random,
> >> > register value which can lead to register corruptions if we ever do a
> >> > read/modify/write cycle.
> >>
> >> Alternatively, if changes to the backend (layers) are guaranteed to happen
> >> while the CRTC is disabled (which seems to be the case after looking at
> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
> >> could just turn on register auto-commit all the time and not deal with
> >> this.
> >
> > As far as I understand, it will only be the case if we need a new
> > modeset or we changed the active CRTC or connectors. But if you change
> > only the format, buffers or properties it won't be the case, and we'll
> > need to commit.
>
> So in other words, if someone were to use it for actual compositing and
> moved the upper composited layer around, we would need commit support to be
> safe.
>
> Sounds more or less like something a video player would do.
Not only that. A change of buffer will happen every frame or so, and
we can change the format whenever we want too (even if it's usually
going to be in sync with a new buffer). Changing a property can happen
any time too (like zpos for example).
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard
<[email protected]> wrote:
> On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote:
>> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
>> <[email protected]> wrote:
>> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
>> >> Hi,
>> >>
>> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
>> >> <[email protected]> wrote:
>> >> > In the earlier display engine designs, any register access while a commit
>> >> > is pending is forbidden.
>> >> >
>> >> > One of the symptoms is that reading a register will return another, random,
>> >> > register value which can lead to register corruptions if we ever do a
>> >> > read/modify/write cycle.
>> >>
>> >> Alternatively, if changes to the backend (layers) are guaranteed to happen
>> >> while the CRTC is disabled (which seems to be the case after looking at
>> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
>> >> could just turn on register auto-commit all the time and not deal with
>> >> this.
>> >
>> > As far as I understand, it will only be the case if we need a new
>> > modeset or we changed the active CRTC or connectors. But if you change
>> > only the format, buffers or properties it won't be the case, and we'll
>> > need to commit.
>>
>> So in other words, if someone were to use it for actual compositing and
>> moved the upper composited layer around, we would need commit support to be
>> safe.
>>
>> Sounds more or less like something a video player would do.
>
> Not only that. A change of buffer will happen every frame or so, and
> we can change the format whenever we want too (even if it's usually
> going to be in sync with a new buffer). Changing a property can happen
> any time too (like zpos for example).
You can upgrade any property change to an atomic modeset by e.g.
setting connector->mode_changed (and then making sure to call
check_modeset() helper again perhaps). This is for cases where your hw
can't handle a property change within 1 vblank. The default is just
the solution for most common hw.
The other way round works too, you can clear these flags in your
atomic_check callbacks. But that requires a bit more care (to make
sure you never clear it when there's something else also changing that
still needs a full modeset sequence to commit to hw).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Maxime,
On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote:
> On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote:
> > On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
> >> The current drm_atomic_helper_commit_tail helper works only if the CRTC
> >> is accessible, and documents an alternative implementation that is
> >> supposed to be used if that happens.
> >>
> >> That implementation is then duplicated by some drivers. Instead of
> >> documenting it, let's implement an helper that all the relevant users
> >> can use directly.
> >>
> >> Signed-off-by: Maxime Ripard <[email protected]>
> >> ---
> >>
> >> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
> >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
> >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
> >
> > I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to
> > avoid flicker" that changes the rcar-du implementation to the standard
> > disable/update planes/enable order, so I'd appreciate if you could drop
> > the rcar-du part of this patch to avoid conflicts.
>
> I will.
>
> > This being said, the reason why I switched back from the "runtime PM" to
> > the "standard" order is probably of interest to you. Quoting the commit
> > message,
> >
> >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> >> start to CRTC resume") changed the order of the plane commit and CRTC
> >> enable operations to accommodate the runtime PM requirements. However,
> >> this introduced corruption in the first displayed frame, as the CRTC is
> >> now enabled without any plane configured. On Gen2 hardware the first
> >> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> >> starting the display before the VSP compositor, which is more
> >> noticeable.
> >>
> >> To fix this, revert the order of the commit operations back, and handle
> >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> >> helper operation handlers.
> >
> > I believe that the "runtime PM" order is problematic in most drivers. The
> > problem usually goes unnoticed as most monitors will not even display the
> > first frame, and I assume many devices will just output it black, but it's
> > an issue nonetheless.
> >
> > Note that my driver hasn't lost the "runtime PM" requirements, so I had to
> > support them with the "standard" order. The best way I've found was to
> > runtime resume in the one of .atomic_begin() and .enable() that is run
> > first. Not very neat, as similar code would be needed in most drivers. I
> > wonder whether it wouldn't be useful to add resume/suspend helper
> > callbacks for the CRTC.
>
> I'm not sure it would apply. Our driver doesn't use runtime_pm at all,
> but in order for the commits to happen, we need to have the CRTC
> active, but it will remain powered up the whole time. I'm not sure if
> we'll ever see such a frame.
>
> But since this seems to be a pretty generic, maybe we should address
> it in the helper itself?
I think that would make sense.
There are a few options that result in too many combinations for separate
commit tail helpers to be provided in my opinion:
- disable/enable/planes vs. disable/planes/enable
- DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs
- drm_atomic_helper_wait_for_vblanks vs drm_atomic_helper_wait_for_flip_done
Maybe we could add a few CRTC commit helper flags along the line of
DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to store
them, and have drm_atomic_helper_commit_tail() use those flags to control the
sequence of operations.
--
Regards,
Laurent Pinchart
On Tue, Jul 18, 2017 at 01:14:12PM +0300, Laurent Pinchart wrote:
> Hi Maxime,
>
> On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote:
> > On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote:
> > > On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
> > >> The current drm_atomic_helper_commit_tail helper works only if the CRTC
> > >> is accessible, and documents an alternative implementation that is
> > >> supposed to be used if that happens.
> > >>
> > >> That implementation is then duplicated by some drivers. Instead of
> > >> documenting it, let's implement an helper that all the relevant users
> > >> can use directly.
> > >>
> > >> Signed-off-by: Maxime Ripard <[email protected]>
> > >> ---
> > >>
> > >> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++++--------
> > >> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
> > >> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
> > >
> > > I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling CRTC to
> > > avoid flicker" that changes the rcar-du implementation to the standard
> > > disable/update planes/enable order, so I'd appreciate if you could drop
> > > the rcar-du part of this patch to avoid conflicts.
> >
> > I will.
> >
> > > This being said, the reason why I switched back from the "runtime PM" to
> > > the "standard" order is probably of interest to you. Quoting the commit
> > > message,
> > >
> > >> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> > >> start to CRTC resume") changed the order of the plane commit and CRTC
> > >> enable operations to accommodate the runtime PM requirements. However,
> > >> this introduced corruption in the first displayed frame, as the CRTC is
> > >> now enabled without any plane configured. On Gen2 hardware the first
> > >> frame will be black and likely unnoticed, but on Gen3 hardware we end up
> > >> starting the display before the VSP compositor, which is more
> > >> noticeable.
> > >>
> > >> To fix this, revert the order of the commit operations back, and handle
> > >> runtime PM requirements in the CRTC .atomic_begin() and .atomic_enable()
> > >> helper operation handlers.
> > >
> > > I believe that the "runtime PM" order is problematic in most drivers. The
> > > problem usually goes unnoticed as most monitors will not even display the
> > > first frame, and I assume many devices will just output it black, but it's
> > > an issue nonetheless.
> > >
> > > Note that my driver hasn't lost the "runtime PM" requirements, so I had to
> > > support them with the "standard" order. The best way I've found was to
> > > runtime resume in the one of .atomic_begin() and .enable() that is run
> > > first. Not very neat, as similar code would be needed in most drivers. I
> > > wonder whether it wouldn't be useful to add resume/suspend helper
> > > callbacks for the CRTC.
> >
> > I'm not sure it would apply. Our driver doesn't use runtime_pm at all,
> > but in order for the commits to happen, we need to have the CRTC
> > active, but it will remain powered up the whole time. I'm not sure if
> > we'll ever see such a frame.
> >
> > But since this seems to be a pretty generic, maybe we should address
> > it in the helper itself?
>
> I think that would make sense.
>
> There are a few options that result in too many combinations for separate
> commit tail helpers to be provided in my opinion:
>
> - disable/enable/planes vs. disable/planes/enable
> - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs
> - drm_atomic_helper_wait_for_vblanks vs drm_atomic_helper_wait_for_flip_done
>
> Maybe we could add a few CRTC commit helper flags along the line of
> DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to store
> them, and have drm_atomic_helper_commit_tail() use those flags to control the
> sequence of operations.
Why not write your own? Yes it's a bit of copypaste, but imo that's really
not horrible. I'm already not happy with the flags for commit_planes
because the docs for them are not great and it's hard to know when to use
them and when not to.
->commit_tail was specifically done to allow drivers to overwrite the hw
commit stage without having to reinvent all the other commit tracking. I
expect most non-simple drivers to have their own commit_tail function.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Daniel,
On Tuesday 18 Jul 2017 14:08:39 Daniel Vetter wrote:
> On Tue, Jul 18, 2017 at 01:14:12PM +0300, Laurent Pinchart wrote:
> > On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote:
> >> On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote:
> >>> On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
> >>>> The current drm_atomic_helper_commit_tail helper works only if the
> >>>> CRTC is accessible, and documents an alternative implementation that
> >>>> is supposed to be used if that happens.
> >>>>
> >>>> That implementation is then duplicated by some drivers. Instead of
> >>>> documenting it, let's implement an helper that all the relevant users
> >>>> can use directly.
> >>>>
> >>>> Signed-off-by: Maxime Ripard <[email protected]>
> >>>> ---
> >>>>
> >>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++--------
> >>>> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
> >>>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
> >>>
> >>> I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling
> >>> CRTC to avoid flicker" that changes the rcar-du implementation to the
> >>> standard disable/update planes/enable order, so I'd appreciate if you
> >>> could drop the rcar-du part of this patch to avoid conflicts.
> >>
> >> I will.
> >>
> >>> This being said, the reason why I switched back from the "runtime PM"
> >>> to the "standard" order is probably of interest to you. Quoting the
> >>> commit message,
> >>>
> >>>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
> >>>> start to CRTC resume") changed the order of the plane commit and CRTC
> >>>> enable operations to accommodate the runtime PM requirements.
> >>>> However, this introduced corruption in the first displayed frame, as
> >>>> the CRTC is now enabled without any plane configured. On Gen2
> >>>> hardware the first frame will be black and likely unnoticed, but on
> >>>> Gen3 hardware we end up starting the display before the VSP
> >>>> compositor, which is more noticeable.
> >>>>
> >>>> To fix this, revert the order of the commit operations back, and
> >>>> handle runtime PM requirements in the CRTC .atomic_begin() and
> >>>> .atomic_enable() helper operation handlers.
> >>>
> >>> I believe that the "runtime PM" order is problematic in most drivers.
> >>> The problem usually goes unnoticed as most monitors will not even
> >>> display the first frame, and I assume many devices will just output it
> >>> black, but it's an issue nonetheless.
> >>>
> >>> Note that my driver hasn't lost the "runtime PM" requirements, so I
> >>> had to support them with the "standard" order. The best way I've found
> >>> was to runtime resume in the one of .atomic_begin() and .enable() that
> >>> is run first. Not very neat, as similar code would be needed in most
> >>> drivers. I wonder whether it wouldn't be useful to add resume/suspend
> >>> helper callbacks for the CRTC.
> >>
> >> I'm not sure it would apply. Our driver doesn't use runtime_pm at all,
> >> but in order for the commits to happen, we need to have the CRTC
> >> active, but it will remain powered up the whole time. I'm not sure if
> >> we'll ever see such a frame.
> >>
> >> But since this seems to be a pretty generic, maybe we should address
> >> it in the helper itself?
> >
> > I think that would make sense.
> >
> > There are a few options that result in too many combinations for separate
> > commit tail helpers to be provided in my opinion:
> >
> > - disable/enable/planes vs. disable/planes/enable
> > - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs
> > - drm_atomic_helper_wait_for_vblanks vs
> > drm_atomic_helper_wait_for_flip_done
> >
> > Maybe we could add a few CRTC commit helper flags along the line of
> > DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to
> > store them, and have drm_atomic_helper_commit_tail() use those flags to
> > control the sequence of operations.
>
> Why not write your own? Yes it's a bit of copypaste, but imo that's really
> not horrible.
I don't find it horrible either, it's not too much code. The question was more
about which version(s) to consider standard enough to provide a core helper
for.
What bothers me a bit more with the copy&paste implementations isn't so much
the commit tail handling itself, but the consequences it has on the rest of
the driver. Drivers pick the order they want based on their requirements, and
that order then leads to different race conditions between the drivers. We
don't document the potential issues there, so new drivers (and for that
matter, possibly existing ones) will likely have bugs if the author is not
aware of the subtle issues related to the particular operation order they
happen to use.
> I'm already not happy with the flags for commit_planes because the docs for
> them are not great and it's hard to know when to use them and when not to.
>
> ->commit_tail was specifically done to allow drivers to overwrite the hw
> commit stage without having to reinvent all the other commit tracking. I
> expect most non-simple drivers to have their own commit_tail function.
Maybe that should be all of them instead of most of them ?
--
Regards,
Laurent Pinchart
On Tue, Jul 18, 2017 at 2:47 PM, Laurent Pinchart
<[email protected]> wrote:
> On Tuesday 18 Jul 2017 14:08:39 Daniel Vetter wrote:
>> On Tue, Jul 18, 2017 at 01:14:12PM +0300, Laurent Pinchart wrote:
>> > On Tuesday 18 Jul 2017 09:05:22 Maxime Ripard wrote:
>> >> On Fri, Jul 14, 2017 at 02:43:12AM +0300, Laurent Pinchart wrote:
>> >>> On Thursday 13 Jul 2017 16:41:13 Maxime Ripard wrote:
>> >>>> The current drm_atomic_helper_commit_tail helper works only if the
>> >>>> CRTC is accessible, and documents an alternative implementation that
>> >>>> is supposed to be used if that happens.
>> >>>>
>> >>>> That implementation is then duplicated by some drivers. Instead of
>> >>>> documenting it, let's implement an helper that all the relevant users
>> >>>> can use directly.
>> >>>>
>> >>>> Signed-off-by: Maxime Ripard <[email protected]>
>> >>>> ---
>> >>>>
>> >>>> drivers/gpu/drm/drm_atomic_helper.c | 47 +++++++++++++--------
>> >>>> drivers/gpu/drm/exynos/exynos_drm_fb.c | 27 +-------------
>> >>>> drivers/gpu/drm/rcar-du/rcar_du_kms.c | 18 +---------
>> >>>
>> >>> I've submitted "[PATCH] drm: rcar-du: Setup planes before enabling
>> >>> CRTC to avoid flicker" that changes the rcar-du implementation to the
>> >>> standard disable/update planes/enable order, so I'd appreciate if you
>> >>> could drop the rcar-du part of this patch to avoid conflicts.
>> >>
>> >> I will.
>> >>
>> >>> This being said, the reason why I switched back from the "runtime PM"
>> >>> to the "standard" order is probably of interest to you. Quoting the
>> >>> commit message,
>> >>>
>> >>>> Commit 52055bafa1ff ("drm: rcar-du: Move plane commit code from CRTC
>> >>>> start to CRTC resume") changed the order of the plane commit and CRTC
>> >>>> enable operations to accommodate the runtime PM requirements.
>> >>>> However, this introduced corruption in the first displayed frame, as
>> >>>> the CRTC is now enabled without any plane configured. On Gen2
>> >>>> hardware the first frame will be black and likely unnoticed, but on
>> >>>> Gen3 hardware we end up starting the display before the VSP
>> >>>> compositor, which is more noticeable.
>> >>>>
>> >>>> To fix this, revert the order of the commit operations back, and
>> >>>> handle runtime PM requirements in the CRTC .atomic_begin() and
>> >>>> .atomic_enable() helper operation handlers.
>> >>>
>> >>> I believe that the "runtime PM" order is problematic in most drivers.
>> >>> The problem usually goes unnoticed as most monitors will not even
>> >>> display the first frame, and I assume many devices will just output it
>> >>> black, but it's an issue nonetheless.
>> >>>
>> >>> Note that my driver hasn't lost the "runtime PM" requirements, so I
>> >>> had to support them with the "standard" order. The best way I've found
>> >>> was to runtime resume in the one of .atomic_begin() and .enable() that
>> >>> is run first. Not very neat, as similar code would be needed in most
>> >>> drivers. I wonder whether it wouldn't be useful to add resume/suspend
>> >>> helper callbacks for the CRTC.
>> >>
>> >> I'm not sure it would apply. Our driver doesn't use runtime_pm at all,
>> >> but in order for the commits to happen, we need to have the CRTC
>> >> active, but it will remain powered up the whole time. I'm not sure if
>> >> we'll ever see such a frame.
>> >>
>> >> But since this seems to be a pretty generic, maybe we should address
>> >> it in the helper itself?
>> >
>> > I think that would make sense.
>> >
>> > There are a few options that result in too many combinations for separate
>> > commit tail helpers to be provided in my opinion:
>> >
>> > - disable/enable/planes vs. disable/planes/enable
>> > - DRM_PLANE_COMMIT_ACTIVE_ONLY vs. all CRTCs
>> > - drm_atomic_helper_wait_for_vblanks vs
>> > drm_atomic_helper_wait_for_flip_done
>> >
>> > Maybe we could add a few CRTC commit helper flags along the line of
>> > DRM_PLANE_COMMIT_ACTIVE_ONLY, add a field to the drm_crtc structure to
>> > store them, and have drm_atomic_helper_commit_tail() use those flags to
>> > control the sequence of operations.
>>
>> Why not write your own? Yes it's a bit of copypaste, but imo that's really
>> not horrible.
>
> I don't find it horrible either, it's not too much code. The question was more
> about which version(s) to consider standard enough to provide a core helper
> for.
>
> What bothers me a bit more with the copy&paste implementations isn't so much
> the commit tail handling itself, but the consequences it has on the rest of
> the driver. Drivers pick the order they want based on their requirements, and
> that order then leads to different race conditions between the drivers. We
> don't document the potential issues there, so new drivers (and for that
> matter, possibly existing ones) will likely have bugs if the author is not
> aware of the subtle issues related to the particular operation order they
> happen to use.
Imo the answer to that is implementing CRC support in your driver and
running igt. That checks whether you get those race conditions right,
at least everywhere where it's well-defined across drivers.
>> I'm already not happy with the flags for commit_planes because the docs for
>> them are not great and it's hard to know when to use them and when not to.
>>
>> ->commit_tail was specifically done to allow drivers to overwrite the hw
>> commit stage without having to reinvent all the other commit tracking. I
>> expect most non-simple drivers to have their own commit_tail function.
>
> Maybe that should be all of them instead of most of them ?
Valid suggestion - the default reflects the legacy crtc helpers, for
easier transition, and I think we've run out of drivers to transition.
Most of the existing drivers are probably better if you rewrite them
in e.g. the simple display pipe helpers.
Imo we could nuke the default commit_tail and replace it purely with
kernel-code, together with the transitional plane/crtc helpers. Otoh
it's still a useful template, to know what all should be in there ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
Hi Daniel,
On Tue, Jul 18, 2017 at 09:35:03AM +0200, Daniel Vetter wrote:
> On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard
> <[email protected]> wrote:
> > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote:
> >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
> >> <[email protected]> wrote:
> >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
> >> >> Hi,
> >> >>
> >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
> >> >> <[email protected]> wrote:
> >> >> > In the earlier display engine designs, any register access while a commit
> >> >> > is pending is forbidden.
> >> >> >
> >> >> > One of the symptoms is that reading a register will return another, random,
> >> >> > register value which can lead to register corruptions if we ever do a
> >> >> > read/modify/write cycle.
> >> >>
> >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen
> >> >> while the CRTC is disabled (which seems to be the case after looking at
> >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
> >> >> could just turn on register auto-commit all the time and not deal with
> >> >> this.
> >> >
> >> > As far as I understand, it will only be the case if we need a new
> >> > modeset or we changed the active CRTC or connectors. But if you change
> >> > only the format, buffers or properties it won't be the case, and we'll
> >> > need to commit.
> >>
> >> So in other words, if someone were to use it for actual compositing and
> >> moved the upper composited layer around, we would need commit support to be
> >> safe.
> >>
> >> Sounds more or less like something a video player would do.
> >
> > Not only that. A change of buffer will happen every frame or so, and
> > we can change the format whenever we want too (even if it's usually
> > going to be in sync with a new buffer). Changing a property can happen
> > any time too (like zpos for example).
>
> You can upgrade any property change to an atomic modeset by e.g.
> setting connector->mode_changed (and then making sure to call
> check_modeset() helper again perhaps). This is for cases where your hw
> can't handle a property change within 1 vblank. The default is just
> the solution for most common hw.
>
> The other way round works too, you can clear these flags in your
> atomic_check callbacks. But that requires a bit more care (to make
> sure you never clear it when there's something else also changing that
> still needs a full modeset sequence to commit to hw).
Hmm, that's good to know, but that would imply disabling the CRTC each
time we change even a small property, with all the visual artifacts it
might imply, right?
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On Thu, Jul 20, 2017 at 11:53:39AM +0200, Maxime Ripard wrote:
> Hi Daniel,
>
> On Tue, Jul 18, 2017 at 09:35:03AM +0200, Daniel Vetter wrote:
> > On Tue, Jul 18, 2017 at 9:07 AM, Maxime Ripard
> > <[email protected]> wrote:
> > > On Mon, Jul 17, 2017 at 02:57:19PM +0800, Chen-Yu Tsai wrote:
> > >> On Mon, Jul 17, 2017 at 2:55 PM, Maxime Ripard
> > >> <[email protected]> wrote:
> > >> > On Fri, Jul 14, 2017 at 04:56:01PM +0800, Chen-Yu Tsai wrote:
> > >> >> Hi,
> > >> >>
> > >> >> On Thu, Jul 13, 2017 at 10:41 PM, Maxime Ripard
> > >> >> <[email protected]> wrote:
> > >> >> > In the earlier display engine designs, any register access while a commit
> > >> >> > is pending is forbidden.
> > >> >> >
> > >> >> > One of the symptoms is that reading a register will return another, random,
> > >> >> > register value which can lead to register corruptions if we ever do a
> > >> >> > read/modify/write cycle.
> > >> >>
> > >> >> Alternatively, if changes to the backend (layers) are guaranteed to happen
> > >> >> while the CRTC is disabled (which seems to be the case after looking at
> > >> >> drm_atomic_helper_commit_planes and drm_atomic_helper_commit_tail), we
> > >> >> could just turn on register auto-commit all the time and not deal with
> > >> >> this.
> > >> >
> > >> > As far as I understand, it will only be the case if we need a new
> > >> > modeset or we changed the active CRTC or connectors. But if you change
> > >> > only the format, buffers or properties it won't be the case, and we'll
> > >> > need to commit.
> > >>
> > >> So in other words, if someone were to use it for actual compositing and
> > >> moved the upper composited layer around, we would need commit support to be
> > >> safe.
> > >>
> > >> Sounds more or less like something a video player would do.
> > >
> > > Not only that. A change of buffer will happen every frame or so, and
> > > we can change the format whenever we want too (even if it's usually
> > > going to be in sync with a new buffer). Changing a property can happen
> > > any time too (like zpos for example).
> >
> > You can upgrade any property change to an atomic modeset by e.g.
> > setting connector->mode_changed (and then making sure to call
> > check_modeset() helper again perhaps). This is for cases where your hw
> > can't handle a property change within 1 vblank. The default is just
> > the solution for most common hw.
> >
> > The other way round works too, you can clear these flags in your
> > atomic_check callbacks. But that requires a bit more care (to make
> > sure you never clear it when there's something else also changing that
> > still needs a full modeset sequence to commit to hw).
>
> Hmm, that's good to know, but that would imply disabling the CRTC each
> time we change even a small property, with all the visual artifacts it
> might imply, right?
This isn't black&white, you only need to set this when needed. Of course
that means you need to have code (and hopefully testcases) to make sure
you only set it when needed. And userspace can ask the driver whether a
given change requires a full modeset or not and then decided whether it
wants to do that switch or not.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch