On Wed, Apr 29, 2020 at 04:32:22PM +0200, Thomas Zimmermann wrote:

The HW cursor of Matrox G200 cards only supports a 16-color palette format. Univeral planes require at least ARGB or a similar component- based format. Converting a cursor image from ARGB to 16 colors does not produce pleasent-looking results in general, so remove the HW cursor.

What impact does this have in useability? Does the cursor behaviour stay the same or?

The patch looks fine, but it seems a bit gross ditching curcor support. But maybe it is the right choice, I dunno.

Sam

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/Makefile | 2 +- drivers/gpu/drm/mgag200/mgag200_cursor.c | 319 ----------------------- drivers/gpu/drm/mgag200/mgag200_drv.h | 13 - drivers/gpu/drm/mgag200/mgag200_main.c | 7 - drivers/gpu/drm/mgag200/mgag200_mode.c | 2 - 5 files changed, 1 insertion(+), 342 deletions(-) delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile index 04b281bcf6558..63403133638a3 100644 --- a/drivers/gpu/drm/mgag200/Makefile +++ b/drivers/gpu/drm/mgag200/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only -mgag200-y := mgag200_main.o mgag200_mode.o mgag200_cursor.o \ +mgag200-y := mgag200_main.o mgag200_mode.o \ mgag200_drv.o mgag200_i2c.o mgag200_ttm.o

obj-$(CONFIG_DRM_MGAG200) += mgag200.o diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c deleted file mode 100644 index d491edd317ff3..0000000000000 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ /dev/null @@ -1,319 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/*

• • Copyright 2013 Matrox Graphics
• • Author: Christopher Harvey charvey@matrox.com
• */

-#include <linux/pci.h>

-#include "mgag200_drv.h"

-static bool warn_transparent = true; -static bool warn_palette = true;

-static int mgag200_cursor_update(struct mga_device *mdev, void *dst, void *src,

• 		 unsigned int width, unsigned int height)
  

-{

• struct drm_device *dev = mdev->dev;
• unsigned int i, row, col;
• uint32_t colour_set[16];
• uint32_t *next_space = &colour_set[0];
• uint32_t *palette_iter;
• uint32_t this_colour;
• bool found = false;
• int colour_count = 0;
• u8 reg_index;
• u8 this_row[48];
• memset(&colour_set[0], 0, sizeof(uint32_t)*16);
• /* width*height*4 = 16384 */
• for (i = 0; i < 16384; i += 4) {

• this_colour = ioread32(src + i);
  

• /* No transparency */
  

• if (this_colour>>24 != 0xff &&
  

• 	this_colour>>24 != 0x0) {
  

• 	if (warn_transparent) {
  

• 		dev_info(&dev->pdev->dev, "Video card doesn't support cursors with partial transparency.\n");
  

• 		dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n");
  

• 		warn_transparent = false; /* Only tell the user once. */
  

• 	}
  

• 	return -EINVAL;
  

• }
  

• /* Don't need to store transparent pixels as colours */
  

• if (this_colour>>24 == 0x0)
  

• 	continue;
  

• found = false;
  

• for (palette_iter = &colour_set[0]; palette_iter != next_space; palette_iter++) {
  

• 	if (*palette_iter == this_colour) {
  

• 		found = true;
  

• 		break;
  

• 	}
  

• }
  

• if (found)
  

• 	continue;
  

• /* We only support 4bit paletted cursors */
  

• if (colour_count >= 16) {
  

• 	if (warn_palette) {
  

• 		dev_info(&dev->pdev->dev, "Video card only supports cursors with up to 16 colours.\n");
  

• 		dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n");
  

• 		warn_palette = false; /* Only tell the user once. */
  

• 	}
  

• 	return -EINVAL;
  

• }
  

• *next_space = this_colour;
  

• next_space++;
  

• colour_count++;
  

• }
• /* Program colours from cursor icon into palette */
• for (i = 0; i < colour_count; i++) {

• if (i <= 2)
  

• 	reg_index = 0x8 + i*0x4;
  

• else
  

• 	reg_index = 0x60 + i*0x3;
  

• WREG_DAC(reg_index, colour_set[i] & 0xff);
  

• WREG_DAC(reg_index+1, colour_set[i]>>8 & 0xff);
  

• WREG_DAC(reg_index+2, colour_set[i]>>16 & 0xff);
  

• BUG_ON((colour_set[i]>>24 & 0xff) != 0xff);
  

• }
• /* now write colour indices into hardware cursor buffer */
• for (row = 0; row < 64; row++) {

• memset(&this_row[0], 0, 48);
  

• for (col = 0; col < 64; col++) {
  

• 	this_colour = ioread32(src + 4*(col + 64*row));
  

• 	/* write transparent pixels */
  

• 	if (this_colour>>24 == 0x0) {
  

• 		this_row[47 - col/8] |= 0x80>>(col%8);
  

• 		continue;
  

• 	}
  

• 	/* write colour index here */
  

• 	for (i = 0; i < colour_count; i++) {
  

• 		if (colour_set[i] == this_colour) {
  

• 			if (col % 2)
  

• 				this_row[col/2] |= i<<4;
  

• 			else
  

• 				this_row[col/2] |= i;
  

• 			break;
  

• 		}
  

• 	}
  

• }
  

• memcpy_toio(dst + row*48, &this_row[0], 48);
  

• }
• return 0;

-}

-static void mgag200_cursor_set_base(struct mga_device *mdev, u64 address) -{

• u8 addrl = (address >> 10) & 0xff;
• u8 addrh = (address >> 18) & 0x3f;
• /* Program gpu address of cursor buffer */
• WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, addrl);
• WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, addrh);

-}

-static int mgag200_show_cursor(struct mga_device *mdev, void *src,

• 	       unsigned int width, unsigned int height)
  

-{

• struct drm_device *dev = mdev->dev;
• struct drm_gem_vram_object *gbo;
• void *dst;
• s64 off;
• int ret;
• gbo = mdev->cursor.gbo[mdev->cursor.next_index];
• if (!gbo) {

• WREG8(MGA_CURPOSXL, 0);
  

• WREG8(MGA_CURPOSXH, 0);
  

• return -ENOTSUPP; /* Didn't allocate space for cursors */
  

• }
• dst = drm_gem_vram_vmap(gbo);
• if (IS_ERR(dst)) {

• ret = PTR_ERR(dst);
  

• dev_err(&dev->pdev->dev,
  

• 	"failed to map cursor updates: %d\n", ret);
  

• return ret;
  

• }
• off = drm_gem_vram_offset(gbo);
• if (off < 0) {

• ret = (int)off;
  

• dev_err(&dev->pdev->dev,
  

• 	"failed to get cursor scanout address: %d\n", ret);
  

• goto err_drm_gem_vram_vunmap;
  

• }
• ret = mgag200_cursor_update(mdev, dst, src, width, height);
• if (ret)

• goto err_drm_gem_vram_vunmap;
  

• mgag200_cursor_set_base(mdev, off);
• /* Adjust cursor control register to turn on the cursor */
• WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */
• drm_gem_vram_vunmap(gbo, dst);
• ++mdev->cursor.next_index;
• mdev->cursor.next_index %= ARRAY_SIZE(mdev->cursor.gbo);
• return 0;

-err_drm_gem_vram_vunmap:

• drm_gem_vram_vunmap(gbo, dst);
• return ret;

-}

-/*

• • Hide the cursor off screen. We can't disable the cursor hardware because
• • it takes too long to re-activate and causes momentary corruption.
• */

-static void mgag200_hide_cursor(struct mga_device *mdev) -{

• WREG8(MGA_CURPOSXL, 0);
• WREG8(MGA_CURPOSXH, 0);

-}

-static void mgag200_move_cursor(struct mga_device *mdev, int x, int y) -{

• if (WARN_ON(x <= 0))

• return;
  

• if (WARN_ON(y <= 0))

• return;
  

• if (WARN_ON(x & ~0xffff))

• return;
  

• if (WARN_ON(y & ~0xffff))

• return;
  

• WREG8(MGA_CURPOSXL, x & 0xff);
• WREG8(MGA_CURPOSXH, (x>>8) & 0xff);
• WREG8(MGA_CURPOSYL, y & 0xff);
• WREG8(MGA_CURPOSYH, (y>>8) & 0xff);

-}

-int mgag200_cursor_init(struct mga_device *mdev) -{

• struct drm_device *dev = mdev->dev;
• size_t ncursors = ARRAY_SIZE(mdev->cursor.gbo);
• size_t size;
• int ret;
• size_t i;
• struct drm_gem_vram_object *gbo;
• size = roundup(64 * 48, PAGE_SIZE);
• if (size * ncursors > mdev->vram_fb_available)

• return -ENOMEM;
  

• for (i = 0; i < ncursors; ++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;
  

• }
  

• mdev->cursor.gbo[i] = gbo;
  

• }
• /*

• * At the high end of video memory, we reserve space for
  

• * buffer objects. The cursor plane uses this memory to store
  

• * a double-buffered image of the current cursor. Hence, it's
  

• * not available for framebuffers.
  

• */
  

• mdev->vram_fb_available -= ncursors * size;
• return 0;

-err_drm_gem_vram_put:

• while (i) {

• --i;
  

• gbo = mdev->cursor.gbo[i];
  

• drm_gem_vram_unpin(gbo);
  

• drm_gem_vram_put(gbo);
  

• mdev->cursor.gbo[i] = NULL;
  

• }
• return ret;

-}

-void mgag200_cursor_fini(struct mga_device *mdev) -{

• size_t i;
• struct drm_gem_vram_object *gbo;
• for (i = 0; i < ARRAY_SIZE(mdev->cursor.gbo); ++i) {

• gbo = mdev->cursor.gbo[i];
  

• drm_gem_vram_unpin(gbo);
  

• drm_gem_vram_put(gbo);
  

• }

-}

-int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,

• 	    uint32_t handle, uint32_t width, uint32_t height)
  

-{

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = (struct mga_device *)dev->dev_private;
• struct drm_gem_object *obj;
• struct drm_gem_vram_object *gbo = NULL;
• int ret;
• u8 *src;
• if (!handle || !file_priv) {

• mgag200_hide_cursor(mdev);
  

• return 0;
  

• }
• if (width != 64 || height != 64) {

• WREG8(MGA_CURPOSXL, 0);
  

• WREG8(MGA_CURPOSXH, 0);
  

• return -EINVAL;
  

• }
• obj = drm_gem_object_lookup(file_priv, handle);
• if (!obj)

• return -ENOENT;
  

• gbo = drm_gem_vram_of_gem(obj);
• src = drm_gem_vram_vmap(gbo);
• if (IS_ERR(src)) {

• ret = PTR_ERR(src);
  

• dev_err(&dev->pdev->dev,
  

• 	"failed to map user buffer updates\n");
  

• goto err_drm_gem_object_put_unlocked;
  

• }
• ret = mgag200_show_cursor(mdev, src, width, height);
• if (ret)

• goto err_drm_gem_vram_vunmap;
  

• /* Now update internal buffer pointers */
• drm_gem_vram_vunmap(gbo, src);
• drm_gem_object_put_unlocked(obj);
• return 0;

-err_drm_gem_vram_vunmap:

• drm_gem_vram_vunmap(gbo, src);

-err_drm_gem_object_put_unlocked:

• drm_gem_object_put_unlocked(obj);
• return ret;

-}

-int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) -{

• struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private;
• /* Our origin is at (64,64) */
• x += 64;
• y += 64;
• mgag200_move_cursor(mdev, x, y);
• return 0;

-} diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 9691252d6233f..c7f2000d46fce 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -121,11 +121,6 @@ struct mga_connector { struct mga_i2c_chan *i2c; };

-struct mga_cursor {

• struct drm_gem_vram_object *gbo[2];
• unsigned int next_index;

-};

struct mga_mc { resource_size_t vram_size; resource_size_t vram_base; @@ -162,8 +157,6 @@ struct mga_device { struct mga_mc mc; struct mga_mode_info mode_info;

• struct mga_cursor cursor;

• size_t vram_fb_available;

