Re: drm/ast something ate high-res modes (5.3->5.6 regression)

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 2525 bytes --]

Hi

Am 08.07.20 um 12:05 schrieb Ilpo Järvinen:
> Hi,
> 
> After upgrading kernel from 5.3 series to 5.6.16 something seems to 
> prevent me from achieving high resolutions with the ast driver.

Are you able to build and run a test kernel?

I'm seriously considering moving ast to the SHMEM memory manager, which
would restore the higher resolutions.

If you're able to test, you need the git tree drm-tip/drm-tip and the
attached patch.

Alternatively, I've pushed all to

  https://gitlab.freedesktop.org/tzimmermann/linux/-/tree/ast-shmem

You'd have to checkout the tree and switch to the ast-shmem branch.

Please report back if that solves the issue for you.

Best regards
Thomas

> 
> With 5.6.16:
> 
> $ xrandr
> Screen 0: minimum 320 x 200, current 1600 x 1200, maximum 1920 x 2048
> VGA-1 connected primary 1600x1200+0+0 (normal left inverted right x axis y axis) 519mm x 324mm
>    1600x1200     60.00* 
>    1680x1050     59.95  
>    1280x1024     75.02    60.02  
>    1440x900      59.89  
>    1280x800      59.81  
>    1024x768      75.03    60.00  
>    800x600       75.00    60.32  
>    640x480       75.00    59.94  
>    1920x1200_60.0  59.95  
> 
> If I try to change to that manually added high-res mode, I get:
> xrandr: Configure crtc 0 failed
> 
> With 5.3 series I've this:
> 
> Screen 0: minimum 320 x 200, current 1920 x 1200, maximum 1920 x 2048
> VGA-1 connected primary 1920x1200+0+0 (normal left inverted right x axis y axis) 519mm x 324mm
>    1920x1200     59.95*+
>    1600x1200     60.00  
>    1680x1050     59.95  
>    1280x1024     75.02    60.02  
>    1440x900      59.89  
>    1280x800      59.81  
>    1024x768      75.03    60.00  
>    800x600       75.00    60.32  
>    640x480       75.00    59.94  
>    1920x1200_60.0  59.95  
> 
> As I've had issues in getting EDID reliably from the monitor, I provide it 
> on kernel command-line (the one dumped from the monitor I use). In 
> addition, I've another workaround for past issues related to EDID which 
> always adds that 1920x1200_60.0 mode but now I cannot use even it to
> enter a high-res mode.
> 
> If you need some additional info or want me to test a patch, just let me 
> know (but some delay is expected in testing patches). Thanks.
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #1.1.1.2: 0001-drm-ast-Convert-ast-to-SHMEM.patch --]
[-- Type: text/x-patch, Size: 13850 bytes --]

From ea15b5d52a4538e40da9605d71ed52baa1c1cdd7 Mon Sep 17 00:00:00 2001
From: Thomas Zimmermann <tzimmermann@suse.de>
Date: Thu, 9 Jul 2020 14:06:43 +0200
Subject: [PATCH] drm/ast: Convert ast to SHMEM

---
 drivers/gpu/drm/ast/Kconfig      |  4 +-
 drivers/gpu/drm/ast/ast_cursor.c | 95 ++++++++------------------------
 drivers/gpu/drm/ast/ast_drv.c    |  5 +-
 drivers/gpu/drm/ast/ast_drv.h    |  8 ++-
 drivers/gpu/drm/ast/ast_main.c   |  1 -
 drivers/gpu/drm/ast/ast_mm.c     | 32 +++++++----
 drivers/gpu/drm/ast/ast_mode.c   | 75 +++++++++++++++++++------
 7 files changed, 112 insertions(+), 108 deletions(-)

