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

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

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

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

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

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

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