  bool suspended;

@@ -210,10 +203,4 @@ int mgag200_mm_init(struct mga_device *mdev); void mgag200_mm_fini(struct mga_device *mdev); int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);

-int mgag200_cursor_init(struct mga_device *mdev); -void mgag200_cursor_fini(struct mga_device *mdev); -int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,

• 	    uint32_t handle, uint32_t width, uint32_t height);
  

-int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y);

#endif /* __MGAG200_DRV_H__ */ diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index b680cf47cbb94..46cc32816f1e1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -176,16 +176,10 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) goto err_modeset; }

• r = mgag200_cursor_init(mdev);
• if (r)

• dev_warn(&dev->pdev->dev,
  

• 	"Could not initialize cursors. Not doing hardware cursors.\n");
  

• return 0;

err_modeset: drm_mode_config_cleanup(dev);

• mgag200_cursor_fini(mdev); mgag200_mm_fini(mdev);

err_mm: dev->dev_private = NULL; @@ -201,7 +195,6 @@ void mgag200_driver_unload(struct drm_device *dev) return; mgag200_modeset_fini(mdev); drm_mode_config_cleanup(dev);

• mgag200_cursor_fini(mdev); mgag200_mm_fini(mdev); dev->dev_private = NULL;

} diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index d90e83959fca1..c9d120b019649 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1414,8 +1414,6 @@ static void mga_crtc_disable(struct drm_crtc *crtc)

/* These provide the minimum set of functions required to handle a CRTC */ static const struct drm_crtc_funcs mga_crtc_funcs = {

• .cursor_set = mgag200_crtc_cursor_set,
• .cursor_move = mgag200_crtc_cursor_move, .gamma_set = mga_crtc_gamma_set, .set_config = drm_crtc_helper_set_config, .destroy = mga_crtc_destroy,

-- 2.26.0

Hi Sam

Am 29.04.20 um 19:51 schrieb Sam Ravnborg:

On Wed, Apr 29, 2020 at 04:32:22PM +0200, Thomas Zimmermann wrote:

...

The HW cursor of Matrox G200 cards only supports a 16-color palette format. Univeral planes require at least ARGB or a similar component- based format. Converting a cursor image from ARGB to 16 colors does not produce pleasent-looking results in general, so remove the HW cursor.

What impact does this have in useability? Does the cursor behaviour stay the same or?

The patch looks fine, but it seems a bit gross ditching curcor support. But maybe it is the right choice, I dunno.

As Gerd said, compositors will render software cursors. Theoretically, you could measure (maybe see) a difference. In practice not so much.

I'd keep HW cursor support if it was useful, but it isn't. The HW supports 16-color palettes. That's simply not enough to be useful for most desktops.

The cursor image is ARGB. The old code used .set_cursor callbacks and returned an error if the ARGB format could not be fit into the 16-color palette. On errors, userspace switched to software cursors. From what I observed, I'd guess that GNOME et al already used SW cursors most of the time.

With the new atomic interfaces and the dirtyfb ioctl, there's no way of signalling an error during palette conversion. So userspace wouldn't know if the HW cursor is visible.

Alternatively to removing the code, the driver could dither the ARGB cursor image to 16 colors; no matter what the result looks like. But that's not an option IMHO.

Best regards Thomas

Sam

...

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/Makefile | 2 +- drivers/gpu/drm/mgag200/mgag200_cursor.c | 319 ----------------------- drivers/gpu/drm/mgag200/mgag200_drv.h | 13 - drivers/gpu/drm/mgag200/mgag200_main.c | 7 - drivers/gpu/drm/mgag200/mgag200_mode.c | 2 - 5 files changed, 1 insertion(+), 342 deletions(-) delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile index 04b281bcf6558..63403133638a3 100644 --- a/drivers/gpu/drm/mgag200/Makefile +++ b/drivers/gpu/drm/mgag200/Makefile @@ -1,5 +1,5 @@ # SPDX-License-Identifier: GPL-2.0-only -mgag200-y := mgag200_main.o mgag200_mode.o mgag200_cursor.o \ +mgag200-y := mgag200_main.o mgag200_mode.o \ mgag200_drv.o mgag200_i2c.o mgag200_ttm.o

obj-$(CONFIG_DRM_MGAG200) += mgag200.o diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c deleted file mode 100644 index d491edd317ff3..0000000000000 --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c +++ /dev/null @@ -1,319 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/*

• • Copyright 2013 Matrox Graphics
• • Author: Christopher Harvey charvey@matrox.com
• */

-#include <linux/pci.h>

-#include "mgag200_drv.h"

-static bool warn_transparent = true; -static bool warn_palette = true;

-static int mgag200_cursor_update(struct mga_device *mdev, void *dst, void *src,

• 		 unsigned int width, unsigned int height)
  

-{

• struct drm_device *dev = mdev->dev;
• unsigned int i, row, col;
• uint32_t colour_set[16];
• uint32_t *next_space = &colour_set[0];
• uint32_t *palette_iter;
• uint32_t this_colour;
• bool found = false;
• int colour_count = 0;
• u8 reg_index;
• u8 this_row[48];
• memset(&colour_set[0], 0, sizeof(uint32_t)*16);
• /* width*height*4 = 16384 */
• for (i = 0; i < 16384; i += 4) {

• this_colour = ioread32(src + i);
  

• /* No transparency */
  

• if (this_colour>>24 != 0xff &&
  

• 	this_colour>>24 != 0x0) {
  

• 	if (warn_transparent) {
  

• 		dev_info(&dev->pdev->dev, "Video card doesn't support cursors with partial transparency.\n");
  

• 		dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n");
  

• 		warn_transparent = false; /* Only tell the user once. */
  

• 	}
  

• 	return -EINVAL;
  

• }
  

• /* Don't need to store transparent pixels as colours */
  

• if (this_colour>>24 == 0x0)
  

• 	continue;
  

• found = false;
  

• for (palette_iter = &colour_set[0]; palette_iter != next_space; palette_iter++) {
  

• 	if (*palette_iter == this_colour) {
  

• 		found = true;
  

• 		break;
  

• 	}
  

• }
  

• if (found)
  

• 	continue;
  

• /* We only support 4bit paletted cursors */
  

• if (colour_count >= 16) {
  

• 	if (warn_palette) {
  

• 		dev_info(&dev->pdev->dev, "Video card only supports cursors with up to 16 colours.\n");
  

• 		dev_info(&dev->pdev->dev, "Not enabling hardware cursor.\n");
  

• 		warn_palette = false; /* Only tell the user once. */
  

• 	}
  

• 	return -EINVAL;
  

• }
  

• *next_space = this_colour;
  

• next_space++;
  

• colour_count++;
  

• }
• /* Program colours from cursor icon into palette */
• for (i = 0; i < colour_count; i++) {

• if (i <= 2)
  

• 	reg_index = 0x8 + i*0x4;
  

• else
  

• 	reg_index = 0x60 + i*0x3;
  

• WREG_DAC(reg_index, colour_set[i] & 0xff);
  

• WREG_DAC(reg_index+1, colour_set[i]>>8 & 0xff);
  

• WREG_DAC(reg_index+2, colour_set[i]>>16 & 0xff);
  

• BUG_ON((colour_set[i]>>24 & 0xff) != 0xff);
  

• }
• /* now write colour indices into hardware cursor buffer */
• for (row = 0; row < 64; row++) {

• memset(&this_row[0], 0, 48);
  

• for (col = 0; col < 64; col++) {
  

• 	this_colour = ioread32(src + 4*(col + 64*row));
  

• 	/* write transparent pixels */
  

• 	if (this_colour>>24 == 0x0) {
  

• 		this_row[47 - col/8] |= 0x80>>(col%8);
  

• 		continue;
  

• 	}
  

• 	/* write colour index here */
  

• 	for (i = 0; i < colour_count; i++) {
  

• 		if (colour_set[i] == this_colour) {
  

• 			if (col % 2)
  

• 				this_row[col/2] |= i<<4;
  

• 			else
  

• 				this_row[col/2] |= i;
  

• 			break;
  

• 		}
  

• 	}
  

• }
  

• memcpy_toio(dst + row*48, &this_row[0], 48);
  

• }
• return 0;

-}

-static void mgag200_cursor_set_base(struct mga_device *mdev, u64 address) -{

• u8 addrl = (address >> 10) & 0xff;
• u8 addrh = (address >> 18) & 0x3f;
• /* Program gpu address of cursor buffer */
• WREG_DAC(MGA1064_CURSOR_BASE_ADR_LOW, addrl);
• WREG_DAC(MGA1064_CURSOR_BASE_ADR_HI, addrh);

-}

-static int mgag200_show_cursor(struct mga_device *mdev, void *src,

• 	       unsigned int width, unsigned int height)
  

-{

• struct drm_device *dev = mdev->dev;
• struct drm_gem_vram_object *gbo;
• void *dst;
• s64 off;
• int ret;
• gbo = mdev->cursor.gbo[mdev->cursor.next_index];
• if (!gbo) {

• WREG8(MGA_CURPOSXL, 0);
  

• WREG8(MGA_CURPOSXH, 0);
  

• return -ENOTSUPP; /* Didn't allocate space for cursors */
  

• }
• dst = drm_gem_vram_vmap(gbo);
• if (IS_ERR(dst)) {

• ret = PTR_ERR(dst);
  

• dev_err(&dev->pdev->dev,
  

• 	"failed to map cursor updates: %d\n", ret);
  

• return ret;
  

• }
• off = drm_gem_vram_offset(gbo);
• if (off < 0) {

• ret = (int)off;
  

• dev_err(&dev->pdev->dev,
  

• 	"failed to get cursor scanout address: %d\n", ret);
  

• goto err_drm_gem_vram_vunmap;
  

• }
• ret = mgag200_cursor_update(mdev, dst, src, width, height);
• if (ret)

• goto err_drm_gem_vram_vunmap;
  

• mgag200_cursor_set_base(mdev, off);
• /* Adjust cursor control register to turn on the cursor */
• WREG_DAC(MGA1064_CURSOR_CTL, 4); /* 16-colour palletized cursor mode */
• drm_gem_vram_vunmap(gbo, dst);
• ++mdev->cursor.next_index;
• mdev->cursor.next_index %= ARRAY_SIZE(mdev->cursor.gbo);
• return 0;

-err_drm_gem_vram_vunmap:

• drm_gem_vram_vunmap(gbo, dst);
• return ret;

-}

-/*

• • Hide the cursor off screen. We can't disable the cursor hardware because
• • it takes too long to re-activate and causes momentary corruption.
• */

-static void mgag200_hide_cursor(struct mga_device *mdev) -{

• WREG8(MGA_CURPOSXL, 0);
• WREG8(MGA_CURPOSXH, 0);

-}

-static void mgag200_move_cursor(struct mga_device *mdev, int x, int y) -{

• if (WARN_ON(x <= 0))

• return;
  

• if (WARN_ON(y <= 0))

• return;
  

• if (WARN_ON(x & ~0xffff))

• return;
  

• if (WARN_ON(y & ~0xffff))

• return;
  

• WREG8(MGA_CURPOSXL, x & 0xff);
• WREG8(MGA_CURPOSXH, (x>>8) & 0xff);
• WREG8(MGA_CURPOSYL, y & 0xff);
• WREG8(MGA_CURPOSYH, (y>>8) & 0xff);

-}

-int mgag200_cursor_init(struct mga_device *mdev) -{

• struct drm_device *dev = mdev->dev;
• size_t ncursors = ARRAY_SIZE(mdev->cursor.gbo);
• size_t size;
• int ret;
• size_t i;
• struct drm_gem_vram_object *gbo;
• size = roundup(64 * 48, PAGE_SIZE);
• if (size * ncursors > mdev->vram_fb_available)

• return -ENOMEM;
  

• for (i = 0; i < ncursors; ++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;
  

• }
  

• mdev->cursor.gbo[i] = gbo;
  

• }
• /*

• * At the high end of video memory, we reserve space for
  

• * buffer objects. The cursor plane uses this memory to store
  

• * a double-buffered image of the current cursor. Hence, it's
  

• * not available for framebuffers.
  

• */
  

• mdev->vram_fb_available -= ncursors * size;
• return 0;

-err_drm_gem_vram_put:

• while (i) {

• --i;
  

• gbo = mdev->cursor.gbo[i];
  

• drm_gem_vram_unpin(gbo);
  

• drm_gem_vram_put(gbo);
  

• mdev->cursor.gbo[i] = NULL;
  

• }
• return ret;

-}