diff --git a/drivers/gpu/drm/ast/Kconfig b/drivers/gpu/drm/ast/Kconfig
index fbcf2f45cef5..d367a90cd3de 100644
--- a/drivers/gpu/drm/ast/Kconfig
+++ b/drivers/gpu/drm/ast/Kconfig
@@ -2,10 +2,8 @@
 config DRM_AST
 	tristate "AST server chips"
 	depends on DRM && PCI && MMU
+	select DRM_GEM_SHMEM_HELPER
 	select DRM_KMS_HELPER
-	select DRM_VRAM_HELPER
-	select DRM_TTM
-	select DRM_TTM_HELPER
 	help
 	 Say yes for experimental AST GPU driver. Do not enable
 	 this driver without having a working -modesetting,
diff --git a/drivers/gpu/drm/ast/ast_cursor.c b/drivers/gpu/drm/ast/ast_cursor.c
index e0f4613918ad..ad4b02967d32 100644
--- a/drivers/gpu/drm/ast/ast_cursor.c
+++ b/drivers/gpu/drm/ast/ast_cursor.c
@@ -27,79 +27,39 @@
  * Authors: Dave Airlie <airlied@redhat.com>
  */
 
-#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_managed.h>
 
 #include "ast_drv.h"
 
-static void ast_cursor_fini(struct ast_private *ast)
-{
-	size_t i;
-	struct drm_gem_vram_object *gbo;
-
-	for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) {
-		gbo = ast->cursor.gbo[i];
-		drm_gem_vram_vunmap(gbo, ast->cursor.vaddr[i]);
-		drm_gem_vram_unpin(gbo);
-		drm_gem_vram_put(gbo);
-	}
-}
-
-static void ast_cursor_release(struct drm_device *dev, void *ptr)
-{
-	struct ast_private *ast = to_ast_private(dev);
-
-	ast_cursor_fini(ast);
-}
-
 /*
  * Allocate cursor BOs and pins them at the end of VRAM.
  */
 int ast_cursor_init(struct ast_private *ast)
 {
-	struct drm_device *dev = &ast->base;
 	size_t size, i;
-	struct drm_gem_vram_object *gbo;
 	void __iomem *vaddr;
-	int ret;
+	u64 offset;
 
 	size = roundup(AST_HWC_SIZE + AST_HWC_SIGNATURE_SIZE, PAGE_SIZE);
 
+	vaddr = ast->vram + ast->vram_fb_available;
+	offset = ast->vram_base + ast->vram_fb_available;
+
 	for (i = 0; i < ARRAY_SIZE(ast->cursor.gbo); ++i) {
-		gbo = drm_gem_vram_create(dev, size, 0);
-		if (IS_ERR(gbo)) {
-			ret = PTR_ERR(gbo);
-			goto err_drm_gem_vram_put;
-		}
-		ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM |
-					    DRM_GEM_VRAM_PL_FLAG_TOPDOWN);
-		if (ret) {
-			drm_gem_vram_put(gbo);
-			goto err_drm_gem_vram_put;
-		}
-		vaddr = drm_gem_vram_vmap(gbo);
-		if (IS_ERR(vaddr)) {
-			ret = PTR_ERR(vaddr);
-			drm_gem_vram_unpin(gbo);
-			drm_gem_vram_put(gbo);
-			goto err_drm_gem_vram_put;
-		}
 
-		ast->cursor.gbo[i] = gbo;
-		ast->cursor.vaddr[i] = vaddr;
-	}
+		if (ast->vram_fb_available < size)
+			return -ENOMEM;
 
-	return drmm_add_action_or_reset(dev, ast_cursor_release, NULL);
+		vaddr -= size;
+		offset -= size;
 
-err_drm_gem_vram_put:
-	while (i) {
-		--i;
-		gbo = ast->cursor.gbo[i];
-		drm_gem_vram_vunmap(gbo, ast->cursor.vaddr[i]);
-		drm_gem_vram_unpin(gbo);
-		drm_gem_vram_put(gbo);
+		ast->cursor.vaddr[i] = vaddr;
+		ast->cursor.offset[i] = offset;
+		ast->vram_fb_available -= size;
 	}
