[PATCH 0/5] drm/mgag200: Embed DRM device in struct mga_device

[PATCH 0/5] drm/mgag200: Embed DRM device in struct mga_device

[Thomas Zimmermann]

After receiving reviews on the conversion of mgag200 to atomic mode
setting, I thought it would make sense to embed the DRM device in
struct mga_device first. Several comments in the atomic-conversion
reviews refer to that.

Patches 1 to 3 do some cleanups and preparation work. Patch 4 changes
the the init functions to allocate struct mga_device before struct
drm_device. Patch 5 does the conversion.

I did not switch over struct mga_device to the new managed release
code. I found that this justifies another round of cleanup patches,
which I did not want to put into this patchset.

The patches were tested on mgag200 hardware.

Thomas Zimmermann (5):
  drm/mgag200: Convert struct drm_device to struct mga_device with macro
  drm/mgag200: Integrate init function into load function
  drm/mgag200: Remove several references to struct mga_device.dev
  drm/mgag200: Init and finalize devices in mgag200_device_{init,fini}()
  drm/mgag200: Embed DRM device instance in struct mga_device

 drivers/gpu/drm/mgag200/mgag200_cursor.c |  10 +-
 drivers/gpu/drm/mgag200/mgag200_drv.c    |  29 +++---
 drivers/gpu/drm/mgag200/mgag200_drv.h    |   8 +-
 drivers/gpu/drm/mgag200/mgag200_i2c.c    |  10 +-
 drivers/gpu/drm/mgag200/mgag200_main.c   | 114 +++++++++++------------
 drivers/gpu/drm/mgag200/mgag200_mode.c   |  35 +++----
 drivers/gpu/drm/mgag200/mgag200_ttm.c    |   4 +-
 7 files changed, 101 insertions(+), 109 deletions(-)

--
2.26.0

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

[PATCH 1/5] drm/mgag200: Convert struct drm_device to struct mga_device with macro

[Thomas Zimmermann]

Mgag200 used dev_private to look up struct mga_device for instances
of struct drm_device. Use of dev_private is deprecated, so hide it in
the macro to_mga_device().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_cursor.c |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_drv.c    |  2 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h    |  1 +
 drivers/gpu/drm/mgag200/mgag200_i2c.c    | 10 +++++-----
 drivers/gpu/drm/mgag200/mgag200_main.c   |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_mode.c   | 18 +++++++++---------
 6 files changed, 20 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