-void mgag200_cursor_fini(struct mga_device *mdev) -{

• size_t i;
• struct drm_gem_vram_object *gbo;
• for (i = 0; i < ARRAY_SIZE(mdev->cursor.gbo); ++i) {

• gbo = mdev->cursor.gbo[i];
  

• drm_gem_vram_unpin(gbo);
  

• drm_gem_vram_put(gbo);
  

• }

-}

-int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,

• 	    uint32_t handle, uint32_t width, uint32_t height)
  

-{

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = (struct mga_device *)dev->dev_private;
• struct drm_gem_object *obj;
• struct drm_gem_vram_object *gbo = NULL;
• int ret;
• u8 *src;
• if (!handle || !file_priv) {

• mgag200_hide_cursor(mdev);
  

• return 0;
  

• }
• if (width != 64 || height != 64) {

• WREG8(MGA_CURPOSXL, 0);
  

• WREG8(MGA_CURPOSXH, 0);
  

• return -EINVAL;
  

• }
• obj = drm_gem_object_lookup(file_priv, handle);
• if (!obj)

• return -ENOENT;
  

• gbo = drm_gem_vram_of_gem(obj);
• src = drm_gem_vram_vmap(gbo);
• if (IS_ERR(src)) {

• ret = PTR_ERR(src);
  

• dev_err(&dev->pdev->dev,
  

• 	"failed to map user buffer updates\n");
  

• goto err_drm_gem_object_put_unlocked;
  

• }
• ret = mgag200_show_cursor(mdev, src, width, height);
• if (ret)

• goto err_drm_gem_vram_vunmap;
  

• /* Now update internal buffer pointers */
• drm_gem_vram_vunmap(gbo, src);
• drm_gem_object_put_unlocked(obj);
• return 0;

-err_drm_gem_vram_vunmap:

• drm_gem_vram_vunmap(gbo, src);

-err_drm_gem_object_put_unlocked:

• drm_gem_object_put_unlocked(obj);
• return ret;

-}

-int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) -{

• struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private;
• /* Our origin is at (64,64) */
• x += 64;
• y += 64;
• mgag200_move_cursor(mdev, x, y);
• return 0;

-} diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 9691252d6233f..c7f2000d46fce 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -121,11 +121,6 @@ struct mga_connector { struct mga_i2c_chan *i2c; };

-struct mga_cursor {

• struct drm_gem_vram_object *gbo[2];
• unsigned int next_index;

-};

struct mga_mc { resource_size_t vram_size; resource_size_t vram_base; @@ -162,8 +157,6 @@ struct mga_device { struct mga_mc mc; struct mga_mode_info mode_info;

• struct mga_cursor cursor;

• size_t vram_fb_available;

  bool suspended;

@@ -210,10 +203,4 @@ int mgag200_mm_init(struct mga_device *mdev); void mgag200_mm_fini(struct mga_device *mdev); int mgag200_mmap(struct file *filp, struct vm_area_struct *vma);

-int mgag200_cursor_init(struct mga_device *mdev); -void mgag200_cursor_fini(struct mga_device *mdev); -int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,

• 	    uint32_t handle, uint32_t width, uint32_t height);
  

-int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y);

#endif /* __MGAG200_DRV_H__ */ diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index b680cf47cbb94..46cc32816f1e1 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -176,16 +176,10 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) goto err_modeset; }

• r = mgag200_cursor_init(mdev);
• if (r)

• dev_warn(&dev->pdev->dev,
  

• 	"Could not initialize cursors. Not doing hardware cursors.\n");
  

• return 0;

err_modeset: drm_mode_config_cleanup(dev);

• mgag200_cursor_fini(mdev); mgag200_mm_fini(mdev);

err_mm: dev->dev_private = NULL; @@ -201,7 +195,6 @@ void mgag200_driver_unload(struct drm_device *dev) return; mgag200_modeset_fini(mdev); drm_mode_config_cleanup(dev);

• mgag200_cursor_fini(mdev); mgag200_mm_fini(mdev); dev->dev_private = NULL;

} diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index d90e83959fca1..c9d120b019649 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1414,8 +1414,6 @@ static void mga_crtc_disable(struct drm_crtc *crtc)

/* These provide the minimum set of functions required to handle a CRTC */ static const struct drm_crtc_funcs mga_crtc_funcs = {

• .cursor_set = mgag200_crtc_cursor_set,
• .cursor_move = mgag200_crtc_cursor_move, .gamma_set = mga_crtc_gamma_set, .set_config = drm_crtc_helper_set_config, .destroy = mga_crtc_destroy,

-- 2.26.0

---

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

Hi Thomas.

On Wed, Apr 29, 2020 at 04:32:24PM +0200, Thomas Zimmermann wrote:

Storing the connector instance in struct mga_device avoids some dynamic memory allocation. Done im preparation of converting mgag200 to simple-KMS helpers.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

One nit below, with that fixed: Acked-by: Sam Ravnborg sam@ravnborg.org

I expect to see mga_i2c_chan embedded in a later patch...

Sam

---

drivers/gpu/drm/mgag200/mgag200_drv.h | 1 + drivers/gpu/drm/mgag200/mgag200_mode.c | 54 ++++++++++++++------------ 2 files changed, 31 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index de3181bd63ca0..09b43a0ff6bbf 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -164,6 +164,7 @@ struct mga_device { /* SE model number stored in reg 0x1e24 */ u32 unique_rev_id;

• struct mga_connector connector; struct drm_encoder encoder;

};

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index ce41bebfdd1a2..eaa3fca7216ac 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1444,6 +1444,10 @@ static void mga_crtc_init(struct mga_device *mdev) drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs); }

+/*

• • Connector
• */

static int mga_vga_get_modes(struct drm_connector *connector) { struct mga_connector *mga_connector = to_mga_connector(connector); @@ -1568,7 +1572,6 @@ static void mga_connector_destroy(struct drm_connector *connector) struct mga_connector *mga_connector = to_mga_connector(connector); mgag200_i2c_destroy(mga_connector->i2c); drm_connector_cleanup(connector);

• kfree(connector);

}

static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = { @@ -1582,37 +1585,39 @@ static const struct drm_connector_funcs mga_vga_connector_funcs = { .destroy = mga_connector_destroy, };

-static struct drm_connector *mga_vga_init(struct drm_device *dev) +static int mgag200_vga_connector_init(struct mga_device *mdev) {

• struct drm_connector *connector;
• struct mga_connector *mga_connector;
• mga_connector = kzalloc(sizeof(struct mga_connector), GFP_KERNEL);
• if (!mga_connector)

• return NULL;
  

• connector = &mga_connector->base;
• mga_connector->i2c = mgag200_i2c_create(dev);
• if (!mga_connector->i2c)

• DRM_ERROR("failed to add ddc bus\n");
  

• struct drm_device *dev = mdev->dev;
• struct mga_connector *mconnector = &mdev->connector;
• struct drm_connector *connector = &mconnector->base;
• struct mga_i2c_chan *i2c;
• int ret;

• drm_connector_init_with_ddc(dev, connector,

• 		    &mga_vga_connector_funcs,
  

• 		    DRM_MODE_CONNECTOR_VGA,
  

• 		    &mga_connector->i2c->adapter);
  

• i2c = mgag200_i2c_create(dev);

• if (!i2c)

• drm_warn(dev, "failed to add DDC bus\n");
  
  

• ret = drm_connector_init_with_ddc(dev, connector,

• 			  &mga_vga_connector_funcs,
  

• 			  DRM_MODE_CONNECTOR_VGA,
  

• 			  &i2c->adapter);
  

• if (ret)

• goto err_mgag200_i2c_destroy;
  

  drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs);

• drm_connector_register(connector);

• mconnector->i2c = i2c;

• return connector;

-}

• return 0;

+err_mgag200_i2c_destroy:

• mgag200_i2c_destroy(i2c);
• return ret;

+}

int mgag200_modeset_init(struct mga_device *mdev) { struct drm_encoder *encoder = &mdev->encoder;

• struct drm_connector *connector;

• struct drm_connector *connector = &mdev->connector.base; int ret;

  mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;

@@ -1632,9 +1637,10 @@ int mgag200_modeset_init(struct mga_device *mdev) } encoder->possible_crtcs = 0x1;

• connector = mga_vga_init(mdev->dev);
• if (!connector) {

• DRM_ERROR("mga_vga_init failed\n");
  

• ret = mgag200_vga_connector_init(mdev);
• if (ret) {

• drm_err(mdev->dev,
  

• 	"mga_vga_connector_init() failed, error %d\n", ret);
  

s/mga_vga_connector_init/mgag200_vga_connector_init/

return -1;

}

-- 2.26.0

On Wed, Apr 29, 2020 at 04:32:25PM +0200, Thomas Zimmermann wrote:

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Nice simplification. Changelog could be a bit more descriptive...

Anyway: Acked-by: Sam Ravnborg sam@ravnborg.org

---

drivers/gpu/drm/mgag200/mgag200_drv.h | 1 - drivers/gpu/drm/mgag200/mgag200_main.c | 18 ------------- drivers/gpu/drm/mgag200/mgag200_mode.c | 37 ++++++++++++++++++++------ 3 files changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 09b43a0ff6bbf..4403145e3593c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -182,7 +182,6 @@ mgag200_flags_from_driver_data(kernel_ulong_t driver_data)

		/* mgag200_mode.c */

int mgag200_modeset_init(struct mga_device *mdev); -void mgag200_modeset_fini(struct mga_device *mdev);

		/* mgag200_main.c */

int mgag200_driver_load(struct drm_device *dev, unsigned long flags); diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c index 698fbf31337d4..cf25012f9b6ec 100644 --- a/drivers/gpu/drm/mgag200/mgag200_main.c +++ b/drivers/gpu/drm/mgag200/mgag200_main.c @@ -10,15 +10,8 @@

#include <linux/pci.h>

-#include <drm/drm_crtc_helper.h> -#include <drm/drm_gem_framebuffer_helper.h>

#include "mgag200_drv.h"

-static const struct drm_mode_config_funcs mga_mode_funcs = {

• .fb_create = drm_gem_fb_create

-};

static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem) { int offset; @@ -159,14 +152,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) if (r) goto err_mm;

• drm_mode_config_init(dev);
• dev->mode_config.funcs = (void *)&mga_mode_funcs;
• if (IS_G200_SE(mdev) && mdev->vram_fb_available < (2048*1024))

• dev->mode_config.preferred_depth = 16;
  

• else

• dev->mode_config.preferred_depth = 32;
  

• dev->mode_config.prefer_shadow = 1;
• r = mgag200_modeset_init(mdev); if (r) { dev_err(&dev->pdev->dev, "Fatal error during modeset init: %d\n", r);

@@ -176,7 +161,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags) return 0;

err_modeset:

• drm_mode_config_cleanup(dev); mgag200_mm_fini(mdev);

err_mm: dev->dev_private = NULL; @@ -190,8 +174,6 @@ void mgag200_driver_unload(struct drm_device *dev)

if (mdev == NULL) return;

• mgag200_modeset_fini(mdev);
• drm_mode_config_cleanup(dev); mgag200_mm_fini(mdev); dev->dev_private = NULL;

} diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index eaa3fca7216ac..3d894b37a0812 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -13,6 +13,7 @@

#include <drm/drm_crtc_helper.h> #include <drm/drm_fourcc.h> +#include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h> @@ -1614,16 +1615,41 @@ static int mgag200_vga_connector_init(struct mga_device *mdev) return ret; }

+static const struct drm_mode_config_funcs mgag200_mode_config_funcs = {

• .fb_create = drm_gem_fb_create

+};

+static unsigned int mgag200_preferred_depth(struct mga_device *mdev) +{

• if (IS_G200_SE(mdev) && mdev->vram_fb_available < (2048*1024))

• return 16;
  

• else

• return 32;
  

+}

int mgag200_modeset_init(struct mga_device *mdev) {

• struct drm_device *dev = mdev->dev; struct drm_encoder *encoder = &mdev->encoder; struct drm_connector *connector = &mdev->connector.base; int ret;

• mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
• mdev->dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;

• ret = drmm_mode_config_init(dev);
• if (ret) {

• drm_err(dev, "drmm_mode_config_init() failed, error %d\n",
  

• 	ret);
  

• return ret;
  

• }
• dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
• dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
• dev->mode_config.preferred_depth = mgag200_preferred_depth(mdev);
• dev->mode_config.prefer_shadow = 1;
• dev->mode_config.fb_base = mdev->mc.vram_base;

• mdev->dev->mode_config.fb_base = mdev->mc.vram_base;

• dev->mode_config.funcs = &mgag200_mode_config_funcs;

  mga_crtc_init(mdev);

@@ -1648,8 +1674,3 @@ int mgag200_modeset_init(struct mga_device *mdev)

return 0; }

-void mgag200_modeset_fini(struct mga_device *mdev) -{

-}

2.26.0

Hi Thomas,

On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote:

All register names and fields are now named according to the MGA programming manuals. The function doesn't need the CRTC, so callers pass in the device structure directly. The logging now uses device-specific macros.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++----------- 2 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4403145e3593c..9b957d9fc7e04 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -61,6 +61,11 @@ WREG8(MGAREG_CRTC_DATA, v); \ } while (0) \

+#define RREG_ECRT(reg, v) \

• do { \

• WREG8(MGAREG_CRTCEXT_INDEX, reg);		\
  

• v = RREG8(MGAREG_CRTCEXT_DATA);			\
  

• } while (0) \

#define WREG_ECRT(reg, v) \ do { \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 3d894b37a0812..b16a73c8617d6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc) }

/*

• This is how the framebuffer base address is stored in g200 cards:
• • Assume @offset is the gpu_addr variable of the framebuffer object
• • Then addr is the number of _pixels_ (not bytes) from the start of

• VRAM to the first pixel we want to display. (divided by 2 for 32bit
  

• framebuffers)
  

• • addr is stored in the CRTCEXT0, CRTCC and CRTCD registers
• addr<20> -> CRTCEXT0<6>
• addr<19-16> -> CRTCEXT0<3-0>
• addr<15-8> -> CRTCC<7-0>
• addr<7-0> -> CRTCD<7-0>
• CRTCEXT0 has to be programmed last to trigger an update and make the
• new addr variable take effect.

• • This is how the framebuffer base address is stored in g200 cards:
• • • Assume @offset is the gpu_addr variable of the framebuffer object
• • • Then addr is the number of _pixels_ (not bytes) from the start of

• • VRAM to the first pixel we want to display. (divided by 2 for 32bit
    

• • framebuffers)
    

• • • addr is stored in the CRTCEXT0, CRTCC and CRTCD registers

• •  addr<20> -> CRTCEXT0<6>
    

• •  addr<19-16> -> CRTCEXT0<3-0>
    

• •  addr<15-8> -> CRTCC<7-0>
    

• •  addr<7-0> -> CRTCD<7-0>
    

• • CRTCEXT0 has to be programmed last to trigger an update and make the
• • new addr variable take effect.
  */

-static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset) +static void mgag200_set_startadd(struct mga_device *mdev,

• 		 unsigned long offset)
  

{

• struct mga_device *mdev = crtc->dev->dev_private;
• u32 addr;
• int count;
• u8 crtcext0;

• struct drm_device *dev = mdev->dev;
• uint32_t startadd;
• uint8_t crtcc, crtcd, crtcext0;

• while (RREG8(0x1fda) & 0x08);
• while (!(RREG8(0x1fda) & 0x08));

• startadd = offset / 8;

• count = RREG8(MGAREG_VCOUNT) + 2;
• while (RREG8(MGAREG_VCOUNT) < count);
• WREG8(MGAREG_CRTCEXT_INDEX, 0);
• crtcext0 = RREG8(MGAREG_CRTCEXT_DATA);
• crtcext0 &= 0xB0;
• addr = offset / 8;
• /* Can't store addresses any higher than that...

•   but we also don't have more than 16MB of memory, so it should be fine. */
  

• WARN_ON(addr > 0x1fffff);
• crtcext0 |= (!!(addr & (1<<20)))<<6;
• WREG_CRT(0x0d, (u8)(addr & 0xff));
• WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff);
• WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0);

• /*

• * Can't store addresses any higher than that, but we also
  

• * don't have more than 16MB of memory, so it should be fine.
  

• */
  

• drm_WARN_ON(dev, startadd > 0x1fffff);
• RREG_ECRT(0x00, crtcext0);
• crtcc = (startadd >> 8) & 0xff;
• crtcd = startadd & 0xff;
• crtcext0 &= 0xb0;

• crtcext0 |= ((startadd >> 14) & BIT(6)) |

It is not so obvious that the value of bit 20 is stored in bit 6 here.

Maybe: crtcext0 |= ((startadd & BIT(20) >> 14) |

I would find the above easier to parse.

•     ((startadd >> 16) & 0x0f);
  

• WREG_CRT(0x0c, crtcc);
• WREG_CRT(0x0d, crtcd);
• WREG_ECRT(0x00, crtcext0);

}

static int mga_crtc_do_set_base(struct drm_crtc *crtc, struct drm_framebuffer *fb, int x, int y, int atomic) {

• struct mga_device *mdev = crtc->dev->dev_private;

Could you use a crtc_to_mdev() macro here. So we avoid adding new users of dev_private?

struct drm_gem_vram_object *gbo; int ret; s64 gpu_addr;

Make this unsigned long and..

@@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, goto err_drm_gem_vram_unpin; }

• mga_set_start_address(crtc, (u32)gpu_addr);

• mgag200_set_startadd(mdev, (unsigned long)gpu_addr);

drop this cast.

return 0;

@@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) {

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• unsigned int count;
• while (RREG8(0x1fda) & 0x08) { }
• while (!(RREG8(0x1fda) & 0x08)) { }
• count = RREG8(MGAREG_VCOUNT) + 2;
• while (RREG8(MGAREG_VCOUNT) < count) { }
• return mga_crtc_do_set_base(crtc, old_fb, x, y, 0);

}

I do not really see why this code was lifter two functions up. Before is was deep in mga_set_start_address(), now it is in mga_crtc_mode_set_base(). Puzzeled?

Sam

-- 2.26.0

Hi

Am 30.04.20 um 10:23 schrieb Thomas Zimmermann:

Hi

Am 29.04.20 um 20:20 schrieb Sam Ravnborg:

...

Hi Thomas,

On Wed, Apr 29, 2020 at 04:32:26PM +0200, Thomas Zimmermann wrote:

...

All register names and fields are now named according to the MGA programming manuals. The function doesn't need the CRTC, so callers pass in the device structure directly. The logging now uses device-specific macros.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/mgag200_drv.h | 5 ++ drivers/gpu/drm/mgag200/mgag200_mode.c | 82 +++++++++++++++----------- 2 files changed, 53 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 4403145e3593c..9b957d9fc7e04 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -61,6 +61,11 @@ WREG8(MGAREG_CRTC_DATA, v); \ } while (0) \

+#define RREG_ECRT(reg, v) \

• do { \

• WREG8(MGAREG_CRTCEXT_INDEX, reg);		\
  

• v = RREG8(MGAREG_CRTCEXT_DATA);			\
  

• } while (0) \

#define WREG_ECRT(reg, v) \ do { \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 3d894b37a0812..b16a73c8617d6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -819,49 +819,53 @@ static void mga_g200wb_commit(struct drm_crtc *crtc) }

/*

• This is how the framebuffer base address is stored in g200 cards:
• • Assume @offset is the gpu_addr variable of the framebuffer object
• • Then addr is the number of _pixels_ (not bytes) from the start of

• VRAM to the first pixel we want to display. (divided by 2 for 32bit
  

• framebuffers)
  

• • addr is stored in the CRTCEXT0, CRTCC and CRTCD registers
• addr<20> -> CRTCEXT0<6>
• addr<19-16> -> CRTCEXT0<3-0>
• addr<15-8> -> CRTCC<7-0>
• addr<7-0> -> CRTCD<7-0>
• CRTCEXT0 has to be programmed last to trigger an update and make the
• new addr variable take effect.

• • This is how the framebuffer base address is stored in g200 cards:
• • • Assume @offset is the gpu_addr variable of the framebuffer object
• • • Then addr is the number of _pixels_ (not bytes) from the start of

• • VRAM to the first pixel we want to display. (divided by 2 for 32bit
    

• • framebuffers)
    

• • • addr is stored in the CRTCEXT0, CRTCC and CRTCD registers

• •  addr<20> -> CRTCEXT0<6>
    

• •  addr<19-16> -> CRTCEXT0<3-0>
    

• •  addr<15-8> -> CRTCC<7-0>
    

• •  addr<7-0> -> CRTCD<7-0>
    

• • CRTCEXT0 has to be programmed last to trigger an update and make the
• • new addr variable take effect.
  */

-static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset) +static void mgag200_set_startadd(struct mga_device *mdev,

• 		 unsigned long offset)
  

{

• struct mga_device *mdev = crtc->dev->dev_private;
• u32 addr;
• int count;
• u8 crtcext0;

• struct drm_device *dev = mdev->dev;
• uint32_t startadd;
• uint8_t crtcc, crtcd, crtcext0;

• while (RREG8(0x1fda) & 0x08);
• while (!(RREG8(0x1fda) & 0x08));

• startadd = offset / 8;

• count = RREG8(MGAREG_VCOUNT) + 2;
• while (RREG8(MGAREG_VCOUNT) < count);
• WREG8(MGAREG_CRTCEXT_INDEX, 0);
• crtcext0 = RREG8(MGAREG_CRTCEXT_DATA);
• crtcext0 &= 0xB0;
• addr = offset / 8;
• /* Can't store addresses any higher than that...

•   but we also don't have more than 16MB of memory, so it should be fine. */
  

• WARN_ON(addr > 0x1fffff);
• crtcext0 |= (!!(addr & (1<<20)))<<6;
• WREG_CRT(0x0d, (u8)(addr & 0xff));
• WREG_CRT(0x0c, (u8)(addr >> 8) & 0xff);
• WREG_ECRT(0x0, ((u8)(addr >> 16) & 0xf) | crtcext0);

• /*

• * Can't store addresses any higher than that, but we also
  

• * don't have more than 16MB of memory, so it should be fine.
  

• */
  

• drm_WARN_ON(dev, startadd > 0x1fffff);
• RREG_ECRT(0x00, crtcext0);
• crtcc = (startadd >> 8) & 0xff;
• crtcd = startadd & 0xff;
• crtcext0 &= 0xb0;

...

• crtcext0 |= ((startadd >> 14) & BIT(6)) |

It is not so obvious that the value of bit 20 is stored in bit 6 here.

Maybe: crtcext0 |= ((startadd & BIT(20) >> 14) |

I would find the above easier to parse.

Ok. I can change that.

Reading the code again, I think it's better to keep it as it is. At least it's consistent with the rest of the function.

Best regards Thomas

...

...

•     ((startadd >> 16) & 0x0f);
  

...

• WREG_CRT(0x0c, crtcc);
• WREG_CRT(0x0d, crtcd);
• WREG_ECRT(0x00, crtcext0);

}

static int mga_crtc_do_set_base(struct drm_crtc *crtc, struct drm_framebuffer *fb, int x, int y, int atomic) {

• struct mga_device *mdev = crtc->dev->dev_private;

Could you use a crtc_to_mdev() macro here. So we avoid adding new users of dev_private?

I introduce such a macro in a later patch. I guess I'll add a separate patch for the macro and the conversion of all these dev_private references.

...

...

struct drm_gem_vram_object *gbo; int ret; s64 gpu_addr;

Make this unsigned long and..

The function that returns gpu_addr can fail (but shouldn't) with a negative error. That's why it's signed.

...

...

@@ -882,7 +886,7 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, goto err_drm_gem_vram_unpin; }

• mga_set_start_address(crtc, (u32)gpu_addr);

• mgag200_set_startadd(mdev, (unsigned long)gpu_addr);

drop this cast.

...

return 0;

@@ -894,6 +898,16 @@ static int mga_crtc_do_set_base(struct drm_crtc *crtc, static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, struct drm_framebuffer *old_fb) {

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• unsigned int count;
• while (RREG8(0x1fda) & 0x08) { }
• while (!(RREG8(0x1fda) & 0x08)) { }
• count = RREG8(MGAREG_VCOUNT) + 2;
• while (RREG8(MGAREG_VCOUNT) < count) { }
• return mga_crtc_do_set_base(crtc, old_fb, x, y, 0);

}

I do not really see why this code was lifter two functions up. Before is was deep in mga_set_start_address(), now it is in mga_crtc_mode_set_base(). Puzzeled?

Ah, that should have probably been explained in the commit message. The above code waits for the vsync flag to go up plus two scanlines. That way the pageflip happens during a vblank period.

It's fairly inefficient AFAICT. I don't want this code in atomic modesetting, but didn't want to throw it away yet. So it's now in the non-atomic callback. Later when the atomic code calls mgag200_set_startadd(), it shouldn't busy-wait for vblanks.

I still have a patch that adds vblank irq support to mgag200. I'd rather merge that instead of keeping this busy waiting in the driver.

Best regards Thomas

...

Sam

...

-- 2.26.0

On Wed, Apr 29, 2020 at 04:32:27PM +0200, Thomas Zimmermann wrote:

The function now only takes the device structure, and the old and new framebuffers.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Acked-by: Sam Ravnborg sam@ravnborg.org

---

drivers/gpu/drm/mgag200/mgag200_mode.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index b16a73c8617d6..12df809d64f7c 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -861,21 +861,20 @@ static void mgag200_set_startadd(struct mga_device *mdev, WREG_ECRT(0x00, crtcext0); }

-static int mga_crtc_do_set_base(struct drm_crtc *crtc,

• 		struct drm_framebuffer *fb,
  

• 		int x, int y, int atomic)
  

+static int mga_crtc_do_set_base(struct mga_device *mdev,

• 		const struct drm_framebuffer *fb,
  

• 		const struct drm_framebuffer *old_fb)
  

{

• struct mga_device *mdev = crtc->dev->dev_private; struct drm_gem_vram_object *gbo; int ret; s64 gpu_addr;

• if (!atomic && fb) {

• gbo = drm_gem_vram_of_gem(fb->obj[0]);
  

• if (old_fb) {

• gbo = drm_gem_vram_of_gem(old_fb->obj[0]);
  

  drm_gem_vram_unpin(gbo); }

• gbo = drm_gem_vram_of_gem(crtc->primary->fb->obj[0]);

• gbo = drm_gem_vram_of_gem(fb->obj[0]);

  ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM); if (ret)

@@ -900,6 +899,7 @@ static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, { struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private;

• struct drm_framebuffer *fb = crtc->primary->fb; unsigned int count;

  while (RREG8(0x1fda) & 0x08) { }

@@ -908,7 +908,7 @@ static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, count = RREG8(MGAREG_VCOUNT) + 2; while (RREG8(MGAREG_VCOUNT) < count) { }

• return mga_crtc_do_set_base(crtc, old_fb, x, y, 0);

• return mga_crtc_do_set_base(mdev, fb, old_fb);

}

static int mga_crtc_mode_set(struct drm_crtc *crtc, @@ -1150,7 +1150,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,

WREG8(MGA_MISC_OUT, misc);

• mga_crtc_do_set_base(crtc, old_fb, x, y, 0);

• mga_crtc_do_set_base(mdev, fb, old_fb);

  /* reset tagfifo */ if (mdev->type == G200_ER) {

-- 2.26.0

On Wed, Apr 29, 2020 at 04:32:28PM +0200, Thomas Zimmermann wrote:

The mode-setting code is now located in mgag200_set_mode_regs(), sans a few flags that will be moved in a later patch for clarity.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

Looked like trivial code movements, did not look carefully. Acked-by: Sam Ravnborg sam@ravnborg.org

---

drivers/gpu/drm/mgag200/mgag200_mode.c | 140 ++++++++++++++----------- 1 file changed, 78 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 12df809d64f7c..749ba6e420ac7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -911,6 +911,79 @@ static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y, return mga_crtc_do_set_base(mdev, fb, old_fb); }

+static void mgag200_set_mode_regs(struct mga_device *mdev,

• 		  const struct drm_display_mode *mode)
  

+{

• unsigned int hdisplay, hsyncstart, hsyncend, htotal;
• unsigned int vdisplay, vsyncstart, vsyncend, vtotal;
• uint8_t misc = 0;
• uint8_t crtcext1, crtcext2, crtcext5;
• hdisplay = mode->hdisplay / 8 - 1;
• hsyncstart = mode->hsync_start / 8 - 1;
• hsyncend = mode->hsync_end / 8 - 1;
• htotal = mode->htotal / 8 - 1;
• /* Work around hardware quirk */
• if ((htotal & 0x07) == 0x06 || (htotal & 0x07) == 0x04)

• htotal++;
  

• vdisplay = mode->vdisplay - 1;
• vsyncstart = mode->vsync_start - 1;
• vsyncend = mode->vsync_end - 1;
• vtotal = mode->vtotal - 2;
• if (mode->flags & DRM_MODE_FLAG_NHSYNC)

• misc |= 0x40;
  

• if (mode->flags & DRM_MODE_FLAG_NVSYNC)

• misc |= 0x80;
  

• crtcext1 = (((htotal - 4) & 0x100) >> 8) |

•    ((hdisplay & 0x100) >> 7) |
  

•    ((hsyncstart & 0x100) >> 6) |
  

•     (htotal & 0x40);
  

• if (mdev->type == G200_WB || mdev->type == G200_EW3)

• crtcext1 |= BIT(7) | /* vrsten */
  

• 	    BIT(3); /* hrsten */
  

• crtcext2 = ((vtotal & 0xc00) >> 10) |

•    ((vdisplay & 0x400) >> 8) |
  

•    ((vdisplay & 0xc00) >> 7) |
  

•    ((vsyncstart & 0xc00) >> 5) |
  

•    ((vdisplay & 0x400) >> 3);
  

• crtcext5 = 0x00;
• WREG_CRT(0, htotal - 4);
• WREG_CRT(1, hdisplay);
• WREG_CRT(2, hdisplay);
• WREG_CRT(3, (htotal & 0x1F) | 0x80);
• WREG_CRT(4, hsyncstart);
• WREG_CRT(5, ((htotal & 0x20) << 2) | (hsyncend & 0x1F));
• WREG_CRT(6, vtotal & 0xFF);
• WREG_CRT(7, ((vtotal & 0x100) >> 8) |

•  ((vdisplay & 0x100) >> 7) |
  

•  ((vsyncstart & 0x100) >> 6) |
  

•  ((vdisplay & 0x100) >> 5) |
  

•  ((vdisplay & 0x100) >> 4) | /* linecomp */
  

•  ((vtotal & 0x200) >> 4) |
  

•  ((vdisplay & 0x200) >> 3) |
  

•  ((vsyncstart & 0x200) >> 2));
  

• WREG_CRT(9, ((vdisplay & 0x200) >> 4) |

•  ((vdisplay & 0x200) >> 3));
  

• WREG_CRT(16, vsyncstart & 0xFF);
• WREG_CRT(17, (vsyncend & 0x0F) | 0x20);
• WREG_CRT(18, vdisplay & 0xFF);
• WREG_CRT(20, 0);
• WREG_CRT(21, vdisplay & 0xFF);
• WREG_CRT(22, (vtotal + 1) & 0xFF);
• WREG_CRT(23, 0xc3);
• WREG_CRT(24, vdisplay & 0xFF);
• WREG_ECRT(0x01, crtcext1);
• WREG_ECRT(0x02, crtcext2);
• WREG_ECRT(0x05, crtcext5);

+}

static int mga_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode, @@ -919,8 +992,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private; const struct drm_framebuffer *fb = crtc->primary->fb;

• int hdisplay, hsyncstart, hsyncend, htotal;
• int vdisplay, vsyncstart, vsyncend, vtotal; int pitch; int option = 0, option2 = 0; int i;

@@ -999,12 +1070,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, break; }

• if (mode->flags & DRM_MODE_FLAG_NHSYNC)

• misc |= 0x40;
  

• if (mode->flags & DRM_MODE_FLAG_NVSYNC)

• misc |= 0x80;
  

• for (i = 0; i < sizeof(dacvalue); i++) { if ((i <= 0x17) || (i == 0x1b) ||

@@ -1044,20 +1109,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, else pitch = pitch >> (4 - bppshift);

• hdisplay = mode->hdisplay / 8 - 1;
• hsyncstart = mode->hsync_start / 8 - 1;
• hsyncend = mode->hsync_end / 8 - 1;
• htotal = mode->htotal / 8 - 1;
• /* Work around hardware quirk */
• if ((htotal & 0x07) == 0x06 || (htotal & 0x07) == 0x04)

• htotal++;
  

• vdisplay = mode->vdisplay - 1;
• vsyncstart = mode->vsync_start - 1;
• vsyncend = mode->vsync_end - 1;
• vtotal = mode->vtotal - 2;
• WREG_GFX(0, 0); WREG_GFX(1, 0); WREG_GFX(2, 0);

@@ -1068,61 +1119,26 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, WREG_GFX(7, 0xf); WREG_GFX(8, 0xf);

• WREG_CRT(0, htotal - 4);
• WREG_CRT(1, hdisplay);
• WREG_CRT(2, hdisplay);
• WREG_CRT(3, (htotal & 0x1F) | 0x80);
• WREG_CRT(4, hsyncstart);
• WREG_CRT(5, ((htotal & 0x20) << 2) | (hsyncend & 0x1F));
• WREG_CRT(6, vtotal & 0xFF);
• WREG_CRT(7, ((vtotal & 0x100) >> 8) |

•  ((vdisplay & 0x100) >> 7) |
  

•  ((vsyncstart & 0x100) >> 6) |
  

•  ((vdisplay & 0x100) >> 5) |
  

•  ((vdisplay & 0x100) >> 4) | /* linecomp */
  

•  ((vtotal & 0x200) >> 4)|
  

•  ((vdisplay & 0x200) >> 3) |
  

•  ((vsyncstart & 0x200) >> 2));
  

• WREG_CRT(9, ((vdisplay & 0x200) >> 4) |

•  ((vdisplay & 0x200) >> 3));
  

  WREG_CRT(10, 0); WREG_CRT(11, 0); WREG_CRT(12, 0); WREG_CRT(13, 0); WREG_CRT(14, 0); WREG_CRT(15, 0);
• WREG_CRT(16, vsyncstart & 0xFF);
• WREG_CRT(17, (vsyncend & 0x0F) | 0x20);
• WREG_CRT(18, vdisplay & 0xFF); WREG_CRT(19, pitch & 0xFF);
• WREG_CRT(20, 0);
• WREG_CRT(21, vdisplay & 0xFF);
• WREG_CRT(22, (vtotal + 1) & 0xFF);
• WREG_CRT(23, 0xc3);
• WREG_CRT(24, vdisplay & 0xFF);

• mgag200_set_mode_regs(mdev, mode);

  ext_vga[0] = 0;

• ext_vga[5] = 0;

  /* TODO interlace */

  ext_vga[0] |= (pitch & 0x300) >> 4;

• ext_vga[1] = (((htotal - 4) & 0x100) >> 8) |

• ((hdisplay & 0x100) >> 7) |
  

• ((hsyncstart & 0x100) >> 6) |
  

• (htotal & 0x40);
  

• ext_vga[2] = ((vtotal & 0xc00) >> 10) |

• ((vdisplay & 0x400) >> 8) |
  

• ((vdisplay & 0xc00) >> 7) |
  

• ((vsyncstart & 0xc00) >> 5) |
  

• ((vdisplay & 0x400) >> 3);
  

  if (fb->format->cpp[0] * 8 == 24) ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80; else ext_vga[3] = ((1 << bppshift) - 1) | 0x80; ext_vga[4] = 0;

• if (mdev->type == G200_WB || mdev->type == G200_EW3)

• ext_vga[1] |= 0x88;
  
  

  /* Set pixel clocks */ misc = 0x2d;

@@ -1130,9 +1146,9 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,

mga_crtc_set_plls(mdev, mode->clock);

• for (i = 0; i < 6; i++) {

• WREG_ECRT(i, ext_vga[i]);
  

• }

• WREG_ECRT(0, ext_vga[0]);

• WREG_ECRT(3, ext_vga[3]);

• WREG_ECRT(4, ext_vga[4]);

  if (mdev->type == G200_ER) WREG_ECRT(0x24, 0x5);

-- 2.26.0

Hi

Am 03.05.20 um 17:34 schrieb Sam Ravnborg:

Hi Thomas.

On Wed, Apr 29, 2020 at 04:32:29PM +0200, Thomas Zimmermann wrote:

...

Set different fields in MISC in their rsp location in the code. This patch also fixes a bug in the original code where the mode's SYNC flags were never written into the MISC register.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/mgag200_mode.c | 37 ++++++++++++++++++-------- drivers/gpu/drm/mgag200/mgag200_reg.h | 5 +++- 2 files changed, 30 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 749ba6e420ac7..b5bb02e9f05d6 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -704,6 +704,8 @@ static int mga_g200er_set_plls(struct mga_device *mdev, long clock)

static int mga_crtc_set_plls(struct mga_device *mdev, long clock) {

• uint8_t misc;

General comment. uint8_t and friends are for uapi stuff. kernel internal prefer u8 and friends.

Ok.

Would be good to clean this up in the drivire, maybe as a follow-up patch after is becomes atomic.

...

• switch(mdev->type) { case G200_SE_A: case G200_SE_B:

@@ -724,6 +726,12 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock) return mga_g200er_set_plls(mdev, clock); break; }

• misc = RREG8(MGA_MISC_IN);
• misc &= ~GENMASK(3, 2);

The code would be easier to read if we had a #define MGAREG_MISC_CLK_SEL_MASK GENMASK(3, 2)

So the above became: misc &= ~MGAREG_MISC_CLK_SEL_MASK;

Then it is more clear that the CLK_SEL bits are clared and then MGA_MSK is set.

Sure.

...

• misc |= MGAREG_MISC_CLK_SEL_MGA_MSK;
• WREG8(MGA_MISC_OUT, misc);
• return 0;

}

@@ -916,7 +924,7 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, { unsigned int hdisplay, hsyncstart, hsyncend, htotal; unsigned int vdisplay, vsyncstart, vsyncend, vtotal;

• uint8_t misc = 0;

• uint8_t misc; uint8_t crtcext1, crtcext2, crtcext5;

  hdisplay = mode->hdisplay / 8 - 1;

@@ -933,10 +941,17 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, vsyncend = mode->vsync_end - 1; vtotal = mode->vtotal - 2;

• misc = RREG8(MGA_MISC_IN);
• if (mode->flags & DRM_MODE_FLAG_NHSYNC)

• misc |= 0x40;
  

• misc |= MGAREG_MISC_HSYNCPOL;
  

• else

• misc &= ~MGAREG_MISC_HSYNCPOL;
  

So the code just assumes DRM_MODE_FLAG_PHSYNC if DRM_MODE_FLAG_NHSYNC is not set. I think that is fine, but it also indicate how hoorible the flags definitions are in mode->flags

If polarity is not negative (i.e., bit set), it is positive (i.e., bit cleared). What else could you set in MISC?

Having multiple flags in mode->flags that signal the same state is somewhat problematic. I expect that the consistency of a mode's flags is validated somewhere within the core.

...

if (mode->flags & DRM_MODE_FLAG_NVSYNC)

• misc |= 0x80;
  

• misc |= MGAREG_MISC_VSYNCPOL;
  

• else

• misc &= ~MGAREG_MISC_VSYNCPOL;
  

And this code was touched in previous patch, but I gess it is better to update it here.

...

crtcext1 = (((htotal - 4) & 0x100) >> 8) | ((hdisplay & 0x100) >> 7) | @@ -982,6 +997,10 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, WREG_ECRT(0x01, crtcext1); WREG_ECRT(0x02, crtcext2); WREG_ECRT(0x05, crtcext5);

• WREG8(MGA_MISC_OUT, misc);
• mga_crtc_set_plls(mdev, mode->clock);

}

static int mga_crtc_mode_set(struct drm_crtc *crtc, @@ -1140,12 +1159,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, ext_vga[3] = ((1 << bppshift) - 1) | 0x80; ext_vga[4] = 0;

• /* Set pixel clocks */
• misc = 0x2d;
• WREG8(MGA_MISC_OUT, misc);
• mga_crtc_set_plls(mdev, mode->clock);
• WREG_ECRT(0, ext_vga[0]); WREG_ECRT(3, ext_vga[3]); WREG_ECRT(4, ext_vga[4]);

@@ -1161,9 +1174,11 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, }

WREG_ECRT(0, ext_vga[0]);

• /* Enable mga pixel clock */
• misc = 0x2d;

• misc = RREG8(MGA_MISC_IN);
• misc |= MGAREG_MISC_IOADSEL |

• MGAREG_MISC_RAMMAPEN |
  

• MGAREG_MISC_HIGH_PG_SEL;
  

  WREG8(MGA_MISC_OUT, misc);

I am left puzzeled why there is several writes to MGA_MISC_OUT. The driver needs to read back and then write again.

Would it be simpler to have one function that only took care of misc register update?

I'm not quite sure what you mean. MISC contains all kinds of unrelated state. In the final atomic code, different state is set in different places. It's simple to have a function read-modify-write the content of MISC for the bits that it cares about. With multiple functions, that leads to some duplicated reads and write, but the code as a whole is simple.

Best regards Thomas

...

mga_crtc_do_set_base(mdev, fb, old_fb); diff --git a/drivers/gpu/drm/mgag200/mgag200_reg.h b/drivers/gpu/drm/mgag200/mgag200_reg.h index c096a9d6bcbc1..89e12c55153cf 100644 --- a/drivers/gpu/drm/mgag200/mgag200_reg.h +++ b/drivers/gpu/drm/mgag200/mgag200_reg.h @@ -16,10 +16,11 @@

• MGA1064SG Mystique register file
  

*/

#ifndef _MGA_REG_H_ #define _MGA_REG_H_

+#include <linux/bits.h>

#define MGAREG_DWGCTL 0x1c00 #define MGAREG_MACCESS 0x1c04 /* the following is a mystique only register */ @@ -227,6 +228,8 @@ #define MGAREG_MISC_CLK_SEL_MGA_MSK (0x3 << 2) #define MGAREG_MISC_VIDEO_DIS (0x1 << 4) #define MGAREG_MISC_HIGH_PG_SEL (0x1 << 5) +#define MGAREG_MISC_HSYNCPOL BIT(6) +#define MGAREG_MISC_VSYNCPOL BIT(7)

I like BIT(), but mixing (1 << N) and BIT() does not look nice. Oh, and do not get me started on GENMASK() :-)

Sam

...

/* MMIO VGA registers */

#define MGAREG_SEQ_INDEX 0x1fc4

2.26.0

---

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

Hi

Am 03.05.20 um 17:42 schrieb Sam Ravnborg:

Hi Thomas.

On Wed, Apr 29, 2020 at 04:32:31PM +0200, Thomas Zimmermann wrote:

...

The framebuffer's pitch is now set in mgag200_set_offset().

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

I failed to follow this code.

Is it because of the line-offset calculation? MGAs want offsets in multiples of pixels, not bytes, and that factor depends on the value of cpp. I guess putting the calculation into a separate function will help with readability.

...

---

drivers/gpu/drm/mgag200/mgag200_mode.c | 41 +++++++++++++++++--------- 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 92dee31f16c42..eb83e471d72fc 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1003,6 +1003,32 @@ static void mgag200_set_mode_regs(struct mga_device *mdev, mga_crtc_set_plls(mdev, mode->clock); }

+static void mgag200_set_offset(struct mga_device *mdev,

• 	       const struct drm_framebuffer *fb)
  

+{

• unsigned int offset;
• uint8_t crtc13, crtcext0;
• u8 bppshift;
• bppshift = mdev->bpp_shifts[fb->format->cpp[0] - 1];

Hmm, use of cpp is deprecated. But is continue to be used all over.

I try to move code without modifying it too much. Replacing cpp with some newer interface would obfuscate the moving to some extend. It's always a balancing act.

...

• offset = fb->pitches[0] / fb->format->cpp[0];
• if (fb->format->cpp[0] * 8 == 24)

• offset = (offset * 3) >> (4 - bppshift);
  

• else

• offset = offset >> (4 - bppshift);
  

• RREG_ECRT(0, crtcext0);
• crtc13 = offset & 0xff;
• crtcext0 &= ~GENMASK(5, 4);
• crtcext0 |= (offset & GENMASK(9, 8)) >> 4;

Lot's of hardcoded numbers. Could the reg file include these so you had more readable defined names?

Sure.

Best regards Thomas

...

• WREG_CRT(0x13, crtc13);
• WREG_ECRT(0x00, crtcext0);

+}

static int mga_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode, @@ -1011,7 +1037,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private; const struct drm_framebuffer *fb = crtc->primary->fb;

• int pitch; int option = 0, option2 = 0; int i; unsigned char misc = 0;

@@ -1122,12 +1147,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, WREG_SEQ(3, 0); WREG_SEQ(4, 0xe);

• pitch = fb->pitches[0] / fb->format->cpp[0];
• if (fb->format->cpp[0] * 8 == 24)

• pitch = (pitch * 3) >> (4 - bppshift);
  

• else

• pitch = pitch >> (4 - bppshift);
  

• WREG_GFX(0, 0); WREG_GFX(1, 0); WREG_GFX(2, 0);

@@ -1144,20 +1163,15 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, WREG_CRT(13, 0); WREG_CRT(14, 0); WREG_CRT(15, 0);

• WREG_CRT(19, pitch & 0xFF);

• ext_vga[0] = 0;

  /* TODO interlace */

• ext_vga[0] |= (pitch & 0x300) >> 4; if (fb->format->cpp[0] * 8 == 24) ext_vga[3] = (((1 << bppshift) * 3) - 1) | 0x80; else ext_vga[3] = ((1 << bppshift) - 1) | 0x80; ext_vga[4] = 0;

• WREG_ECRT(0, ext_vga[0]); WREG_ECRT(3, ext_vga[3]); WREG_ECRT(4, ext_vga[4]);

@@ -1171,8 +1185,6 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, WREG_ECRT(6, 0); }

• WREG_ECRT(0, ext_vga[0]);
• misc = RREG8(MGA_MISC_IN); misc |= MGAREG_MISC_IOADSEL | MGAREG_MISC_RAMMAPEN |

@@ -1180,6 +1192,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc, WREG8(MGA_MISC_OUT, misc);

mga_crtc_do_set_base(mdev, fb, old_fb);

• mgag200_set_offset(mdev, fb);

  mgag200_set_mode_regs(mdev, mode);

Sam

Hi

Am 03.05.20 um 18:25 schrieb Sam Ravnborg:

Hi Thomas.

One nit about a bit name below. Acked-by: Sam Ravnborg sam@ravnborg.org

On Wed, Apr 29, 2020 at 04:32:33PM +0200, Thomas Zimmermann wrote:

...

5

The TAGFIFO state is now reset in mgag200_g200er_reset_tagfifo().

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/mgag200_drv.h | 6 ++++ drivers/gpu/drm/mgag200/mgag200_mode.c | 45 +++++++++++++++++--------- 2 files changed, 35 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index 9b957d9fc7e04..b10da90e0f35a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -49,6 +49,12 @@ WREG8(ATTR_DATA, v); \ } while (0) \

+#define RREG_SEQ(reg, v) \

• do { \

• WREG8(MGAREG_SEQ_INDEX, reg);			\
  

• v = RREG8(MGAREG_SEQ_DATA);			\
  

• } while (0) \

#define WREG_SEQ(reg, v) \ do { \ WREG8(MGAREG_SEQ_INDEX, reg); \ diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 73f7135cbb3d8..6b88c306ff4d7 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -1091,6 +1091,33 @@ static void mgag200_set_format_regs(struct mga_device *mdev, WREG_ECRT(3, crtcext3); }

+static void mgag200_g200er_reset_tagfifo(struct mga_device *mdev) +{

• static uint32_t RESET_FLAG = 0x00200000; /* undocumented magic value */
• u8 seq1;
• u32 memctl;
• /* screen off */
• RREG_SEQ(0x01, seq1);
• seq1 |= 0x20;

This looks like this: #define M_SEQ1_SCROFF 0x20

...

• WREG_SEQ(0x01, seq1);
• memctl = RREG32(MGAREG_MEMCTL);
• memctl |= RESET_FLAG;
• WREG32(MGAREG_MEMCTL, memctl);
• udelay(1000);
• memctl &= ~RESET_FLAG;
• WREG32(MGAREG_MEMCTL, memctl);
• /* screen on */
• RREG_SEQ(0x01, seq1);
• seq1 &= ~0x20;
• WREG_SEQ(0x01, seq1);

Here seq1 is read again, the old code used the old value. I think new code is better.

You mean 'the old code was better,' right?

Best regards Thomas

...

+}

static int mga_crtc_mode_set(struct drm_crtc *crtc, struct drm_display_mode *mode, struct drm_display_mode *adjusted_mode, @@ -1225,22 +1252,8 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,

mgag200_set_mode_regs(mdev, mode);

• /* reset tagfifo */
• if (mdev->type == G200_ER) {

• u32 mem_ctl = RREG32(MGAREG_MEMCTL);
  

• u8 seq1;
  

• /* screen off */
  

• WREG8(MGAREG_SEQ_INDEX, 0x01);
  

• seq1 = RREG8(MGAREG_SEQ_DATA) | 0x20;
  

• WREG8(MGAREG_SEQ_DATA, seq1);
  

• WREG32(MGAREG_MEMCTL, mem_ctl | 0x00200000);
  

• udelay(1000);
  

• WREG32(MGAREG_MEMCTL, mem_ctl & ~0x00200000);
  

• WREG8(MGAREG_SEQ_DATA, seq1 & ~0x20);
  

• }

• if (mdev->type == G200_ER)

• mgag200_g200er_reset_tagfifo(mdev);
  
  
  

  if (IS_G200_SE(mdev)) {

-- 2.26.0

---

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

Hi

Am 04.05.20 um 14:10 schrieb Daniel Vetter:

On Wed, Apr 29, 2020 at 04:32:36PM +0200, Thomas Zimmermann wrote:

...

The mgag200 drivers waits for the VSYNC flag to get signalled (i.e., the page flip happens) before changing DPMS settings. This doesn't work reliably if no mode has been programmed. Therefore remove the waiting code. Synchronization with page flips should be done by DRM's vblank handlers anyway.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

This looks a bit dangerous, hw might get angry if we drop these waits.

Generally with atomic you should never have a situation in driver code where you expect the display to be on, but it isn't. So this should be fixable by making sure we're calling this dpms function at the right spot, e.g. for the enable path obviously the display is always going to be off, and for the disable path the display is guaranteed to be on. So maybe just a bool enable, or split the dpms function into two.

I think this code was taken from the X11 userspace driver, which does that same waiting.

But it's waiting even for the DPMS_ON case. If the signal generation is disabled, why does it wait for the vsync flag? After a while the code times out from a timeout given in HZ/jiffies. I would have expected a value in usec. All this makes it somewhat dubious and I doubt that it's actually correct.

If we want to keep the waiting, I agree on splitting the code into an enable and a disable function.

The driver also busy waited during pageflips (see patch 5). That should really be done with interrupts.

Best regards Thomas

The old legacy helpers where a lot more fast&loose in this regard. -Daniel

...

---

drivers/gpu/drm/mgag200/mgag200_mode.c | 26 -------------------------- 1 file changed, 26 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index ee1cbe5decd71..884fc668a6dae 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -75,30 +75,6 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) } }

-static inline void mga_wait_vsync(struct mga_device *mdev) -{

• unsigned long timeout = jiffies + HZ/10;
• unsigned int status = 0;
• do {

• status = RREG32(MGAREG_Status);
  

• } while ((status & 0x08) && time_before(jiffies, timeout));
• timeout = jiffies + HZ/10;
• status = 0;
• do {

• status = RREG32(MGAREG_Status);
  

• } while (!(status & 0x08) && time_before(jiffies, timeout));

-}

-static inline void mga_wait_busy(struct mga_device *mdev) -{

• unsigned long timeout = jiffies + HZ;
• unsigned int status = 0;
• do {

• status = RREG8(MGAREG_Status + 2);
  

• } while ((status & 0x01) && time_before(jiffies, timeout));

-}

#define P_ARRAY_SIZE 9

static int mga_g200se_set_plls(struct mga_device *mdev, long clock) @@ -1435,8 +1411,6 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode) #endif WREG8(MGAREG_SEQ_INDEX, 0x01); seq1 |= RREG8(MGAREG_SEQ_DATA) & ~0x20;

• mga_wait_vsync(mdev);
• mga_wait_busy(mdev); WREG8(MGAREG_SEQ_DATA, seq1); msleep(20); WREG8(MGAREG_CRTCEXT_INDEX, 0x01);