-	return ret;
+
+	return 0;
 }
 
 static void update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int height)
@@ -169,7 +129,6 @@ static void update_cursor_image(u8 __iomem *dst, const u8 *src, int width, int h
 int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = &ast->base;
-	struct drm_gem_vram_object *gbo;
 	int ret;
 	void *src;
 	void __iomem *dst;
@@ -178,15 +137,13 @@ int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 	    drm_WARN_ON_ONCE(dev, fb->height > AST_MAX_HWC_HEIGHT))
 		return -EINVAL;
 
-	gbo = drm_gem_vram_of_gem(fb->obj[0]);
-
-	ret = drm_gem_vram_pin(gbo, 0);
+	ret = drm_gem_shmem_pin(fb->obj[0]);
 	if (ret)
 		return ret;
-	src = drm_gem_vram_vmap(gbo);
+	src = drm_gem_shmem_vmap(fb->obj[0]);
 	if (IS_ERR(src)) {
 		ret = PTR_ERR(src);
-		goto err_drm_gem_vram_unpin;
+		goto err_drm_gem_shmem_unpin;
 	}
 
 	dst = ast->cursor.vaddr[ast->cursor.next_index];
@@ -194,13 +151,13 @@ int ast_cursor_blit(struct ast_private *ast, struct drm_framebuffer *fb)
 	/* do data transfer to cursor BO */
 	update_cursor_image(dst, src, fb->width, fb->height);
 
-	drm_gem_vram_vunmap(gbo, src);
-	drm_gem_vram_unpin(gbo);
+	drm_gem_shmem_vunmap(fb->obj[0], src);
+	drm_gem_shmem_unpin(fb->obj[0]);
 
 	return 0;
 
-err_drm_gem_vram_unpin:
-	drm_gem_vram_unpin(gbo);
+err_drm_gem_shmem_unpin:
+	drm_gem_shmem_unpin(fb->obj[0]);
 	return ret;
 }
 
@@ -217,15 +174,7 @@ static void ast_cursor_set_base(struct ast_private *ast, u64 address)
 
 void ast_cursor_page_flip(struct ast_private *ast)
 {
-	struct drm_device *dev = &ast->base;
-	struct drm_gem_vram_object *gbo;
-	s64 off;
-
-	gbo = ast->cursor.gbo[ast->cursor.next_index];
-
-	off = drm_gem_vram_offset(gbo);
-	if (drm_WARN_ON_ONCE(dev, off < 0))
-		return; /* Bug: we didn't pin the cursor HW BO to VRAM. */
+	u64 off = ast->cursor.offset[ast->cursor.next_index];
 
 	ast_cursor_set_base(ast, off);
 
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index f0b4af1c390a..85748ef869d3 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -33,7 +33,7 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fb_helper.h>
-#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
 #include <drm/drm_probe_helper.h>
 
 #include "ast_drv.h"
@@ -62,7 +62,8 @@ static struct drm_driver ast_driver = {
 	.minor = DRIVER_MINOR,
 	.patchlevel = DRIVER_PATCHLEVEL,
 
-	DRM_GEM_VRAM_DRIVER
+	.gem_create_object = drm_gem_shmem_create_object_cached,
+	DRM_GEM_SHMEM_DRIVER_OPS
 };
 
 /*
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 467049ca8430..d1fcbf7d8026 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -129,9 +129,15 @@ struct ast_private {
 
 	int fb_mtrr;
 
+	void __iomem	*vram;
+	unsigned long	vram_base;
+	unsigned long	vram_size;
+	unsigned long	vram_fb_available;
+
 	struct {
-		struct drm_gem_vram_object *gbo[AST_DEFAULT_HWC_NUM];
+		struct drm_gem_shmem_object *gbo[AST_DEFAULT_HWC_NUM];
 		void __iomem *vaddr[AST_DEFAULT_HWC_NUM];
+		u64 offset[AST_DEFAULT_HWC_NUM];
 		unsigned int next_index;
 	} cursor;
 
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 77066bca8793..2e905d827df8 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -32,7 +32,6 @@
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_gem.h>
-#include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_managed.h>
 
 #include "ast_drv.h"
diff --git a/drivers/gpu/drm/ast/ast_mm.c b/drivers/gpu/drm/ast/ast_mm.c
index 8392ebde504b..6a5fbba52441 100644
--- a/drivers/gpu/drm/ast/ast_mm.c
+++ b/drivers/gpu/drm/ast/ast_mm.c
@@ -28,7 +28,6 @@
 
 #include <linux/pci.h>
 
-#include <drm/drm_gem_vram_helper.h>
 #include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 
@@ -86,22 +85,33 @@ static void ast_mm_release(struct drm_device *dev, void *ptr)
 int ast_mm_init(struct ast_private *ast)
 {
 	struct drm_device *dev = &ast->base;
+	resource_size_t start, len;
 	u32 vram_size;
 	int ret;
 
-	vram_size = ast_get_vram_size(ast);
+	/* BAR 0 is VRAM */
+	start = pci_resource_start(dev->pdev, 0);
+	len = pci_resource_len(dev->pdev, 0);
+
+	arch_io_reserve_memtype_wc(start, len);
+	ast->fb_mtrr = arch_phys_wc_add(start, len);
 
-	ret = drmm_vram_helper_init(dev, pci_resource_start(dev->pdev, 0),
-				    vram_size);
-	if (ret) {
-		drm_err(dev, "Error initializing VRAM MM; %d\n", ret);
-		return ret;
+	ast->vram = ioremap(start, len);
+	if (!ast->vram) {
+		ret = -ENOMEM;
+		goto err_arch_phys_wc_del;
 	}
 
-	arch_io_reserve_memtype_wc(pci_resource_start(dev->pdev, 0),
-				   pci_resource_len(dev->pdev, 0));
-	ast->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
-					pci_resource_len(dev->pdev, 0));
+	vram_size = ast_get_vram_size(ast);
+
+	ast->vram_base = start;
+	ast->vram_size = vram_size;
+	ast->vram_fb_available = vram_size;
 
 	return drmm_add_action_or_reset(dev, ast_mm_release, NULL);
+
+err_arch_phys_wc_del:
+	arch_phys_wc_del(ast->fb_mtrr);
+	arch_io_free_memtype_wc(start, len);
+	return ret;
 }
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 834a156e3a75..7e3293e56caa 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -36,9 +36,12 @@
 #include <drm/drm_atomic_state_helper.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_damage_helper.h>
+#include <drm/drm_format_helper.h>
 #include <drm/drm_fourcc.h>
 #include <drm/drm_gem_framebuffer_helper.h>
-#include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_gem_shmem_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -563,6 +566,27 @@ static int ast_primary_plane_helper_atomic_check(struct drm_plane *plane,
 	return 0;
 }
 
+static void ast_handle_damage(struct ast_private *ast,
+			      struct drm_framebuffer *fb,
+			      struct drm_rect *clip)
+{
+	struct drm_device *dev = &ast->base;
+	void *vmap;
+
+	vmap = drm_gem_shmem_vmap(fb->obj[0]);
+	if (drm_WARN_ON(dev, !vmap))
+		return; /* BUG: SHMEM BO should always be vmapped */
+
+	drm_fb_memcpy_dstclip(ast->vram, vmap, fb, clip);
+
+	drm_gem_shmem_vunmap(fb->obj[0], vmap);
+
+	ast_set_offset_reg(ast, fb);
+	ast_set_start_address_crt1(ast, (u32)0);
+
+	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
+}
+
 static void
 ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 				       struct drm_plane_state *old_state)
@@ -570,10 +594,12 @@ ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 	struct drm_device *dev = plane->dev;
 	struct ast_private *ast = to_ast_private(dev);
 	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;
+	struct drm_rect damage;
+
+	if (!fb)
+		return;
 
 	if (!old_fb || (fb->format != old_fb->format)) {
 		struct drm_crtc_state *crtc_state = state->crtc->state;
@@ -584,15 +610,8 @@ ast_primary_plane_helper_atomic_update(struct drm_plane *plane,
 		ast_set_vbios_color_reg(ast, fb->format, vbios_mode_info);
 	}
 
-	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, fb);
-	ast_set_start_address_crt1(ast, (u32)gpu_addr);
-
-	ast_set_index_reg_mask(ast, AST_IO_SEQ_PORT, 0x1, 0xdf, 0x00);
+	if (drm_atomic_helper_damage_merged(old_state, state, &damage))
+		ast_handle_damage(ast, fb, &damage);
 }
 
 static void
@@ -605,8 +624,7 @@ ast_primary_plane_helper_atomic_disable(struct drm_plane *plane,
 }
 
 static const struct drm_plane_helper_funcs ast_primary_plane_helper_funcs = {
-	.prepare_fb = drm_gem_vram_plane_helper_prepare_fb,
-	.cleanup_fb = drm_gem_vram_plane_helper_cleanup_fb,
+	.prepare_fb = drm_gem_fb_prepare_fb,
 	.atomic_check = ast_primary_plane_helper_atomic_check,
 	.atomic_update = ast_primary_plane_helper_atomic_update,
 	.atomic_disable = ast_primary_plane_helper_atomic_disable,
@@ -641,6 +659,10 @@ ast_cursor_plane_helper_prepare_fb(struct drm_plane *plane,
 	if (!crtc || !fb)
 		return 0;
 
+	ret = drm_gem_fb_prepare_fb(plane, new_state);
+	if (ret)
+		return ret;
+
 	ast = to_ast_private(plane->dev);
 
 	ret = ast_cursor_blit(ast, fb);
@@ -1073,9 +1095,28 @@ ast_mode_config_helper_funcs = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
+static enum drm_mode_status
+ast_mode_config_mode_valid(struct drm_device *dev,
+			   const struct drm_display_mode *mode)
+{
+	static const unsigned long max_bpp = 4; /* DRM_FORMAT_XRGB8888 */
+	struct ast_private *ast = to_ast_private(dev);
+	unsigned long fbsize, fbpages, max_fbpages;
+
+	max_fbpages = (ast->vram_size) >> PAGE_SHIFT;
+
+	fbsize = mode->hdisplay * mode->vdisplay * max_bpp;
+	fbpages = DIV_ROUND_UP(fbsize, PAGE_SIZE);
+
+	if (fbpages > max_fbpages)
+		return MODE_MEM;
+
+	return MODE_OK;
+}
+
 static const struct drm_mode_config_funcs ast_mode_config_funcs = {
-	.fb_create = drm_gem_fb_create,
-	.mode_valid = drm_vram_helper_mode_valid,
+	.fb_create = drm_gem_fb_create_with_dirty,
+	.mode_valid = ast_mode_config_mode_valid,
 	.atomic_check = drm_atomic_helper_check,
 	.atomic_commit = drm_atomic_helper_commit,
 };
@@ -1097,7 +1138,7 @@ int ast_mode_config_init(struct ast_private *ast)
 	dev->mode_config.min_width = 0;
 	dev->mode_config.min_height = 0;
 	dev->mode_config.preferred_depth = 24;
-	dev->mode_config.prefer_shadow = 1;
+	dev->mode_config.prefer_shadow = 0;
 	dev->mode_config.fb_base = pci_resource_start(dev->pdev, 0);
 
 	if (ast->chip == AST2100 ||
-- 
2.28.0


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 516 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel