Re: [PATCH 01/17] drm/mgag200: Remove HW cursor
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
>
> 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
> >> ---
> >> 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..0
> >> --- 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
> >> - */
> >> -
> >> -#include
> >> -
> >> -#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
Re: [PATCH 01/17] drm/mgag200: Remove HW cursor
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
>> ---
>> 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..0
>> --- 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
>> - */
>> -
>> -#include
>> -
>> -#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) {
>> -
Re: [PATCH 01/17] drm/mgag200: Remove HW cursor
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 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 01/17] drm/mgag200: Remove HW cursor
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
> ---
> 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..0
> --- 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
> - */
> -
> -#include
> -
> -#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);
> - }
> -
> - /*
