diff mbox series

[01/17] drm/mgag200: Remove HW cursor

Message ID 20200429143238.10115-2-tzimmermann@suse.de (mailing list archive)
State New, archived
Headers show
Series drm/mgag200: Convert to atomic modesetting | expand

Commit Message

Thomas Zimmermann April 29, 2020, 2:32 p.m. UTC
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.

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

Comments

Sam Ravnborg April 29, 2020, 5:51 p.m. UTC | #1
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
Gerd Hoffmann April 30, 2020, 7:03 a.m. UTC | #2
On Wed, Apr 29, 2020 at 07:51:07PM +0200, Sam Ravnborg wrote:
> 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?

xorg/wayland switch to software cursor then.  Shouldn't be a big
difference.  If you wanna check how userspace behaves without g200
hardware you can try qemu.  stdvga (bochs-drm.ko) has no hardware
cursor, virtio-vga (virtio-gpu.ko) has a hardware cursor.

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

cirrus driver does the same.  The hardware has cursor support, but not
rgba, and it is not used.

take care,
  Gerd
Thomas Zimmermann April 30, 2020, 8:10 a.m. UTC | #3
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
>
Sam Ravnborg April 30, 2020, 9:19 a.m. UTC | #4
Hi Thomas.

On Thu, Apr 30, 2020 at 10:10:53AM +0200, Thomas Zimmermann wrote:
> 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.

Could you re-phrase this a little and add to the changelog.
So later if one wonders, get an explanation why removing the curosr
support is OK.

I think, with the above, I would not have questioned the removal.

With the updated changelog:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

	Sam

> 
> 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
> > 
> 
> -- 
> 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
>
diff mbox series

Patch

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,