[PATCH 00/14] drm/mgag200: Use managed interfaces for auto-cleanup

[PATCH 00/14] drm/mgag200: Use managed interfaces for auto-cleanup

[Thomas Zimmermann]

This patchset cleans up mgag200 device initialization, embeds the
DRM device instance in struct mga_device and finally converts device
initialization to managed interfaces.

Patches 1 and 2 are actually unrelated. Both remove artifacts that got
lost from earlier patch series. We're fixing this before introducing new
changes.

Patches 3 to 7 cleanup the memory management code and convert it to
managed. Specifically, all MM code is being moved into a the same file.
That makes it more obvious what's going on and will allow for further
cleanups later on.

Modesetting is already cleaned up automatically, so there's nothing
to do here.

With modesetting and MM done, patches 8 to 14 convert the device
initialization to managed interfaces. The allocation is split among
several functions and we move it to the same place in patches 11 and
12. That is also a good opportunity to embed the DRM device instance
in struct mga_device in patch 13. Patch 14 adds managed release of the
device structure.

Tested on Matrox G200SE HW.

Thomas Zimmermann (14):
  drm/mgag200: Remove declaration of mgag200_mmap() from header file
  drm/mgag200: Remove mgag200_cursor.c
  drm/mgag200: Use pcim_enable_device()
  drm/mgag200: Rename mgag200_ttm.c to mgag200_mm.c
  drm/mgag200: Lookup VRAM PCI BAR start and length only once
  drm/mgag200: Merge VRAM setup into MM initialization
  drm/mgag200: Switch to managed MM
  drm/mgag200: Separate DRM and PCI functionality from each other
  drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_
  drm/mgag200: Move device init and cleanup to mgag200_drv.c
  drm/mgag200: Separate device initialization into allocation
  drm/mgag200: Allocate device structures in mgag200_driver_load()
  drm/mgag200: Embed instance of struct drm_device in struct mga_device
  drm/mgag200: Use managed device initialization

 drivers/gpu/drm/mgag200/Makefile              |   3 +-
 drivers/gpu/drm/mgag200/mgag200_cursor.c      | 319 ------------------
 drivers/gpu/drm/mgag200/mgag200_drv.c         | 161 ++++++---
 drivers/gpu/drm/mgag200/mgag200_drv.h         |  11 +-
 drivers/gpu/drm/mgag200/mgag200_main.c        | 155 ---------
 .../mgag200/{mgag200_ttm.c => mgag200_mm.c}   |  99 ++++--
 drivers/gpu/drm/mgag200/mgag200_mode.c        |  12 +-
 7 files changed, 195 insertions(+), 565 deletions(-)
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c
 rename drivers/gpu/drm/mgag200/{mgag200_ttm.c => mgag200_mm.c} (51%)

--
2.26.2


Thomas Zimmermann (14):
  drm/mgag200: Remove declaration of mgag200_mmap() from header file
  drm/mgag200: Remove mgag200_cursor.c
  drm/mgag200: Use pcim_enable_device()
  drm/mgag200: Rename mgag200_ttm.c to mgag200_mm.c
  drm/mgag200: Lookup VRAM PCI BAR start and length only once
  drm/mgag200: Merge VRAM setup into MM initialization
  drm/mgag200: Switch to managed MM
  drm/mgag200: Separate DRM and PCI functionality from each other
  drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_
  drm/mgag200: Move device init and cleanup to mgag200_drv.c
  drm/mgag200: Separate device initialization into allocation
  drm/mgag200: Allocate device structures in mgag200_driver_load()
  drm/mgag200: Embed instance of struct drm_device in struct mga_device
  drm/mgag200: Use managed device initialization

 drivers/gpu/drm/mgag200/Makefile              |   3 +-
 drivers/gpu/drm/mgag200/mgag200_cursor.c      | 319 ------------------
 drivers/gpu/drm/mgag200/mgag200_drv.c         | 161 ++++++---
 drivers/gpu/drm/mgag200/mgag200_drv.h         |  11 +-
 drivers/gpu/drm/mgag200/mgag200_main.c        | 155 ---------
 .../mgag200/{mgag200_ttm.c => mgag200_mm.c}   |  99 ++++--
 drivers/gpu/drm/mgag200/mgag200_mode.c        |  12 +-
 7 files changed, 195 insertions(+), 565 deletions(-)
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c
 rename drivers/gpu/drm/mgag200/{mgag200_ttm.c => mgag200_mm.c} (51%)

--
2.26.2

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

[PATCH 01/14] drm/mgag200: Remove declaration of mgag200_mmap() from header file

[Thomas Zimmermann]

Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
removed the implementation of mgag200_mmap(). Also remove the declaration.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Armijn Hemel <armijn@tjaldur.nl>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: <stable@vger.kernel.org> # v5.3+
---
 drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 47df62b1ad290..92b6679029fe5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 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);
 
 #endif				/* __MGAG200_DRV_H__ */
-- 
2.26.2

[PATCH 01/14] drm/mgag200: Remove declaration of mgag200_mmap() from header file

[Thomas Zimmermann]

Commit 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
removed the implementation of mgag200_mmap(). Also remove the declaration.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 94668ac796a5 ("drm/mgag200: Convert mgag200 driver to VRAM MM")
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Krzysztof Kozlowski <krzk@kernel.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Armijn Hemel <armijn@tjaldur.nl>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: <stable@vger.kernel.org> # v5.3+
---
 drivers/gpu/drm/mgag200/mgag200_drv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 47df62b1ad290..92b6679029fe5 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 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);
 
 #endif				/* __MGAG200_DRV_H__ */
-- 
2.26.2

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

[PATCH 02/14] drm/mgag200: Remove mgag200_cursor.c

[Thomas Zimmermann]

Support for HW cursors got remove by commit 5a77e2bfdd4f ("drm/mgag200:
Remove HW cursor") Apparently the source file was not deleted. Removed
it now.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 5a77e2bfdd4f ("drm/mgag200: Remove HW cursor")
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Emil Velikov <emil.velikov@collabora.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: "Noralf Trønnes" <noralf@tronnes.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Kate Stewart <kstewart@linuxfoundation.org>
Cc: Allison Randal <allison@lohutok.net>
Cc: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
Cc: "José Roberto de Souza" <jose.souza@intel.com>
---
 drivers/gpu/drm/mgag200/mgag200_cursor.c | 319 -----------------------
 1 file changed, 319 deletions(-)
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c

diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
deleted file mode 100644
index c6932dc8acf2e..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 = to_mga_device(dev);
-	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;
-	}
-
-	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(obj);
-
-	return 0;
-err_drm_gem_vram_vunmap:
-	drm_gem_vram_vunmap(gbo, src);
-err_drm_gem_object_put:
-	drm_gem_object_put(obj);
-	return ret;
-}
-
-int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
-{
-	struct mga_device *mdev = to_mga_device(crtc->dev);
-
-	/* Our origin is at (64,64) */
-	x += 64;
-	y += 64;
-
-	mgag200_move_cursor(mdev, x, y);
-
-	return 0;
-}
-- 
2.26.2

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

[PATCH 03/14] drm/mgag200: Use pcim_enable_device()

[Thomas Zimmermann]

Using the managed function simplifies the error handling. After
unloading the driver, the PCI device should now get disabled as
well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 00ddea7d7d270..140ae86082c8e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -52,15 +52,13 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 	drm_fb_helper_remove_conflicting_pci_framebuffers(pdev, "mgag200drmfb");
 
-	ret = pci_enable_device(pdev);
+	ret = pcim_enable_device(pdev);
 	if (ret)
 		return ret;
 
 	dev = drm_dev_alloc(&driver, &pdev->dev);
-	if (IS_ERR(dev)) {
-		ret = PTR_ERR(dev);
-		goto err_pci_disable_device;
-	}
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
 
 	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
@@ -81,8 +79,6 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	mgag200_driver_unload(dev);
 err_drm_dev_put:
 	drm_dev_put(dev);
-err_pci_disable_device:
-	pci_disable_device(pdev);
 	return ret;
 }
 
-- 
2.26.2

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

[PATCH 04/14] drm/mgag200: Rename mgag200_ttm.c to mgag200_mm.c

[Thomas Zimmermann]

The mgag200 driver does not use TTM any longer. Rename the related file
to mgag200_mm.c (as in 'memory management').

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/Makefile                        | 4 ++--
 drivers/gpu/drm/mgag200/mgag200_drv.h                   | 1 +
 drivers/gpu/drm/mgag200/{mgag200_ttm.c => mgag200_mm.c} | 0
 3 files changed, 3 insertions(+), 2 deletions(-)
 rename drivers/gpu/drm/mgag200/{mgag200_ttm.c => mgag200_mm.c} (100%)

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
index 63403133638a3..e6a933874a88c 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_drv.o mgag200_i2c.o mgag200_ttm.o
+mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_main.o mgag200_mm.o \
+	       mgag200_mode.o
 
 obj-$(CONFIG_DRM_MGAG200) += mgag200.o
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 92b6679029fe5..cd786ffe319b8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -196,6 +196,7 @@ void mgag200_driver_unload(struct drm_device *dev);
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
 void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
+				/* mgag200_mm.c */
 int mgag200_mm_init(struct mga_device *mdev);
 void mgag200_mm_fini(struct mga_device *mdev);
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
similarity index 100%
rename from drivers/gpu/drm/mgag200/mgag200_ttm.c
rename to drivers/gpu/drm/mgag200/mgag200_mm.c
-- 
2.26.2

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

[PATCH 05/14] drm/mgag200: Lookup VRAM PCI BAR start and length only once

[Thomas Zimmermann]

The MM setup code on mgag200 reads PCI BAR 0's start and length
several times. Reusing these values makes the code more readable.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mm.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index a683642fe4682..73e30901e0631 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -33,16 +33,18 @@
 int mgag200_mm_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = mdev->dev;
+	resource_size_t start, len;
 	int ret;
 
-	arch_io_reserve_memtype_wc(pci_resource_start(dev->pdev, 0),
-				   pci_resource_len(dev->pdev, 0));
+	/* BAR 0 is VRAM */
+	start = pci_resource_start(dev->pdev, 0);
+	len = pci_resource_len(dev->pdev, 0);
 
-	mdev->fb_mtrr = arch_phys_wc_add(pci_resource_start(dev->pdev, 0),
-					 pci_resource_len(dev->pdev, 0));
+	arch_io_reserve_memtype_wc(start, len);
 
-	mdev->vram = ioremap(pci_resource_start(dev->pdev, 0),
-			     pci_resource_len(dev->pdev, 0));
+	mdev->fb_mtrr = arch_phys_wc_add(start, len);
+
+	mdev->vram = ioremap(start, len);
 	if (!mdev->vram) {
 		ret = -ENOMEM;
 		goto err_arch_phys_wc_del;
@@ -54,8 +56,7 @@ int mgag200_mm_init(struct mga_device *mdev)
 
 err_arch_phys_wc_del:
 	arch_phys_wc_del(mdev->fb_mtrr);
-	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
-				pci_resource_len(dev->pdev, 0));
+	arch_io_free_memtype_wc(start, len);
 	return ret;
 }
 
-- 
2.26.2

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

[PATCH 06/14] drm/mgag200: Merge VRAM setup into MM initialization

[Thomas Zimmermann]

The VRAM setup in mgag200_drv.c is part of memory management and
should be done in the same place. Merge the code into the memory
management's init function.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 75 --------------------------
 drivers/gpu/drm/mgag200/mgag200_mm.c   | 52 ++++++++++++++++++
 2 files changed, 52 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 3298eff7bd1b4..e9ad783c2b44d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -12,77 +12,6 @@
 
 #include "mgag200_drv.h"
 
-static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
-{
-	int offset;
-	int orig;
-	int test1, test2;
-	int orig1, orig2;
-	unsigned int vram_size;
-
-	/* Probe */
-	orig = ioread16(mem);
-	iowrite16(0, mem);
-
-	vram_size = mdev->mc.vram_window;
-
-	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000)) {
-		vram_size = vram_size - 0x400000;
-	}
-
-	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
-		orig1 = ioread8(mem + offset);
-		orig2 = ioread8(mem + offset + 0x100);
-
-		iowrite16(0xaa55, mem + offset);
-		iowrite16(0xaa55, mem + offset + 0x100);
-
-		test1 = ioread16(mem + offset);
-		test2 = ioread16(mem);
-
-		iowrite16(orig1, mem + offset);
-		iowrite16(orig2, mem + offset + 0x100);
-
-		if (test1 != 0xaa55) {
-			break;
-		}
-
-		if (test2) {
-			break;
-		}
-	}
-
-	iowrite16(orig, mem);
-	return offset - 65536;
-}
-
-/* Map the framebuffer from the card and configure the core */
-static int mga_vram_init(struct mga_device *mdev)
-{
-	struct drm_device *dev = mdev->dev;
-	void __iomem *mem;
-
-	/* BAR 0 is VRAM */
-	mdev->mc.vram_base = pci_resource_start(dev->pdev, 0);
-	mdev->mc.vram_window = pci_resource_len(dev->pdev, 0);
-
-	if (!devm_request_mem_region(dev->dev, mdev->mc.vram_base,
-				     mdev->mc.vram_window, "mgadrmfb_vram")) {
-		DRM_ERROR("can't reserve VRAM\n");
-		return -ENXIO;
-	}
-
-	mem = pci_iomap(dev->pdev, 0, 0);
-	if (!mem)
-		return -ENOMEM;
-
-	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
-
-	pci_iounmap(dev->pdev, mem);
-
-	return 0;
-}
-
 int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 {
 	struct mga_device *mdev;
@@ -121,10 +50,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 			mdev->unique_rev_id);
 	}
 
-	ret = mga_vram_init(mdev);
-	if (ret)
-		return ret;
-
 	ret = mgag200_mm_init(mdev);
 	if (ret)
 		goto err_mm;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index 73e30901e0631..f56b0456994f4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -30,6 +30,49 @@
 
 #include "mgag200_drv.h"
 
+static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
+				 size_t size)
+{
+	int offset;
+	int orig;
+	int test1, test2;
+	int orig1, orig2;
+	size_t vram_size;
+
+	/* Probe */
+	orig = ioread16(mem);
+	iowrite16(0, mem);
+
+	vram_size = size;
+
+	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
+		vram_size = vram_size - 0x400000;
+
+	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
+		orig1 = ioread8(mem + offset);
+		orig2 = ioread8(mem + offset + 0x100);
+
+		iowrite16(0xaa55, mem + offset);
+		iowrite16(0xaa55, mem + offset + 0x100);
+
+		test1 = ioread16(mem + offset);
+		test2 = ioread16(mem);
+
+		iowrite16(orig1, mem + offset);
+		iowrite16(orig2, mem + offset + 0x100);
+
+		if (test1 != 0xaa55)
+			break;
+
+		if (test2)
+			break;
+	}
+
+	iowrite16(orig, mem);
+
+	return offset - 65536;
+}
+
 int mgag200_mm_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = mdev->dev;
@@ -40,6 +83,11 @@ int mgag200_mm_init(struct mga_device *mdev)
 	start = pci_resource_start(dev->pdev, 0);
 	len = pci_resource_len(dev->pdev, 0);
 
+	if (!devm_request_mem_region(dev->dev, start, len, "mgadrmfb_vram")) {
+		drm_err(dev, "can't reserve VRAM\n");
+		return -ENXIO;
+	}
+
 	arch_io_reserve_memtype_wc(start, len);
 
 	mdev->fb_mtrr = arch_phys_wc_add(start, len);
@@ -50,6 +98,10 @@ int mgag200_mm_init(struct mga_device *mdev)
 		goto err_arch_phys_wc_del;
 	}
 
+	mdev->mc.vram_size = mgag200_probe_vram(mdev, mdev->vram, len);
+	mdev->mc.vram_base = start;
+	mdev->mc.vram_window = len;
+
 	mdev->vram_fb_available = mdev->mc.vram_size;
 
 	return 0;
-- 
2.26.2

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

[PATCH 07/14] drm/mgag200: Switch to managed MM

[Thomas Zimmermann]

The memory-management code now cleans up automatically as part of
device destruction.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
 drivers/gpu/drm/mgag200/mgag200_main.c |  5 +----
 drivers/gpu/drm/mgag200/mgag200_mm.c   | 28 ++++++++++++++------------
 3 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index cd786ffe319b8..7b6e6827a9a21 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
 
 				/* mgag200_mm.c */
 int mgag200_mm_init(struct mga_device *mdev);
-void mgag200_mm_fini(struct mga_device *mdev);
 
 #endif				/* __MGAG200_DRV_H__ */
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index e9ad783c2b44d..49bcdfcb40a4e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -57,13 +57,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	ret = mgag200_modeset_init(mdev);
 	if (ret) {
 		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
-		goto err_mgag200_mm_fini;
+		goto err_mm;
 	}
 
 	return 0;
 
-err_mgag200_mm_fini:
-	mgag200_mm_fini(mdev);
 err_mm:
 	dev->dev_private = NULL;
 	return ret;
@@ -75,6 +73,5 @@ void mgag200_driver_unload(struct drm_device *dev)
 
 	if (mdev == NULL)
 		return;
-	mgag200_mm_fini(mdev);
 	dev->dev_private = NULL;
 }
diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index f56b0456994f4..1b1e5ec5d1ceb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -28,6 +28,8 @@
 
 #include <linux/pci.h>
 
+#include <drm/drm_managed.h>
+
 #include "mgag200_drv.h"
 
 static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
@@ -73,6 +75,18 @@ static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
 	return offset - 65536;
 }
 
+static void mgag200_mm_release(struct drm_device *dev, void *ptr)
+{
+	struct mga_device *mdev = to_mga_device(dev);
+
+	mdev->vram_fb_available = 0;
+	iounmap(mdev->vram);
+	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
+				pci_resource_len(dev->pdev, 0));
+	arch_phys_wc_del(mdev->fb_mtrr);
+	mdev->fb_mtrr = 0;
+}
+
 int mgag200_mm_init(struct mga_device *mdev)
 {
 	struct drm_device *dev = mdev->dev;
@@ -104,22 +118,10 @@ int mgag200_mm_init(struct mga_device *mdev)
 
 	mdev->vram_fb_available = mdev->mc.vram_size;
 
-	return 0;
+	return drmm_add_action_or_reset(dev, mgag200_mm_release, NULL);
 
 err_arch_phys_wc_del:
 	arch_phys_wc_del(mdev->fb_mtrr);
 	arch_io_free_memtype_wc(start, len);
 	return ret;
 }
-
-void mgag200_mm_fini(struct mga_device *mdev)
-{
-	struct drm_device *dev = mdev->dev;
-
-	mdev->vram_fb_available = 0;
-	iounmap(mdev->vram);
-	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
-				pci_resource_len(dev->pdev, 0));
-	arch_phys_wc_del(mdev->fb_mtrr);
-	mdev->fb_mtrr = 0;
-}
-- 
2.26.2

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

[PATCH 08/14] drm/mgag200: Separate DRM and PCI functionality from each other

[Thomas Zimmermann]

Moving the DRM driver structures from the middle of the PCI code to
the top of the file makes it more readable. Also remove an obsolete
comment.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 42 +++++++++++++--------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 140ae86082c8e..670e12d57dea8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -17,17 +17,31 @@
 
 #include "mgag200_drv.h"
 
-/*
- * This is the generic driver code. This binds the driver to the drm core,
- * which then performs further device association and calls our graphics init
- * functions
- */
-
 int mgag200_modeset = -1;
 MODULE_PARM_DESC(modeset, "Disable/Enable modesetting");
 module_param_named(modeset, mgag200_modeset, int, 0400);
 
-static struct drm_driver driver;
+/*
+ * DRM driver
+ */
+
+DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
+
+static struct drm_driver driver = {
+	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
+	.fops = &mgag200_driver_fops,
+	.name = DRIVER_NAME,
+	.desc = DRIVER_DESC,
+	.date = DRIVER_DATE,
+	.major = DRIVER_MAJOR,
+	.minor = DRIVER_MINOR,
+	.patchlevel = DRIVER_PATCHLEVEL,
+	DRM_GEM_SHMEM_DRIVER_OPS,
+};
+
+/*
+ * PCI driver
+ */
 
 static const struct pci_device_id pciidlist[] = {
 	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
@@ -91,20 +105,6 @@ static void mga_pci_remove(struct pci_dev *pdev)
 	drm_dev_put(dev);
 }
 
-DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
-
-static struct drm_driver driver = {
-	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
-	.fops = &mgag200_driver_fops,
-	.name = DRIVER_NAME,
-	.desc = DRIVER_DESC,
-	.date = DRIVER_DATE,
-	.major = DRIVER_MAJOR,
-	.minor = DRIVER_MINOR,
-	.patchlevel = DRIVER_PATCHLEVEL,
-	DRM_GEM_SHMEM_DRIVER_OPS,
-};
-
 static struct pci_driver mgag200_pci_driver = {
 	.name = DRIVER_NAME,
 	.id_table = pciidlist,
-- 
2.26.2

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

[PATCH 09/14] drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_

[Thomas Zimmermann]

The naming of global symbols in mgag200_drv.c is inconsistent. Fix
that by prefixing all names with mgag200_.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 670e12d57dea8..ad74e02d8f251 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -27,7 +27,7 @@ module_param_named(modeset, mgag200_modeset, int, 0400);
 
 DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
 
-static struct drm_driver driver = {
+static struct drm_driver mgag200_driver = {
 	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
 	.fops = &mgag200_driver_fops,
 	.name = DRIVER_NAME,
@@ -43,7 +43,7 @@ static struct drm_driver driver = {
  * PCI driver
  */
 
-static const struct pci_device_id pciidlist[] = {
+static const struct pci_device_id mgag200_pciidlist[] = {
 	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
 		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
 	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B },
@@ -56,10 +56,10 @@ static const struct pci_device_id pciidlist[] = {
 	{0,}
 };
 
-MODULE_DEVICE_TABLE(pci, pciidlist);
+MODULE_DEVICE_TABLE(pci, mgag200_pciidlist);
 
-
-static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
+static int
+mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	struct drm_device *dev;
 	int ret;
@@ -70,7 +70,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
-	dev = drm_dev_alloc(&driver, &pdev->dev);
+	dev = drm_dev_alloc(&mgag200_driver, &pdev->dev);
 	if (IS_ERR(dev))
 		return PTR_ERR(dev);
 
@@ -96,7 +96,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	return ret;
 }
 
-static void mga_pci_remove(struct pci_dev *pdev)
+static void mgag200_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
@@ -107,9 +107,9 @@ static void mga_pci_remove(struct pci_dev *pdev)
 
 static struct pci_driver mgag200_pci_driver = {
 	.name = DRIVER_NAME,
-	.id_table = pciidlist,
-	.probe = mga_pci_probe,
-	.remove = mga_pci_remove,
+	.id_table = mgag200_pciidlist,
+	.probe = mgag200_pci_probe,
+	.remove = mgag200_pci_remove,
 };
 
 static int __init mgag200_init(void)
-- 
2.26.2

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

[PATCH 10/14] drm/mgag200: Move device init and cleanup to mgag200_drv.c

[Thomas Zimmermann]

Moving the initializer and cleanup functions for device instances
to mgag200_drv.c prepares for the conversion to managed code. No
functional changes are made. Remove mgag200_main.c, which is now
empty.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/Makefile       |  3 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 68 +++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  4 --
 drivers/gpu/drm/mgag200/mgag200_main.c | 77 --------------------------
 4 files changed, 69 insertions(+), 83 deletions(-)
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
index e6a933874a88c..42fedef538826 100644
--- a/drivers/gpu/drm/mgag200/Makefile
+++ b/drivers/gpu/drm/mgag200/Makefile
@@ -1,5 +1,4 @@
 # SPDX-License-Identifier: GPL-2.0-only
-mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_main.o mgag200_mm.o \
-	       mgag200_mode.o
+mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_mm.o mgag200_mode.o
 
 obj-$(CONFIG_DRM_MGAG200) += mgag200.o
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index ad74e02d8f251..f8bb24199643d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -39,6 +39,74 @@ static struct drm_driver mgag200_driver = {
 	DRM_GEM_SHMEM_DRIVER_OPS,
 };
 
+/*
+ * DRM device
+ */
+
+static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
+{
+	struct mga_device *mdev;
+	int ret, option;
+
+	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
+	if (mdev == NULL)
+		return -ENOMEM;
+	dev->dev_private = (void *)mdev;
+	mdev->dev = dev;
+
+	mdev->flags = mgag200_flags_from_driver_data(flags);
+	mdev->type = mgag200_type_from_driver_data(flags);
+
+	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
+	mdev->has_sdram = !(option & (1 << 14));
+
+	/* BAR 0 is the framebuffer, BAR 1 contains registers */
+	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
+	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
+
+	if (!devm_request_mem_region(dev->dev, mdev->rmmio_base,
+				     mdev->rmmio_size, "mgadrmfb_mmio")) {
+		drm_err(dev, "can't reserve mmio registers\n");
+		return -ENOMEM;
+	}
+
+	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
+	if (mdev->rmmio == NULL)
+		return -ENOMEM;
+
+	/* stash G200 SE model number for later use */
+	if (IS_G200_SE(mdev)) {
+		mdev->unique_rev_id = RREG32(0x1e24);
+		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
+			mdev->unique_rev_id);
+	}
+
+	ret = mgag200_mm_init(mdev);
+	if (ret)
+		goto err_mm;
+
+	ret = mgag200_modeset_init(mdev);
+	if (ret) {
+		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
+		goto err_mm;
+	}
+
+	return 0;
+
+err_mm:
+	dev->dev_private = NULL;
+	return ret;
+}
+
+static void mgag200_driver_unload(struct drm_device *dev)
+{
+	struct mga_device *mdev = to_mga_device(dev);
+
+	if (mdev == NULL)
+		return;
+	dev->dev_private = NULL;
+}
+
 /*
  * PCI driver
  */
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 7b6e6827a9a21..b38e5ce4ee20b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -188,10 +188,6 @@ mgag200_flags_from_driver_data(kernel_ulong_t driver_data)
 				/* mgag200_mode.c */
 int mgag200_modeset_init(struct mga_device *mdev);
 
-				/* mgag200_main.c */
-int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
-void mgag200_driver_unload(struct drm_device *dev);
-
 				/* mgag200_i2c.c */
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
 void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
deleted file mode 100644
index 49bcdfcb40a4e..0000000000000
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ /dev/null
@@ -1,77 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright 2010 Matt Turner.
- * Copyright 2012 Red Hat
- *
- * Authors: Matthew Garrett
- *          Matt Turner
- *          Dave Airlie
- */
-
-#include <linux/pci.h>
-
-#include "mgag200_drv.h"
-
-int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
-{
-	struct mga_device *mdev;
-	int ret, option;
-
-	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
-	if (mdev == NULL)
-		return -ENOMEM;
-	dev->dev_private = (void *)mdev;
-	mdev->dev = dev;
-
-	mdev->flags = mgag200_flags_from_driver_data(flags);
-	mdev->type = mgag200_type_from_driver_data(flags);
-
-	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
-	mdev->has_sdram = !(option & (1 << 14));
-
-	/* BAR 0 is the framebuffer, BAR 1 contains registers */
-	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
-	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
-
-	if (!devm_request_mem_region(dev->dev, mdev->rmmio_base,
-				     mdev->rmmio_size, "mgadrmfb_mmio")) {
-		drm_err(dev, "can't reserve mmio registers\n");
-		return -ENOMEM;
-	}
-
-	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
-	if (mdev->rmmio == NULL)
-		return -ENOMEM;
-
-	/* stash G200 SE model number for later use */
-	if (IS_G200_SE(mdev)) {
-		mdev->unique_rev_id = RREG32(0x1e24);
-		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
-			mdev->unique_rev_id);
-	}
-
-	ret = mgag200_mm_init(mdev);
-	if (ret)
-		goto err_mm;
-
-	ret = mgag200_modeset_init(mdev);
-	if (ret) {
-		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
-		goto err_mm;
-	}
-
-	return 0;
-
-err_mm:
-	dev->dev_private = NULL;
-	return ret;
-}
-
-void mgag200_driver_unload(struct drm_device *dev)
-{
-	struct mga_device *mdev = to_mga_device(dev);
-
-	if (mdev == NULL)
-		return;
-	dev->dev_private = NULL;
-}
-- 
2.26.2

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

[PATCH 11/14] drm/mgag200: Separate device initialization into allocation

[Thomas Zimmermann]

Embedding the DRM device instance in struct mga_device will require
changes to device allocation. Moving the device initialization into
its own functions gets it out of the way.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 32 ++++++++++++++++++---------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index f8bb24199643d..926437a27a228 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -43,17 +43,11 @@ static struct drm_driver mgag200_driver = {
  * DRM device
  */
 
-static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
+static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 {
-	struct mga_device *mdev;
+	struct drm_device *dev = mdev->dev;
 	int ret, option;
 
-	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
-	if (mdev == NULL)
-		return -ENOMEM;
-	dev->dev_private = (void *)mdev;
-	mdev->dev = dev;
-
 	mdev->flags = mgag200_flags_from_driver_data(flags);
 	mdev->type = mgag200_type_from_driver_data(flags);
 
@@ -83,15 +77,33 @@ static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ret = mgag200_mm_init(mdev);
 	if (ret)
-		goto err_mm;
+		return ret;
 
 	ret = mgag200_modeset_init(mdev);
 	if (ret) {
 		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
-		goto err_mm;
+		return ret;
 	}
 
 	return 0;
+}
+
+static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
+{
+	struct mga_device *mdev;
+	int ret;
+
+	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
+	if (mdev == NULL)
+		return -ENOMEM;
+	dev->dev_private = (void *)mdev;
+	mdev->dev = dev;
+
+	ret = mgag200_device_init(mdev, flags);
+	if (ret)
+		goto err_mm;
+
+	return 0;
 
 err_mm:
 	dev->dev_private = NULL;
-- 
2.26.2

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

[PATCH 12/14] drm/mgag200: Allocate device structures in mgag200_driver_load()

[Thomas Zimmermann]

Instances of struct drm_device and struct mga_device are now allocated
next to each other in mgag200_driver_load(). Yet another preparation
before embedding the DRM device instance in struct mga_device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 38 +++++++++++++++------------
 1 file changed, 21 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 926437a27a228..592e484f87ee7 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -88,26 +88,36 @@ static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 	return 0;
 }
 
-static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
+static struct mga_device *
+mgag200_driver_load(struct pci_dev *pdev, unsigned long flags)
 {
+	struct drm_device *dev;
 	struct mga_device *mdev;
 	int ret;
 
+	dev = drm_dev_alloc(&mgag200_driver, &pdev->dev);
+	if (IS_ERR(dev))
+		return ERR_CAST(dev);
+
+	dev->pdev = pdev;
+	pci_set_drvdata(pdev, dev);
+
 	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
 	if (mdev == NULL)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 	dev->dev_private = (void *)mdev;
 	mdev->dev = dev;
 
 	ret = mgag200_device_init(mdev, flags);
 	if (ret)
-		goto err_mm;
+		goto err_drm_dev_put;
 
-	return 0;
+	return mdev;
 
-err_mm:
+err_drm_dev_put:
+	drm_dev_put(dev);
 	dev->dev_private = NULL;
-	return ret;
+	return ERR_PTR(ret);
 }
 
 static void mgag200_driver_unload(struct drm_device *dev)
@@ -141,6 +151,7 @@ MODULE_DEVICE_TABLE(pci, mgag200_pciidlist);
 static int
 mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
+	struct mga_device *mdev;
 	struct drm_device *dev;
 	int ret;
 
@@ -150,16 +161,10 @@ mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
-	dev = drm_dev_alloc(&mgag200_driver, &pdev->dev);
-	if (IS_ERR(dev))
-		return PTR_ERR(dev);
-
-	dev->pdev = pdev;
-	pci_set_drvdata(pdev, dev);
-
-	ret = mgag200_driver_load(dev, ent->driver_data);
-	if (ret)
-		goto err_drm_dev_put;
+	mdev = mgag200_driver_load(pdev, ent->driver_data);
+	if (IS_ERR(mdev))
+		return PTR_ERR(mdev);
+	dev = mdev->dev;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
@@ -171,7 +176,6 @@ mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 
 err_mgag200_driver_unload:
 	mgag200_driver_unload(dev);
-err_drm_dev_put:
 	drm_dev_put(dev);
 	return ret;
 }
-- 
2.26.2

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

[PATCH 13/14] drm/mgag200: Embed instance of struct drm_device in struct mga_device

[Thomas Zimmermann]

Following current best practice, the instance of struct drm_device is now
embedded in struct mga_device. The respective field has been renamed from
'dev' to 'base' to reflect the relationship. Conversion from DRM device is
done via upcast. Using dev_private is no longer possible.

The patch also open-codes drm_dev_alloc() and DRM device initialization
is now performed by a call to drm_device_init().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 47 ++++++++++----------------
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  4 +--
 drivers/gpu/drm/mgag200/mgag200_mm.c   |  2 +-
 drivers/gpu/drm/mgag200/mgag200_mode.c | 12 +++----
 4 files changed, 27 insertions(+), 38 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 592e484f87ee7..6dfb7c5f79e3c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -45,7 +45,7 @@ static struct drm_driver mgag200_driver = {
 
 static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	int ret, option;
 
 	mdev->flags = mgag200_flags_from_driver_data(flags);
@@ -89,25 +89,24 @@ static int mgag200_device_init(struct mga_device *mdev, unsigned long flags)
 }
 
 static struct mga_device *
-mgag200_driver_load(struct pci_dev *pdev, unsigned long flags)
+mgag200_device_create(struct pci_dev *pdev, unsigned long flags)
 {
 	struct drm_device *dev;
 	struct mga_device *mdev;
 	int ret;
 
-	dev = drm_dev_alloc(&mgag200_driver, &pdev->dev);
-	if (IS_ERR(dev))
-		return ERR_CAST(dev);
+	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
+	if (!mdev)
+		return ERR_PTR(-ENOMEM);
+	dev = &mdev->base;
+
+	ret = drm_dev_init(dev, &mgag200_driver, &pdev->dev);
+	if (ret)
+		return ERR_PTR(ret);
 
 	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
 
-	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
-	if (mdev == NULL)
-		return ERR_PTR(-ENOMEM);
-	dev->dev_private = (void *)mdev;
-	mdev->dev = dev;
-
 	ret = mgag200_device_init(mdev, flags);
 	if (ret)
 		goto err_drm_dev_put;
@@ -116,19 +115,9 @@ mgag200_driver_load(struct pci_dev *pdev, unsigned long flags)
 
 err_drm_dev_put:
 	drm_dev_put(dev);
-	dev->dev_private = NULL;
 	return ERR_PTR(ret);
 }
 
-static void mgag200_driver_unload(struct drm_device *dev)
-{
-	struct mga_device *mdev = to_mga_device(dev);
-
-	if (mdev == NULL)
-		return;
-	dev->dev_private = NULL;
-}
-
 /*
  * PCI driver
  */
@@ -161,21 +150,22 @@ mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		return ret;
 
-	mdev = mgag200_driver_load(pdev, ent->driver_data);
-	if (IS_ERR(mdev))
-		return PTR_ERR(mdev);
-	dev = mdev->dev;
+	mdev = mgag200_device_create(pdev, ent->driver_data);
+	if (IS_ERR(mdev)) {
+		ret = PTR_ERR(mdev);
+		goto err_drm_dev_put;
+	}
+	dev = &mdev->base;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
-		goto err_mgag200_driver_unload;
+		goto err_drm_dev_put;
 
 	drm_fbdev_generic_setup(dev, 0);
 
 	return 0;
 
-err_mgag200_driver_unload:
-	mgag200_driver_unload(dev);
+err_drm_dev_put:
 	drm_dev_put(dev);
 	return ret;
 }
@@ -185,7 +175,6 @@ static void mgag200_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(dev);
-	mgag200_driver_unload(dev);
 	drm_dev_put(dev);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index b38e5ce4ee20b..270c2f9a67666 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -142,7 +142,7 @@ enum mga_type {
 #define IS_G200_SE(mdev) (mdev->type == G200_SE_A || mdev->type == G200_SE_B)
 
 struct mga_device {
-	struct drm_device		*dev;
+	struct drm_device		base;
 	unsigned long			flags;
 
 	resource_size_t			rmmio_base;
@@ -170,7 +170,7 @@ struct mga_device {
 
 static inline struct mga_device *to_mga_device(struct drm_device *dev)
 {
-	return dev->dev_private;
+	return container_of(dev, struct mga_device, base);
 }
 
 static inline enum mga_type
diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
index 1b1e5ec5d1ceb..7b69392bcb891 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
@@ -89,7 +89,7 @@ static void mgag200_mm_release(struct drm_device *dev, void *ptr)
 
 int mgag200_mm_init(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	resource_size_t start, len;
 	int ret;
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 0155d4eb5fa6b..f16bd278ab7e4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -854,7 +854,7 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
 static void mgag200_set_startadd(struct mga_device *mdev,
 				 unsigned long offset)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	u32 startadd;
 	u8 crtcc, crtcd, crtcext0;
 
@@ -882,7 +882,7 @@ static void mgag200_set_startadd(struct mga_device *mdev,
 static void mgag200_set_pci_regs(struct mga_device *mdev)
 {
 	uint32_t option = 0, option2 = 0;
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 
 	switch (mdev->type) {
 	case G200_SE_A:
@@ -1153,7 +1153,7 @@ static void mgag200_set_offset(struct mga_device *mdev,
 static void mgag200_set_format_regs(struct mga_device *mdev,
 				    const struct drm_framebuffer *fb)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	const struct drm_format_info *format = fb->format;
 	unsigned int bpp, bppshift, scale;
 	u8 crtcext3, xmulctrl;
@@ -1537,7 +1537,7 @@ static const struct drm_connector_funcs mga_vga_connector_funcs = {
 
 static int mgag200_vga_connector_init(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	struct mga_connector *mconnector = &mdev->connector;
 	struct drm_connector *connector = &mconnector->base;
 	struct mga_i2c_chan *i2c;
@@ -1579,7 +1579,7 @@ static void
 mgag200_handle_damage(struct mga_device *mdev, struct drm_framebuffer *fb,
 		      struct drm_rect *clip)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	void *vmap;
 
 	vmap = drm_gem_shmem_vmap(fb->obj[0]);
@@ -1718,7 +1718,7 @@ 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_device *dev = &mdev->base;
 	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);
-- 
2.26.2

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

[PATCH 14/14] drm/mgag200: Use managed device initialization

[Thomas Zimmermann]

The mgag200 driver now uses managed functions for DRM devices. The
individual helpers for modesetting and memory managed are already
covered, so only device allocation and initialization is left for
conversion.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c | 30 +++++++--------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 6dfb7c5f79e3c..e19660f4a6371 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -95,27 +95,20 @@ mgag200_device_create(struct pci_dev *pdev, unsigned long flags)
 	struct mga_device *mdev;
 	int ret;
 
-	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
-	if (!mdev)
-		return ERR_PTR(-ENOMEM);
+	mdev = devm_drm_dev_alloc(&pdev->dev, &mgag200_driver,
+				  struct mga_device, base);
+	if (IS_ERR(mdev))
+		return mdev;
 	dev = &mdev->base;
 
-	ret = drm_dev_init(dev, &mgag200_driver, &pdev->dev);
-	if (ret)
-		return ERR_PTR(ret);
-
 	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
 
 	ret = mgag200_device_init(mdev, flags);
 	if (ret)
-		goto err_drm_dev_put;
+		return ERR_PTR(ret);
 
 	return mdev;
-
-err_drm_dev_put:
-	drm_dev_put(dev);
-	return ERR_PTR(ret);
 }
 
 /*
@@ -151,23 +144,17 @@ mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 		return ret;
 
 	mdev = mgag200_device_create(pdev, ent->driver_data);
-	if (IS_ERR(mdev)) {
-		ret = PTR_ERR(mdev);
-		goto err_drm_dev_put;
-	}
+	if (IS_ERR(mdev))
+		return PTR_ERR(mdev);
 	dev = &mdev->base;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
-		goto err_drm_dev_put;
+		return ret;
 
 	drm_fbdev_generic_setup(dev, 0);
 
 	return 0;
-
-err_drm_dev_put:
-	drm_dev_put(dev);
-	return ret;
 }
 
 static void mgag200_pci_remove(struct pci_dev *pdev)
@@ -175,7 +162,6 @@ static void mgag200_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(dev);
-	drm_dev_put(dev);
 }
 
 static struct pci_driver mgag200_pci_driver = {
-- 
2.26.2

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

Re: [PATCH 06/14] drm/mgag200: Merge VRAM setup into MM initialization

[Sam Ravnborg]

Hi Thomas.

Some parts I did not understand here.

On Fri, Jun 05, 2020 at 03:57:55PM +0200, Thomas Zimmermann wrote:
> The VRAM setup in mgag200_drv.c is part of memory management and
> should be done in the same place. Merge the code into the memory
> management's init function.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 75 --------------------------
>  drivers/gpu/drm/mgag200/mgag200_mm.c   | 52 ++++++++++++++++++
>  2 files changed, 52 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 3298eff7bd1b4..e9ad783c2b44d 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -12,77 +12,6 @@
>  
>  #include "mgag200_drv.h"
>  
> -static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
> -{
> -	int offset;
> -	int orig;
> -	int test1, test2;
> -	int orig1, orig2;
> -	unsigned int vram_size;
> -
> -	/* Probe */
> -	orig = ioread16(mem);
> -	iowrite16(0, mem);
> -
> -	vram_size = mdev->mc.vram_window;
> -
> -	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000)) {
> -		vram_size = vram_size - 0x400000;
> -	}
> -
> -	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
> -		orig1 = ioread8(mem + offset);
> -		orig2 = ioread8(mem + offset + 0x100);
> -
> -		iowrite16(0xaa55, mem + offset);
> -		iowrite16(0xaa55, mem + offset + 0x100);
> -
> -		test1 = ioread16(mem + offset);
> -		test2 = ioread16(mem);
> -
> -		iowrite16(orig1, mem + offset);
> -		iowrite16(orig2, mem + offset + 0x100);
> -
> -		if (test1 != 0xaa55) {
> -			break;
> -		}
> -
> -		if (test2) {
> -			break;
> -		}
> -	}
> -
> -	iowrite16(orig, mem);
> -	return offset - 65536;
> -}
> -
> -/* Map the framebuffer from the card and configure the core */
> -static int mga_vram_init(struct mga_device *mdev)
> -{
> -	struct drm_device *dev = mdev->dev;
> -	void __iomem *mem;
> -
> -	/* BAR 0 is VRAM */
> -	mdev->mc.vram_base = pci_resource_start(dev->pdev, 0);
> -	mdev->mc.vram_window = pci_resource_len(dev->pdev, 0);
> -
> -	if (!devm_request_mem_region(dev->dev, mdev->mc.vram_base,
> -				     mdev->mc.vram_window, "mgadrmfb_vram")) {
> -		DRM_ERROR("can't reserve VRAM\n");
> -		return -ENXIO;
> -	}
> -
> -	mem = pci_iomap(dev->pdev, 0, 0);
> -	if (!mem)
> -		return -ENOMEM;
> -
> -	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
> -
> -	pci_iounmap(dev->pdev, mem);
mem is the result of pci_iomap() - but I do not see any call
to pci_iomap() in the converted code.

mdev->vram is used as argument in new code where mem was used in the old
code.
mdev->vram comes from ioremap(start, len) - will it result in the same?

	Sam


> -
> -	return 0;
> -}
> -
>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  {
>  	struct mga_device *mdev;
> @@ -121,10 +50,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  			mdev->unique_rev_id);
>  	}
>  
> -	ret = mga_vram_init(mdev);
> -	if (ret)
> -		return ret;
> -
>  	ret = mgag200_mm_init(mdev);
>  	if (ret)
>  		goto err_mm;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
> index 73e30901e0631..f56b0456994f4 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mm.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
> @@ -30,6 +30,49 @@
>  
>  #include "mgag200_drv.h"
>  
> +static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
> +				 size_t size)
> +{
> +	int offset;
> +	int orig;
> +	int test1, test2;
> +	int orig1, orig2;
> +	size_t vram_size;
> +
> +	/* Probe */
> +	orig = ioread16(mem);
> +	iowrite16(0, mem);
> +
> +	vram_size = size;
> +
> +	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
> +		vram_size = vram_size - 0x400000;
> +
> +	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
> +		orig1 = ioread8(mem + offset);
> +		orig2 = ioread8(mem + offset + 0x100);
> +
> +		iowrite16(0xaa55, mem + offset);
> +		iowrite16(0xaa55, mem + offset + 0x100);
> +
> +		test1 = ioread16(mem + offset);
> +		test2 = ioread16(mem);
> +
> +		iowrite16(orig1, mem + offset);
> +		iowrite16(orig2, mem + offset + 0x100);
> +
> +		if (test1 != 0xaa55)
> +			break;
> +
> +		if (test2)
> +			break;
> +	}
> +
> +	iowrite16(orig, mem);
> +
> +	return offset - 65536;
> +}
> +
>  int mgag200_mm_init(struct mga_device *mdev)
>  {
>  	struct drm_device *dev = mdev->dev;
> @@ -40,6 +83,11 @@ int mgag200_mm_init(struct mga_device *mdev)
>  	start = pci_resource_start(dev->pdev, 0);
>  	len = pci_resource_len(dev->pdev, 0);
>  
> +	if (!devm_request_mem_region(dev->dev, start, len, "mgadrmfb_vram")) {
> +		drm_err(dev, "can't reserve VRAM\n");
> +		return -ENXIO;
> +	}
> +
>  	arch_io_reserve_memtype_wc(start, len);
>  
>  	mdev->fb_mtrr = arch_phys_wc_add(start, len);
> @@ -50,6 +98,10 @@ int mgag200_mm_init(struct mga_device *mdev)
>  		goto err_arch_phys_wc_del;
>  	}
>  
> +	mdev->mc.vram_size = mgag200_probe_vram(mdev, mdev->vram, len);
> +	mdev->mc.vram_base = start;
> +	mdev->mc.vram_window = len;
> +
>  	mdev->vram_fb_available = mdev->mc.vram_size;
>  
>  	return 0;
> -- 
> 2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 09/14] drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_

[Sam Ravnborg]

On Fri, Jun 05, 2020 at 03:57:58PM +0200, Thomas Zimmermann wrote:
> The naming of global symbols in mgag200_drv.c is inconsistent. Fix
> that by prefixing all names with mgag200_.

Hmm, static symbols are hardly global symbols.
Patch is fine, but changelog seems a litte off.

	Sam

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 670e12d57dea8..ad74e02d8f251 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -27,7 +27,7 @@ module_param_named(modeset, mgag200_modeset, int, 0400);
>  
>  DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
>  
> -static struct drm_driver driver = {
> +static struct drm_driver mgag200_driver = {
>  	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
>  	.fops = &mgag200_driver_fops,
>  	.name = DRIVER_NAME,
> @@ -43,7 +43,7 @@ static struct drm_driver driver = {
>   * PCI driver
>   */
>  
> -static const struct pci_device_id pciidlist[] = {
> +static const struct pci_device_id mgag200_pciidlist[] = {
>  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
>  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B },
> @@ -56,10 +56,10 @@ static const struct pci_device_id pciidlist[] = {
>  	{0,}
>  };
>  
> -MODULE_DEVICE_TABLE(pci, pciidlist);
> +MODULE_DEVICE_TABLE(pci, mgag200_pciidlist);
>  
> -
> -static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +static int
> +mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
>  	struct drm_device *dev;
>  	int ret;
> @@ -70,7 +70,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (ret)
>  		return ret;
>  
> -	dev = drm_dev_alloc(&driver, &pdev->dev);
> +	dev = drm_dev_alloc(&mgag200_driver, &pdev->dev);
>  	if (IS_ERR(dev))
>  		return PTR_ERR(dev);
>  
> @@ -96,7 +96,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	return ret;
>  }
>  
> -static void mga_pci_remove(struct pci_dev *pdev)
> +static void mgag200_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  
> @@ -107,9 +107,9 @@ static void mga_pci_remove(struct pci_dev *pdev)
>  
>  static struct pci_driver mgag200_pci_driver = {
>  	.name = DRIVER_NAME,
> -	.id_table = pciidlist,
> -	.probe = mga_pci_probe,
> -	.remove = mga_pci_remove,
> +	.id_table = mgag200_pciidlist,
> +	.probe = mgag200_pci_probe,
> +	.remove = mgag200_pci_remove,
>  };
>  
>  static int __init mgag200_init(void)
> -- 
> 2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/14] drm/mgag200: Move device init and cleanup to mgag200_drv.c

[Sam Ravnborg]

On Fri, Jun 05, 2020 at 03:57:59PM +0200, Thomas Zimmermann wrote:
> Moving the initializer and cleanup functions for device instances
> to mgag200_drv.c prepares for the conversion to managed code. No
> functional changes are made. Remove mgag200_main.c, which is now
> empty.

The functions are still named xx_load() - which comes from old days
where drm_driver has a load callback.
We can always fight about naming, but I am just left with the impression
that better naming exists today.
Just a drive by comment - feel free to ignore.

	Sam

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/Makefile       |  3 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 68 +++++++++++++++++++++++
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  4 --
>  drivers/gpu/drm/mgag200/mgag200_main.c | 77 --------------------------
>  4 files changed, 69 insertions(+), 83 deletions(-)
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c
> 
> diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
> index e6a933874a88c..42fedef538826 100644
> --- a/drivers/gpu/drm/mgag200/Makefile
> +++ b/drivers/gpu/drm/mgag200/Makefile
> @@ -1,5 +1,4 @@
>  # SPDX-License-Identifier: GPL-2.0-only
> -mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_main.o mgag200_mm.o \
> -	       mgag200_mode.o
> +mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_mm.o mgag200_mode.o
>  
>  obj-$(CONFIG_DRM_MGAG200) += mgag200.o
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index ad74e02d8f251..f8bb24199643d 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -39,6 +39,74 @@ static struct drm_driver mgag200_driver = {
>  	DRM_GEM_SHMEM_DRIVER_OPS,
>  };
>  
> +/*
> + * DRM device
> + */
> +
> +static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> +{
> +	struct mga_device *mdev;
> +	int ret, option;
> +
> +	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
> +	if (mdev == NULL)
> +		return -ENOMEM;
> +	dev->dev_private = (void *)mdev;
> +	mdev->dev = dev;
> +
> +	mdev->flags = mgag200_flags_from_driver_data(flags);
> +	mdev->type = mgag200_type_from_driver_data(flags);
> +
> +	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
> +	mdev->has_sdram = !(option & (1 << 14));
> +
> +	/* BAR 0 is the framebuffer, BAR 1 contains registers */
> +	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
> +	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
> +
> +	if (!devm_request_mem_region(dev->dev, mdev->rmmio_base,
> +				     mdev->rmmio_size, "mgadrmfb_mmio")) {
> +		drm_err(dev, "can't reserve mmio registers\n");
> +		return -ENOMEM;
> +	}
> +
> +	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
> +	if (mdev->rmmio == NULL)
> +		return -ENOMEM;
> +
> +	/* stash G200 SE model number for later use */
> +	if (IS_G200_SE(mdev)) {
> +		mdev->unique_rev_id = RREG32(0x1e24);
> +		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
> +			mdev->unique_rev_id);
> +	}
> +
> +	ret = mgag200_mm_init(mdev);
> +	if (ret)
> +		goto err_mm;
> +
> +	ret = mgag200_modeset_init(mdev);
> +	if (ret) {
> +		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
> +		goto err_mm;
> +	}
> +
> +	return 0;
> +
> +err_mm:
> +	dev->dev_private = NULL;
> +	return ret;
> +}
> +
> +static void mgag200_driver_unload(struct drm_device *dev)
> +{
> +	struct mga_device *mdev = to_mga_device(dev);
> +
> +	if (mdev == NULL)
> +		return;
> +	dev->dev_private = NULL;
> +}
> +
>  /*
>   * PCI driver
>   */
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 7b6e6827a9a21..b38e5ce4ee20b 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -188,10 +188,6 @@ mgag200_flags_from_driver_data(kernel_ulong_t driver_data)
>  				/* mgag200_mode.c */
>  int mgag200_modeset_init(struct mga_device *mdev);
>  
> -				/* mgag200_main.c */
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
> -void mgag200_driver_unload(struct drm_device *dev);
> -
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>  void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> deleted file mode 100644
> index 49bcdfcb40a4e..0000000000000
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ /dev/null
> @@ -1,77 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0-only
> -/*
> - * Copyright 2010 Matt Turner.
> - * Copyright 2012 Red Hat
> - *
> - * Authors: Matthew Garrett
> - *          Matt Turner
> - *          Dave Airlie
> - */
> -
> -#include <linux/pci.h>
> -
> -#include "mgag200_drv.h"
> -
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct mga_device *mdev;
> -	int ret, option;
> -
> -	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
> -	if (mdev == NULL)
> -		return -ENOMEM;
> -	dev->dev_private = (void *)mdev;
> -	mdev->dev = dev;
> -
> -	mdev->flags = mgag200_flags_from_driver_data(flags);
> -	mdev->type = mgag200_type_from_driver_data(flags);
> -
> -	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
> -	mdev->has_sdram = !(option & (1 << 14));
> -
> -	/* BAR 0 is the framebuffer, BAR 1 contains registers */
> -	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
> -	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
> -
> -	if (!devm_request_mem_region(dev->dev, mdev->rmmio_base,
> -				     mdev->rmmio_size, "mgadrmfb_mmio")) {
> -		drm_err(dev, "can't reserve mmio registers\n");
> -		return -ENOMEM;
> -	}
> -
> -	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
> -	if (mdev->rmmio == NULL)
> -		return -ENOMEM;
> -
> -	/* stash G200 SE model number for later use */
> -	if (IS_G200_SE(mdev)) {
> -		mdev->unique_rev_id = RREG32(0x1e24);
> -		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
> -			mdev->unique_rev_id);
> -	}
> -
> -	ret = mgag200_mm_init(mdev);
> -	if (ret)
> -		goto err_mm;
> -
> -	ret = mgag200_modeset_init(mdev);
> -	if (ret) {
> -		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
> -		goto err_mm;
> -	}
> -
> -	return 0;
> -
> -err_mm:
> -	dev->dev_private = NULL;
> -	return ret;
> -}
> -
> -void mgag200_driver_unload(struct drm_device *dev)
> -{
> -	struct mga_device *mdev = to_mga_device(dev);
> -
> -	if (mdev == NULL)
> -		return;
> -	dev->dev_private = NULL;
> -}
> -- 
> 2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/14] drm/mgag200: Use managed interfaces for auto-cleanup

[Sam Ravnborg]

Hi Thomas.

On Fri, Jun 05, 2020 at 03:57:49PM +0200, Thomas Zimmermann wrote:
> This patchset cleans up mgag200 device initialization, embeds the
> DRM device instance in struct mga_device and finally converts device
> initialization to managed interfaces.
> 
> Patches 1 and 2 are actually unrelated. Both remove artifacts that got
> lost from earlier patch series. We're fixing this before introducing new
> changes.
> 
> Patches 3 to 7 cleanup the memory management code and convert it to
> managed. Specifically, all MM code is being moved into a the same file.
> That makes it more obvious what's going on and will allow for further
> cleanups later on.
> 
> Modesetting is already cleaned up automatically, so there's nothing
> to do here.
> 
> With modesetting and MM done, patches 8 to 14 convert the device
> initialization to managed interfaces. The allocation is split among
> several functions and we move it to the same place in patches 11 and
> 12. That is also a good opportunity to embed the DRM device instance
> in struct mga_device in patch 13. Patch 14 adds managed release of the
> device structure.
> 
> Tested on Matrox G200SE HW.
> 
> Thomas Zimmermann (14):
>   drm/mgag200: Remove declaration of mgag200_mmap() from header file
>   drm/mgag200: Remove mgag200_cursor.c
>   drm/mgag200: Use pcim_enable_device()
>   drm/mgag200: Rename mgag200_ttm.c to mgag200_mm.c
>   drm/mgag200: Lookup VRAM PCI BAR start and length only once
>   drm/mgag200: Merge VRAM setup into MM initialization
>   drm/mgag200: Switch to managed MM
>   drm/mgag200: Separate DRM and PCI functionality from each other
>   drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_
>   drm/mgag200: Move device init and cleanup to mgag200_drv.c
>   drm/mgag200: Separate device initialization into allocation
>   drm/mgag200: Allocate device structures in mgag200_driver_load()
>   drm/mgag200: Embed instance of struct drm_device in struct mga_device
>   drm/mgag200: Use managed device initialization

Looked through all patches.
A few triggered some small comments. With the comments addressed all
patches are:
Acked-by: Sam Ravnborg <sam@ravnborg.org>

My comments can, if any chenges are required, be addressed when
applying. No need for a next round.

	Sam



> 
>  drivers/gpu/drm/mgag200/Makefile              |   3 +-
>  drivers/gpu/drm/mgag200/mgag200_cursor.c      | 319 ------------------
>  drivers/gpu/drm/mgag200/mgag200_drv.c         | 161 ++++++---
>  drivers/gpu/drm/mgag200/mgag200_drv.h         |  11 +-
>  drivers/gpu/drm/mgag200/mgag200_main.c        | 155 ---------
>  .../mgag200/{mgag200_ttm.c => mgag200_mm.c}   |  99 ++++--
>  drivers/gpu/drm/mgag200/mgag200_mode.c        |  12 +-
>  7 files changed, 195 insertions(+), 565 deletions(-)
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c
>  rename drivers/gpu/drm/mgag200/{mgag200_ttm.c => mgag200_mm.c} (51%)
> 
> --
> 2.26.2
> 
> 
> Thomas Zimmermann (14):
>   drm/mgag200: Remove declaration of mgag200_mmap() from header file
>   drm/mgag200: Remove mgag200_cursor.c
>   drm/mgag200: Use pcim_enable_device()
>   drm/mgag200: Rename mgag200_ttm.c to mgag200_mm.c
>   drm/mgag200: Lookup VRAM PCI BAR start and length only once
>   drm/mgag200: Merge VRAM setup into MM initialization
>   drm/mgag200: Switch to managed MM
>   drm/mgag200: Separate DRM and PCI functionality from each other
>   drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_
>   drm/mgag200: Move device init and cleanup to mgag200_drv.c
>   drm/mgag200: Separate device initialization into allocation
>   drm/mgag200: Allocate device structures in mgag200_driver_load()
>   drm/mgag200: Embed instance of struct drm_device in struct mga_device
>   drm/mgag200: Use managed device initialization
> 
>  drivers/gpu/drm/mgag200/Makefile              |   3 +-
>  drivers/gpu/drm/mgag200/mgag200_cursor.c      | 319 ------------------
>  drivers/gpu/drm/mgag200/mgag200_drv.c         | 161 ++++++---
>  drivers/gpu/drm/mgag200/mgag200_drv.h         |  11 +-
>  drivers/gpu/drm/mgag200/mgag200_main.c        | 155 ---------
>  .../mgag200/{mgag200_ttm.c => mgag200_mm.c}   |  99 ++++--
>  drivers/gpu/drm/mgag200/mgag200_mode.c        |  12 +-
>  7 files changed, 195 insertions(+), 565 deletions(-)
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_cursor.c
>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c
>  rename drivers/gpu/drm/mgag200/{mgag200_ttm.c => mgag200_mm.c} (51%)
> 
> --
> 2.26.2
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 07/14] drm/mgag200: Switch to managed MM

[Daniel Vetter]

On Fri, Jun 05, 2020 at 03:57:56PM +0200, Thomas Zimmermann wrote:
> The memory-management code now cleans up automatically as part of
> device destruction.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
>  drivers/gpu/drm/mgag200/mgag200_main.c |  5 +----
>  drivers/gpu/drm/mgag200/mgag200_mm.c   | 28 ++++++++++++++------------
>  3 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index cd786ffe319b8..7b6e6827a9a21 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
>  
>  				/* mgag200_mm.c */
>  int mgag200_mm_init(struct mga_device *mdev);
> -void mgag200_mm_fini(struct mga_device *mdev);
>  
>  #endif				/* __MGAG200_DRV_H__ */
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index e9ad783c2b44d..49bcdfcb40a4e 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -57,13 +57,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	ret = mgag200_modeset_init(mdev);
>  	if (ret) {
>  		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
> -		goto err_mgag200_mm_fini;
> +		goto err_mm;
>  	}
>  
>  	return 0;
>  
> -err_mgag200_mm_fini:
> -	mgag200_mm_fini(mdev);
>  err_mm:
>  	dev->dev_private = NULL;
>  	return ret;
> @@ -75,6 +73,5 @@ void mgag200_driver_unload(struct drm_device *dev)
>  
>  	if (mdev == NULL)
>  		return;
> -	mgag200_mm_fini(mdev);
>  	dev->dev_private = NULL;
>  }
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
> index f56b0456994f4..1b1e5ec5d1ceb 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mm.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
> @@ -28,6 +28,8 @@
>  
>  #include <linux/pci.h>
>  
> +#include <drm/drm_managed.h>
> +
>  #include "mgag200_drv.h"
>  
>  static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
> @@ -73,6 +75,18 @@ static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
>  	return offset - 65536;
>  }
>  
> +static void mgag200_mm_release(struct drm_device *dev, void *ptr)
> +{
> +	struct mga_device *mdev = to_mga_device(dev);
> +
> +	mdev->vram_fb_available = 0;
> +	iounmap(mdev->vram);
> +	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
> +				pci_resource_len(dev->pdev, 0));
> +	arch_phys_wc_del(mdev->fb_mtrr);
> +	mdev->fb_mtrr = 0;
> +}
> +
>  int mgag200_mm_init(struct mga_device *mdev)
>  {
>  	struct drm_device *dev = mdev->dev;
> @@ -104,22 +118,10 @@ int mgag200_mm_init(struct mga_device *mdev)
>  
>  	mdev->vram_fb_available = mdev->mc.vram_size;
>  
> -	return 0;
> +	return drmm_add_action_or_reset(dev, mgag200_mm_release, NULL);
>  
>  err_arch_phys_wc_del:
>  	arch_phys_wc_del(mdev->fb_mtrr);
>  	arch_io_free_memtype_wc(start, len);

Btw I think devm versions of these two would benefit a bunch of drivers in
their cleanup code. devm_ not drmm_ since it's hw resource cleanup. In
case you ever run out of ideas :-)

Cheeres, Daniel

>  	return ret;
>  }
> -
> -void mgag200_mm_fini(struct mga_device *mdev)
> -{
> -	struct drm_device *dev = mdev->dev;
> -
> -	mdev->vram_fb_available = 0;
> -	iounmap(mdev->vram);
> -	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
> -				pci_resource_len(dev->pdev, 0));
> -	arch_phys_wc_del(mdev->fb_mtrr);
> -	mdev->fb_mtrr = 0;
> -}
> -- 
> 2.26.2
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/14] drm/mgag200: Move device init and cleanup to mgag200_drv.c

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 7899 bytes --]

Hi Sam

Am 05.06.20 um 16:45 schrieb Sam Ravnborg:
> On Fri, Jun 05, 2020 at 03:57:59PM +0200, Thomas Zimmermann wrote:
>> Moving the initializer and cleanup functions for device instances
>> to mgag200_drv.c prepares for the conversion to managed code. No
>> functional changes are made. Remove mgag200_main.c, which is now
>> empty.
> 
> The functions are still named xx_load() - which comes from old days
> where drm_driver has a load callback.
> We can always fight about naming, but I am just left with the impression
> that better naming exists today.

... and is available in patch 11. :) The driver will have
mgag200_device_create() to allocate and initialize a device. The cleanup
is done automatically, so a _destroy() function is not necessary.

My first attempt on this series was 8 or 9 patches long. I split up some
of them to make the conversion easier to follow and more logical.
Occasionally I had to keep obsolete artifacts, such as load and unload,
 in a patch if it benefits the series as a whole.

Best regards
Thomas

> Just a drive by comment - feel free to ignore.
> 
> 	Sam
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/Makefile       |  3 +-
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 68 +++++++++++++++++++++++
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  4 --
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 77 --------------------------
>>  4 files changed, 69 insertions(+), 83 deletions(-)
>>  delete mode 100644 drivers/gpu/drm/mgag200/mgag200_main.c
>>
>> diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
>> index e6a933874a88c..42fedef538826 100644
>> --- a/drivers/gpu/drm/mgag200/Makefile
>> +++ b/drivers/gpu/drm/mgag200/Makefile
>> @@ -1,5 +1,4 @@
>>  # SPDX-License-Identifier: GPL-2.0-only
>> -mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_main.o mgag200_mm.o \
>> -	       mgag200_mode.o
>> +mgag200-y   := mgag200_drv.o mgag200_i2c.o mgag200_mm.o mgag200_mode.o
>>  
>>  obj-$(CONFIG_DRM_MGAG200) += mgag200.o
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index ad74e02d8f251..f8bb24199643d 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -39,6 +39,74 @@ static struct drm_driver mgag200_driver = {
>>  	DRM_GEM_SHMEM_DRIVER_OPS,
>>  };
>>  
>> +/*
>> + * DRM device
>> + */
>> +
>> +static int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>> +{
>> +	struct mga_device *mdev;
>> +	int ret, option;
>> +
>> +	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
>> +	if (mdev == NULL)
>> +		return -ENOMEM;
>> +	dev->dev_private = (void *)mdev;
>> +	mdev->dev = dev;
>> +
>> +	mdev->flags = mgag200_flags_from_driver_data(flags);
>> +	mdev->type = mgag200_type_from_driver_data(flags);
>> +
>> +	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
>> +	mdev->has_sdram = !(option & (1 << 14));
>> +
>> +	/* BAR 0 is the framebuffer, BAR 1 contains registers */
>> +	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
>> +	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
>> +
>> +	if (!devm_request_mem_region(dev->dev, mdev->rmmio_base,
>> +				     mdev->rmmio_size, "mgadrmfb_mmio")) {
>> +		drm_err(dev, "can't reserve mmio registers\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
>> +	if (mdev->rmmio == NULL)
>> +		return -ENOMEM;
>> +
>> +	/* stash G200 SE model number for later use */
>> +	if (IS_G200_SE(mdev)) {
>> +		mdev->unique_rev_id = RREG32(0x1e24);
>> +		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
>> +			mdev->unique_rev_id);
>> +	}
>> +
>> +	ret = mgag200_mm_init(mdev);
>> +	if (ret)
>> +		goto err_mm;
>> +
>> +	ret = mgag200_modeset_init(mdev);
>> +	if (ret) {
>> +		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
>> +		goto err_mm;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_mm:
>> +	dev->dev_private = NULL;
>> +	return ret;
>> +}
>> +
>> +static void mgag200_driver_unload(struct drm_device *dev)
>> +{
>> +	struct mga_device *mdev = to_mga_device(dev);
>> +
>> +	if (mdev == NULL)
>> +		return;
>> +	dev->dev_private = NULL;
>> +}
>> +
>>  /*
>>   * PCI driver
>>   */
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 7b6e6827a9a21..b38e5ce4ee20b 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -188,10 +188,6 @@ mgag200_flags_from_driver_data(kernel_ulong_t driver_data)
>>  				/* mgag200_mode.c */
>>  int mgag200_modeset_init(struct mga_device *mdev);
>>  
>> -				/* mgag200_main.c */
>> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>> -void mgag200_driver_unload(struct drm_device *dev);
>> -
>>  				/* mgag200_i2c.c */
>>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>>  void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> deleted file mode 100644
>> index 49bcdfcb40a4e..0000000000000
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ /dev/null
>> @@ -1,77 +0,0 @@
>> -// SPDX-License-Identifier: GPL-2.0-only
>> -/*
>> - * Copyright 2010 Matt Turner.
>> - * Copyright 2012 Red Hat
>> - *
>> - * Authors: Matthew Garrett
>> - *          Matt Turner
>> - *          Dave Airlie
>> - */
>> -
>> -#include <linux/pci.h>
>> -
>> -#include "mgag200_drv.h"
>> -
>> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>> -{
>> -	struct mga_device *mdev;
>> -	int ret, option;
>> -
>> -	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
>> -	if (mdev == NULL)
>> -		return -ENOMEM;
>> -	dev->dev_private = (void *)mdev;
>> -	mdev->dev = dev;
>> -
>> -	mdev->flags = mgag200_flags_from_driver_data(flags);
>> -	mdev->type = mgag200_type_from_driver_data(flags);
>> -
>> -	pci_read_config_dword(dev->pdev, PCI_MGA_OPTION, &option);
>> -	mdev->has_sdram = !(option & (1 << 14));
>> -
>> -	/* BAR 0 is the framebuffer, BAR 1 contains registers */
>> -	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
>> -	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
>> -
>> -	if (!devm_request_mem_region(dev->dev, mdev->rmmio_base,
>> -				     mdev->rmmio_size, "mgadrmfb_mmio")) {
>> -		drm_err(dev, "can't reserve mmio registers\n");
>> -		return -ENOMEM;
>> -	}
>> -
>> -	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
>> -	if (mdev->rmmio == NULL)
>> -		return -ENOMEM;
>> -
>> -	/* stash G200 SE model number for later use */
>> -	if (IS_G200_SE(mdev)) {
>> -		mdev->unique_rev_id = RREG32(0x1e24);
>> -		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
>> -			mdev->unique_rev_id);
>> -	}
>> -
>> -	ret = mgag200_mm_init(mdev);
>> -	if (ret)
>> -		goto err_mm;
>> -
>> -	ret = mgag200_modeset_init(mdev);
>> -	if (ret) {
>> -		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
>> -		goto err_mm;
>> -	}
>> -
>> -	return 0;
>> -
>> -err_mm:
>> -	dev->dev_private = NULL;
>> -	return ret;
>> -}
>> -
>> -void mgag200_driver_unload(struct drm_device *dev)
>> -{
>> -	struct mga_device *mdev = to_mga_device(dev);
>> -
>> -	if (mdev == NULL)
>> -		return;
>> -	dev->dev_private = NULL;
>> -}
>> -- 
>> 2.26.2
> _______________________________________________
> 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


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

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

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

Re: [PATCH 07/14] drm/mgag200: Switch to managed MM

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 4405 bytes --]

Hi

Am 05.06.20 um 18:22 schrieb Daniel Vetter:
> On Fri, Jun 05, 2020 at 03:57:56PM +0200, Thomas Zimmermann wrote:
>> The memory-management code now cleans up automatically as part of
>> device destruction.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  1 -
>>  drivers/gpu/drm/mgag200/mgag200_main.c |  5 +----
>>  drivers/gpu/drm/mgag200/mgag200_mm.c   | 28 ++++++++++++++------------
>>  3 files changed, 16 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index cd786ffe319b8..7b6e6827a9a21 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -198,6 +198,5 @@ void mgag200_i2c_destroy(struct mga_i2c_chan *i2c);
>>  
>>  				/* mgag200_mm.c */
>>  int mgag200_mm_init(struct mga_device *mdev);
>> -void mgag200_mm_fini(struct mga_device *mdev);
>>  
>>  #endif				/* __MGAG200_DRV_H__ */
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index e9ad783c2b44d..49bcdfcb40a4e 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -57,13 +57,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	ret = mgag200_modeset_init(mdev);
>>  	if (ret) {
>>  		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
>> -		goto err_mgag200_mm_fini;
>> +		goto err_mm;
>>  	}
>>  
>>  	return 0;
>>  
>> -err_mgag200_mm_fini:
>> -	mgag200_mm_fini(mdev);
>>  err_mm:
>>  	dev->dev_private = NULL;
>>  	return ret;
>> @@ -75,6 +73,5 @@ void mgag200_driver_unload(struct drm_device *dev)
>>  
>>  	if (mdev == NULL)
>>  		return;
>> -	mgag200_mm_fini(mdev);
>>  	dev->dev_private = NULL;
>>  }
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
>> index f56b0456994f4..1b1e5ec5d1ceb 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mm.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
>> @@ -28,6 +28,8 @@
>>  
>>  #include <linux/pci.h>
>>  
>> +#include <drm/drm_managed.h>
>> +
>>  #include "mgag200_drv.h"
>>  
>>  static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
>> @@ -73,6 +75,18 @@ static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
>>  	return offset - 65536;
>>  }
>>  
>> +static void mgag200_mm_release(struct drm_device *dev, void *ptr)
>> +{
>> +	struct mga_device *mdev = to_mga_device(dev);
>> +
>> +	mdev->vram_fb_available = 0;
>> +	iounmap(mdev->vram);
>> +	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
>> +				pci_resource_len(dev->pdev, 0));
>> +	arch_phys_wc_del(mdev->fb_mtrr);
>> +	mdev->fb_mtrr = 0;
>> +}
>> +
>>  int mgag200_mm_init(struct mga_device *mdev)
>>  {
>>  	struct drm_device *dev = mdev->dev;
>> @@ -104,22 +118,10 @@ int mgag200_mm_init(struct mga_device *mdev)
>>  
>>  	mdev->vram_fb_available = mdev->mc.vram_size;
>>  
>> -	return 0;
>> +	return drmm_add_action_or_reset(dev, mgag200_mm_release, NULL);
>>  
>>  err_arch_phys_wc_del:
>>  	arch_phys_wc_del(mdev->fb_mtrr);
>>  	arch_io_free_memtype_wc(start, len);
> 
> Btw I think devm versions of these two would benefit a bunch of drivers in
> their cleanup code. devm_ not drmm_ since it's hw resource cleanup. In

Yup, I was looking and couldn't find them. Their non-existence is the
reason for the release function. Having them would make sense.

> case you ever run out of ideas :-)

I already have patches for further mgag200 mode-setting cleanups and ast
managed initialization. So no shortage of idea here. :)

Best regards
Thomas

> 
> Cheeres, Daniel
> 
>>  	return ret;
>>  }
>> -
>> -void mgag200_mm_fini(struct mga_device *mdev)
>> -{
>> -	struct drm_device *dev = mdev->dev;
>> -
>> -	mdev->vram_fb_available = 0;
>> -	iounmap(mdev->vram);
>> -	arch_io_free_memtype_wc(pci_resource_start(dev->pdev, 0),
>> -				pci_resource_len(dev->pdev, 0));
>> -	arch_phys_wc_del(mdev->fb_mtrr);
>> -	mdev->fb_mtrr = 0;
>> -}
>> -- 
>> 2.26.2
>>
> 

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


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

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

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

Re: [PATCH 06/14] drm/mgag200: Merge VRAM setup into MM initialization

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 6112 bytes --]

Hi

Am 05.06.20 um 16:39 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> Some parts I did not understand here.
> 
> On Fri, Jun 05, 2020 at 03:57:55PM +0200, Thomas Zimmermann wrote:
>> The VRAM setup in mgag200_drv.c is part of memory management and
>> should be done in the same place. Merge the code into the memory
>> management's init function.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 75 --------------------------
>>  drivers/gpu/drm/mgag200/mgag200_mm.c   | 52 ++++++++++++++++++
>>  2 files changed, 52 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index 3298eff7bd1b4..e9ad783c2b44d 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -12,77 +12,6 @@
>>  
>>  #include "mgag200_drv.h"
>>  
>> -static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
>> -{
>> -	int offset;
>> -	int orig;
>> -	int test1, test2;
>> -	int orig1, orig2;
>> -	unsigned int vram_size;
>> -
>> -	/* Probe */
>> -	orig = ioread16(mem);
>> -	iowrite16(0, mem);
>> -
>> -	vram_size = mdev->mc.vram_window;
>> -
>> -	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000)) {
>> -		vram_size = vram_size - 0x400000;
>> -	}
>> -
>> -	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
>> -		orig1 = ioread8(mem + offset);
>> -		orig2 = ioread8(mem + offset + 0x100);
>> -
>> -		iowrite16(0xaa55, mem + offset);
>> -		iowrite16(0xaa55, mem + offset + 0x100);
>> -
>> -		test1 = ioread16(mem + offset);
>> -		test2 = ioread16(mem);
>> -
>> -		iowrite16(orig1, mem + offset);
>> -		iowrite16(orig2, mem + offset + 0x100);
>> -
>> -		if (test1 != 0xaa55) {
>> -			break;
>> -		}
>> -
>> -		if (test2) {
>> -			break;
>> -		}
>> -	}
>> -
>> -	iowrite16(orig, mem);
>> -	return offset - 65536;
>> -}
>> -
>> -/* Map the framebuffer from the card and configure the core */
>> -static int mga_vram_init(struct mga_device *mdev)
>> -{
>> -	struct drm_device *dev = mdev->dev;
>> -	void __iomem *mem;
>> -
>> -	/* BAR 0 is VRAM */
>> -	mdev->mc.vram_base = pci_resource_start(dev->pdev, 0);
>> -	mdev->mc.vram_window = pci_resource_len(dev->pdev, 0);
>> -
>> -	if (!devm_request_mem_region(dev->dev, mdev->mc.vram_base,
>> -				     mdev->mc.vram_window, "mgadrmfb_vram")) {
>> -		DRM_ERROR("can't reserve VRAM\n");
>> -		return -ENXIO;
>> -	}
>> -
>> -	mem = pci_iomap(dev->pdev, 0, 0);
>> -	if (!mem)
>> -		return -ENOMEM;
>> -
>> -	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
>> -
>> -	pci_iounmap(dev->pdev, mem);
> mem is the result of pci_iomap() - but I do not see any call
> to pci_iomap() in the converted code.
> 
> mdev->vram is used as argument in new code where mem was used in the old
> code.
> mdev->vram comes from ioremap(start, len) - will it result in the same?

Yes. pci_iomap() maps the whole PCI BAR (i.e., 0 in this case) memory.
The driver code reads the bar's start and length, which also covers the
full PCI BAR range. So in the end the probe function runs on the same
range of VRAM.

Best regards
Thomas

> 
> 	Sam
> 
> 
>> -
>> -	return 0;
>> -}
>> -
>>  int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  {
>>  	struct mga_device *mdev;
>> @@ -121,10 +50,6 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  			mdev->unique_rev_id);
>>  	}
>>  
>> -	ret = mga_vram_init(mdev);
>> -	if (ret)
>> -		return ret;
>> -
>>  	ret = mgag200_mm_init(mdev);
>>  	if (ret)
>>  		goto err_mm;
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mm.c b/drivers/gpu/drm/mgag200/mgag200_mm.c
>> index 73e30901e0631..f56b0456994f4 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mm.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mm.c
>> @@ -30,6 +30,49 @@
>>  
>>  #include "mgag200_drv.h"
>>  
>> +static size_t mgag200_probe_vram(struct mga_device *mdev, void __iomem *mem,
>> +				 size_t size)
>> +{
>> +	int offset;
>> +	int orig;
>> +	int test1, test2;
>> +	int orig1, orig2;
>> +	size_t vram_size;
>> +
>> +	/* Probe */
>> +	orig = ioread16(mem);
>> +	iowrite16(0, mem);
>> +
>> +	vram_size = size;
>> +
>> +	if ((mdev->type == G200_EW3) && (vram_size >= 0x1000000))
>> +		vram_size = vram_size - 0x400000;
>> +
>> +	for (offset = 0x100000; offset < vram_size; offset += 0x4000) {
>> +		orig1 = ioread8(mem + offset);
>> +		orig2 = ioread8(mem + offset + 0x100);
>> +
>> +		iowrite16(0xaa55, mem + offset);
>> +		iowrite16(0xaa55, mem + offset + 0x100);
>> +
>> +		test1 = ioread16(mem + offset);
>> +		test2 = ioread16(mem);
>> +
>> +		iowrite16(orig1, mem + offset);
>> +		iowrite16(orig2, mem + offset + 0x100);
>> +
>> +		if (test1 != 0xaa55)
>> +			break;
>> +
>> +		if (test2)
>> +			break;
>> +	}
>> +
>> +	iowrite16(orig, mem);
>> +
>> +	return offset - 65536;
>> +}
>> +
>>  int mgag200_mm_init(struct mga_device *mdev)
>>  {
>>  	struct drm_device *dev = mdev->dev;
>> @@ -40,6 +83,11 @@ int mgag200_mm_init(struct mga_device *mdev)
>>  	start = pci_resource_start(dev->pdev, 0);
>>  	len = pci_resource_len(dev->pdev, 0);
>>  
>> +	if (!devm_request_mem_region(dev->dev, start, len, "mgadrmfb_vram")) {
>> +		drm_err(dev, "can't reserve VRAM\n");
>> +		return -ENXIO;
>> +	}
>> +
>>  	arch_io_reserve_memtype_wc(start, len);
>>  
>>  	mdev->fb_mtrr = arch_phys_wc_add(start, len);
>> @@ -50,6 +98,10 @@ int mgag200_mm_init(struct mga_device *mdev)
>>  		goto err_arch_phys_wc_del;
>>  	}
>>  
>> +	mdev->mc.vram_size = mgag200_probe_vram(mdev, mdev->vram, len);
>> +	mdev->mc.vram_base = start;
>> +	mdev->mc.vram_window = len;
>> +
>>  	mdev->vram_fb_available = mdev->mc.vram_size;
>>  
>>  	return 0;
>> -- 
>> 2.26.2

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


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

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

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

Re: [PATCH 09/14] drm/mgag200: Prefix global names in mgag200_drv.c with mgag200_

[Thomas Zimmermann]

[-- Attachment #1.1.1: Type: text/plain, Size: 3306 bytes --]

Hi

Am 05.06.20 um 16:42 schrieb Sam Ravnborg:
> On Fri, Jun 05, 2020 at 03:57:58PM +0200, Thomas Zimmermann wrote:
>> The naming of global symbols in mgag200_drv.c is inconsistent. Fix
>> that by prefixing all names with mgag200_.
> 
> Hmm, static symbols are hardly global symbols.
> Patch is fine, but changelog seems a litte off.

OK, I'll try to clarify this.

Best regards
Thomas

> 
> 	Sam
> 
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.c | 20 ++++++++++----------
>>  1 file changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index 670e12d57dea8..ad74e02d8f251 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -27,7 +27,7 @@ module_param_named(modeset, mgag200_modeset, int, 0400);
>>  
>>  DEFINE_DRM_GEM_FOPS(mgag200_driver_fops);
>>  
>> -static struct drm_driver driver = {
>> +static struct drm_driver mgag200_driver = {
>>  	.driver_features = DRIVER_ATOMIC | DRIVER_GEM | DRIVER_MODESET,
>>  	.fops = &mgag200_driver_fops,
>>  	.name = DRIVER_NAME,
>> @@ -43,7 +43,7 @@ static struct drm_driver driver = {
>>   * PCI driver
>>   */
>>  
>> -static const struct pci_device_id pciidlist[] = {
>> +static const struct pci_device_id mgag200_pciidlist[] = {
>>  	{ PCI_VENDOR_ID_MATROX, 0x522, PCI_ANY_ID, PCI_ANY_ID, 0, 0,
>>  		G200_SE_A | MGAG200_FLAG_HW_BUG_NO_STARTADD},
>>  	{ PCI_VENDOR_ID_MATROX, 0x524, PCI_ANY_ID, PCI_ANY_ID, 0, 0, G200_SE_B },
>> @@ -56,10 +56,10 @@ static const struct pci_device_id pciidlist[] = {
>>  	{0,}
>>  };
>>  
>> -MODULE_DEVICE_TABLE(pci, pciidlist);
>> +MODULE_DEVICE_TABLE(pci, mgag200_pciidlist);
>>  
>> -
>> -static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +static int
>> +mgag200_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>>  	struct drm_device *dev;
>>  	int ret;
>> @@ -70,7 +70,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  	if (ret)
>>  		return ret;
>>  
>> -	dev = drm_dev_alloc(&driver, &pdev->dev);
>> +	dev = drm_dev_alloc(&mgag200_driver, &pdev->dev);
>>  	if (IS_ERR(dev))
>>  		return PTR_ERR(dev);
>>  
>> @@ -96,7 +96,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  	return ret;
>>  }
>>  
>> -static void mga_pci_remove(struct pci_dev *pdev)
>> +static void mgag200_pci_remove(struct pci_dev *pdev)
>>  {
>>  	struct drm_device *dev = pci_get_drvdata(pdev);
>>  
>> @@ -107,9 +107,9 @@ static void mga_pci_remove(struct pci_dev *pdev)
>>  
>>  static struct pci_driver mgag200_pci_driver = {
>>  	.name = DRIVER_NAME,
>> -	.id_table = pciidlist,
>> -	.probe = mga_pci_probe,
>> -	.remove = mga_pci_remove,
>> +	.id_table = mgag200_pciidlist,
>> +	.probe = mgag200_pci_probe,
>> +	.remove = mgag200_pci_remove,
>>  };
>>  
>>  static int __init mgag200_init(void)
>> -- 
>> 2.26.2

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


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

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

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