-- 2.26.0

Hi Thomas.

Introducing drm_simple_display_pipe to hold some of the data, and then a later patch to change over would have helped the readability I think.

Anyway - deleted unused code belongs to another patch. Other than tthose details I did not spot anything, but then I did not read this patch as carefully as some of the others.

With the removed code migrated to another aptch, and the missing code explained the patch is: Acked-by: Sam Ravnborg sam@ravnborg.org

On Wed, Apr 29, 2020 at 04:32:37PM +0200, Thomas Zimmermann wrote:

The mgag200 supports a single pipeline with only a primary plane. It can be converted to simple KMS helpers. This also adds support for atomic modesetting. Wayland compositors, which use pageflip ioctls, can now be used with mgag200.

Signed-off-by: Thomas Zimmermann tzimmermann@suse.de

drivers/gpu/drm/mgag200/mgag200_drv.c | 2 +- drivers/gpu/drm/mgag200/mgag200_drv.h | 4 +- drivers/gpu/drm/mgag200/mgag200_mode.c | 396 +++++++++++-------------- 3 files changed, 171 insertions(+), 231 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c index 3298b7ef18b03..b1272165621ed 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.c +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c @@ -140,7 +140,7 @@ int mgag200_driver_dumb_create(struct drm_file *file, }

static struct drm_driver driver = {

• .driver_features = DRIVER_GEM | DRIVER_MODESET,

• .driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET, .fops = &mgag200_driver_fops, .name = DRIVER_NAME, .desc = DRIVER_DESC,

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h index b10da90e0f35a..2e407508714c8 100644 --- a/drivers/gpu/drm/mgag200/mgag200_drv.h +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h @@ -19,6 +19,7 @@ #include <drm/drm_fb_helper.h> #include <drm/drm_gem.h> #include <drm/drm_gem_vram_helper.h> +#include <drm/drm_simple_kms_helper.h>

#include "mgag200_reg.h"

@@ -107,6 +108,7 @@

#define to_mga_crtc(x) container_of(x, struct mga_crtc, base) #define to_mga_connector(x) container_of(x, struct mga_connector, base) +#define to_mga_device(x) (dev->dev_private)

Upclassing please.

struct mga_crtc { struct drm_crtc base; @@ -176,7 +178,7 @@ struct mga_device { u32 unique_rev_id;

struct mga_connector connector;

• struct drm_encoder encoder;

• struct drm_simple_display_pipe display_pipe;

};

static inline enum mga_type diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c index 884fc668a6dae..d9b4055e38982 100644 --- a/drivers/gpu/drm/mgag200/mgag200_mode.c +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c @@ -11,10 +11,13 @@ #include <linux/delay.h> #include <linux/pci.h>

+#include <drm/drm_atomic_helper.h> +#include <drm/drm_atomic_state_helper.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fourcc.h> #include <drm/drm_gem_framebuffer_helper.h> #include <drm/drm_plane_helper.h> +#include <drm/drm_print.h> #include <drm/drm_probe_helper.h> #include <drm/drm_simple_kms_helper.h>

@@ -30,13 +33,18 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private;

• struct drm_framebuffer *fb = crtc->primary->fb;

• struct drm_framebuffer *fb; u16 *r_ptr, *g_ptr, *b_ptr; int i;

  if (!crtc->enabled) return;

• if (!mdev->display_pipe.plane.state)

• return;
  

• fb = mdev->display_pipe.plane.state->fb;

• r_ptr = crtc->gamma_store; g_ptr = r_ptr + crtc->gamma_size; b_ptr = g_ptr + crtc->gamma_size;

@@ -845,56 +853,6 @@ static void mgag200_set_startadd(struct mga_device *mdev, WREG_ECRT(0x00, crtcext0); }

-static int mga_crtc_do_set_base(struct mga_device *mdev,

• 		const struct drm_framebuffer *fb,
  

• 		const struct drm_framebuffer *old_fb)
  

-{

• struct drm_gem_vram_object *gbo;
• int ret;
• s64 gpu_addr;
• if (old_fb) {

• gbo = drm_gem_vram_of_gem(old_fb->obj[0]);
  

• drm_gem_vram_unpin(gbo);
  

• }
• gbo = drm_gem_vram_of_gem(fb->obj[0]);
• ret = drm_gem_vram_pin(gbo, DRM_GEM_VRAM_PL_FLAG_VRAM);
• if (ret)

• return ret;
  

• gpu_addr = drm_gem_vram_offset(gbo);
• if (gpu_addr < 0) {

• ret = (int)gpu_addr;
  

• goto err_drm_gem_vram_unpin;
  

• }
• mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
• return 0;

-err_drm_gem_vram_unpin:

• drm_gem_vram_unpin(gbo);
• return ret;

-}

-static int mga_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,

• 		  struct drm_framebuffer *old_fb)
  

-{

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• struct drm_framebuffer *fb = crtc->primary->fb;
• unsigned int count;
• while (RREG8(0x1fda) & 0x08) { }
• while (!(RREG8(0x1fda) & 0x08)) { }
• count = RREG8(MGAREG_VCOUNT) + 2;
• while (RREG8(MGAREG_VCOUNT) < count) { }

I think we discussed the above lines before. I cannot find then in the converted code.

• return mga_crtc_do_set_base(mdev, fb, old_fb);

-}

static void mgag200_set_pci_regs(struct mga_device *mdev) { uint32_t option = 0, option2 = 0; @@ -1291,93 +1249,6 @@ static void mgag200_g200ev_set_hiprilvl(struct mga_device *mdev) WREG_ECRT(0x06, 0x00); }

-static int mga_crtc_mode_set(struct drm_crtc *crtc,

• 		struct drm_display_mode *mode,
  

• 		struct drm_display_mode *adjusted_mode,
  

• 		int x, int y, struct drm_framebuffer *old_fb)
  

-{

• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• const struct drm_framebuffer *fb = crtc->primary->fb;
• mgag200_init_regs(mdev);
• mgag200_set_format_regs(mdev, fb);
• mga_crtc_do_set_base(mdev, fb, old_fb);
• mgag200_set_offset(mdev, fb);
• mgag200_set_mode_regs(mdev, mode);
• if (mdev->type == G200_ER)

• mgag200_g200er_reset_tagfifo(mdev);
  

• if (IS_G200_SE(mdev))

• mgag200_g200se_set_hiprilvl(mdev, mode, fb);
  

• else if (mdev->type == G200_EV)

• mgag200_g200ev_set_hiprilvl(mdev);
  

• return 0;

-}

This part

-#if 0 /* code from mjg to attempt D3 on crtc dpms off - revisit later */ -static int mga_suspend(struct drm_crtc *crtc) -{

• struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• struct pci_dev *pdev = dev->pdev;
• int option;
• if (mdev->suspended)

• return 0;
  

• WREG_SEQ(1, 0x20);
• WREG_ECRT(1, 0x30);
• /* Disable the pixel clock */
• WREG_DAC(0x1a, 0x05);
• /* Power down the DAC */
• WREG_DAC(0x1e, 0x18);
• /* Power down the pixel PLL */
• WREG_DAC(0x1a, 0x0d);
• /* Disable PLLs and clocks */
• pci_read_config_dword(pdev, PCI_MGA_OPTION, &option);
• option &= ~(0x1F8024);
• pci_write_config_dword(pdev, PCI_MGA_OPTION, option);
• pci_set_power_state(pdev, PCI_D3hot);
• pci_disable_device(pdev);
• mdev->suspended = true;
• return 0;

-}

-static int mga_resume(struct drm_crtc *crtc) -{

• struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = dev->dev_private;
• struct pci_dev *pdev = dev->pdev;
• int option;
• if (!mdev->suspended)

• return 0;
  

• pci_set_power_state(pdev, PCI_D0);
• pci_enable_device(pdev);
• /* Disable sysclk */
• pci_read_config_dword(pdev, PCI_MGA_OPTION, &option);
• option &= ~(0x4);
• pci_write_config_dword(pdev, PCI_MGA_OPTION, option);
• mdev->suspended = false;
• return 0;

-}

-#endif

ending here - does not belong in this patch.

static void mga_crtc_dpms(struct drm_crtc *crtc, int mode) { struct drm_device *dev = crtc->dev; @@ -1470,7 +1341,6 @@ static void mga_crtc_commit(struct drm_crtc *crtc) { struct drm_device *dev = crtc->dev; struct mga_device *mdev = dev->dev_private;

• const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private; u8 tmp;

  if (mdev->type == G200_WB || mdev->type == G200_EW3)

@@ -1489,78 +1359,7 @@ static void mga_crtc_commit(struct drm_crtc *crtc) WREG_SEQ(0x1, tmp); WREG_SEQ(0, 3); }

• crtc_funcs->dpms(crtc, DRM_MODE_DPMS_ON);

-}

-/*

• • The core can pass us a set of gamma values to program. We actually only
• • use this for 8-bit mode so can't perform smooth fades on deeper modes,
• • but it's a requirement that we provide the function
• */

-static int mga_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,

• 	      u16 *blue, uint32_t size,
  

• 	      struct drm_modeset_acquire_ctx *ctx)
  

-{

• mga_crtc_load_lut(crtc);
• return 0;

-}

-/* Simple cleanup function */ -static void mga_crtc_destroy(struct drm_crtc *crtc) -{

• struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
• drm_crtc_cleanup(crtc);
• kfree(mga_crtc);

-}

-static void mga_crtc_disable(struct drm_crtc *crtc) -{

• DRM_DEBUG_KMS("\n");
• mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
• if (crtc->primary->fb) {

• struct drm_framebuffer *fb = crtc->primary->fb;
  

• struct drm_gem_vram_object *gbo =
  

• 	drm_gem_vram_of_gem(fb->obj[0]);
  

• drm_gem_vram_unpin(gbo);
  

• }
• crtc->primary->fb = NULL;

-}

-/* These provide the minimum set of functions required to handle a CRTC */ -static const struct drm_crtc_funcs mga_crtc_funcs = {

• .gamma_set = mga_crtc_gamma_set,
• .set_config = drm_crtc_helper_set_config,
• .destroy = mga_crtc_destroy,

-};

-static const struct drm_crtc_helper_funcs mga_helper_funcs = {

• .disable = mga_crtc_disable,
• .dpms = mga_crtc_dpms,
• .mode_set = mga_crtc_mode_set,
• .mode_set_base = mga_crtc_mode_set_base,
• .prepare = mga_crtc_prepare,
• .commit = mga_crtc_commit,

-};

-/* CRTC setup */ -static void mga_crtc_init(struct mga_device *mdev) -{

• struct mga_crtc *mga_crtc;
• mga_crtc = kzalloc(sizeof(struct mga_crtc) +

• 	      (MGAG200FB_CONN_LIMIT * sizeof(struct drm_connector *)),
  

• 	      GFP_KERNEL);
  

• if (mga_crtc == NULL)

• return;
  

• drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
• drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
• drm_crtc_helper_add(&mga_crtc->base, &mga_helper_funcs);

• mga_crtc_dpms(crtc, DRM_MODE_DPMS_ON);

}

/* @@ -1694,14 +1493,16 @@ static void mga_connector_destroy(struct drm_connector *connector) }

static const struct drm_connector_helper_funcs mga_vga_connector_helper_funcs = {

• .get_modes = mga_vga_get_modes,

• .get_modes = mga_vga_get_modes, .mode_valid = mga_vga_mode_valid,

};

static const struct drm_connector_funcs mga_vga_connector_funcs = {

• .dpms = drm_helper_connector_dpms,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = mga_connector_destroy,

• .reset = drm_atomic_helper_connector_reset,
• .fill_modes = drm_helper_probe_single_connector_modes,
• .destroy = mga_connector_destroy,
• .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
• .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,

};

static int mgag200_vga_connector_init(struct mga_device *mdev) @@ -1733,8 +1534,138 @@ static int mgag200_vga_connector_init(struct mga_device *mdev) return ret; }

+/*

• • Simple Display Pipe
• */

+static enum drm_mode_status +mgag200_simple_display_pipe_mode_valid(struct drm_simple_display_pipe *pipe,

• 		       const struct drm_display_mode *mode)
  

+{

• return MODE_OK;

+}

+static void +mgag200_simple_display_pipe_enable(struct drm_simple_display_pipe *pipe,

• 		   struct drm_crtc_state *crtc_state,
  

• 		   struct drm_plane_state *plane_state)
  

+{

• struct drm_crtc *crtc = &pipe->crtc;
• struct drm_device *dev = crtc->dev;
• struct mga_device *mdev = to_mga_device(dev);
• struct drm_display_mode *adjusted_mode = &crtc_state->adjusted_mode;
• struct drm_framebuffer *fb = plane_state->fb;
• struct drm_gem_vram_object *gbo;
• s64 gpu_addr;
• 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: BO should have been pinned to VRAM. */
  

• mga_crtc_prepare(crtc);
• mgag200_set_format_regs(mdev, fb);
• mgag200_set_mode_regs(mdev, adjusted_mode);
• if (mdev->type == G200_ER)

• mgag200_g200er_reset_tagfifo(mdev);
  

• if (IS_G200_SE(mdev))

• mgag200_g200se_set_hiprilvl(mdev, adjusted_mode, fb);
  

• else if (mdev->type == G200_EV)

• mgag200_g200ev_set_hiprilvl(mdev);
  

• mga_crtc_commit(crtc);

+}

+static void +mgag200_simple_display_pipe_disable(struct drm_simple_display_pipe *pipe) +{

• struct drm_crtc *crtc = &pipe->crtc;
• mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);

+}

+static int +mgag200_simple_display_pipe_check(struct drm_simple_display_pipe *pipe,

• 		  struct drm_plane_state *plane_state,
  

• 		  struct drm_crtc_state *crtc_state)
  

+{

• struct drm_plane *plane = plane_state->plane;
• struct drm_framebuffer *new_fb = plane_state->fb;
• struct drm_framebuffer *fb = NULL;
• if (!new_fb)

• return 0;
  

• if (plane->state)

• fb = plane->state->fb;
  

• if (!fb || (fb->format != new_fb->format))

• crtc_state->mode_changed = true; /* update PLL settings */
  

• return 0;

+}

+static void +mgag200_simple_display_pipe_update(struct drm_simple_display_pipe *pipe,

• 		   struct drm_plane_state *old_state)
  

+{

• struct drm_plane *plane = &pipe->plane;
• struct drm_device *dev = plane->dev;
• struct mga_device *mdev = to_mga_device(dev);
• struct drm_plane_state *state = plane->state;
• struct drm_framebuffer *fb = state->fb;
• struct drm_gem_vram_object *gbo;
• s64 gpu_addr;
• if (!fb)

• return;
  

• 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: BO should have been pinned to VRAM. */
  

• mgag200_set_format_regs(mdev, fb);
• mgag200_set_startadd(mdev, (unsigned long)gpu_addr);
• mgag200_set_offset(mdev, fb);

+}

+static const struct drm_simple_display_pipe_funcs +mgag200_simple_display_pipe_funcs = {

• .mode_valid = mgag200_simple_display_pipe_mode_valid,
• .enable = mgag200_simple_display_pipe_enable,
• .disable = mgag200_simple_display_pipe_disable,
• .check = mgag200_simple_display_pipe_check,
• .update = mgag200_simple_display_pipe_update,
• .prepare_fb = drm_gem_vram_simple_display_pipe_prepare_fb,
• .cleanup_fb = drm_gem_vram_simple_display_pipe_cleanup_fb,

+};

+static const uint32_t mgag200_simple_display_pipe_formats[] = {

• DRM_FORMAT_XRGB8888,
• DRM_FORMAT_RGB565,
• DRM_FORMAT_RGB888,

+};

+static const uint64_t mgag200_simple_display_pipe_modifiers[] = {

• DRM_FORMAT_MOD_LINEAR,
• DRM_FORMAT_MOD_INVALID

+};

+/*

• • Mode config
• */

static const struct drm_mode_config_funcs mgag200_mode_config_funcs = {

• .fb_create = drm_gem_fb_create

• .fb_create = drm_gem_fb_create,
• .mode_valid = drm_vram_helper_mode_valid,
• .atomic_check = drm_atomic_helper_check,
• .atomic_commit = drm_atomic_helper_commit,

};

static unsigned int mgag200_preferred_depth(struct mga_device *mdev) @@ -1748,10 +1679,13 @@ static unsigned int mgag200_preferred_depth(struct mga_device *mdev) int mgag200_modeset_init(struct mga_device *mdev) { struct drm_device *dev = mdev->dev;

• struct drm_encoder *encoder = &mdev->encoder; struct drm_connector *connector = &mdev->connector.base;

• struct drm_simple_display_pipe *pipe = &mdev->display_pipe;

• size_t format_count = ARRAY_SIZE(mgag200_simple_display_pipe_formats); int ret;

• mgag200_init_regs(mdev);

• ret = drmm_mode_config_init(dev); if (ret) { drm_err(dev, "drmm_mode_config_init() failed, error %d\n",

@@ -1769,26 +1703,30 @@ int mgag200_modeset_init(struct mga_device *mdev)

dev->mode_config.funcs = &mgag200_mode_config_funcs;

• mga_crtc_init(mdev);
• ret = drm_simple_encoder_init(mdev->dev, encoder,

• 		      DRM_MODE_ENCODER_DAC);
  

• ret = mgag200_vga_connector_init(mdev); if (ret) {

• drm_err(mdev->dev,
  

• 	"drm_simple_encoder_init() failed, error %d\n",
  

• drm_err(dev, "mga_vga_connector_init() failed, error %d\n",
  ret);
  

  return ret; }

• encoder->possible_crtcs = 0x1;

• ret = mgag200_vga_connector_init(mdev);

• ret = drm_simple_display_pipe_init(dev, pipe,

• 			   &mgag200_simple_display_pipe_funcs,
  

• 			   mgag200_simple_display_pipe_formats,
  

• 			   format_count,
  

• 			   mgag200_simple_display_pipe_modifiers,
  

• 			   connector);
  

  if (ret) {

• drm_err(mdev->dev,
  

• 	"mga_vga_connector_init() failed, error %d\n", ret);
  

• return -1;
  

• drm_err(dev,
  

• 	"drm_simple_display_pipe_init() failed, error %d\n",
  

• 	ret);
  

• return ret;
  

  }

• drm_connector_attach_encoder(connector, encoder);

• /* FIXME: legacy gamma tables; convert to CRTC state */

• drm_mode_crtc_set_gamma_size(&pipe->crtc, MGAG200_LUT_SIZE);

• drm_mode_config_reset(dev);

  return 0;

}

2.26.0

---

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