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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxxx>

	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@xxxxxxx>
> >> ---
> >>  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@xxxxxxxxxx>
> >> - */
> >> -
> >> -#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@xxxxxxxxxxxxxxxxxxxxx
> > 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
> 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux