* [PATCH v3 1/4] drm/ast: Set format registers in primary plane's update
2020-09-14 7:22 [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Thomas Zimmermann
@ 2020-09-14 7:22 ` Thomas Zimmermann
2020-09-14 7:22 ` [PATCH v3 2/4] drm/ast: Disable planes while switching display modes Thomas Zimmermann
` (3 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2020-09-14 7:22 UTC (permalink / raw)
To: daniel, airlied, sam, kraxel, emil.l.velikov; +Cc: Thomas Zimmermann, dri-devel
The atomic modesetting code tried to distinguish format changes from
full modesetting operations. But the implementation was buggy and the
format registers were often updated even for simple pageflips.
Fix this problem by handling format changes in the primary plane's
update function.
v3:
* program format in primary plane's update function
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_mode.c | 44 +++++++++++++++++-----------------
1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 62fe682a7de6..2323fe0b71bd 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -562,13 +562,24 @@ ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
struct drm_plane_state *state = plane->state;
struct drm_gem_vram_object *gbo;
s64 gpu_addr;
+ struct drm_framebuffer *fb = state->fb;
+ struct drm_framebuffer *old_fb = old_state->fb;
+
+ if (!old_fb || (fb->format != old_fb->format)) {
+ struct drm_crtc_state *crtc_state = state->crtc->state;
+ struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
+ struct ast_vbios_mode_info *vbios_mode_info = &ast_crtc_state->vbios_mode_info;
+
+ ast_set_color_reg(ast, fb->format);
+ ast_set_vbios_color_reg(ast, fb->format, vbios_mode_info);
+ }
- gbo = drm_gem_vram_of_gem(state->fb->obj[0]);
+ gbo = drm_gem_vram_of_gem(fb->obj[0]);
gpu_addr = drm_gem_vram_offset(gbo);
if (drm_WARN_ON_ONCE(dev, gpu_addr < 0))
return; /* Bug: we didn't pin the BO to VRAM in prepare_fb. */
- ast_set_offset_reg(ast, state->fb);
+ ast_set_offset_reg(ast, fb);
ast_set_start_address_crt1(ast, (u32)gpu_addr);
ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
@@ -733,6 +744,7 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
struct drm_crtc_state *state)
{
+ struct drm_device *dev = crtc->dev;
struct ast_crtc_state *ast_state;
const struct drm_format_info *format;
bool succ;
@@ -743,8 +755,8 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
ast_state = to_ast_crtc_state(state);
format = ast_state->format;
- if (!format)
- return 0;
+ if (drm_WARN_ON_ONCE(dev, !format))
+ return -EINVAL; /* BUG: We didn't set format in primary check(). */
succ = ast_get_vbios_mode_info(format, &state->mode,
&state->adjusted_mode,
@@ -768,27 +780,15 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
{
struct drm_device *dev = crtc->dev;
struct ast_private *ast = to_ast_private(dev);
- struct ast_crtc_state *ast_state;
- const struct drm_format_info *format;
- struct ast_vbios_mode_info *vbios_mode_info;
- struct drm_display_mode *adjusted_mode;
+ struct drm_crtc_state *crtc_state = crtc->state;
+ struct ast_crtc_state *ast_crtc_state = to_ast_crtc_state(crtc_state);
+ struct ast_vbios_mode_info *vbios_mode_info =
+ &ast_crtc_state->vbios_mode_info;
+ struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
- ast_state = to_ast_crtc_state(crtc->state);
-
- format = ast_state->format;
- if (!format)
- return;
-
- vbios_mode_info = &ast_state->vbios_mode_info;
-
- ast_set_color_reg(ast, format);
- ast_set_vbios_color_reg(ast, format, vbios_mode_info);
-
- if (!crtc->state->mode_changed)
+ if (!drm_atomic_crtc_needs_modeset(crtc_state))
return;
- adjusted_mode = &crtc->state->adjusted_mode;
-
ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
ast_set_std_reg(ast, adjusted_mode, vbios_mode_info);
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 2/4] drm/ast: Disable planes while switching display modes
2020-09-14 7:22 [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Thomas Zimmermann
2020-09-14 7:22 ` [PATCH v3 1/4] drm/ast: Set format registers in primary plane's update Thomas Zimmermann
@ 2020-09-14 7:22 ` Thomas Zimmermann
2020-09-14 7:22 ` [PATCH v3 3/4] drm/ast: Program display mode in CRTC's atomic_enable() Thomas Zimmermann
` (2 subsequent siblings)
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2020-09-14 7:22 UTC (permalink / raw)
To: daniel, airlied, sam, kraxel, emil.l.velikov; +Cc: Thomas Zimmermann, dri-devel
The ast HW cursor requires the primary plane and CRTC to display at
a valid mode and format. This is not the case while switching
display modes, which can lead to the screen turing permanently dark.
As a workaround, the ast driver now disables active planes while the
mode or format switch takes place. It also synchronizes with the vertical
refresh to give CRTC and planes some time to catch up on each other.
The active planes planes (primary or cursor) will be re-enabled by
each plane's atomic_update() function.
v3:
* move the logic into the CRTC's atomic_disable function
v2:
* move the logic into the commit-tail function
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_drv.h | 2 ++
drivers/gpu/drm/ast/ast_mode.c | 31 +++++++++++++++++++++++++++++++
2 files changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index c1af6b725933..467049ca8430 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -177,6 +177,8 @@ struct ast_private *ast_device_create(struct drm_driver *drv,
#define AST_IO_MM_OFFSET (0x380)
+#define AST_IO_VGAIR1_VREFRESH BIT(3)
+
#define __ast_read(x) \
static inline u##x ast_read##x(struct ast_private *ast, u32 reg) { \
u##x val = 0;\
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 2323fe0b71bd..5b45b57c3d17 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -514,6 +514,16 @@ static void ast_set_start_address_crt1(struct ast_private *ast,
}
+static void ast_wait_for_vretrace(struct ast_private *ast)
+{
+ unsigned long timeout = jiffies + HZ;
+ u8 vgair1;
+
+ do {
+ vgair1 = ast_io_read8(ast, AST_IO_INPUT_STATUS1_READ);
+ } while (!(vgair1 & AST_IO_VGAIR1_VREFRESH) && time_before(jiffies, timeout));
+}
+
/*
* Primary plane
*/
@@ -809,7 +819,28 @@ static void
ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
struct drm_crtc_state *old_crtc_state)
{
+ struct drm_device *dev = crtc->dev;
+ struct ast_private *ast = to_ast_private(dev);
+
ast_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+
+ /*
+ * HW cursors require the underlying primary plane and CRTC to
+ * display a valid mode and image. This is not the case during
+ * full modeset operations. So we temporarily disable any active
+ * plane, including the HW cursor. Each plane's atomic_update()
+ * helper will re-enable it if necessary.
+ *
+ * We only do this during *full* modesets. It does not affect
+ * simple pageflips on the planes.
+ */
+ drm_atomic_helper_disable_planes_on_crtc(old_crtc_state, false);
+
+ /*
+ * Ensure that no scanout takes place before reprogramming mode
+ * and format registers.
+ */
+ ast_wait_for_vretrace(ast);
}
static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 3/4] drm/ast: Program display mode in CRTC's atomic_enable()
2020-09-14 7:22 [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Thomas Zimmermann
2020-09-14 7:22 ` [PATCH v3 1/4] drm/ast: Set format registers in primary plane's update Thomas Zimmermann
2020-09-14 7:22 ` [PATCH v3 2/4] drm/ast: Disable planes while switching display modes Thomas Zimmermann
@ 2020-09-14 7:22 ` Thomas Zimmermann
2020-09-14 7:22 ` [PATCH v3 4/4] drm/ast: Enable CRTC before planes Thomas Zimmermann
2020-09-16 9:24 ` [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Daniel Vetter
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2020-09-14 7:22 UTC (permalink / raw)
To: daniel, airlied, sam, kraxel, emil.l.velikov; +Cc: Thomas Zimmermann, dri-devel
This change simplifies ast's modesetting code. The display mode
is now programmed from within the CRTC's atomic_enable(), which
only runs if we actually want to program the mode.
Corresponding code in atomic_flush() is being removed. Also removed
is atomic_begin(), which serves no purpose at all.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_mode.c | 23 +++--------------------
1 file changed, 3 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 5b45b57c3d17..fd0127f48fb9 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -777,16 +777,9 @@ static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
return 0;
}
-static void ast_crtc_helper_atomic_begin(struct drm_crtc *crtc,
- struct drm_crtc_state *old_crtc_state)
-{
- struct ast_private *ast = to_ast_private(crtc->dev);
-
- ast_open_key(ast);
-}
-
-static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
- struct drm_crtc_state *old_crtc_state)
+static void
+ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
+ struct drm_crtc_state *old_crtc_state)
{
struct drm_device *dev = crtc->dev;
struct ast_private *ast = to_ast_private(dev);
@@ -796,9 +789,6 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
&ast_crtc_state->vbios_mode_info;
struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
- if (!drm_atomic_crtc_needs_modeset(crtc_state))
- return;
-
ast_set_vbios_mode_reg(ast, adjusted_mode, vbios_mode_info);
ast_set_index_reg(ast, AST_IO_CRTC_PORT, 0xa1, 0x06);
ast_set_std_reg(ast, adjusted_mode, vbios_mode_info);
@@ -806,12 +796,7 @@ static void ast_crtc_helper_atomic_flush(struct drm_crtc *crtc,
ast_set_dclk_reg(ast, adjusted_mode, vbios_mode_info);
ast_set_crtthd_reg(ast);
ast_set_sync_reg(ast, adjusted_mode, vbios_mode_info);
-}
-static void
-ast_crtc_helper_atomic_enable(struct drm_crtc *crtc,
- struct drm_crtc_state *old_crtc_state)
-{
ast_crtc_dpms(crtc, DRM_MODE_DPMS_ON);
}
@@ -845,8 +830,6 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
.atomic_check = ast_crtc_helper_atomic_check,
- .atomic_begin = ast_crtc_helper_atomic_begin,
- .atomic_flush = ast_crtc_helper_atomic_flush,
.atomic_enable = ast_crtc_helper_atomic_enable,
.atomic_disable = ast_crtc_helper_atomic_disable,
};
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v3 4/4] drm/ast: Enable CRTC before planes
2020-09-14 7:22 [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Thomas Zimmermann
` (2 preceding siblings ...)
2020-09-14 7:22 ` [PATCH v3 3/4] drm/ast: Program display mode in CRTC's atomic_enable() Thomas Zimmermann
@ 2020-09-14 7:22 ` Thomas Zimmermann
2020-09-16 9:24 ` [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Daniel Vetter
4 siblings, 0 replies; 6+ messages in thread
From: Thomas Zimmermann @ 2020-09-14 7:22 UTC (permalink / raw)
To: daniel, airlied, sam, kraxel, emil.l.velikov; +Cc: Thomas Zimmermann, dri-devel
An active cursor plane requires a valid display mode. Change the
commit_tail callback, so that it sets up the CRTC's mode before
updating planes.
Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
drivers/gpu/drm/ast/ast_mode.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index fd0127f48fb9..834a156e3a75 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1068,6 +1068,11 @@ static int ast_connector_init(struct drm_device *dev)
* Mode config
*/
+static const struct drm_mode_config_helper_funcs
+ast_mode_config_helper_funcs = {
+ .atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
+};
+
static const struct drm_mode_config_funcs ast_mode_config_funcs = {
.fb_create = drm_gem_fb_create,
.mode_valid = drm_vram_helper_mode_valid,
@@ -1107,6 +1112,8 @@ int ast_mode_config_init(struct ast_private *ast)
dev->mode_config.max_height = 1200;
}
+ dev->mode_config.helper_private = &ast_mode_config_helper_funcs;
+
memset(&ast->primary_plane, 0, sizeof(ast->primary_plane));
ret = drm_universal_plane_init(dev, &ast->primary_plane, 0x01,
&ast_primary_plane_funcs,
--
2.28.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes
2020-09-14 7:22 [RESEND][PATCH v3 0/4] drm/ast: Disable HW cursor when switching modes Thomas Zimmermann
` (3 preceding siblings ...)
2020-09-14 7:22 ` [PATCH v3 4/4] drm/ast: Enable CRTC before planes Thomas Zimmermann
@ 2020-09-16 9:24 ` Daniel Vetter
4 siblings, 0 replies; 6+ messages in thread
From: Daniel Vetter @ 2020-09-16 9:24 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: emil.l.velikov, dri-devel, kraxel, airlied, sam
On Mon, Sep 14, 2020 at 09:22:32AM +0200, Thomas Zimmermann wrote:
> Since converting the ast driver to atomic modesetting, modesetting
> occationally locks up the graphics hardware and turns the display
> permanently dark. This happens once or twice per 10 mode switches.
> Investigation shows that the ast hardware presumably requires the HW
> cursor to be disabled while the modeswitch takes place.
>
> This patchset fixes the problem by disabling planes before programming
> the CRTC mode. After the switch, the planes gets re-enabled if they were
> enabled before. For mere pageflip operations, nothing changes.
>
> Patches #1 makes format changes work as intended: format registers are
> only updated if necessary. They used to be changed on each pageflip.
>
> Patches #2 to #4 change the modesetting logic such that planes will be
> disabled with the CRTC, then the CRTC's new mode is being programmed,
> and finally planes are reenabled. The primary plane is enabled before
> the cursor plane. With this setup, the cursor plane always has a valid
> mode and framebuffer available.
>
> Tested on AST2100 HW. The issue is not 100% reproducible, but does not
> show up after applying the patchset. I think the problem has been fixed.
>
> Thomas Zimmermann (4):
> drm/ast: Set format registers in primary plane's update
> drm/ast: Disable planes while switching display modes
> drm/ast: Program display mode in CRTC's atomic_enable()
> drm/ast: Enable CRTC before planes
Ofc I can't check all the details, but I think this is using atomic
helpers a lot more like they're intended to be used. So definitely a good
cleanup. On the series:
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> drivers/gpu/drm/ast/ast_drv.h | 2 +
> drivers/gpu/drm/ast/ast_mode.c | 103 ++++++++++++++++++++-------------
> 2 files changed, 64 insertions(+), 41 deletions(-)
>
> --
> 2.28.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 6+ messages in thread