Hi,
Changes since v1: - normalize_zpos flag moved to drm_mode_config - Added comment to note the side effect of normalization and updated the comment for normalized_zpos in the header file as well. - Added Acked-by from Daniel to patch 2-6 but not for patch 1 as I'm not sure if the comments I have added matches with what is expected to be.
The first patch is adding a flag to drm_device that drivers can set if they want the zpos to be normalized.
Then convert exynos, tegra, sti and rcar-du to use this flag instead of re-implementing the drm_atomic_helper_check() locally just to add the call to drm_atomic_normalize_zpos().
The last patch is moving omapdrm to use the zpos normalization as well to comply with the UAPI documentation regarding to zpos.
Laurent's note in an earlier thread: https://marc.info/?l=dri-devel&m=151567355225029&w=2
"The problem is that zpos normalization requires accessing the state of all enabled planes for a CRTC in order to compute (and store) the normalized zpos values. This thus forces all planes to be added to the commit state, even when the commit doesn't touch the zpos property. I assume this caused issues (possibly performance issues) in drivers that then performed hardware setup of all planes as a result."
can be addressed later in the core for all users of drm_atomic_normalize_zpos()
Regards, Peter --- Peter Ujfalusi (6): drm: Add drm_mode_config->normalize_zpos boolean drm/exynos: Let core take care of normalizing the zpos drm/tegra: Let core take care of normalizing the zpos drm/sti: Let core take care of normalizing the zpos drm: rcar-du: Let core take care of normalizing the zpos drm/omap: Use normalized zpos for plane placement
drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ drivers/gpu/drm/exynos/exynos_drm_drv.c | 20 -------------------- drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 - drivers/gpu/drm/exynos/exynos_drm_fb.c | 4 +++- drivers/gpu/drm/omapdrm/omap_drv.c | 3 +++ drivers/gpu/drm/omapdrm/omap_plane.c | 2 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 ++--------- drivers/gpu/drm/sti/sti_drv.c | 24 +++--------------------- drivers/gpu/drm/tegra/drm.c | 27 +++------------------------ include/drm/drm_mode_config.h | 8 ++++++++ include/drm/drm_plane.h | 4 ++-- 11 files changed, 36 insertions(+), 79 deletions(-)
Instead of drivers duplicating the drm_atomic_helper_check() code to be able to normalize the zpos they can use the normalize_zpos flag to let the drm core to do it.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++ include/drm/drm_mode_config.h | 8 ++++++++ include/drm/drm_plane.h | 4 ++-- 3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ab4032167094..0f6a4949e6dc 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -873,6 +873,11 @@ EXPORT_SYMBOL(drm_atomic_helper_check_planes); * functions depend upon an updated adjusted_mode.clock to e.g. properly compute * watermarks. * + * Note that zpos normalization will add all enable planes to the state which + * might not desired for some drivers. + * For example enable/disable of a cursor plane which have fixed zpos value + * would trigger all other enabled planes to be forced to the state change. + * * RETURNS: * Zero for success or -errno */ @@ -885,6 +890,12 @@ int drm_atomic_helper_check(struct drm_device *dev, if (ret) return ret;
+ if (dev->mode_config.normalize_zpos) { + ret = drm_atomic_normalize_zpos(dev, state); + if (ret) + return ret; + } + ret = drm_atomic_helper_check_planes(dev, state); if (ret) return ret; diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 2cb6f02df64a..987ab63ae037 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -792,6 +792,14 @@ struct drm_mode_config { /* cursor size */ uint32_t cursor_width, cursor_height;
+ /** + * @normalize_zpos: + * + * If true the drm core will call drm_atomic_normalize_zpos() as part of + * atomic mode checking from drm_atomic_helper_check() + */ + bool normalize_zpos; + /** * @suspend_state: * diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..2c0adb124e0f 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -50,8 +50,8 @@ struct drm_modeset_acquire_ctx; * plane with a lower ID. * @normalized_zpos: normalized value of zpos: unique, range from 0 to N-1 * where N is the number of active planes for given crtc. Note that - * the driver must call drm_atomic_normalize_zpos() to update this before - * it can be trusted. + * the driver must set drm_mode_config.normalize_zpos or call + * drm_atomic_normalize_zpos() to update this before it can be trusted. * @src: clipped source coordinates of the plane (in 16.16) * @dst: clipped destination coordinates of the plane * @state: backpointer to global drm_atomic_state
Instead of re-implementing the drm_atomic_helper_check() locally with just adding drm_atomic_normalize_zpos() into it, set the drm_device->normalize_zpos.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Inki Dae inki.dae@samsung.com CC: Joonyoung Shim jy0922.shim@samsung.com CC: Seung-Woo Kim sw0312.kim@samsung.com CC: Kyungmin Park kyungmin.park@samsung.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/exynos/exynos_drm_drv.c | 20 -------------------- drivers/gpu/drm/exynos/exynos_drm_drv.h | 1 - drivers/gpu/drm/exynos/exynos_drm_fb.c | 4 +++- 3 files changed, 3 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c index b96bd5a781b2..8791373a0f24 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c @@ -38,26 +38,6 @@ #define DRIVER_MAJOR 1 #define DRIVER_MINOR 0
-int exynos_atomic_check(struct drm_device *dev, - struct drm_atomic_state *state) -{ - int ret; - - ret = drm_atomic_helper_check_modeset(dev, state); - if (ret) - return ret; - - ret = drm_atomic_normalize_zpos(dev, state); - if (ret) - return ret; - - ret = drm_atomic_helper_check_planes(dev, state); - if (ret) - return ret; - - return ret; -} - static int exynos_drm_open(struct drm_device *dev, struct drm_file *file) { struct drm_exynos_file_private *file_priv; diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h index 589d465a7f88..4585d9e00c87 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h @@ -276,7 +276,6 @@ static inline int exynos_dpi_bind(struct drm_device *dev,
int exynos_atomic_commit(struct drm_device *dev, struct drm_atomic_state *state, bool nonblock); -int exynos_atomic_check(struct drm_device *dev, struct drm_atomic_state *state);
extern struct platform_driver fimd_driver; diff --git a/drivers/gpu/drm/exynos/exynos_drm_fb.c b/drivers/gpu/drm/exynos/exynos_drm_fb.c index 0faaf829f5bf..2379d732da67 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_fb.c +++ b/drivers/gpu/drm/exynos/exynos_drm_fb.c @@ -206,7 +206,7 @@ static struct drm_mode_config_helper_funcs exynos_drm_mode_config_helpers = { static const struct drm_mode_config_funcs exynos_drm_mode_config_funcs = { .fb_create = exynos_user_fb_create, .output_poll_changed = drm_fb_helper_output_poll_changed, - .atomic_check = exynos_atomic_check, + .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, };
@@ -227,4 +227,6 @@ void exynos_drm_mode_config_init(struct drm_device *dev) dev->mode_config.helper_private = &exynos_drm_mode_config_helpers;
dev->mode_config.allow_fb_modifiers = true; + + dev->mode_config.normalize_zpos = true; }
Instead of re-implementing the drm_atomic_helper_check() locally with just adding drm_atomic_normalize_zpos() into it, set the drm_device->normalize_zpos.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Thierry Reding thierry.reding@gmail.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/tegra/drm.c | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c index d50bddb2e447..037c79d9c6fd 100644 --- a/drivers/gpu/drm/tegra/drm.c +++ b/drivers/gpu/drm/tegra/drm.c @@ -33,29 +33,6 @@ struct tegra_drm_file { struct mutex lock; };
-static int tegra_atomic_check(struct drm_device *drm, - struct drm_atomic_state *state) -{ - int err; - - err = drm_atomic_helper_check_modeset(drm, state); - if (err < 0) - return err; - - err = drm_atomic_normalize_zpos(drm, state); - if (err < 0) - return err; - - err = drm_atomic_helper_check_planes(drm, state); - if (err < 0) - return err; - - if (state->legacy_cursor_update) - state->async_update = !drm_atomic_helper_async_check(drm, state); - - return 0; -} - static struct drm_atomic_state * tegra_atomic_state_alloc(struct drm_device *drm) { @@ -90,7 +67,7 @@ static const struct drm_mode_config_funcs tegra_drm_mode_config_funcs = { #ifdef CONFIG_DRM_FBDEV_EMULATION .output_poll_changed = drm_fb_helper_output_poll_changed, #endif - .atomic_check = tegra_atomic_check, + .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, .atomic_state_alloc = tegra_atomic_state_alloc, .atomic_state_clear = tegra_atomic_state_clear, @@ -179,6 +156,8 @@ static int tegra_drm_load(struct drm_device *drm, unsigned long flags)
drm->mode_config.allow_fb_modifiers = true;
+ drm->mode_config.normalize_zpos = true; + drm->mode_config.funcs = &tegra_drm_mode_config_funcs; drm->mode_config.helper_private = &tegra_drm_mode_config_helpers;
Instead of re-implementing the drm_atomic_helper_check() locally with just adding drm_atomic_normalize_zpos() into it, set the drm_device->normalize_zpos.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Benjamin Gaignard benjamin.gaignard@linaro.org CC: Vincent Abriou vincent.abriou@st.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/sti/sti_drv.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c index 55b6967d27e1..90c46b49c931 100644 --- a/drivers/gpu/drm/sti/sti_drv.c +++ b/drivers/gpu/drm/sti/sti_drv.c @@ -119,30 +119,10 @@ static int sti_drm_dbg_init(struct drm_minor *minor) return ret; }
-static int sti_atomic_check(struct drm_device *dev, - struct drm_atomic_state *state) -{ - int ret; - - ret = drm_atomic_helper_check_modeset(dev, state); - if (ret) - return ret; - - ret = drm_atomic_normalize_zpos(dev, state); - if (ret) - return ret; - - ret = drm_atomic_helper_check_planes(dev, state); - if (ret) - return ret; - - return ret; -} - static const struct drm_mode_config_funcs sti_mode_config_funcs = { .fb_create = drm_gem_fb_create, .output_poll_changed = drm_fb_helper_output_poll_changed, - .atomic_check = sti_atomic_check, + .atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit, };
@@ -160,6 +140,8 @@ static void sti_mode_config_init(struct drm_device *dev) dev->mode_config.max_height = STI_MAX_FB_HEIGHT;
dev->mode_config.funcs = &sti_mode_config_funcs; + + dev->mode_config.normalize_zpos = true; }
DEFINE_DRM_GEM_CMA_FOPS(sti_driver_fops);
2018-01-12 14:21 GMT+01:00 Peter Ujfalusi peter.ujfalusi@ti.com:
Instead of re-implementing the drm_atomic_helper_check() locally with just adding drm_atomic_normalize_zpos() into it, set the drm_device->normalize_zpos.
Acked-by: Benjamin Gaignard benjamin.gaignard@linaro.org
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Benjamin Gaignard benjamin.gaignard@linaro.org CC: Vincent Abriou vincent.abriou@st.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/sti/sti_drv.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c index 55b6967d27e1..90c46b49c931 100644 --- a/drivers/gpu/drm/sti/sti_drv.c +++ b/drivers/gpu/drm/sti/sti_drv.c @@ -119,30 +119,10 @@ static int sti_drm_dbg_init(struct drm_minor *minor) return ret; }
-static int sti_atomic_check(struct drm_device *dev,
struct drm_atomic_state *state)-{
int ret;ret = drm_atomic_helper_check_modeset(dev, state);if (ret)return ret;ret = drm_atomic_normalize_zpos(dev, state);if (ret)return ret;ret = drm_atomic_helper_check_planes(dev, state);if (ret)return ret;return ret;-}
static const struct drm_mode_config_funcs sti_mode_config_funcs = { .fb_create = drm_gem_fb_create, .output_poll_changed = drm_fb_helper_output_poll_changed,
.atomic_check = sti_atomic_check,
.atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit,};
@@ -160,6 +140,8 @@ static void sti_mode_config_init(struct drm_device *dev) dev->mode_config.max_height = STI_MAX_FB_HEIGHT;
dev->mode_config.funcs = &sti_mode_config_funcs;
dev->mode_config.normalize_zpos = true;}
DEFINE_DRM_GEM_CMA_FOPS(sti_driver_fops);
Peter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi,
On 2018-01-12 16:07, Benjamin Gaignard wrote:
2018-01-12 14:21 GMT+01:00 Peter Ujfalusi peter.ujfalusi@ti.com:
Instead of re-implementing the drm_atomic_helper_check() locally with just adding drm_atomic_normalize_zpos() into it, set the drm_device->normalize_zpos.
Acked-by: Benjamin Gaignard benjamin.gaignard@linaro.org
Thanks Benjamin!
I just noticed that I forgot to update the commit message with s/drm_device/drm_mode_config
I will resend with updated commit message on Monday.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Benjamin Gaignard benjamin.gaignard@linaro.org CC: Vincent Abriou vincent.abriou@st.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch
drivers/gpu/drm/sti/sti_drv.c | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c index 55b6967d27e1..90c46b49c931 100644 --- a/drivers/gpu/drm/sti/sti_drv.c +++ b/drivers/gpu/drm/sti/sti_drv.c @@ -119,30 +119,10 @@ static int sti_drm_dbg_init(struct drm_minor *minor) return ret; }
-static int sti_atomic_check(struct drm_device *dev,
struct drm_atomic_state *state)-{
int ret;ret = drm_atomic_helper_check_modeset(dev, state);if (ret)return ret;ret = drm_atomic_normalize_zpos(dev, state);if (ret)return ret;ret = drm_atomic_helper_check_planes(dev, state);if (ret)return ret;return ret;-}
static const struct drm_mode_config_funcs sti_mode_config_funcs = { .fb_create = drm_gem_fb_create, .output_poll_changed = drm_fb_helper_output_poll_changed,
.atomic_check = sti_atomic_check,
.atomic_check = drm_atomic_helper_check, .atomic_commit = drm_atomic_helper_commit,};
@@ -160,6 +140,8 @@ static void sti_mode_config_init(struct drm_device *dev) dev->mode_config.max_height = STI_MAX_FB_HEIGHT;
dev->mode_config.funcs = &sti_mode_config_funcs;
dev->mode_config.normalize_zpos = true;}
DEFINE_DRM_GEM_CMA_FOPS(sti_driver_fops);
Peter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
- Péter
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Set the drm_device->normalize_zpos and call drm_atomic_helper_check() from rcar_du_atomic_check() instead of re implementing the function locally.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Laurent Pinchart laurent.pinchart@ideasonboard.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 566d1a948c8f..384f98aa4ca3 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -234,15 +234,7 @@ static int rcar_du_atomic_check(struct drm_device *dev, struct rcar_du_device *rcdu = dev->dev_private; int ret;
- ret = drm_atomic_helper_check_modeset(dev, state); - if (ret) - return ret; - - ret = drm_atomic_normalize_zpos(dev, state); - if (ret) - return ret; - - ret = drm_atomic_helper_check_planes(dev, state); + ret = drm_atomic_helper_check(dev, state); if (ret) return ret;
@@ -531,6 +523,7 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu) dev->mode_config.min_height = 0; dev->mode_config.max_width = 4095; dev->mode_config.max_height = 2047; + dev->mode_config.normalize_zpos = true; dev->mode_config.funcs = &rcar_du_mode_config_funcs; dev->mode_config.helper_private = &rcar_du_mode_config_helper;
Planes with identical zpos value will result undefined behavior: disappearing planes, screen flickering and it is not supported by the hardware.
Use normalized zpos to make sure that we don't encounter invalid configuration.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com CC: Tomi Valkeinen tomi.valkeinen@ti.com Acked-by: Daniel Vetter daniel.vetter@ffwll.ch --- drivers/gpu/drm/omapdrm/omap_drv.c | 3 +++ drivers/gpu/drm/omapdrm/omap_plane.c | 2 +- 2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 2c02b955dd52..5ea46a1bab46 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -422,6 +422,9 @@ static int omap_modeset_init(struct drm_device *dev) dev->mode_config.max_width = 2048; dev->mode_config.max_height = 2048;
+ /* We want the zpos to be normalized */ + dev->mode_config.normalize_zpos = true; + dev->mode_config.funcs = &omap_mode_config_funcs; dev->mode_config.helper_private = &omap_mode_config_helper_funcs;
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c b/drivers/gpu/drm/omapdrm/omap_plane.c index 7d789d1551a1..2c19d2239bc5 100644 --- a/drivers/gpu/drm/omapdrm/omap_plane.c +++ b/drivers/gpu/drm/omapdrm/omap_plane.c @@ -65,7 +65,7 @@ static void omap_plane_atomic_update(struct drm_plane *plane, info.rotation_type = OMAP_DSS_ROT_NONE; info.rotation = DRM_MODE_ROTATE_0; info.global_alpha = 0xff; - info.zorder = state->zpos; + info.zorder = state->normalized_zpos;
/* update scanout: */ omap_framebuffer_update_scanout(state->fb, state, &info);
dri-devel@lists.freedesktop.org
-
Benjamin Gaignard -
Peter Ujfalusi