index d491edd317ff3..aebc9ce43d551 100644
--- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -260,7 +260,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 			    uint32_t handle, uint32_t width, uint32_t height)
 {
 	struct drm_device *dev = crtc->dev;
-	struct mga_device *mdev = (struct mga_device *)dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	struct drm_gem_object *obj;
 	struct drm_gem_vram_object *gbo = NULL;
 	int ret;
@@ -307,7 +307,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 
 int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 {
-	struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(crtc->dev);
 
 	/* Our origin is at (64,64) */
 	x += 64;
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index 3298b7ef18b03..c2f0e4b40b052 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -120,7 +120,7 @@ int mgag200_driver_dumb_create(struct drm_file *file,
 			       struct drm_device *dev,
 			       struct drm_mode_create_dumb *args)
 {
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	unsigned long pg_align;
 
 	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 9691252d6233f..632bbb50465c9 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -96,6 +96,7 @@
 
 #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
 #define to_mga_connector(x) container_of(x, struct mga_connector, base)
+#define to_mga_device(x) ((struct mga_device *)(x)->dev_private)
 
 struct mga_crtc {
 	struct drm_crtc base;
diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index 9f4635916d322..09731e614e46d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -61,34 +61,34 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
 static void mga_gpio_setsda(void *data, int state)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = i2c->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(i2c->dev);
 	mga_i2c_set(mdev, i2c->data, state);
 }
 
 static void mga_gpio_setscl(void *data, int state)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = i2c->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(i2c->dev);
 	mga_i2c_set(mdev, i2c->clock, state);
 }
 
 static int mga_gpio_getsda(void *data)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = i2c->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(i2c->dev);
 	return (mga_i2c_read_gpio(mdev) & i2c->data) ? 1 : 0;
 }
 
 static int mga_gpio_getscl(void *data)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = i2c->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(i2c->dev);
 	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
 }
 
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
 {
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	struct mga_i2c_chan *i2c;
 	int ret;
 	int data, clock;
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index b680cf47cbb94..b705b7776d2fc 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -92,7 +92,7 @@ static int mga_vram_init(struct mga_device *mdev)
 static int mgag200_device_init(struct drm_device *dev,
 			       uint32_t flags)
 {
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	int ret, option;
 
 	mdev->flags = mgag200_flags_from_driver_data(flags);
@@ -195,7 +195,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 
 void mgag200_driver_unload(struct drm_device *dev)
 {
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 
 	if (mdev == NULL)
 		return;
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index d90e83959fca1..fa91869c0db52 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -28,7 +28,7 @@
 static void mga_crtc_load_lut(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	struct drm_framebuffer *fb = crtc->primary->fb;
 	u16 *r_ptr, *g_ptr, *b_ptr;
 	int i;
@@ -728,7 +728,7 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock)
 
 static void mga_g200wb_prepare(struct drm_crtc *crtc)
 {
-	struct mga_device *mdev = crtc->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(crtc->dev);
 	u8 tmp;
 	int iter_max;
 
@@ -783,7 +783,7 @@ static void mga_g200wb_prepare(struct drm_crtc *crtc)
 static void mga_g200wb_commit(struct drm_crtc *crtc)
 {
 	u8 tmp;
-	struct mga_device *mdev = crtc->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(crtc->dev);
 
 	/* 1- The first step is to ensure that the vrsten and hrsten are set */
 	WREG8(MGAREG_CRTCEXT_INDEX, 1);
@@ -833,7 +833,7 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
  */
 static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
 {
-	struct mga_device *mdev = crtc->dev->dev_private;
+	struct mga_device *mdev = to_mga_device(crtc->dev);
 	u32 addr;
 	int count;
 	u8 crtcext0;
@@ -902,7 +902,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 				int x, int y, struct drm_framebuffer *old_fb)
 {
 	struct drm_device *dev = crtc->dev;
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	const struct drm_framebuffer *fb = crtc->primary->fb;
 	int hdisplay, hsyncstart, hsyncend, htotal;
 	int vdisplay, vsyncstart, vsyncend, vtotal;
@@ -1263,7 +1263,7 @@ static int mga_resume(struct drm_crtc *crtc)
 static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
 {
 	struct drm_device *dev = crtc->dev;
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	u8 seq1 = 0, crtcext1 = 0;
 
 	switch (mode) {
@@ -1317,7 +1317,7 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
 static void mga_crtc_prepare(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	u8 tmp;
 
 	/*	mga_resume(crtc);*/
@@ -1353,7 +1353,7 @@ static void mga_crtc_prepare(struct drm_crtc *crtc)
 static void mga_crtc_commit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct mga_device *mdev = dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
 	u8 tmp;
 
@@ -1495,7 +1495,7 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
 				 struct drm_display_mode *mode)
 {
 	struct drm_device *dev = connector->dev;
-	struct mga_device *mdev = (struct mga_device*)dev->dev_private;
+	struct mga_device *mdev = to_mga_device(dev);
 	int bpp = 32;
 
 	if (IS_G200_SE(mdev)) {
-- 
2.26.0

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

[PATCH 2/5] drm/mgag200: Integrate init function into load function

[Thomas Zimmermann]

Done to simplify initialization code before embedding the DRM device
instance in struct mga_device.

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index b705b7776d2fc..3830d3f3c9fa2 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -89,12 +89,23 @@ static int mga_vram_init(struct mga_device *mdev)
 	return 0;
 }
 
-static int mgag200_device_init(struct drm_device *dev,
-			       uint32_t flags)
+/*
+ * Functions here will be called by the core once it's bound the driver to
+ * a PCI device
+ */
+
+
+int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 {
-	struct mga_device *mdev = to_mga_device(dev);
+	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);
 
@@ -110,7 +121,7 @@ static int mgag200_device_init(struct drm_device *dev,
 
 	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
 				"mgadrmfb_mmio")) {
-		DRM_ERROR("can't reserve mmio registers\n");
+		drm_err(dev, "can't reserve mmio registers\n");
 		return -ENOMEM;
 	}
 
@@ -121,8 +132,8 @@ static int mgag200_device_init(struct drm_device *dev,
 	/* stash G200 SE model number for later use */
 	if (IS_G200_SE(mdev)) {
 		mdev->unique_rev_id = RREG32(0x1e24);
-		DRM_DEBUG("G200 SE unique revision id is 0x%x\n",
-			  mdev->unique_rev_id);
+		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
+			mdev->unique_rev_id);
 	}
 
 	ret = mga_vram_init(mdev);
@@ -133,33 +144,9 @@ static int mgag200_device_init(struct drm_device *dev,
 	mdev->bpp_shifts[1] = 1;
 	mdev->bpp_shifts[2] = 0;
 	mdev->bpp_shifts[3] = 2;
-	return 0;
-}
 
-/*
- * Functions here will be called by the core once it's bound the driver to
- * a PCI device
- */
-
-
-int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
-{
-	struct mga_device *mdev;
-	int r;
-
-	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
-	if (mdev == NULL)
-		return -ENOMEM;
-	dev->dev_private = (void *)mdev;
-	mdev->dev = dev;
-
-	r = mgag200_device_init(dev, flags);
-	if (r) {
-		dev_err(&dev->pdev->dev, "Fatal error during GPU init: %d\n", r);
-		return r;
-	}
-	r = mgag200_mm_init(mdev);
-	if (r)
+	ret = mgag200_mm_init(mdev);
+	if (ret)
 		goto err_mm;
 
 	drm_mode_config_init(dev);
@@ -170,16 +157,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 		dev->mode_config.preferred_depth = 32;
 	dev->mode_config.prefer_shadow = 1;
 
-	r = mgag200_modeset_init(mdev);
-	if (r) {
-		dev_err(&dev->pdev->dev, "Fatal error during modeset init: %d\n", r);
+	ret = mgag200_modeset_init(mdev);
+	if (ret) {
+		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
 		goto err_modeset;
 	}
 
-	r = mgag200_cursor_init(mdev);
-	if (r)
-		dev_warn(&dev->pdev->dev,
-			"Could not initialize cursors. Not doing hardware cursors.\n");
+	ret = mgag200_cursor_init(mdev);
+	if (ret)
+		drm_err(dev, "Could not initialize cursors. Not doing hardware cursors.\n");
 
 	return 0;
 
@@ -189,8 +175,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	mgag200_mm_fini(mdev);
 err_mm:
 	dev->dev_private = NULL;
-
-	return r;
+	return ret;
 }
 
 void mgag200_driver_unload(struct drm_device *dev)
-- 
2.26.0

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

[PATCH 3/5] drm/mgag200: Remove several references to struct mga_device.dev

[Thomas Zimmermann]

Done in preparation of embedding the DRM device in struct mga_device.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 21 +++++++++++----------
 drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++++++++--------
 2 files changed, 20 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 3830d3f3c9fa2..010b309c01fc4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -66,25 +66,26 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
 /* 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(mdev->dev->pdev, 0);
-	mdev->mc.vram_window = pci_resource_len(mdev->dev->pdev, 0);
+	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(mdev->dev->dev, mdev->mc.vram_base, mdev->mc.vram_window,
-				"mgadrmfb_vram")) {
+	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(mdev->dev->pdev, 0, 0);
+	mem = pci_iomap(dev->pdev, 0, 0);
 	if (!mem)
 		return -ENOMEM;
 
 	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
 
-	pci_iounmap(mdev->dev->pdev, mem);
+	pci_iounmap(dev->pdev, mem);
 
 	return 0;
 }
@@ -116,11 +117,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	mdev->has_sdram = !(option & (1 << 14));
 
 	/* BAR 0 is the framebuffer, BAR 1 contains registers */
-	mdev->rmmio_base = pci_resource_start(mdev->dev->pdev, 1);
-	mdev->rmmio_size = pci_resource_len(mdev->dev->pdev, 1);
+	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
+	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
 
-	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
-				"mgadrmfb_mmio")) {
+	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;
 	}
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index fa91869c0db52..aaa73b29b04f0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1433,6 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
 /* CRTC setup */
 static void mga_crtc_init(struct mga_device *mdev)
 {
+	struct drm_device *dev = mdev->dev;
 	struct mga_crtc *mga_crtc;
 
 	mga_crtc = kzalloc(sizeof(struct mga_crtc) +
@@ -1442,7 +1443,7 @@ static void mga_crtc_init(struct mga_device *mdev)
 	if (mga_crtc == NULL)
 		return;
 
-	drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
+	drm_crtc_init(dev, &mga_crtc->base, &mga_crtc_funcs);
 
 	drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
 	mdev->mode_info.crtc = mga_crtc;
@@ -1617,30 +1618,30 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
 
 int mgag200_modeset_init(struct mga_device *mdev)
 {
+	struct drm_device *dev = mdev->dev;
 	struct drm_encoder *encoder = &mdev->encoder;
 	struct drm_connector *connector;
 	int ret;
 
 	mdev->mode_info.mode_config_initialized = true;
 
-	mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
-	mdev->dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
+	dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
+	dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
 
-	mdev->dev->mode_config.fb_base = mdev->mc.vram_base;
+	dev->mode_config.fb_base = mdev->mc.vram_base;
 
 	mga_crtc_init(mdev);
 
-	ret = drm_simple_encoder_init(mdev->dev, encoder,
-				      DRM_MODE_ENCODER_DAC);
+	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
 	if (ret) {
-		drm_err(mdev->dev,
+		drm_err(dev,
 			"drm_simple_encoder_init() failed, error %d\n",
 			ret);
 		return ret;
 	}
 	encoder->possible_crtcs = 0x1;
 
-	connector = mga_vga_init(mdev->dev);
+	connector = mga_vga_init(dev);
 	if (!connector) {
 		DRM_ERROR("mga_vga_init failed\n");
 		return -1;
-- 
2.26.0

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

[PATCH 4/5] drm/mgag200: Init and finalize devices in mgag200_device_{init, fini}()

[Thomas Zimmermann]

Device initialization is now done in mgag200_device_init(). Specifically,
the function allocates the DRM device and sets up the respective fields
in struct mga_device.

A call to mgag200_device_fini() finalizes struct mga_device.

The old function mgag200_driver_load() and mgag200_driver_unload() were
left over from the DRM driver's load callbacks and have now been removed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.c  | 27 ++++++++++----------
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++--
 drivers/gpu/drm/mgag200/mgag200_main.c | 34 ++++++++++++++++----------
 3 files changed, 37 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index c2f0e4b40b052..ad12c1b7c66cc 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -51,6 +51,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
 
 static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
+	struct mga_device *mdev;
 	struct drm_device *dev;
 	int ret;
 
@@ -60,31 +61,28 @@ 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);
-	if (IS_ERR(dev)) {
-		ret = PTR_ERR(dev);
+	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
+	if (!mdev) {
+		ret = -ENOMEM;
 		goto err_pci_disable_device;
 	}
 
-	dev->pdev = pdev;
-	pci_set_drvdata(pdev, dev);
-
-	ret = mgag200_driver_load(dev, ent->driver_data);
+	ret = mgag200_device_init(mdev, &driver, pdev, ent->driver_data);
 	if (ret)
-		goto err_drm_dev_put;
+		goto err_pci_disable_device;
+
+	dev = mdev->dev;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
-		goto err_mgag200_driver_unload;
+		goto err_mgag200_device_fini;
 
 	drm_fbdev_generic_setup(dev, 0);
 
 	return 0;
 
-err_mgag200_driver_unload:
-	mgag200_driver_unload(dev);
-err_drm_dev_put:
-	drm_dev_put(dev);
+err_mgag200_device_fini:
+	mgag200_device_fini(mdev);
 err_pci_disable_device:
 	pci_disable_device(pdev);
 	return ret;
@@ -93,9 +91,10 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 static void mga_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
+	struct mga_device *mdev = to_mga_device(dev);
 
 	drm_dev_unregister(dev);
-	mgag200_driver_unload(dev);
+	mgag200_device_fini(mdev);
 	drm_dev_put(dev);
 }
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 632bbb50465c9..1ce0386669ffa 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -200,8 +200,9 @@ int mgag200_modeset_init(struct mga_device *mdev);
 void mgag200_modeset_fini(struct mga_device *mdev);
 
 				/* mgag200_main.c */
-int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
-void mgag200_driver_unload(struct drm_device *dev);
+int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
+			struct pci_dev *pdev, unsigned long flags);
+void mgag200_device_fini(struct mga_device *mdev);
 
 				/* mgag200_i2c.c */
 struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 010b309c01fc4..070ff1f433df2 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -11,6 +11,7 @@
 #include <linux/pci.h>
 
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_drv.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 
 #include "mgag200_drv.h"
@@ -96,17 +97,21 @@ static int mga_vram_init(struct mga_device *mdev)
  */
 
 
-int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
+int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
+			struct pci_dev *pdev, 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 = drm_dev_alloc(drv, &pdev->dev);
+	if (IS_ERR(dev))
+		return PTR_ERR(dev);
 	dev->dev_private = (void *)mdev;
 	mdev->dev = dev;
 
+	dev->pdev = pdev;
+	pci_set_drvdata(pdev, dev);
+
 	mdev->flags = mgag200_flags_from_driver_data(flags);
 	mdev->type = mgag200_type_from_driver_data(flags);
 
@@ -123,12 +128,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	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;
+		ret = -ENOMEM;
+		goto err_drm_dev_put;
 	}
 
 	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
-	if (mdev->rmmio == NULL)
-		return -ENOMEM;
+	if (mdev->rmmio == NULL) {
+		ret = -ENOMEM;
+		goto err_drm_dev_put;
+	}
 
 	/* stash G200 SE model number for later use */
 	if (IS_G200_SE(mdev)) {
@@ -139,7 +147,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 
 	ret = mga_vram_init(mdev);
 	if (ret)
-		return ret;
+		goto err_drm_dev_put;
 
 	mdev->bpp_shifts[0] = 0;
 	mdev->bpp_shifts[1] = 1;
@@ -174,17 +182,17 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
 	drm_mode_config_cleanup(dev);
 	mgag200_cursor_fini(mdev);
 	mgag200_mm_fini(mdev);
+err_drm_dev_put:
+	drm_dev_put(dev);
 err_mm:
 	dev->dev_private = NULL;
 	return ret;
 }
 
-void mgag200_driver_unload(struct drm_device *dev)
+void mgag200_device_fini(struct mga_device *mdev)
 {
-	struct mga_device *mdev = to_mga_device(dev);
+	struct drm_device *dev = mdev->dev;
 
-	if (mdev == NULL)
-		return;
 	mgag200_modeset_fini(mdev);
 	drm_mode_config_cleanup(dev);
 	mgag200_cursor_fini(mdev);
-- 
2.26.0

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

[PATCH 5/5] drm/mgag200: Embed DRM device instance in struct mga_device

[Thomas Zimmermann]

As it is best practice now, the DRM device instance is now embedded in
struct mga_device. All references to dev_private have been removed.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_cursor.c |  6 +++---
 drivers/gpu/drm/mgag200/mgag200_drv.c    |  2 +-
 drivers/gpu/drm/mgag200/mgag200_drv.h    |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_main.c   | 16 ++++++----------
 drivers/gpu/drm/mgag200/mgag200_mode.c   |  4 ++--
 drivers/gpu/drm/mgag200/mgag200_ttm.c    |  4 ++--
 6 files changed, 16 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
index aebc9ce43d551..e3c717c0cffc0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
+++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
@@ -15,7 +15,7 @@ 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;
+	struct drm_device *dev = &mdev->base;
 	unsigned int i, row, col;
 	uint32_t colour_set[16];
 	uint32_t *next_space = &colour_set[0];
@@ -119,7 +119,7 @@ static void mgag200_cursor_set_base(struct mga_device *mdev, u64 address)
 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_device *dev = &mdev->base;
 	struct drm_gem_vram_object *gbo;
 	void *dst;
 	s64 off;
@@ -196,7 +196,7 @@ static void mgag200_move_cursor(struct mga_device *mdev, int x, int y)
 
 int mgag200_cursor_init(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	size_t ncursors = ARRAY_SIZE(mdev->cursor.gbo);
 	size_t size;
 	int ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index ad12c1b7c66cc..fc0775694c097 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -71,7 +71,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (ret)
 		goto err_pci_disable_device;
 
-	dev = mdev->dev;
+	dev = &mdev->base;
 
 	ret = drm_dev_register(dev, ent->driver_data);
 	if (ret)
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 1ce0386669ffa..fb2797d6ff690 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -96,7 +96,7 @@
 
 #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
 #define to_mga_connector(x) container_of(x, struct mga_connector, base)
-#define to_mga_device(x) ((struct mga_device *)(x)->dev_private)
+#define to_mga_device(x) container_of(x, struct mga_device, base)
 
 struct mga_crtc {
 	struct drm_crtc base;
@@ -153,7 +153,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;
diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 070ff1f433df2..ca3ed463c2d41 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -67,7 +67,7 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
 /* 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;
+	struct drm_device *dev = &mdev->base;
 	void __iomem *mem;
 
 	/* BAR 0 is VRAM */
@@ -100,14 +100,12 @@ static int mga_vram_init(struct mga_device *mdev)
 int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
 			struct pci_dev *pdev, unsigned long flags)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	int ret, option;
 
-	dev = drm_dev_alloc(drv, &pdev->dev);
-	if (IS_ERR(dev))
-		return PTR_ERR(dev);
-	dev->dev_private = (void *)mdev;
-	mdev->dev = dev;
+	ret = drm_dev_init(dev, drv, &pdev->dev);
+	if (ret)
+		return ret;
 
 	dev->pdev = pdev;
 	pci_set_drvdata(pdev, dev);
@@ -185,17 +183,15 @@ int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
 err_drm_dev_put:
 	drm_dev_put(dev);
 err_mm:
-	dev->dev_private = NULL;
 	return ret;
 }
 
 void mgag200_device_fini(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 
 	mgag200_modeset_fini(mdev);
 	drm_mode_config_cleanup(dev);
 	mgag200_cursor_fini(mdev);
 	mgag200_mm_fini(mdev);
-	dev->dev_private = NULL;
 }
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index aaa73b29b04f0..eb58742026adf 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -1433,7 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
 /* CRTC setup */
 static void mga_crtc_init(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	struct mga_crtc *mga_crtc;
 
 	mga_crtc = kzalloc(sizeof(struct mga_crtc) +
@@ -1618,7 +1618,7 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
 
 int mgag200_modeset_init(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 	struct drm_encoder *encoder = &mdev->encoder;
 	struct drm_connector *connector;
 	int ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index e89657630ea71..86a582490bb67 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -34,7 +34,7 @@ int mgag200_mm_init(struct mga_device *mdev)
 {
 	struct drm_vram_mm *vmm;
 	int ret;
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 
 	vmm = drm_vram_helper_alloc_mm(dev, pci_resource_start(dev->pdev, 0),
 				       mdev->mc.vram_size);
@@ -57,7 +57,7 @@ int mgag200_mm_init(struct mga_device *mdev)
 
 void mgag200_mm_fini(struct mga_device *mdev)
 {
-	struct drm_device *dev = mdev->dev;
+	struct drm_device *dev = &mdev->base;
 
 	mdev->vram_fb_available = 0;
 
-- 
2.26.0

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

Re: [PATCH 1/5] drm/mgag200: Convert struct drm_device to struct mga_device with macro

[Daniel Vetter]

On Tue, May 05, 2020 at 11:56:45AM +0200, Thomas Zimmermann wrote:
> Mgag200 used dev_private to look up struct mga_device for instances
> of struct drm_device. Use of dev_private is deprecated, so hide it in
> the macro to_mga_device().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/mgag200/mgag200_cursor.c |  4 ++--
>  drivers/gpu/drm/mgag200/mgag200_drv.c    |  2 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.h    |  1 +
>  drivers/gpu/drm/mgag200/mgag200_i2c.c    | 10 +++++-----
>  drivers/gpu/drm/mgag200/mgag200_main.c   |  4 ++--
>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 18 +++++++++---------
>  6 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> index d491edd317ff3..aebc9ce43d551 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> @@ -260,7 +260,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
>  			    uint32_t handle, uint32_t width, uint32_t height)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = (struct mga_device *)dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	struct drm_gem_object *obj;
>  	struct drm_gem_vram_object *gbo = NULL;
>  	int ret;
> @@ -307,7 +307,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
>  
>  int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  {
> -	struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  
>  	/* Our origin is at (64,64) */
>  	x += 64;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 3298b7ef18b03..c2f0e4b40b052 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -120,7 +120,7 @@ int mgag200_driver_dumb_create(struct drm_file *file,
>  			       struct drm_device *dev,
>  			       struct drm_mode_create_dumb *args)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	unsigned long pg_align;
>  
>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 9691252d6233f..632bbb50465c9 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -96,6 +96,7 @@
>  
>  #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
>  #define to_mga_connector(x) container_of(x, struct mga_connector, base)
> +#define to_mga_device(x) ((struct mga_device *)(x)->dev_private)
>  
>  struct mga_crtc {
>  	struct drm_crtc base;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
> index 9f4635916d322..09731e614e46d 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
> @@ -61,34 +61,34 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
>  static void mga_gpio_setsda(void *data, int state)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	mga_i2c_set(mdev, i2c->data, state);
>  }
>  
>  static void mga_gpio_setscl(void *data, int state)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	mga_i2c_set(mdev, i2c->clock, state);
>  }
>  
>  static int mga_gpio_getsda(void *data)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	return (mga_i2c_read_gpio(mdev) & i2c->data) ? 1 : 0;
>  }
>  
>  static int mga_gpio_getscl(void *data)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
>  }
>  
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	struct mga_i2c_chan *i2c;
>  	int ret;
>  	int data, clock;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index b680cf47cbb94..b705b7776d2fc 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -92,7 +92,7 @@ static int mga_vram_init(struct mga_device *mdev)
>  static int mgag200_device_init(struct drm_device *dev,
>  			       uint32_t flags)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	int ret, option;
>  
>  	mdev->flags = mgag200_flags_from_driver_data(flags);
> @@ -195,7 +195,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  void mgag200_driver_unload(struct drm_device *dev)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  
>  	if (mdev == NULL)
>  		return;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index d90e83959fca1..fa91869c0db52 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -28,7 +28,7 @@
>  static void mga_crtc_load_lut(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	struct drm_framebuffer *fb = crtc->primary->fb;
>  	u16 *r_ptr, *g_ptr, *b_ptr;
>  	int i;
> @@ -728,7 +728,7 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock)
>  
>  static void mga_g200wb_prepare(struct drm_crtc *crtc)
>  {
> -	struct mga_device *mdev = crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  	u8 tmp;
>  	int iter_max;
>  
> @@ -783,7 +783,7 @@ static void mga_g200wb_prepare(struct drm_crtc *crtc)
>  static void mga_g200wb_commit(struct drm_crtc *crtc)
>  {
>  	u8 tmp;
> -	struct mga_device *mdev = crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  
>  	/* 1- The first step is to ensure that the vrsten and hrsten are set */
>  	WREG8(MGAREG_CRTCEXT_INDEX, 1);
> @@ -833,7 +833,7 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
>   */
>  static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
>  {
> -	struct mga_device *mdev = crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  	u32 addr;
>  	int count;
>  	u8 crtcext0;
> @@ -902,7 +902,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  				int x, int y, struct drm_framebuffer *old_fb)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>  	int hdisplay, hsyncstart, hsyncend, htotal;
>  	int vdisplay, vsyncstart, vsyncend, vtotal;
> @@ -1263,7 +1263,7 @@ static int mga_resume(struct drm_crtc *crtc)
>  static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	u8 seq1 = 0, crtcext1 = 0;
>  
>  	switch (mode) {
> @@ -1317,7 +1317,7 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
>  static void mga_crtc_prepare(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	u8 tmp;
>  
>  	/*	mga_resume(crtc);*/
> @@ -1353,7 +1353,7 @@ static void mga_crtc_prepare(struct drm_crtc *crtc)
>  static void mga_crtc_commit(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
>  	u8 tmp;
>  
> @@ -1495,7 +1495,7 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
>  				 struct drm_display_mode *mode)
>  {
>  	struct drm_device *dev = connector->dev;
> -	struct mga_device *mdev = (struct mga_device*)dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	int bpp = 32;
>  
>  	if (IS_G200_SE(mdev)) {
> -- 
> 2.26.0
> 

-- 
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 2/5] drm/mgag200: Integrate init function into load function

[Daniel Vetter]

On Tue, May 05, 2020 at 11:56:46AM +0200, Thomas Zimmermann wrote:
> Done to simplify initialization code before embedding the DRM device
> instance in struct mga_device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 67 ++++++++++----------------
>  1 file changed, 26 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index b705b7776d2fc..3830d3f3c9fa2 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -89,12 +89,23 @@ static int mga_vram_init(struct mga_device *mdev)
>  	return 0;
>  }
>  
> -static int mgag200_device_init(struct drm_device *dev,
> -			       uint32_t flags)
> +/*
> + * Functions here will be called by the core once it's bound the driver to
> + * a PCI device
> + */
> +
> +
> +int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  {
> -	struct mga_device *mdev = to_mga_device(dev);
> +	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);
>  
> @@ -110,7 +121,7 @@ static int mgag200_device_init(struct drm_device *dev,
>  
>  	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
>  				"mgadrmfb_mmio")) {
> -		DRM_ERROR("can't reserve mmio registers\n");
> +		drm_err(dev, "can't reserve mmio registers\n");
>  		return -ENOMEM;
>  	}
>  
> @@ -121,8 +132,8 @@ static int mgag200_device_init(struct drm_device *dev,
>  	/* stash G200 SE model number for later use */
>  	if (IS_G200_SE(mdev)) {
>  		mdev->unique_rev_id = RREG32(0x1e24);
> -		DRM_DEBUG("G200 SE unique revision id is 0x%x\n",
> -			  mdev->unique_rev_id);
> +		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
> +			mdev->unique_rev_id);
>  	}
>  
>  	ret = mga_vram_init(mdev);
> @@ -133,33 +144,9 @@ static int mgag200_device_init(struct drm_device *dev,
>  	mdev->bpp_shifts[1] = 1;
>  	mdev->bpp_shifts[2] = 0;
>  	mdev->bpp_shifts[3] = 2;
> -	return 0;
> -}
>  
> -/*
> - * Functions here will be called by the core once it's bound the driver to
> - * a PCI device
> - */
> -
> -
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct mga_device *mdev;
> -	int r;
> -
> -	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
> -	if (mdev == NULL)
> -		return -ENOMEM;
> -	dev->dev_private = (void *)mdev;
> -	mdev->dev = dev;
> -
> -	r = mgag200_device_init(dev, flags);
> -	if (r) {
> -		dev_err(&dev->pdev->dev, "Fatal error during GPU init: %d\n", r);
> -		return r;
> -	}
> -	r = mgag200_mm_init(mdev);
> -	if (r)
> +	ret = mgag200_mm_init(mdev);
> +	if (ret)
>  		goto err_mm;
>  
>  	drm_mode_config_init(dev);
> @@ -170,16 +157,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  		dev->mode_config.preferred_depth = 32;
>  	dev->mode_config.prefer_shadow = 1;
>  
> -	r = mgag200_modeset_init(mdev);
> -	if (r) {
> -		dev_err(&dev->pdev->dev, "Fatal error during modeset init: %d\n", r);
> +	ret = mgag200_modeset_init(mdev);
> +	if (ret) {
> +		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
>  		goto err_modeset;
>  	}
>  
> -	r = mgag200_cursor_init(mdev);
> -	if (r)
> -		dev_warn(&dev->pdev->dev,
> -			"Could not initialize cursors. Not doing hardware cursors.\n");
> +	ret = mgag200_cursor_init(mdev);
> +	if (ret)
> +		drm_err(dev, "Could not initialize cursors. Not doing hardware cursors.\n");
>  
>  	return 0;
>  
> @@ -189,8 +175,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	mgag200_mm_fini(mdev);
>  err_mm:
>  	dev->dev_private = NULL;
> -
> -	return r;
> +	return ret;
>  }
>  
>  void mgag200_driver_unload(struct drm_device *dev)
> -- 
> 2.26.0
> 

-- 
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 3/5] drm/mgag200: Remove several references to struct mga_device.dev

[Daniel Vetter]

On Tue, May 05, 2020 at 11:56:47AM +0200, Thomas Zimmermann wrote:
> Done in preparation of embedding the DRM device in struct mga_device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Not exactly sure what the goal is, since mga_vga_init still uses
drm_device and not the mdev struct. *shrug*

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>


> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 21 +++++++++++----------
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++++++++--------
>  2 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 3830d3f3c9fa2..010b309c01fc4 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -66,25 +66,26 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
>  /* 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(mdev->dev->pdev, 0);
> -	mdev->mc.vram_window = pci_resource_len(mdev->dev->pdev, 0);
> +	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(mdev->dev->dev, mdev->mc.vram_base, mdev->mc.vram_window,
> -				"mgadrmfb_vram")) {
> +	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(mdev->dev->pdev, 0, 0);
> +	mem = pci_iomap(dev->pdev, 0, 0);
>  	if (!mem)
>  		return -ENOMEM;
>  
>  	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
>  
> -	pci_iounmap(mdev->dev->pdev, mem);
> +	pci_iounmap(dev->pdev, mem);
>  
>  	return 0;
>  }
> @@ -116,11 +117,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	mdev->has_sdram = !(option & (1 << 14));
>  
>  	/* BAR 0 is the framebuffer, BAR 1 contains registers */
> -	mdev->rmmio_base = pci_resource_start(mdev->dev->pdev, 1);
> -	mdev->rmmio_size = pci_resource_len(mdev->dev->pdev, 1);
> +	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
> +	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
>  
> -	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
> -				"mgadrmfb_mmio")) {
> +	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;
>  	}
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index fa91869c0db52..aaa73b29b04f0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1433,6 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
>  /* CRTC setup */
>  static void mga_crtc_init(struct mga_device *mdev)
>  {
> +	struct drm_device *dev = mdev->dev;
>  	struct mga_crtc *mga_crtc;
>  
>  	mga_crtc = kzalloc(sizeof(struct mga_crtc) +
> @@ -1442,7 +1443,7 @@ static void mga_crtc_init(struct mga_device *mdev)
>  	if (mga_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
> +	drm_crtc_init(dev, &mga_crtc->base, &mga_crtc_funcs);
>  
>  	drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
>  	mdev->mode_info.crtc = mga_crtc;
> @@ -1617,30 +1618,30 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
>  
>  int mgag200_modeset_init(struct mga_device *mdev)
>  {
> +	struct drm_device *dev = mdev->dev;
>  	struct drm_encoder *encoder = &mdev->encoder;
>  	struct drm_connector *connector;
>  	int ret;
>  
>  	mdev->mode_info.mode_config_initialized = true;
>  
> -	mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
> -	mdev->dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
> +	dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
> +	dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
>  
> -	mdev->dev->mode_config.fb_base = mdev->mc.vram_base;
> +	dev->mode_config.fb_base = mdev->mc.vram_base;
>  
>  	mga_crtc_init(mdev);
>  
> -	ret = drm_simple_encoder_init(mdev->dev, encoder,
> -				      DRM_MODE_ENCODER_DAC);
> +	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
>  	if (ret) {
> -		drm_err(mdev->dev,
> +		drm_err(dev,
>  			"drm_simple_encoder_init() failed, error %d\n",
>  			ret);
>  		return ret;
>  	}
>  	encoder->possible_crtcs = 0x1;
>  
> -	connector = mga_vga_init(mdev->dev);
> +	connector = mga_vga_init(dev);
>  	if (!connector) {
>  		DRM_ERROR("mga_vga_init failed\n");
>  		return -1;
> -- 
> 2.26.0
> 

-- 
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 4/5] drm/mgag200: Init and finalize devices in mgag200_device_{init,fini}()

[Daniel Vetter]

On Tue, May 05, 2020 at 11:56:48AM +0200, Thomas Zimmermann wrote:
> Device initialization is now done in mgag200_device_init(). Specifically,
> the function allocates the DRM device and sets up the respective fields
> in struct mga_device.
> 
> A call to mgag200_device_fini() finalizes struct mga_device.
> 
> The old function mgag200_driver_load() and mgag200_driver_unload() were
> left over from the DRM driver's load callbacks and have now been removed.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 27 ++++++++++----------
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++--
>  drivers/gpu/drm/mgag200/mgag200_main.c | 34 ++++++++++++++++----------
>  3 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index c2f0e4b40b052..ad12c1b7c66cc 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -51,6 +51,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>  
>  static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
> +	struct mga_device *mdev;
>  	struct drm_device *dev;
>  	int ret;
>  
> @@ -60,31 +61,28 @@ 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);
> -	if (IS_ERR(dev)) {
> -		ret = PTR_ERR(dev);
> +	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
> +	if (!mdev) {
> +		ret = -ENOMEM;
>  		goto err_pci_disable_device;
>  	}
>  
> -	dev->pdev = pdev;
> -	pci_set_drvdata(pdev, dev);
> -
> -	ret = mgag200_driver_load(dev, ent->driver_data);
> +	ret = mgag200_device_init(mdev, &driver, pdev, ent->driver_data);
>  	if (ret)
> -		goto err_drm_dev_put;
> +		goto err_pci_disable_device;
> +
> +	dev = mdev->dev;
>  
>  	ret = drm_dev_register(dev, ent->driver_data);
>  	if (ret)
> -		goto err_mgag200_driver_unload;
> +		goto err_mgag200_device_fini;
>  
>  	drm_fbdev_generic_setup(dev, 0);
>  
>  	return 0;
>  
> -err_mgag200_driver_unload:
> -	mgag200_driver_unload(dev);
> -err_drm_dev_put:
> -	drm_dev_put(dev);

Moving the drm_dev_put away from here will make the conversion to
devm_drm_dev_alloc a bit more tricky I think. I'm not sure whether this is
actually better than just directly going to devm_drm_dev_alloc and then
cleaning up the fallout, that's at least what I've done in the conversions
I've attempted thus far.

Either way, this looks correct.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +err_mgag200_device_fini:
> +	mgag200_device_fini(mdev);
>  err_pci_disable_device:
>  	pci_disable_device(pdev);
>  	return ret;
> @@ -93,9 +91,10 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  static void mga_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct mga_device *mdev = to_mga_device(dev);
>  
>  	drm_dev_unregister(dev);
> -	mgag200_driver_unload(dev);
> +	mgag200_device_fini(mdev);
>  	drm_dev_put(dev);
>  }
>  
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 632bbb50465c9..1ce0386669ffa 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -200,8 +200,9 @@ int mgag200_modeset_init(struct mga_device *mdev);
>  void mgag200_modeset_fini(struct mga_device *mdev);
>  
>  				/* mgag200_main.c */
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
> -void mgag200_driver_unload(struct drm_device *dev);
> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
> +			struct pci_dev *pdev, unsigned long flags);
> +void mgag200_device_fini(struct mga_device *mdev);
>  
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 010b309c01fc4..070ff1f433df2 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -11,6 +11,7 @@
>  #include <linux/pci.h>
>  
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
>  #include "mgag200_drv.h"
> @@ -96,17 +97,21 @@ static int mga_vram_init(struct mga_device *mdev)
>   */
>  
>  
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
> +			struct pci_dev *pdev, 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 = drm_dev_alloc(drv, &pdev->dev);
> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
>  	dev->dev_private = (void *)mdev;
>  	mdev->dev = dev;
>  
> +	dev->pdev = pdev;
> +	pci_set_drvdata(pdev, dev);
> +
>  	mdev->flags = mgag200_flags_from_driver_data(flags);
>  	mdev->type = mgag200_type_from_driver_data(flags);
>  
> @@ -123,12 +128,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	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;
> +		ret = -ENOMEM;
> +		goto err_drm_dev_put;
>  	}
>  
>  	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
> -	if (mdev->rmmio == NULL)
> -		return -ENOMEM;
> +	if (mdev->rmmio == NULL) {
> +		ret = -ENOMEM;
> +		goto err_drm_dev_put;
> +	}
>  
>  	/* stash G200 SE model number for later use */
>  	if (IS_G200_SE(mdev)) {
> @@ -139,7 +147,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	ret = mga_vram_init(mdev);
>  	if (ret)
> -		return ret;
> +		goto err_drm_dev_put;
>  
>  	mdev->bpp_shifts[0] = 0;
>  	mdev->bpp_shifts[1] = 1;
> @@ -174,17 +182,17 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	drm_mode_config_cleanup(dev);
>  	mgag200_cursor_fini(mdev);
>  	mgag200_mm_fini(mdev);
> +err_drm_dev_put:
> +	drm_dev_put(dev);
>  err_mm:
>  	dev->dev_private = NULL;
>  	return ret;
>  }
>  
> -void mgag200_driver_unload(struct drm_device *dev)
> +void mgag200_device_fini(struct mga_device *mdev)
>  {
> -	struct mga_device *mdev = to_mga_device(dev);
> +	struct drm_device *dev = mdev->dev;
>  
> -	if (mdev == NULL)
> -		return;
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_cursor_fini(mdev);
> -- 
> 2.26.0
> 

-- 
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 5/5] drm/mgag200: Embed DRM device instance in struct mga_device

[Daniel Vetter]

On Tue, May 05, 2020 at 11:56:49AM +0200, Thomas Zimmermann wrote:
> As it is best practice now, the DRM device instance is now embedded in
> struct mga_device. All references to dev_private have been removed.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_cursor.c |  6 +++---
>  drivers/gpu/drm/mgag200/mgag200_drv.c    |  2 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.h    |  4 ++--
>  drivers/gpu/drm/mgag200/mgag200_main.c   | 16 ++++++----------
>  drivers/gpu/drm/mgag200/mgag200_mode.c   |  4 ++--
>  drivers/gpu/drm/mgag200/mgag200_ttm.c    |  4 ++--
>  6 files changed, 16 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> index aebc9ce43d551..e3c717c0cffc0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> @@ -15,7 +15,7 @@ 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;
> +	struct drm_device *dev = &mdev->base;
>  	unsigned int i, row, col;
>  	uint32_t colour_set[16];
>  	uint32_t *next_space = &colour_set[0];
> @@ -119,7 +119,7 @@ static void mgag200_cursor_set_base(struct mga_device *mdev, u64 address)
>  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_device *dev = &mdev->base;
>  	struct drm_gem_vram_object *gbo;
>  	void *dst;
>  	s64 off;
> @@ -196,7 +196,7 @@ static void mgag200_move_cursor(struct mga_device *mdev, int x, int y)
>  
>  int mgag200_cursor_init(struct mga_device *mdev)
>  {
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  	size_t ncursors = ARRAY_SIZE(mdev->cursor.gbo);
>  	size_t size;
>  	int ret;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index ad12c1b7c66cc..fc0775694c097 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -71,7 +71,7 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	if (ret)
>  		goto err_pci_disable_device;
>  
> -	dev = mdev->dev;
> +	dev = &mdev->base;
>  
>  	ret = drm_dev_register(dev, ent->driver_data);
>  	if (ret)
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 1ce0386669ffa..fb2797d6ff690 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -96,7 +96,7 @@
>  
>  #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
>  #define to_mga_connector(x) container_of(x, struct mga_connector, base)
> -#define to_mga_device(x) ((struct mga_device *)(x)->dev_private)
> +#define to_mga_device(x) container_of(x, struct mga_device, base)
>  
>  struct mga_crtc {
>  	struct drm_crtc base;
> @@ -153,7 +153,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;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 070ff1f433df2..ca3ed463c2d41 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -67,7 +67,7 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
>  /* 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;
> +	struct drm_device *dev = &mdev->base;
>  	void __iomem *mem;
>  
>  	/* BAR 0 is VRAM */
> @@ -100,14 +100,12 @@ static int mga_vram_init(struct mga_device *mdev)
>  int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
>  			struct pci_dev *pdev, unsigned long flags)
>  {
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  	int ret, option;
>  
> -	dev = drm_dev_alloc(drv, &pdev->dev);
> -	if (IS_ERR(dev))
> -		return PTR_ERR(dev);
> -	dev->dev_private = (void *)mdev;
> -	mdev->dev = dev;
> +	ret = drm_dev_init(dev, drv, &pdev->dev);

You devm_kzalloc this structure in the previous patch. That's kinda less
correct than what we have now ... I think this patch and the previous one
are needless detour, and straight going to embedding and
devm_drm_dev_alloc like I've done e.g. in

commit cd8294540776f7986b7cf658a3579d576853610c
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Apr 15 09:40:30 2020 +0200

    drm/aspeed: Use devm_drm_dev_alloc

Then clean up all the fallout later on (i.e. switch over to mga_device and
away from drm_device, heck even the to_mag_device pointer upcasting you
can all do afterwards).

The intermediate stages all have tricky rules for what exactly can and
can't be done, for no real gain, so here a massively split patch series
imo just increases the risks you break something somewhere.

Cheers, Daniel

> +	if (ret)
> +		return ret;
>  
>  	dev->pdev = pdev;
>  	pci_set_drvdata(pdev, dev);
> @@ -185,17 +183,15 @@ int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
>  err_drm_dev_put:
>  	drm_dev_put(dev);
>  err_mm:
> -	dev->dev_private = NULL;
>  	return ret;
>  }
>  
>  void mgag200_device_fini(struct mga_device *mdev)
>  {
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_cursor_fini(mdev);
>  	mgag200_mm_fini(mdev);
> -	dev->dev_private = NULL;
>  }
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index aaa73b29b04f0..eb58742026adf 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1433,7 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
>  /* CRTC setup */
>  static void mga_crtc_init(struct mga_device *mdev)
>  {
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  	struct mga_crtc *mga_crtc;
>  
>  	mga_crtc = kzalloc(sizeof(struct mga_crtc) +
> @@ -1618,7 +1618,7 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
>  
>  int mgag200_modeset_init(struct mga_device *mdev)
>  {
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  	struct drm_encoder *encoder = &mdev->encoder;
>  	struct drm_connector *connector;
>  	int ret;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
> index e89657630ea71..86a582490bb67 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
> @@ -34,7 +34,7 @@ int mgag200_mm_init(struct mga_device *mdev)
>  {
>  	struct drm_vram_mm *vmm;
>  	int ret;
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  
>  	vmm = drm_vram_helper_alloc_mm(dev, pci_resource_start(dev->pdev, 0),
>  				       mdev->mc.vram_size);
> @@ -57,7 +57,7 @@ int mgag200_mm_init(struct mga_device *mdev)
>  
>  void mgag200_mm_fini(struct mga_device *mdev)
>  {
> -	struct drm_device *dev = mdev->dev;
> +	struct drm_device *dev = &mdev->base;
>  
>  	mdev->vram_fb_available = 0;
>  
> -- 
> 2.26.0
> 

-- 
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 0/5] drm/mgag200: Embed DRM device in struct mga_device

[Sam Ravnborg]

Hi Thomas.

On Tue, May 05, 2020 at 11:56:44AM +0200, Thomas Zimmermann wrote:
> After receiving reviews on the conversion of mgag200 to atomic mode
> setting, I thought it would make sense to embed the DRM device in
> struct mga_device first. Several comments in the atomic-conversion
> reviews refer to that.
I would have preferred that this was on top of at least some of the
existing patches, as they are tested by at least one other person
and reveiwed too (at least some of them, but maybe only by me).

Anyway, now you did it like this and Daniel has reviewed so let's
move on from here.

	Sam
> 
> Patches 1 to 3 do some cleanups and preparation work. Patch 4 changes
> the the init functions to allocate struct mga_device before struct
> drm_device. Patch 5 does the conversion.
> 
> I did not switch over struct mga_device to the new managed release
> code. I found that this justifies another round of cleanup patches,
> which I did not want to put into this patchset.
> 
> The patches were tested on mgag200 hardware.
> 
> Thomas Zimmermann (5):
>   drm/mgag200: Convert struct drm_device to struct mga_device with macro
>   drm/mgag200: Integrate init function into load function
>   drm/mgag200: Remove several references to struct mga_device.dev
>   drm/mgag200: Init and finalize devices in mgag200_device_{init,fini}()
>   drm/mgag200: Embed DRM device instance in struct mga_device
> 
>  drivers/gpu/drm/mgag200/mgag200_cursor.c |  10 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.c    |  29 +++---
>  drivers/gpu/drm/mgag200/mgag200_drv.h    |   8 +-
>  drivers/gpu/drm/mgag200/mgag200_i2c.c    |  10 +-
>  drivers/gpu/drm/mgag200/mgag200_main.c   | 114 +++++++++++------------
>  drivers/gpu/drm/mgag200/mgag200_mode.c   |  35 +++----
>  drivers/gpu/drm/mgag200/mgag200_ttm.c    |   4 +-
>  7 files changed, 101 insertions(+), 109 deletions(-)
> 
> --
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/mgag200: Convert struct drm_device to struct mga_device with macro

[Sam Ravnborg]

Hi Thomas.

On Tue, May 05, 2020 at 11:56:45AM +0200, Thomas Zimmermann wrote:
> Mgag200 used dev_private to look up struct mga_device for instances
> of struct drm_device. Use of dev_private is deprecated, so hide it in
> the macro to_mga_device().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

But one nit below.

	Sam

> ---
>  drivers/gpu/drm/mgag200/mgag200_cursor.c |  4 ++--
>  drivers/gpu/drm/mgag200/mgag200_drv.c    |  2 +-
>  drivers/gpu/drm/mgag200/mgag200_drv.h    |  1 +
>  drivers/gpu/drm/mgag200/mgag200_i2c.c    | 10 +++++-----
>  drivers/gpu/drm/mgag200/mgag200_main.c   |  4 ++--
>  drivers/gpu/drm/mgag200/mgag200_mode.c   | 18 +++++++++---------
>  6 files changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> index d491edd317ff3..aebc9ce43d551 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> @@ -260,7 +260,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
>  			    uint32_t handle, uint32_t width, uint32_t height)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = (struct mga_device *)dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	struct drm_gem_object *obj;
>  	struct drm_gem_vram_object *gbo = NULL;
>  	int ret;
> @@ -307,7 +307,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
>  
>  int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
>  {
> -	struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  
>  	/* Our origin is at (64,64) */
>  	x += 64;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 3298b7ef18b03..c2f0e4b40b052 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -120,7 +120,7 @@ int mgag200_driver_dumb_create(struct drm_file *file,
>  			       struct drm_device *dev,
>  			       struct drm_mode_create_dumb *args)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	unsigned long pg_align;
>  
>  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 9691252d6233f..632bbb50465c9 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -96,6 +96,7 @@
>  
>  #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
>  #define to_mga_connector(x) container_of(x, struct mga_connector, base)
> +#define to_mga_device(x) ((struct mga_device *)(x)->dev_private)
A inline function would have been better, as this provide no typecheck.
But we assume that it is always a drm_device *.
We would most likely catch it as no one else has a ->dev_prvate.

>  
>  struct mga_crtc {
>  	struct drm_crtc base;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
> index 9f4635916d322..09731e614e46d 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
> @@ -61,34 +61,34 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
>  static void mga_gpio_setsda(void *data, int state)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	mga_i2c_set(mdev, i2c->data, state);
>  }
>  
>  static void mga_gpio_setscl(void *data, int state)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	mga_i2c_set(mdev, i2c->clock, state);
>  }
>  
>  static int mga_gpio_getsda(void *data)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	return (mga_i2c_read_gpio(mdev) & i2c->data) ? 1 : 0;
>  }
>  
>  static int mga_gpio_getscl(void *data)
>  {
>  	struct mga_i2c_chan *i2c = data;
> -	struct mga_device *mdev = i2c->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(i2c->dev);
>  	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
>  }
>  
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	struct mga_i2c_chan *i2c;
>  	int ret;
>  	int data, clock;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index b680cf47cbb94..b705b7776d2fc 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -92,7 +92,7 @@ static int mga_vram_init(struct mga_device *mdev)
>  static int mgag200_device_init(struct drm_device *dev,
>  			       uint32_t flags)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	int ret, option;
>  
>  	mdev->flags = mgag200_flags_from_driver_data(flags);
> @@ -195,7 +195,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  void mgag200_driver_unload(struct drm_device *dev)
>  {
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  
>  	if (mdev == NULL)
>  		return;
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index d90e83959fca1..fa91869c0db52 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -28,7 +28,7 @@
>  static void mga_crtc_load_lut(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	struct drm_framebuffer *fb = crtc->primary->fb;
>  	u16 *r_ptr, *g_ptr, *b_ptr;
>  	int i;
> @@ -728,7 +728,7 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock)
>  
>  static void mga_g200wb_prepare(struct drm_crtc *crtc)
>  {
> -	struct mga_device *mdev = crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  	u8 tmp;
>  	int iter_max;
>  
> @@ -783,7 +783,7 @@ static void mga_g200wb_prepare(struct drm_crtc *crtc)
>  static void mga_g200wb_commit(struct drm_crtc *crtc)
>  {
>  	u8 tmp;
> -	struct mga_device *mdev = crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  
>  	/* 1- The first step is to ensure that the vrsten and hrsten are set */
>  	WREG8(MGAREG_CRTCEXT_INDEX, 1);
> @@ -833,7 +833,7 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
>   */
>  static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
>  {
> -	struct mga_device *mdev = crtc->dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(crtc->dev);
>  	u32 addr;
>  	int count;
>  	u8 crtcext0;
> @@ -902,7 +902,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
>  				int x, int y, struct drm_framebuffer *old_fb)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	const struct drm_framebuffer *fb = crtc->primary->fb;
>  	int hdisplay, hsyncstart, hsyncend, htotal;
>  	int vdisplay, vsyncstart, vsyncend, vtotal;
> @@ -1263,7 +1263,7 @@ static int mga_resume(struct drm_crtc *crtc)
>  static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	u8 seq1 = 0, crtcext1 = 0;
>  
>  	switch (mode) {
> @@ -1317,7 +1317,7 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
>  static void mga_crtc_prepare(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	u8 tmp;
>  
>  	/*	mga_resume(crtc);*/
> @@ -1353,7 +1353,7 @@ static void mga_crtc_prepare(struct drm_crtc *crtc)
>  static void mga_crtc_commit(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> -	struct mga_device *mdev = dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
>  	u8 tmp;
>  
> @@ -1495,7 +1495,7 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
>  				 struct drm_display_mode *mode)
>  {
>  	struct drm_device *dev = connector->dev;
> -	struct mga_device *mdev = (struct mga_device*)dev->dev_private;
> +	struct mga_device *mdev = to_mga_device(dev);
>  	int bpp = 32;
>  
>  	if (IS_G200_SE(mdev)) {
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/5] drm/mgag200: Integrate init function into load function

[Sam Ravnborg]

On Tue, May 05, 2020 at 11:56:46AM +0200, Thomas Zimmermann wrote:
> Done to simplify initialization code before embedding the DRM device
> instance in struct mga_device.
And replace DRM_ERROR with drm_err
And replace r with ret.

I could not follow all the code re-shuffeling, but I expect it to be
fine.
Acked-by: Sam Ravnborg <sam@ravnborg.org>

> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 67 ++++++++++----------------
>  1 file changed, 26 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index b705b7776d2fc..3830d3f3c9fa2 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -89,12 +89,23 @@ static int mga_vram_init(struct mga_device *mdev)
>  	return 0;
>  }
>  
> -static int mgag200_device_init(struct drm_device *dev,
> -			       uint32_t flags)
> +/*
> + * Functions here will be called by the core once it's bound the driver to
> + * a PCI device
> + */
> +
> +
> +int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  {
> -	struct mga_device *mdev = to_mga_device(dev);
> +	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);
>  
> @@ -110,7 +121,7 @@ static int mgag200_device_init(struct drm_device *dev,
>  
>  	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
>  				"mgadrmfb_mmio")) {
> -		DRM_ERROR("can't reserve mmio registers\n");
> +		drm_err(dev, "can't reserve mmio registers\n");
>  		return -ENOMEM;
>  	}
>  
> @@ -121,8 +132,8 @@ static int mgag200_device_init(struct drm_device *dev,
>  	/* stash G200 SE model number for later use */
>  	if (IS_G200_SE(mdev)) {
>  		mdev->unique_rev_id = RREG32(0x1e24);
> -		DRM_DEBUG("G200 SE unique revision id is 0x%x\n",
> -			  mdev->unique_rev_id);
> +		drm_dbg(dev, "G200 SE unique revision id is 0x%x\n",
> +			mdev->unique_rev_id);
>  	}
>  
>  	ret = mga_vram_init(mdev);
> @@ -133,33 +144,9 @@ static int mgag200_device_init(struct drm_device *dev,
>  	mdev->bpp_shifts[1] = 1;
>  	mdev->bpp_shifts[2] = 0;
>  	mdev->bpp_shifts[3] = 2;
> -	return 0;
> -}
>  
> -/*
> - * Functions here will be called by the core once it's bound the driver to
> - * a PCI device
> - */
> -
> -
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> -{
> -	struct mga_device *mdev;
> -	int r;
> -
> -	mdev = devm_kzalloc(dev->dev, sizeof(struct mga_device), GFP_KERNEL);
> -	if (mdev == NULL)
> -		return -ENOMEM;
> -	dev->dev_private = (void *)mdev;
> -	mdev->dev = dev;
> -
> -	r = mgag200_device_init(dev, flags);
> -	if (r) {
> -		dev_err(&dev->pdev->dev, "Fatal error during GPU init: %d\n", r);
> -		return r;
> -	}
> -	r = mgag200_mm_init(mdev);
> -	if (r)
> +	ret = mgag200_mm_init(mdev);
> +	if (ret)
>  		goto err_mm;
>  
>  	drm_mode_config_init(dev);
> @@ -170,16 +157,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  		dev->mode_config.preferred_depth = 32;
>  	dev->mode_config.prefer_shadow = 1;
>  
> -	r = mgag200_modeset_init(mdev);
> -	if (r) {
> -		dev_err(&dev->pdev->dev, "Fatal error during modeset init: %d\n", r);
> +	ret = mgag200_modeset_init(mdev);
> +	if (ret) {
> +		drm_err(dev, "Fatal error during modeset init: %d\n", ret);
>  		goto err_modeset;
>  	}
>  
> -	r = mgag200_cursor_init(mdev);
> -	if (r)
> -		dev_warn(&dev->pdev->dev,
> -			"Could not initialize cursors. Not doing hardware cursors.\n");
> +	ret = mgag200_cursor_init(mdev);
> +	if (ret)
> +		drm_err(dev, "Could not initialize cursors. Not doing hardware cursors.\n");
>  
>  	return 0;
>  
> @@ -189,8 +175,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	mgag200_mm_fini(mdev);
>  err_mm:
>  	dev->dev_private = NULL;
> -
> -	return r;
> +	return ret;
>  }
>  
>  void mgag200_driver_unload(struct drm_device *dev)
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/mgag200: Remove several references to struct mga_device.dev

[Sam Ravnborg]

Hi Thomas.

On Tue, May 05, 2020 at 11:56:47AM +0200, Thomas Zimmermann wrote:
> Done in preparation of embedding the DRM device in struct mga_device.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Trivial, one nit you can fix while applying, or ignore.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

> ---
>  drivers/gpu/drm/mgag200/mgag200_main.c | 21 +++++++++++----------
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++++++++--------
>  2 files changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 3830d3f3c9fa2..010b309c01fc4 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -66,25 +66,26 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
>  /* 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(mdev->dev->pdev, 0);
> -	mdev->mc.vram_window = pci_resource_len(mdev->dev->pdev, 0);
> +	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(mdev->dev->dev, mdev->mc.vram_base, mdev->mc.vram_window,
> -				"mgadrmfb_vram")) {
> +	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(mdev->dev->pdev, 0, 0);
> +	mem = pci_iomap(dev->pdev, 0, 0);
>  	if (!mem)
>  		return -ENOMEM;
>  
>  	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
>  
> -	pci_iounmap(mdev->dev->pdev, mem);
> +	pci_iounmap(dev->pdev, mem);
>  
>  	return 0;
>  }
> @@ -116,11 +117,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	mdev->has_sdram = !(option & (1 << 14));
>  
>  	/* BAR 0 is the framebuffer, BAR 1 contains registers */
> -	mdev->rmmio_base = pci_resource_start(mdev->dev->pdev, 1);
> -	mdev->rmmio_size = pci_resource_len(mdev->dev->pdev, 1);
> +	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
> +	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
>  
> -	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
> -				"mgadrmfb_mmio")) {
> +	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;
>  	}
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index fa91869c0db52..aaa73b29b04f0 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -1433,6 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
>  /* CRTC setup */
>  static void mga_crtc_init(struct mga_device *mdev)
>  {
> +	struct drm_device *dev = mdev->dev;
>  	struct mga_crtc *mga_crtc;
>  
>  	mga_crtc = kzalloc(sizeof(struct mga_crtc) +
> @@ -1442,7 +1443,7 @@ static void mga_crtc_init(struct mga_device *mdev)
>  	if (mga_crtc == NULL)
>  		return;
>  
> -	drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
> +	drm_crtc_init(dev, &mga_crtc->base, &mga_crtc_funcs);
>  
>  	drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
>  	mdev->mode_info.crtc = mga_crtc;
> @@ -1617,30 +1618,30 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
>  
>  int mgag200_modeset_init(struct mga_device *mdev)
>  {
> +	struct drm_device *dev = mdev->dev;
>  	struct drm_encoder *encoder = &mdev->encoder;
>  	struct drm_connector *connector;
>  	int ret;
>  
>  	mdev->mode_info.mode_config_initialized = true;
>  
> -	mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
> -	mdev->dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
> +	dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
> +	dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
>  
> -	mdev->dev->mode_config.fb_base = mdev->mc.vram_base;
> +	dev->mode_config.fb_base = mdev->mc.vram_base;
>  
>  	mga_crtc_init(mdev);
>  
> -	ret = drm_simple_encoder_init(mdev->dev, encoder,
> -				      DRM_MODE_ENCODER_DAC);
> +	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
>  	if (ret) {
> -		drm_err(mdev->dev,
> +		drm_err(dev,
>  			"drm_simple_encoder_init() failed, error %d\n",
Join with line before.

>  			ret);
>  		return ret;
>  	}
>  	encoder->possible_crtcs = 0x1;
>  
> -	connector = mga_vga_init(mdev->dev);
> +	connector = mga_vga_init(dev);
>  	if (!connector) {
>  		DRM_ERROR("mga_vga_init failed\n");
>  		return -1;
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/mgag200: Init and finalize devices in mgag200_device_{init,fini}()

[Sam Ravnborg]

Hi Thomas.

On Tue, May 05, 2020 at 11:56:48AM +0200, Thomas Zimmermann wrote:
> Device initialization is now done in mgag200_device_init(). Specifically,
> the function allocates the DRM device and sets up the respective fields
> in struct mga_device.
> 
> A call to mgag200_device_fini() finalizes struct mga_device.
> 
> The old function mgag200_driver_load() and mgag200_driver_unload() were
> left over from the DRM driver's load callbacks and have now been removed.

Not too big fan of this patch, due to the changes allocation.
I would prefer if you merged patch 4+5 and then take it from there.

You have patch 1+2+3 and they are now reviewed so why not push them
and work on top of them.
And then you could also push the patch that removes the cursor stuff
so we do not need to look at that anymore.

	Sam
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 27 ++++++++++----------
>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++--
>  drivers/gpu/drm/mgag200/mgag200_main.c | 34 ++++++++++++++++----------
>  3 files changed, 37 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index c2f0e4b40b052..ad12c1b7c66cc 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -51,6 +51,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>  
>  static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  {
> +	struct mga_device *mdev;
>  	struct drm_device *dev;
>  	int ret;
>  
> @@ -60,31 +61,28 @@ 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);
> -	if (IS_ERR(dev)) {
> -		ret = PTR_ERR(dev);
> +	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
> +	if (!mdev) {
> +		ret = -ENOMEM;
>  		goto err_pci_disable_device;
>  	}
>  
> -	dev->pdev = pdev;
> -	pci_set_drvdata(pdev, dev);
> -
> -	ret = mgag200_driver_load(dev, ent->driver_data);
> +	ret = mgag200_device_init(mdev, &driver, pdev, ent->driver_data);
>  	if (ret)
> -		goto err_drm_dev_put;
> +		goto err_pci_disable_device;
> +
> +	dev = mdev->dev;
>  
>  	ret = drm_dev_register(dev, ent->driver_data);
>  	if (ret)
> -		goto err_mgag200_driver_unload;
> +		goto err_mgag200_device_fini;
>  
>  	drm_fbdev_generic_setup(dev, 0);
>  
>  	return 0;
>  
> -err_mgag200_driver_unload:
> -	mgag200_driver_unload(dev);
> -err_drm_dev_put:
> -	drm_dev_put(dev);
> +err_mgag200_device_fini:
> +	mgag200_device_fini(mdev);
>  err_pci_disable_device:
>  	pci_disable_device(pdev);
>  	return ret;
> @@ -93,9 +91,10 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>  static void mga_pci_remove(struct pci_dev *pdev)
>  {
>  	struct drm_device *dev = pci_get_drvdata(pdev);
> +	struct mga_device *mdev = to_mga_device(dev);
>  
>  	drm_dev_unregister(dev);
> -	mgag200_driver_unload(dev);
> +	mgag200_device_fini(mdev);
>  	drm_dev_put(dev);
>  }
>  
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> index 632bbb50465c9..1ce0386669ffa 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> @@ -200,8 +200,9 @@ int mgag200_modeset_init(struct mga_device *mdev);
>  void mgag200_modeset_fini(struct mga_device *mdev);
>  
>  				/* mgag200_main.c */
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
> -void mgag200_driver_unload(struct drm_device *dev);
> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
> +			struct pci_dev *pdev, unsigned long flags);
> +void mgag200_device_fini(struct mga_device *mdev);
>  
>  				/* mgag200_i2c.c */
>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> index 010b309c01fc4..070ff1f433df2 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> @@ -11,6 +11,7 @@
>  #include <linux/pci.h>
>  
>  #include <drm/drm_crtc_helper.h>
> +#include <drm/drm_drv.h>
>  #include <drm/drm_gem_framebuffer_helper.h>
>  
>  #include "mgag200_drv.h"
> @@ -96,17 +97,21 @@ static int mga_vram_init(struct mga_device *mdev)
>   */
>  
>  
> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
> +			struct pci_dev *pdev, 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 = drm_dev_alloc(drv, &pdev->dev);
> +	if (IS_ERR(dev))
> +		return PTR_ERR(dev);
>  	dev->dev_private = (void *)mdev;
>  	mdev->dev = dev;
>  
> +	dev->pdev = pdev;
> +	pci_set_drvdata(pdev, dev);
> +
>  	mdev->flags = mgag200_flags_from_driver_data(flags);
>  	mdev->type = mgag200_type_from_driver_data(flags);
>  
> @@ -123,12 +128,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	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;
> +		ret = -ENOMEM;
> +		goto err_drm_dev_put;
>  	}
>  
>  	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
> -	if (mdev->rmmio == NULL)
> -		return -ENOMEM;
> +	if (mdev->rmmio == NULL) {
> +		ret = -ENOMEM;
> +		goto err_drm_dev_put;
> +	}
>  
>  	/* stash G200 SE model number for later use */
>  	if (IS_G200_SE(mdev)) {
> @@ -139,7 +147,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  
>  	ret = mga_vram_init(mdev);
>  	if (ret)
> -		return ret;
> +		goto err_drm_dev_put;
>  
>  	mdev->bpp_shifts[0] = 0;
>  	mdev->bpp_shifts[1] = 1;
> @@ -174,17 +182,17 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>  	drm_mode_config_cleanup(dev);
>  	mgag200_cursor_fini(mdev);
>  	mgag200_mm_fini(mdev);
> +err_drm_dev_put:
> +	drm_dev_put(dev);
>  err_mm:
>  	dev->dev_private = NULL;
>  	return ret;
>  }
>  
> -void mgag200_driver_unload(struct drm_device *dev)
> +void mgag200_device_fini(struct mga_device *mdev)
>  {
> -	struct mga_device *mdev = to_mga_device(dev);
> +	struct drm_device *dev = mdev->dev;
>  
> -	if (mdev == NULL)
> -		return;
>  	mgag200_modeset_fini(mdev);
>  	drm_mode_config_cleanup(dev);
>  	mgag200_cursor_fini(mdev);
> -- 
> 2.26.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/5] drm/mgag200: Remove several references to struct mga_device.dev

[Thomas Zimmermann]

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

Hi

Am 05.05.20 um 16:09 schrieb Daniel Vetter:
> On Tue, May 05, 2020 at 11:56:47AM +0200, Thomas Zimmermann wrote:
>> Done in preparation of embedding the DRM device in struct mga_device.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Not exactly sure what the goal is, since mga_vga_init still uses
> drm_device and not the mdev struct. *shrug*

It replaces 'mdev->dev' with simply 'dev'. That makes patch 5 easier to
read.

Best regards
Thomas

> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 21 +++++++++++----------
>>  drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++++++++--------
>>  2 files changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index 3830d3f3c9fa2..010b309c01fc4 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -66,25 +66,26 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
>>  /* 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(mdev->dev->pdev, 0);
>> -	mdev->mc.vram_window = pci_resource_len(mdev->dev->pdev, 0);
>> +	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(mdev->dev->dev, mdev->mc.vram_base, mdev->mc.vram_window,
>> -				"mgadrmfb_vram")) {
>> +	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(mdev->dev->pdev, 0, 0);
>> +	mem = pci_iomap(dev->pdev, 0, 0);
>>  	if (!mem)
>>  		return -ENOMEM;
>>  
>>  	mdev->mc.vram_size = mga_probe_vram(mdev, mem);
>>  
>> -	pci_iounmap(mdev->dev->pdev, mem);
>> +	pci_iounmap(dev->pdev, mem);
>>  
>>  	return 0;
>>  }
>> @@ -116,11 +117,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	mdev->has_sdram = !(option & (1 << 14));
>>  
>>  	/* BAR 0 is the framebuffer, BAR 1 contains registers */
>> -	mdev->rmmio_base = pci_resource_start(mdev->dev->pdev, 1);
>> -	mdev->rmmio_size = pci_resource_len(mdev->dev->pdev, 1);
>> +	mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
>> +	mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
>>  
>> -	if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
>> -				"mgadrmfb_mmio")) {
>> +	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;
>>  	}
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> index fa91869c0db52..aaa73b29b04f0 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
>> @@ -1433,6 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
>>  /* CRTC setup */
>>  static void mga_crtc_init(struct mga_device *mdev)
>>  {
>> +	struct drm_device *dev = mdev->dev;
>>  	struct mga_crtc *mga_crtc;
>>  
>>  	mga_crtc = kzalloc(sizeof(struct mga_crtc) +
>> @@ -1442,7 +1443,7 @@ static void mga_crtc_init(struct mga_device *mdev)
>>  	if (mga_crtc == NULL)
>>  		return;
>>  
>> -	drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
>> +	drm_crtc_init(dev, &mga_crtc->base, &mga_crtc_funcs);
>>  
>>  	drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
>>  	mdev->mode_info.crtc = mga_crtc;
>> @@ -1617,30 +1618,30 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
>>  
>>  int mgag200_modeset_init(struct mga_device *mdev)
>>  {
>> +	struct drm_device *dev = mdev->dev;
>>  	struct drm_encoder *encoder = &mdev->encoder;
>>  	struct drm_connector *connector;
>>  	int ret;
>>  
>>  	mdev->mode_info.mode_config_initialized = true;
>>  
>> -	mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
>> -	mdev->dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
>> +	dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
>> +	dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
>>  
>> -	mdev->dev->mode_config.fb_base = mdev->mc.vram_base;
>> +	dev->mode_config.fb_base = mdev->mc.vram_base;
>>  
>>  	mga_crtc_init(mdev);
>>  
>> -	ret = drm_simple_encoder_init(mdev->dev, encoder,
>> -				      DRM_MODE_ENCODER_DAC);
>> +	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
>>  	if (ret) {
>> -		drm_err(mdev->dev,
>> +		drm_err(dev,
>>  			"drm_simple_encoder_init() failed, error %d\n",
>>  			ret);
>>  		return ret;
>>  	}
>>  	encoder->possible_crtcs = 0x1;
>>  
>> -	connector = mga_vga_init(mdev->dev);
>> +	connector = mga_vga_init(dev);
>>  	if (!connector) {
>>  		DRM_ERROR("mga_vga_init failed\n");
>>  		return -1;
>> -- 
>> 2.26.0
>>
> 

-- 
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 4/5] drm/mgag200: Init and finalize devices in mgag200_device_{init,fini}()

[Thomas Zimmermann]

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

Hi

Am 05.05.20 um 16:14 schrieb Daniel Vetter:
> On Tue, May 05, 2020 at 11:56:48AM +0200, Thomas Zimmermann wrote:
>> Device initialization is now done in mgag200_device_init(). Specifically,
>> the function allocates the DRM device and sets up the respective fields
>> in struct mga_device.
>>
>> A call to mgag200_device_fini() finalizes struct mga_device.
>>
>> The old function mgag200_driver_load() and mgag200_driver_unload() were
>> left over from the DRM driver's load callbacks and have now been removed.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 27 ++++++++++----------
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++--
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 34 ++++++++++++++++----------
>>  3 files changed, 37 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index c2f0e4b40b052..ad12c1b7c66cc 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -51,6 +51,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>>  
>>  static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>> +	struct mga_device *mdev;
>>  	struct drm_device *dev;
>>  	int ret;
>>  
>> @@ -60,31 +61,28 @@ 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);
>> -	if (IS_ERR(dev)) {
>> -		ret = PTR_ERR(dev);
>> +	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
>> +	if (!mdev) {
>> +		ret = -ENOMEM;
>>  		goto err_pci_disable_device;
>>  	}
>>  
>> -	dev->pdev = pdev;
>> -	pci_set_drvdata(pdev, dev);
>> -
>> -	ret = mgag200_driver_load(dev, ent->driver_data);
>> +	ret = mgag200_device_init(mdev, &driver, pdev, ent->driver_data);
>>  	if (ret)
>> -		goto err_drm_dev_put;
>> +		goto err_pci_disable_device;
>> +
>> +	dev = mdev->dev;
>>  
>>  	ret = drm_dev_register(dev, ent->driver_data);
>>  	if (ret)
>> -		goto err_mgag200_driver_unload;
>> +		goto err_mgag200_device_fini;
>>  
>>  	drm_fbdev_generic_setup(dev, 0);
>>  
>>  	return 0;
>>  
>> -err_mgag200_driver_unload:
>> -	mgag200_driver_unload(dev);
>> -err_drm_dev_put:
>> -	drm_dev_put(dev);
> 
> Moving the drm_dev_put away from here will make the conversion to
> devm_drm_dev_alloc a bit more tricky I think. I'm not sure whether this is
> actually better than just directly going to devm_drm_dev_alloc and then
> cleaning up the fallout, that's at least what I've done in the conversions
> I've attempted thus far.

It's only a first step. Sam suggested to take patches 1 to 3 and build
the atomic conversion on top of that. That's probably what I'll do.

In the longer term, I'd like to use fully managed DRM functions, but
that requires another major patchset. The code in mgag200_main.c and
_ttm.c would go to either _drv.c or _mode.c. From there, the
initialization and shut down can be rewritten with managed helpers.
That's best done after the atomic conversion. Patches 4 and 5 can be
part of this. The VRAM helpers and cursor code will be gone then, which
also helps a bit.

Best regards
Thomas

> 
> Either way, this looks correct.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
>> +err_mgag200_device_fini:
>> +	mgag200_device_fini(mdev);
>>  err_pci_disable_device:
>>  	pci_disable_device(pdev);
>>  	return ret;
>> @@ -93,9 +91,10 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  static void mga_pci_remove(struct pci_dev *pdev)
>>  {
>>  	struct drm_device *dev = pci_get_drvdata(pdev);
>> +	struct mga_device *mdev = to_mga_device(dev);
>>  
>>  	drm_dev_unregister(dev);
>> -	mgag200_driver_unload(dev);
>> +	mgag200_device_fini(mdev);
>>  	drm_dev_put(dev);
>>  }
>>  
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 632bbb50465c9..1ce0386669ffa 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -200,8 +200,9 @@ int mgag200_modeset_init(struct mga_device *mdev);
>>  void mgag200_modeset_fini(struct mga_device *mdev);
>>  
>>  				/* mgag200_main.c */
>> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>> -void mgag200_driver_unload(struct drm_device *dev);
>> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
>> +			struct pci_dev *pdev, unsigned long flags);
>> +void mgag200_device_fini(struct mga_device *mdev);
>>  
>>  				/* mgag200_i2c.c */
>>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index 010b309c01fc4..070ff1f433df2 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/pci.h>
>>  
>>  #include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_drv.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  
>>  #include "mgag200_drv.h"
>> @@ -96,17 +97,21 @@ static int mga_vram_init(struct mga_device *mdev)
>>   */
>>  
>>  
>> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
>> +			struct pci_dev *pdev, 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 = drm_dev_alloc(drv, &pdev->dev);
>> +	if (IS_ERR(dev))
>> +		return PTR_ERR(dev);
>>  	dev->dev_private = (void *)mdev;
>>  	mdev->dev = dev;
>>  
>> +	dev->pdev = pdev;
>> +	pci_set_drvdata(pdev, dev);
>> +
>>  	mdev->flags = mgag200_flags_from_driver_data(flags);
>>  	mdev->type = mgag200_type_from_driver_data(flags);
>>  
>> @@ -123,12 +128,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	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;
>> +		ret = -ENOMEM;
>> +		goto err_drm_dev_put;
>>  	}
>>  
>>  	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
>> -	if (mdev->rmmio == NULL)
>> -		return -ENOMEM;
>> +	if (mdev->rmmio == NULL) {
>> +		ret = -ENOMEM;
>> +		goto err_drm_dev_put;
>> +	}
>>  
>>  	/* stash G200 SE model number for later use */
>>  	if (IS_G200_SE(mdev)) {
>> @@ -139,7 +147,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  
>>  	ret = mga_vram_init(mdev);
>>  	if (ret)
>> -		return ret;
>> +		goto err_drm_dev_put;
>>  
>>  	mdev->bpp_shifts[0] = 0;
>>  	mdev->bpp_shifts[1] = 1;
>> @@ -174,17 +182,17 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	drm_mode_config_cleanup(dev);
>>  	mgag200_cursor_fini(mdev);
>>  	mgag200_mm_fini(mdev);
>> +err_drm_dev_put:
>> +	drm_dev_put(dev);
>>  err_mm:
>>  	dev->dev_private = NULL;
>>  	return ret;
>>  }
>>  
>> -void mgag200_driver_unload(struct drm_device *dev)
>> +void mgag200_device_fini(struct mga_device *mdev)
>>  {
>> -	struct mga_device *mdev = to_mga_device(dev);
>> +	struct drm_device *dev = mdev->dev;
>>  
>> -	if (mdev == NULL)
>> -		return;
>>  	mgag200_modeset_fini(mdev);
>>  	drm_mode_config_cleanup(dev);
>>  	mgag200_cursor_fini(mdev);
>> -- 
>> 2.26.0
>>
> 

-- 
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 4/5] drm/mgag200: Init and finalize devices in mgag200_device_{init,fini}()

[Thomas Zimmermann]

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

Hi Sam

Am 05.05.20 um 18:49 schrieb Sam Ravnborg:
> Hi Thomas.
> 
> On Tue, May 05, 2020 at 11:56:48AM +0200, Thomas Zimmermann wrote:
>> Device initialization is now done in mgag200_device_init(). Specifically,
>> the function allocates the DRM device and sets up the respective fields
>> in struct mga_device.
>>
>> A call to mgag200_device_fini() finalizes struct mga_device.
>>
>> The old function mgag200_driver_load() and mgag200_driver_unload() were
>> left over from the DRM driver's load callbacks and have now been removed.
> 
> Not too big fan of this patch, due to the changes allocation.
> I would prefer if you merged patch 4+5 and then take it from there.
> 
> You have patch 1+2+3 and they are now reviewed so why not push them
> and work on top of them.

Good idea. I'll do this and postpone patches 4 and 5 to a later patch set.

Best regards
Thomas

> And then you could also push the patch that removes the cursor stuff
> so we do not need to look at that anymore.
> 
> 	Sam
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>  drivers/gpu/drm/mgag200/mgag200_drv.c  | 27 ++++++++++----------
>>  drivers/gpu/drm/mgag200/mgag200_drv.h  |  5 ++--
>>  drivers/gpu/drm/mgag200/mgag200_main.c | 34 ++++++++++++++++----------
>>  3 files changed, 37 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> index c2f0e4b40b052..ad12c1b7c66cc 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
>> @@ -51,6 +51,7 @@ MODULE_DEVICE_TABLE(pci, pciidlist);
>>  
>>  static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  {
>> +	struct mga_device *mdev;
>>  	struct drm_device *dev;
>>  	int ret;
>>  
>> @@ -60,31 +61,28 @@ 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);
>> -	if (IS_ERR(dev)) {
>> -		ret = PTR_ERR(dev);
>> +	mdev = devm_kzalloc(&pdev->dev, sizeof(*mdev), GFP_KERNEL);
>> +	if (!mdev) {
>> +		ret = -ENOMEM;
>>  		goto err_pci_disable_device;
>>  	}
>>  
>> -	dev->pdev = pdev;
>> -	pci_set_drvdata(pdev, dev);
>> -
>> -	ret = mgag200_driver_load(dev, ent->driver_data);
>> +	ret = mgag200_device_init(mdev, &driver, pdev, ent->driver_data);
>>  	if (ret)
>> -		goto err_drm_dev_put;
>> +		goto err_pci_disable_device;
>> +
>> +	dev = mdev->dev;
>>  
>>  	ret = drm_dev_register(dev, ent->driver_data);
>>  	if (ret)
>> -		goto err_mgag200_driver_unload;
>> +		goto err_mgag200_device_fini;
>>  
>>  	drm_fbdev_generic_setup(dev, 0);
>>  
>>  	return 0;
>>  
>> -err_mgag200_driver_unload:
>> -	mgag200_driver_unload(dev);
>> -err_drm_dev_put:
>> -	drm_dev_put(dev);
>> +err_mgag200_device_fini:
>> +	mgag200_device_fini(mdev);
>>  err_pci_disable_device:
>>  	pci_disable_device(pdev);
>>  	return ret;
>> @@ -93,9 +91,10 @@ static int mga_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>>  static void mga_pci_remove(struct pci_dev *pdev)
>>  {
>>  	struct drm_device *dev = pci_get_drvdata(pdev);
>> +	struct mga_device *mdev = to_mga_device(dev);
>>  
>>  	drm_dev_unregister(dev);
>> -	mgag200_driver_unload(dev);
>> +	mgag200_device_fini(mdev);
>>  	drm_dev_put(dev);
>>  }
>>  
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> index 632bbb50465c9..1ce0386669ffa 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
>> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
>> @@ -200,8 +200,9 @@ int mgag200_modeset_init(struct mga_device *mdev);
>>  void mgag200_modeset_fini(struct mga_device *mdev);
>>  
>>  				/* mgag200_main.c */
>> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags);
>> -void mgag200_driver_unload(struct drm_device *dev);
>> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
>> +			struct pci_dev *pdev, unsigned long flags);
>> +void mgag200_device_fini(struct mga_device *mdev);
>>  
>>  				/* mgag200_i2c.c */
>>  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
>> index 010b309c01fc4..070ff1f433df2 100644
>> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
>> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
>> @@ -11,6 +11,7 @@
>>  #include <linux/pci.h>
>>  
>>  #include <drm/drm_crtc_helper.h>
>> +#include <drm/drm_drv.h>
>>  #include <drm/drm_gem_framebuffer_helper.h>
>>  
>>  #include "mgag200_drv.h"
>> @@ -96,17 +97,21 @@ static int mga_vram_init(struct mga_device *mdev)
>>   */
>>  
>>  
>> -int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>> +int mgag200_device_init(struct mga_device *mdev, struct drm_driver *drv,
>> +			struct pci_dev *pdev, 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 = drm_dev_alloc(drv, &pdev->dev);
>> +	if (IS_ERR(dev))
>> +		return PTR_ERR(dev);
>>  	dev->dev_private = (void *)mdev;
>>  	mdev->dev = dev;
>>  
>> +	dev->pdev = pdev;
>> +	pci_set_drvdata(pdev, dev);
>> +
>>  	mdev->flags = mgag200_flags_from_driver_data(flags);
>>  	mdev->type = mgag200_type_from_driver_data(flags);
>>  
>> @@ -123,12 +128,15 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	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;
>> +		ret = -ENOMEM;
>> +		goto err_drm_dev_put;
>>  	}
>>  
>>  	mdev->rmmio = pcim_iomap(dev->pdev, 1, 0);
>> -	if (mdev->rmmio == NULL)
>> -		return -ENOMEM;
>> +	if (mdev->rmmio == NULL) {
>> +		ret = -ENOMEM;
>> +		goto err_drm_dev_put;
>> +	}
>>  
>>  	/* stash G200 SE model number for later use */
>>  	if (IS_G200_SE(mdev)) {
>> @@ -139,7 +147,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  
>>  	ret = mga_vram_init(mdev);
>>  	if (ret)
>> -		return ret;
>> +		goto err_drm_dev_put;
>>  
>>  	mdev->bpp_shifts[0] = 0;
>>  	mdev->bpp_shifts[1] = 1;
>> @@ -174,17 +182,17 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
>>  	drm_mode_config_cleanup(dev);
>>  	mgag200_cursor_fini(mdev);
>>  	mgag200_mm_fini(mdev);
>> +err_drm_dev_put:
>> +	drm_dev_put(dev);
>>  err_mm:
>>  	dev->dev_private = NULL;
>>  	return ret;
>>  }
>>  
>> -void mgag200_driver_unload(struct drm_device *dev)
>> +void mgag200_device_fini(struct mga_device *mdev)
>>  {
>> -	struct mga_device *mdev = to_mga_device(dev);
>> +	struct drm_device *dev = mdev->dev;
>>  
>> -	if (mdev == NULL)
>> -		return;
>>  	mgag200_modeset_fini(mdev);
>>  	drm_mode_config_cleanup(dev);
>>  	mgag200_cursor_fini(mdev);
>> -- 
>> 2.26.0

-- 
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 3/5] drm/mgag200: Remove several references to struct mga_device.dev

[Daniel Vetter]

On Wed, May 6, 2020 at 9:48 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> Hi
>
> Am 05.05.20 um 16:09 schrieb Daniel Vetter:
> > On Tue, May 05, 2020 at 11:56:47AM +0200, Thomas Zimmermann wrote:
> >> Done in preparation of embedding the DRM device in struct mga_device.
> >>
> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> >
> > Not exactly sure what the goal is, since mga_vga_init still uses
> > drm_device and not the mdev struct. *shrug*
>
> It replaces 'mdev->dev' with simply 'dev'. That makes patch 5 easier to
> read.

Ah right, maybe spell that out a bit clearer so that dense people like
me get it. You explained it in the commit message, it simply didn't
click that this is about avoiding tons of s/mdev->dev/mdev.dev/
replacement noise.
-Daniel

>
> Best regards
> Thomas
>
> >
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> >
> >> ---
> >>  drivers/gpu/drm/mgag200/mgag200_main.c | 21 +++++++++++----------
> >>  drivers/gpu/drm/mgag200/mgag200_mode.c | 17 +++++++++--------
> >>  2 files changed, 20 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> >> index 3830d3f3c9fa2..010b309c01fc4 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> >> @@ -66,25 +66,26 @@ static int mga_probe_vram(struct mga_device *mdev, void __iomem *mem)
> >>  /* 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(mdev->dev->pdev, 0);
> >> -    mdev->mc.vram_window = pci_resource_len(mdev->dev->pdev, 0);
> >> +    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(mdev->dev->dev, mdev->mc.vram_base, mdev->mc.vram_window,
> >> -                            "mgadrmfb_vram")) {
> >> +    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(mdev->dev->pdev, 0, 0);
> >> +    mem = pci_iomap(dev->pdev, 0, 0);
> >>      if (!mem)
> >>              return -ENOMEM;
> >>
> >>      mdev->mc.vram_size = mga_probe_vram(mdev, mem);
> >>
> >> -    pci_iounmap(mdev->dev->pdev, mem);
> >> +    pci_iounmap(dev->pdev, mem);
> >>
> >>      return 0;
> >>  }
> >> @@ -116,11 +117,11 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> >>      mdev->has_sdram = !(option & (1 << 14));
> >>
> >>      /* BAR 0 is the framebuffer, BAR 1 contains registers */
> >> -    mdev->rmmio_base = pci_resource_start(mdev->dev->pdev, 1);
> >> -    mdev->rmmio_size = pci_resource_len(mdev->dev->pdev, 1);
> >> +    mdev->rmmio_base = pci_resource_start(dev->pdev, 1);
> >> +    mdev->rmmio_size = pci_resource_len(dev->pdev, 1);
> >>
> >> -    if (!devm_request_mem_region(mdev->dev->dev, mdev->rmmio_base, mdev->rmmio_size,
> >> -                            "mgadrmfb_mmio")) {
> >> +    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;
> >>      }
> >> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> >> index fa91869c0db52..aaa73b29b04f0 100644
> >> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> >> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> >> @@ -1433,6 +1433,7 @@ static const struct drm_crtc_helper_funcs mga_helper_funcs = {
> >>  /* CRTC setup */
> >>  static void mga_crtc_init(struct mga_device *mdev)
> >>  {
> >> +    struct drm_device *dev = mdev->dev;
> >>      struct mga_crtc *mga_crtc;
> >>
> >>      mga_crtc = kzalloc(sizeof(struct mga_crtc) +
> >> @@ -1442,7 +1443,7 @@ static void mga_crtc_init(struct mga_device *mdev)
> >>      if (mga_crtc == NULL)
> >>              return;
> >>
> >> -    drm_crtc_init(mdev->dev, &mga_crtc->base, &mga_crtc_funcs);
> >> +    drm_crtc_init(dev, &mga_crtc->base, &mga_crtc_funcs);
> >>
> >>      drm_mode_crtc_set_gamma_size(&mga_crtc->base, MGAG200_LUT_SIZE);
> >>      mdev->mode_info.crtc = mga_crtc;
> >> @@ -1617,30 +1618,30 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
> >>
> >>  int mgag200_modeset_init(struct mga_device *mdev)
> >>  {
> >> +    struct drm_device *dev = mdev->dev;
> >>      struct drm_encoder *encoder = &mdev->encoder;
> >>      struct drm_connector *connector;
> >>      int ret;
> >>
> >>      mdev->mode_info.mode_config_initialized = true;
> >>
> >> -    mdev->dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
> >> -    mdev->dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
> >> +    dev->mode_config.max_width = MGAG200_MAX_FB_WIDTH;
> >> +    dev->mode_config.max_height = MGAG200_MAX_FB_HEIGHT;
> >>
> >> -    mdev->dev->mode_config.fb_base = mdev->mc.vram_base;
> >> +    dev->mode_config.fb_base = mdev->mc.vram_base;
> >>
> >>      mga_crtc_init(mdev);
> >>
> >> -    ret = drm_simple_encoder_init(mdev->dev, encoder,
> >> -                                  DRM_MODE_ENCODER_DAC);
> >> +    ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
> >>      if (ret) {
> >> -            drm_err(mdev->dev,
> >> +            drm_err(dev,
> >>                      "drm_simple_encoder_init() failed, error %d\n",
> >>                      ret);
> >>              return ret;
> >>      }
> >>      encoder->possible_crtcs = 0x1;
> >>
> >> -    connector = mga_vga_init(mdev->dev);
> >> +    connector = mga_vga_init(dev);
> >>      if (!connector) {
> >>              DRM_ERROR("mga_vga_init failed\n");
> >>              return -1;
> >> --
> >> 2.26.0
> >>
> >
>
> --
> 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
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm/mgag200: Convert struct drm_device to struct mga_device with macro

[Daniel Vetter]

On Tue, May 05, 2020 at 06:36:29PM +0200, Sam Ravnborg wrote:
> Hi Thomas.
> 
> On Tue, May 05, 2020 at 11:56:45AM +0200, Thomas Zimmermann wrote:
> > Mgag200 used dev_private to look up struct mga_device for instances
> > of struct drm_device. Use of dev_private is deprecated, so hide it in
> > the macro to_mga_device().
> > 
> > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> But one nit below.
> 
> 	Sam
> 
> > ---
> >  drivers/gpu/drm/mgag200/mgag200_cursor.c |  4 ++--
> >  drivers/gpu/drm/mgag200/mgag200_drv.c    |  2 +-
> >  drivers/gpu/drm/mgag200/mgag200_drv.h    |  1 +
> >  drivers/gpu/drm/mgag200/mgag200_i2c.c    | 10 +++++-----
> >  drivers/gpu/drm/mgag200/mgag200_main.c   |  4 ++--
> >  drivers/gpu/drm/mgag200/mgag200_mode.c   | 18 +++++++++---------
> >  6 files changed, 20 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_cursor.c b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> > index d491edd317ff3..aebc9ce43d551 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_cursor.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_cursor.c
> > @@ -260,7 +260,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> >  			    uint32_t handle, uint32_t width, uint32_t height)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct mga_device *mdev = (struct mga_device *)dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	struct drm_gem_object *obj;
> >  	struct drm_gem_vram_object *gbo = NULL;
> >  	int ret;
> > @@ -307,7 +307,7 @@ int mgag200_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
> >  
> >  int mgag200_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
> >  {
> > -	struct mga_device *mdev = (struct mga_device *)crtc->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(crtc->dev);
> >  
> >  	/* Our origin is at (64,64) */
> >  	x += 64;
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > index 3298b7ef18b03..c2f0e4b40b052 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> > @@ -120,7 +120,7 @@ int mgag200_driver_dumb_create(struct drm_file *file,
> >  			       struct drm_device *dev,
> >  			       struct drm_mode_create_dumb *args)
> >  {
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	unsigned long pg_align;
> >  
> >  	if (WARN_ONCE(!dev->vram_mm, "VRAM MM not initialized"))
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > index 9691252d6233f..632bbb50465c9 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_drv.h
> > +++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
> > @@ -96,6 +96,7 @@
> >  
> >  #define to_mga_crtc(x) container_of(x, struct mga_crtc, base)
> >  #define to_mga_connector(x) container_of(x, struct mga_connector, base)
> > +#define to_mga_device(x) ((struct mga_device *)(x)->dev_private)
> A inline function would have been better, as this provide no typecheck.
> But we assume that it is always a drm_device *.
> We would most likely catch it as no one else has a ->dev_prvate.

Once we're using container_of it's fully typesafe, so not sure it's worth
the interim bother.
-Daniel

> 
> >  
> >  struct mga_crtc {
> >  	struct drm_crtc base;
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
> > index 9f4635916d322..09731e614e46d 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
> > @@ -61,34 +61,34 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
> >  static void mga_gpio_setsda(void *data, int state)
> >  {
> >  	struct mga_i2c_chan *i2c = data;
> > -	struct mga_device *mdev = i2c->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(i2c->dev);
> >  	mga_i2c_set(mdev, i2c->data, state);
> >  }
> >  
> >  static void mga_gpio_setscl(void *data, int state)
> >  {
> >  	struct mga_i2c_chan *i2c = data;
> > -	struct mga_device *mdev = i2c->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(i2c->dev);
> >  	mga_i2c_set(mdev, i2c->clock, state);
> >  }
> >  
> >  static int mga_gpio_getsda(void *data)
> >  {
> >  	struct mga_i2c_chan *i2c = data;
> > -	struct mga_device *mdev = i2c->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(i2c->dev);
> >  	return (mga_i2c_read_gpio(mdev) & i2c->data) ? 1 : 0;
> >  }
> >  
> >  static int mga_gpio_getscl(void *data)
> >  {
> >  	struct mga_i2c_chan *i2c = data;
> > -	struct mga_device *mdev = i2c->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(i2c->dev);
> >  	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
> >  }
> >  
> >  struct mga_i2c_chan *mgag200_i2c_create(struct drm_device *dev)
> >  {
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	struct mga_i2c_chan *i2c;
> >  	int ret;
> >  	int data, clock;
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
> > index b680cf47cbb94..b705b7776d2fc 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_main.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_main.c
> > @@ -92,7 +92,7 @@ static int mga_vram_init(struct mga_device *mdev)
> >  static int mgag200_device_init(struct drm_device *dev,
> >  			       uint32_t flags)
> >  {
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	int ret, option;
> >  
> >  	mdev->flags = mgag200_flags_from_driver_data(flags);
> > @@ -195,7 +195,7 @@ int mgag200_driver_load(struct drm_device *dev, unsigned long flags)
> >  
> >  void mgag200_driver_unload(struct drm_device *dev)
> >  {
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  
> >  	if (mdev == NULL)
> >  		return;
> > diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > index d90e83959fca1..fa91869c0db52 100644
> > --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> > +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> > @@ -28,7 +28,7 @@
> >  static void mga_crtc_load_lut(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	struct drm_framebuffer *fb = crtc->primary->fb;
> >  	u16 *r_ptr, *g_ptr, *b_ptr;
> >  	int i;
> > @@ -728,7 +728,7 @@ static int mga_crtc_set_plls(struct mga_device *mdev, long clock)
> >  
> >  static void mga_g200wb_prepare(struct drm_crtc *crtc)
> >  {
> > -	struct mga_device *mdev = crtc->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(crtc->dev);
> >  	u8 tmp;
> >  	int iter_max;
> >  
> > @@ -783,7 +783,7 @@ static void mga_g200wb_prepare(struct drm_crtc *crtc)
> >  static void mga_g200wb_commit(struct drm_crtc *crtc)
> >  {
> >  	u8 tmp;
> > -	struct mga_device *mdev = crtc->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(crtc->dev);
> >  
> >  	/* 1- The first step is to ensure that the vrsten and hrsten are set */
> >  	WREG8(MGAREG_CRTCEXT_INDEX, 1);
> > @@ -833,7 +833,7 @@ static void mga_g200wb_commit(struct drm_crtc *crtc)
> >   */
> >  static void mga_set_start_address(struct drm_crtc *crtc, unsigned offset)
> >  {
> > -	struct mga_device *mdev = crtc->dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(crtc->dev);
> >  	u32 addr;
> >  	int count;
> >  	u8 crtcext0;
> > @@ -902,7 +902,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
> >  				int x, int y, struct drm_framebuffer *old_fb)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	const struct drm_framebuffer *fb = crtc->primary->fb;
> >  	int hdisplay, hsyncstart, hsyncend, htotal;
> >  	int vdisplay, vsyncstart, vsyncend, vtotal;
> > @@ -1263,7 +1263,7 @@ static int mga_resume(struct drm_crtc *crtc)
> >  static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	u8 seq1 = 0, crtcext1 = 0;
> >  
> >  	switch (mode) {
> > @@ -1317,7 +1317,7 @@ static void mga_crtc_dpms(struct drm_crtc *crtc, int mode)
> >  static void mga_crtc_prepare(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	u8 tmp;
> >  
> >  	/*	mga_resume(crtc);*/
> > @@ -1353,7 +1353,7 @@ static void mga_crtc_prepare(struct drm_crtc *crtc)
> >  static void mga_crtc_commit(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > -	struct mga_device *mdev = dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	const struct drm_crtc_helper_funcs *crtc_funcs = crtc->helper_private;
> >  	u8 tmp;
> >  
> > @@ -1495,7 +1495,7 @@ static enum drm_mode_status mga_vga_mode_valid(struct drm_connector *connector,
> >  				 struct drm_display_mode *mode)
> >  {
> >  	struct drm_device *dev = connector->dev;
> > -	struct mga_device *mdev = (struct mga_device*)dev->dev_private;
> > +	struct mga_device *mdev = to_mga_device(dev);
> >  	int bpp = 32;
> >  
> >  	if (IS_G200_SE(mdev)) {
> > -- 
> > 2.26.0

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