Skip to contentSkip to conversation

drm/mgag200: Fixes and enhancements for the MGAG200 KMS driver

PATCH In review Egbert Eich 5 participants 28 messages Jul 17, 2013 – Jul 23, 2013

Subsystems (3)
DRM drivers DRM misc DRM driver for mga G200 graphics chips
lore
Share & tools
lore

Egbert Eich

author
#0 lore raw

cover drm/mgag200: Fixes and enhancements for the MGAG200 KMS driver

This set of patches contains several fixes and enhancements for the MGAG200 KMS driver.

Egbert Eich (15):
drm/mgag200: Fix memleaks in error path in mgag200_fb_create() drm/mgag200: Fix memleak in error path in mgag200_bo_create() drm/mgag200: Free container instead of member in
mga_user_framebuffer_destroy()
drm/mgag200: Don't unreference when handle creation failed drm/mgag200: Copy fb name string before using it in

    mgag200_fb_create()

drm/mgag200: Fix logic in mgag200_bo_pin()
drm/mgag200: Make local function mgag200_gem_init_object() static drm/mgag200: Simplify function mgag200_bo_unpin() drm/mgag200: Add an crtc_disable callback to the crtc helper funcs drm/mgag200: Invalidate page tables when pinning a BO drm/mgag200: Initialize data needed to map fbdev memory drm/mgag200: Add sysfs support for connectors drm/mgag200: Fix LUT programming for 16bpp
drm/mgag200: Reject modes when h-parameters are no multiple of 8 drm/mgag200: Add doublescan and interlace support

Takashi Iwai (1):
drm/mgag200: Fix framebuffer pitch calculation

 drivers/gpu/drm/mgag200/mgag200_fb.c   | 27 +++++++++++----
 drivers/gpu/drm/mgag200/mgag200_main.c |  7 ++--
 drivers/gpu/drm/mgag200/mgag200_mode.c | 60 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/mgag200/mgag200_ttm.c  | 42 +++++++++++-------------
 4 files changed, 100 insertions(+), 36 deletions(-)
Signature · 1 line
1.8.1.4
author
#1 lore raw

PATCH 1/16 drm/mgag200: Fix memleaks in error path in mgag200_fb_create()

Some mmemory allocated in mgag200fb_create() was not properly freed before the function returned with an error. This patch takes care of this.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)
drivers/gpu/drm/mgag200/mgag200_fb.c +16 -5
index 964f58c..6453e4c 100644
@@ -162,13 +162,13 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
 	struct drm_device *dev = mfbdev->helper.dev;
 	struct drm_mode_fb_cmd2 mode_cmd;
 	struct mga_device *mdev = dev->dev_private;
-	struct fb_info *info;
+	struct fb_info *info = NULL;
 	struct drm_framebuffer *fb;
 	struct drm_gem_object *gobj = NULL;
 	struct device *device = &dev->pdev->dev;
 	struct mgag200_bo *bo;
 	int ret;
-	void *sysram;
+	void *sysram = NULL;
 	int size;
 
 	mode_cmd.width = sizes->surface_width;

@@ -191,14 +191,16 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
 		return -ENOMEM;
 
 	info = framebuffer_alloc(0, device);
-	if (info == NULL)
-		return -ENOMEM;
+	if (info == NULL) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	info->par = mfbdev;
 
 	ret = mgag200_framebuffer_init(dev, &mfbdev->mfb, &mode_cmd, gobj);
 	if (ret)
-		return ret;
+		goto out;
 
 	mfbdev->sysram = sysram;
 	mfbdev->size = size;

@@ -242,6 +244,15 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
 		      fb->width, fb->height);
 	return 0;
 out:
+	mfbdev->mfb.obj = NULL;
+	vfree(sysram);
+	mfbdev->sysram = NULL;
+	if (info && info->cmap.len)
+		fb_dealloc_cmap(&info->cmap);
+	framebuffer_release(info);
+	mfbdev->helper.fb = NULL;
+	mfbdev->helper.fbdev = NULL;
+
 	return ret;
 }
Signature · 1 line
1.8.1.4
author
#2 lore raw

PATCH 2/16 drm/mgag200: Fix memleak in error path in mgag200_bo_create()

In reply to Egbert Eich

The allocated struct mgag200_bo was not freed in all error paths. This patch consolidates error handling and fixes this.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
drivers/gpu/drm/mgag200/mgag200_ttm.c +6 -5
index 3acb2b0..6461fd2 100644
@@ -316,10 +316,8 @@ int mgag200_bo_create(struct drm_device *dev, int size, int align,
 		return -ENOMEM;
 
 	ret = drm_gem_object_init(dev, &mgabo->gem, size);
-	if (ret) {
-		kfree(mgabo);
-		return ret;
-	}
+	if (ret)
+		goto err;
 
 	mgabo->gem.driver_private = NULL;
 	mgabo->bo.bdev = &mdev->ttm.bdev;

@@ -334,10 +332,13 @@ int mgag200_bo_create(struct drm_device *dev, int size, int align,
 			  align >> PAGE_SHIFT, false, NULL, acc_size,
 			  NULL, mgag200_bo_ttm_destroy);
 	if (ret)
-		return ret;
+		goto err;
 
 	*pmgabo = mgabo;
 	return 0;
+err:
+	kfree(mgabo);
+	return ret;
 }
 
 static inline u64 mgag200_bo_gpu_offset(struct mgag200_bo *bo)
Signature · 1 line
1.8.1.4
author
#3 lore raw

PATCH 3/16 drm/mgag200: Free container instead of member in mga_user_framebuffer_destroy()

In reply to Egbert Eich

Technically freeing mga_fb->base is the same as freeing mga_fb as 'base' the first member of the data structure.
Still this makes it cleaner.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/mgag200/mgag200_main.c +1 -1
index 9fa5685..43ebf8d 100644
@@ -20,7 +20,7 @@ static void mga_user_framebuffer_destroy(struct drm_framebuffer *fb)
 	if (mga_fb->obj)
 		drm_gem_object_unreference_unlocked(mga_fb->obj);
 	drm_framebuffer_cleanup(fb);
-	kfree(fb);
+	kfree(mga_fb);
 }
 
 static const struct drm_framebuffer_funcs mga_fb_funcs = {
Signature · 1 line
1.8.1.4
author
#4 lore raw

PATCH 4/16 drm/mgag200: Don't unreference when handle creation failed

In reply to Egbert Eich

drm_gem_handle_create() should not have referenced an object when it's creation has failed for some reason.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/gpu/drm/mgag200/mgag200_main.c +2 -1
index 43ebf8d..d51096c 100644
@@ -302,10 +302,11 @@ int mgag200_dumb_create(struct drm_file *file,
 		return ret;
 
 	ret = drm_gem_handle_create(file, gobj, &handle);
-	drm_gem_object_unreference_unlocked(gobj);
 	if (ret)
 		return ret;
 
+	drm_gem_object_unreference_unlocked(gobj);
+
 	args->handle = handle;
 	return 0;
 }
Signature · 1 line
1.8.1.4
#5 lore raw

Re: [PATCH 04/16] drm/mgag200: Don't unreference when handle creation failed

Quoted 4 lines · Egbert Eich

drm_gem_handle_create() should not have referenced an object when it's creation has failed for some reason.

Signed-off-by Egbert Eich <[email protected]>

Nak. The unreference here is to free the locally created resource before returning the error to the user. In the case of success, the only reference to the object that then remains is through the new handle. -Chris

Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
author
#6 lore raw

Re: [PATCH 04/16] drm/mgag200: Don't unreference when handle creation failed

Quoted 10 lines · Chris Wilson
Quoted 4 lines · Egbert Eich

drm_gem_handle_create() should not have referenced an object when it's creation has failed for some reason.

Signed-off-by Egbert Eich <[email protected]>

Nak. The unreference here is to free the locally created resource before returning the error to the user. In the case of success, the only reference to the object that then remains is through the new handle. -Chris

Argh, yeah, you're right. Sorry.

Cheers,
Egbert.

author
#7 lore raw

PATCH 5/16 drm/mgag200: Copy fb name string before using it in mgag200_fb_create()

In reply to Egbert Eich

The fb id string was used in an error message right before it was set. Reversing the order of the code fixes this.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/gpu/drm/mgag200/mgag200_fb.c +2 -2
index 6453e4c..371f3eb 100644
@@ -211,6 +211,8 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
 	mfbdev->helper.fb = fb;
 	mfbdev->helper.fbdev = info;
 
+	strcpy(info->fix.id, "mgadrmfb");
+
 	ret = fb_alloc_cmap(&info->cmap, 256, 0);
 	if (ret) {
 		DRM_ERROR("%s: can't allocate color map\n", info->fix.id);

@@ -218,8 +220,6 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
 		goto out;
 	}
 
-	strcpy(info->fix.id, "mgadrmfb");
-
 	info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
 	info->fbops = &mgag200fb_ops;
Signature · 1 line
1.8.1.4
author
#8 lore raw

PATCH 6/16 drm/mgag200: Fix logic in mgag200_bo_pin()

In reply to Egbert Eich
Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/gpu/drm/mgag200/mgag200_ttm.c +9 -13
index 6461fd2..2606031 100644
@@ -350,20 +350,16 @@ int mgag200_bo_pin(struct mgag200_bo *bo, u32 pl_flag, u64 *gpu_addr)
 {
 	int i, ret;
 
-	if (bo->pin_count) {
-		bo->pin_count++;
-		if (gpu_addr)
-			*gpu_addr = mgag200_bo_gpu_offset(bo);
-	}
-
-	mgag200_ttm_placement(bo, pl_flag);
-	for (i = 0; i < bo->placement.num_placement; i++)
-		bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
-	if (ret)
-		return ret;
 
-	bo->pin_count = 1;
+	if (!bo->pin_count) {
+		mgag200_ttm_placement(bo, pl_flag);
+		for (i = 0; i < bo->placement.num_placement; i++)
+			bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
+		ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
+		if (ret)
+			return ret;
+	}
+	bo->pin_count++;
 	if (gpu_addr)
 		*gpu_addr = mgag200_bo_gpu_offset(bo);
 	return 0;
Signature · 1 line
1.8.1.4
maintainer
#9 lore raw

Re: [PATCH 06/16] drm/mgag200: Fix logic in mgag200_bo_pin()

Quoted 1 line · Egbert Eich
Signed-off-by Egbert Eich <[email protected]>

Is this just missing a return 0; ? in the bo->pin_count > 0 case? seems like a simpler patch.

Dave.

Quoted 45 lines
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)
drivers/gpu/drm/mgag200/mgag200_ttm.c +9 -14
index 6461fd2..2606031 100644
@@ -350,20 +350,16 @@ int mgag200_bo_pin(struct mgag200_bo *bo, u32 pl_flag, u64 *gpu_addr)
 {
        int i, ret;
 
-       if (bo->pin_count) {
-               bo->pin_count++;
-               if (gpu_addr)
-                       *gpu_addr = mgag200_bo_gpu_offset(bo);
-       }
-
-       mgag200_ttm_placement(bo, pl_flag);
-       for (i = 0; i < bo->placement.num_placement; i++)
-               bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
-       ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
-       if (ret)
-               return ret;
 
-       bo->pin_count = 1;
+       if (!bo->pin_count) {
+               mgag200_ttm_placement(bo, pl_flag);
+               for (i = 0; i < bo->placement.num_placement; i++)
+                       bo->placements[i] |= TTM_PL_FLAG_NO_EVICT;
+               ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
+               if (ret)
+                       return ret;
+       }
+       bo->pin_count++;
        if (gpu_addr)
                *gpu_addr = mgag200_bo_gpu_offset(bo);
        return 0;
--

1.8.1.4

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#10 lore raw

PATCH v2 6/16 drm/mgag200: Fix logic in mgag200_bo_pin() (v2)

Add missing 'return 0;'.

v2: Simplified patch as suggested by Dave Airlie <[email protected]>

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/mgag200/mgag200_ttm.c +1 -0
index 6461fd2..1e8746c 100644
@@ -354,6 +354,7 @@ int mgag200_bo_pin(struct mgag200_bo *bo, u32 pl_flag, u64 *gpu_addr)
 		bo->pin_count++;
 		if (gpu_addr)
 			*gpu_addr = mgag200_bo_gpu_offset(bo);
+		return 0;
 	}
 
 	mgag200_ttm_placement(bo, pl_flag);
Signature · 1 line
1.8.1.4
author
#11 lore raw

PATCH 7/16 drm/mgag200: Make local function mgag200_gem_init_object() static

In reply to Egbert Eich
Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/mgag200/mgag200_main.c +1 -1
index d51096c..fe8ed66 100644
@@ -324,7 +324,7 @@ int mgag200_gem_init_object(struct drm_gem_object *obj)
 	return 0;
 }
 
-void mgag200_bo_unref(struct mgag200_bo **bo)
+static void mgag200_bo_unref(struct mgag200_bo **bo)
 {
 	struct ttm_buffer_object *tbo;
Signature · 1 line
1.8.1.4
author
#12 lore raw

PATCH 8/16 drm/mgag200: Simplify function mgag200_bo_unpin()

In reply to Egbert Eich
Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)
drivers/gpu/drm/mgag200/mgag200_ttm.c +2 -5
index 2606031..3a2e5e2 100644
@@ -367,7 +367,7 @@ int mgag200_bo_pin(struct mgag200_bo *bo, u32 pl_flag, u64 *gpu_addr)
 
 int mgag200_bo_unpin(struct mgag200_bo *bo)
 {
-	int i, ret;
+	int i;
 	if (!bo->pin_count) {
 		DRM_ERROR("unpin bad %p\n", bo);
 		return 0;

@@ -378,11 +378,8 @@ int mgag200_bo_unpin(struct mgag200_bo *bo)
 
 	for (i = 0; i < bo->placement.num_placement ; i++)
 		bo->placements[i] &= ~TTM_PL_FLAG_NO_EVICT;
-	ret = ttm_bo_validate(&bo->bo, &bo->placement, false, false);
-	if (ret)
-		return ret;
 
-	return 0;
+	return ttm_bo_validate(&bo->bo, &bo->placement, false, false);
 }
 
 int mgag200_bo_push_sysram(struct mgag200_bo *bo)
Signature · 1 line
1.8.1.4
author
#13 lore raw

PATCH 9/16 drm/mgag200: Add an crtc_disable callback to the crtc helper funcs

In reply to Egbert Eich
Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)
drivers/gpu/drm/mgag200/mgag200_mode.c +19 -0
index 251784a..2fe1f64 100644
@@ -1251,6 +1251,24 @@ static void mga_crtc_destroy(struct drm_crtc *crtc)
 	kfree(mga_crtc);
 }
 
+static void mga_crtc_disable(struct drm_crtc *crtc)
+{
+	int ret;
+	DRM_DEBUG_KMS("\n");
+	mga_crtc_dpms(crtc, DRM_MODE_DPMS_OFF);
+	if (crtc->fb) {
+		struct mga_framebuffer *mga_fb = to_mga_framebuffer(crtc->fb);
+		struct drm_gem_object *obj = mga_fb->obj;
+		struct mgag200_bo *bo = gem_to_mga_bo(obj);
+		ret = mgag200_bo_reserve(bo, false);
+		if (ret)
+			return;
+		mgag200_bo_push_sysram(bo);
+		mgag200_bo_unreserve(bo);
+	}
+	crtc->fb = NULL;
+}
+
 /* These provide the minimum set of functions required to handle a CRTC */
 static const struct drm_crtc_funcs mga_crtc_funcs = {
 	.cursor_set = mga_crtc_cursor_set,

@@ -1261,6 +1279,7 @@ static const struct drm_crtc_funcs mga_crtc_funcs = {
 };
 
 static const struct drm_crtc_helper_funcs mga_helper_funcs = {
+	.disable = mga_crtc_disable,
 	.dpms = mga_crtc_dpms,
 	.mode_fixup = mga_crtc_mode_fixup,
 	.mode_set = mga_crtc_mode_set,
Signature · 1 line
1.8.1.4
author
#14 lore raw

PATCH 10/16 drm/mgag200: Invalidate page tables when pinning a BO

In reply to Egbert Eich

When a BO gets pinned the placement may get changed. If the memory is mapped into user space and user space has already accessed the mapped range the page tables are set up but now point to the wrong memory. A call to ttm_bo_unmap_virtual() will invalidate all page tables of all mappings of this BO. When user space accesses this memory again we will receive a page fault and are able to set up the page tables to point to the correct physical memory.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/mgag200/mgag200_ttm.c +2 -0
index 3a2e5e2..8ceeb0c 100644
@@ -321,6 +321,7 @@ int mgag200_bo_create(struct drm_device *dev, int size, int align,
 
 	mgabo->gem.driver_private = NULL;
 	mgabo->bo.bdev = &mdev->ttm.bdev;
+	mgabo->bo.bdev->dev_mapping = dev->dev_mapping;
 
 	mgag200_ttm_placement(mgabo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
 

@@ -359,6 +360,7 @@ int mgag200_bo_pin(struct mgag200_bo *bo, u32 pl_flag, u64 *gpu_addr)
 		if (ret)
 			return ret;
 	}
+	ttm_bo_unmap_virtual(&bo->bo);
 	bo->pin_count++;
 	if (gpu_addr)
 		*gpu_addr = mgag200_bo_gpu_offset(bo);
Signature · 1 line
1.8.1.4
author
#15 lore raw

PATCH v2 1/1 drm/mgag200: Invalidate page tables when pinning a BO

When a BO gets pinned the placement may get changed. If the memory is mapped into user space and user space has already accessed the mapped range the page tables are set up but now point to the wrong memory. Set bo.mdev->dev_mapping in mgag200_bo_create() to make sure that ttm_bo_unmap_virtual() called from ttm_bo_handle_move_mem() will take care of this.

v2: Don't call ttm_bo_unmap_virtual() in mgag200_bo_pin(), fix comment.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 1 +
 1 file changed, 1 insertion(+)
drivers/gpu/drm/mgag200/mgag200_ttm.c +1 -0
index 3a2e5e2..6aa8fad 100644
@@ -321,6 +321,7 @@ int mgag200_bo_create(struct drm_device *dev, int size, int align,
 
 	mgabo->gem.driver_private = NULL;
 	mgabo->bo.bdev = &mdev->ttm.bdev;
+	mgabo->bo.bdev->dev_mapping = dev->dev_mapping;
 
 	mgag200_ttm_placement(mgabo, TTM_PL_FLAG_VRAM | TTM_PL_FLAG_SYSTEM);
Signature · 1 line
1.8.1.4
author
#16 lore raw

PATCH 11/16 drm/mgag200: Initialize data needed to map fbdev memory

In reply to Egbert Eich

Due to a missing initialization there was no way to map fbdev memory. Thus for example using the Xserver with the fbdev driver failed. This fix adds initialization for fix.smem_start and fix.smem_len in the fb_info structure, which fixes this problem.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/mgag200/mgag200_fb.c +2 -0
index 371f3eb..94c64de 100644
@@ -231,6 +231,8 @@ static int mgag200fb_create(struct drm_fb_helper *helper,
 	}
 	info->apertures->ranges[0].base = mdev->dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = mdev->mc.vram_size;
+	info->fix.smem_start = mdev->dev->mode_config.fb_base;
+	info->fix.smem_len = size;
 
 	drm_fb_helper_fill_fix(info, fb->pitches[0], fb->depth);
 	drm_fb_helper_fill_var(info, &mfbdev->helper, sizes->fb_width,
Signature · 1 line
1.8.1.4
maintainer
#17 lore raw

Re: [PATCH 11/16] drm/mgag200: Initialize data needed to map fbdev memory

Quoted 4 lines · Egbert Eich

Due to a missing initialization there was no way to map fbdev memory. Thus for example using the Xserver with the fbdev driver failed. This fix adds initialization for fix.smem_start and fix.smem_len in the fb_info structure, which fixes this problem.

I don't think this is correct, since it'll give userspace a direct phy addr for the base of the video ram, though I'm not in a position to say exactly what should be set here, guess I need to get an mgag200 machine up and running.

Dave.

author
#18 lore raw

PATCH 12/16 drm/mgag200: Add sysfs support for connectors

In reply to Egbert Eich
Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 2 ++
 1 file changed, 2 insertions(+)
drivers/gpu/drm/mgag200/mgag200_mode.c +2 -0
index 2fe1f64..020a623 100644
@@ -1600,6 +1600,8 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
 
 	drm_connector_helper_add(connector, &mga_vga_connector_helper_funcs);
 
+	drm_sysfs_connector_add(connector);
+
 	mga_connector->i2c = mgag200_i2c_create(dev);
 	if (!mga_connector->i2c)
 		DRM_ERROR("failed to add ddc bus\n");
Signature · 1 line
1.8.1.4
author
#19 lore raw

PATCH 13/16 drm/mgag200: Fix framebuffer pitch calculation

In reply to Egbert Eich

From: Takashi Iwai <[email protected]>

The framebuffer pitch calculation needs to be done differently for bpp == 24
- check xf86-video-mga for reference.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/gpu/drm/mgag200/mgag200_mode.c +1 -1
index 020a623..c8983f9 100644
@@ -877,7 +877,7 @@ static int mga_crtc_mode_set(struct drm_crtc *crtc,
 
 	pitch = crtc->fb->pitches[0] / (crtc->fb->bits_per_pixel / 8);
 	if (crtc->fb->bits_per_pixel == 24)
-		pitch = pitch >> (4 - bppshift);
+		pitch = (pitch * 3) >> (4 - bppshift);
 	else
 		pitch = pitch >> (4 - bppshift);
Signature · 1 line
1.8.1.4
author
#20 lore raw

PATCH 14/16 drm/mgag200: Fix LUT programming for 16bpp

In reply to Egbert Eich

Since there are only 32 (64) distinct color values for each color in 16bpp Matrox hardware expects those in a 'dense' manner, ie in the first 32 (64) entries of the respective color.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
drivers/gpu/drm/mgag200/mgag200_mode.c +23 -0
index c8983f9..503a414 100644
@@ -29,6 +29,7 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc)
 	struct mga_crtc *mga_crtc = to_mga_crtc(crtc);
 	struct drm_device *dev = crtc->dev;
 	struct mga_device *mdev = dev->dev_private;
+	struct drm_framebuffer *fb = crtc->fb;
 	int i;
 
 	if (!crtc->enabled)

@@ -36,6 +37,28 @@ static void mga_crtc_load_lut(struct drm_crtc *crtc)
 
 	WREG8(DAC_INDEX + MGA1064_INDEX, 0);
 
+	if (fb && fb->bits_per_pixel == 16) {
+		int inc = (fb->depth == 15) ? 8 : 4;
+		u8 r, b;
+		for (i = 0; i < MGAG200_LUT_SIZE; i += inc) {
+			if (fb->depth == 16) {
+				if (i > (MGAG200_LUT_SIZE >> 1)) {
+					r = b = 0;
+				} else {
+					r = mga_crtc->lut_r[i << 1];
+					b = mga_crtc->lut_b[i << 1];
+				}
+			} else {
+				r = mga_crtc->lut_r[i];
+				b = mga_crtc->lut_b[i];
+			}
+			/* VGA registers */
+			WREG8(DAC_INDEX + MGA1064_COL_PAL, r);
+			WREG8(DAC_INDEX + MGA1064_COL_PAL, mga_crtc->lut_g[i]);
+			WREG8(DAC_INDEX + MGA1064_COL_PAL, b);
+		}
+		return;
+	}
 	for (i = 0; i < MGAG200_LUT_SIZE; i++) {
 		/* VGA registers */
 		WREG8(DAC_INDEX + MGA1064_COL_PAL, mga_crtc->lut_r[i]);
Signature · 1 line
1.8.1.4
author
#21 lore raw

PATCH 15/16 drm/mgag200: Reject modes when h-parameters are no multiple of 8

In reply to Egbert Eich

Matrox hardware only supports modes whose horizontal parameters are multiples of 8. This rules out a mode like 1366x768 for example.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++++
 1 file changed, 4 insertions(+)
drivers/gpu/drm/mgag200/mgag200_mode.c +4 -0
index 503a414..0bb0e1e 100644
@@ -1491,6 +1491,10 @@ static int mga_vga_mode_valid(struct drm_connector *connector,
 	int bpp = 32;
 	int i = 0;
 
+	if (mode->hdisplay % 8 || mode->hsync_start % 8 ||
+	    mode->hsync_end % 8 || mode->htotal % 8)
+		return MODE_H_ILLEGAL;
+
 	if (IS_G200_SE(mdev)) {
 		if (mdev->unique_rev_id == 0x01) {
 			if (mode->hdisplay > 1600)
Signature · 1 line
1.8.1.4
maintainer
#22 lore raw

Re: [PATCH 15/16] drm/mgag200: Reject modes when h-parameters are no multiple of 8

Quoted 4 lines · Egbert Eich

Matrox hardware only supports modes whose horizontal parameters are multiples of 8. This rules out a mode like 1366x768 for example.

Signed-off-by Egbert Eich <[email protected]>

I'd like to get a second opinion from ajax for where this should be dealt with, though this is probably okay.

Dave.

#23 lore raw

Re: [PATCH 15/16] drm/mgag200: Reject modes when h-parameters are no multiple of 8

Quoted 8 lines · Dave Airlie
Quoted 4 lines · Egbert Eich

Matrox hardware only supports modes whose horizontal parameters are multiples of 8. This rules out a mode like 1366x768 for example.

Signed-off-by Egbert Eich <[email protected]>

I'd like to get a second opinion from ajax for where this should be dealt with, though this is probably okay.

Patch seems like it's in the right place to me.

- ajax

#24 lore raw

Re: [PATCH 15/16] drm/mgag200: Reject modes when h-parameters are no multiple of 8

Quoted 11 lines · Adam Jackson
Quoted 8 lines · Dave Airlie
Quoted 4 lines · Egbert Eich

Matrox hardware only supports modes whose horizontal parameters are multiples of 8. This rules out a mode like 1366x768 for example.

Signed-off-by Egbert Eich <[email protected]>

I'd like to get a second opinion from ajax for where this should be dealt with, though this is probably okay.

Patch seems like it's in the right place to me.

->mode_valid is only used when parsing the list of modes obtained from the EDID. Userspace can still add new modes of its own and force the kernel to use them. To plug that gap mgag200 also needs to reject any modes in the crtc_helper->mode_fixup callback.

I've pondered whether we should unify these too callers, e.g. by using the ->mode_fixup function from ->mode_valid with a fake adjusted_mode. But I never got around to that ...
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

author
#25 lore raw

PATCH 16/16 drm/mgag200: Add doublescan and interlace support

In reply to Egbert Eich

This code was ported from the xorg mga driver.

Signed-off-by Egbert Eich <[email protected]>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)
drivers/gpu/drm/mgag200/mgag200_mode.c +8 -2
index 0bb0e1e..3faacbe 100644
@@ -1488,6 +1488,7 @@ static int mga_vga_mode_valid(struct drm_connector *connector,
 	struct mga_fbdev *mfbdev = mdev->mfbdev;
 	struct drm_fb_helper *fb_helper = &mfbdev->helper;
 	struct drm_fb_helper_connector *fb_helper_conn = NULL;
+	int lace = 1 + ((mode->flags & DRM_MODE_FLAG_INTERLACE) ? 1 : 0);
 	int bpp = 32;
 	int i = 0;
 

@@ -1537,8 +1538,10 @@ static int mga_vga_mode_valid(struct drm_connector *connector,
 
 	if (mode->crtc_hdisplay > 2048 || mode->crtc_hsync_start > 4096 ||
 	    mode->crtc_hsync_end > 4096 || mode->crtc_htotal > 4096 ||
-	    mode->crtc_vdisplay > 2048 || mode->crtc_vsync_start > 4096 ||
-	    mode->crtc_vsync_end > 4096 || mode->crtc_vtotal > 4096) {
+	    mode->crtc_vdisplay > 2048 * lace ||
+	    mode->crtc_vsync_start > 4096 * lace ||
+	    mode->crtc_vsync_end > 4096 * lace ||
+	    mode->crtc_vtotal > 4096 * lace) {
 		return MODE_BAD;
 	}
 

@@ -1622,6 +1625,9 @@ static struct drm_connector *mga_vga_init(struct drm_device *dev)
 
 	connector = &mga_connector->base;
 
+	connector->interlace_allowed = true;
+	connector->doublescan_allowed = true;
+
 	drm_connector_init(dev, connector,
 			   &mga_vga_connector_funcs, DRM_MODE_CONNECTOR_VGA);
Signature · 1 line
1.8.1.4
maintainer
#26 lore raw
In reply to Egbert Eich
Quoted 2 lines · Egbert Eich

This set of patches contains several fixes and enhancements for the MGAG200 KMS driver.

Hi Egbert,

I've pushed some of these into a branch in my repo, mgag200-queue,

I'll try and review the rest soon, I'll probably split these up into obvious fixes for Linus, and leave the cleanups/features for -next if they aren't fixing any actual problems.

Dave.

Quoted 36 lines

Egbert Eich (15):
drm/mgag200: Fix memleaks in error path in mgag200_fb_create() drm/mgag200: Fix memleak in error path in mgag200_bo_create() drm/mgag200: Free container instead of member in
mga_user_framebuffer_destroy()
drm/mgag200: Don't unreference when handle creation failed drm/mgag200: Copy fb name string before using it in

    mgag200_fb_create()

drm/mgag200: Fix logic in mgag200_bo_pin()
drm/mgag200: Make local function mgag200_gem_init_object() static drm/mgag200: Simplify function mgag200_bo_unpin() drm/mgag200: Add an crtc_disable callback to the crtc helper funcs drm/mgag200: Invalidate page tables when pinning a BO drm/mgag200: Initialize data needed to map fbdev memory drm/mgag200: Add sysfs support for connectors drm/mgag200: Fix LUT programming for 16bpp
drm/mgag200: Reject modes when h-parameters are no multiple of 8 drm/mgag200: Add doublescan and interlace support

Takashi Iwai (1):
drm/mgag200: Fix framebuffer pitch calculation

 drivers/gpu/drm/mgag200/mgag200_fb.c   | 27 +++++++++++----
 drivers/gpu/drm/mgag200/mgag200_main.c |  7 ++--
 drivers/gpu/drm/mgag200/mgag200_mode.c | 60 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/mgag200/mgag200_ttm.c  | 42 +++++++++++-------------
 4 files changed, 100 insertions(+), 36 deletions(-)

--
1.8.1.4

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

author
#27 lore raw

Hi Dave,

Quoted 11 lines · Dave Airlie
Quoted 2 lines · Egbert Eich

This set of patches contains several fixes and enhancements for the MGAG200 KMS driver.

Hi Egbert,

I've pushed some of these into a branch in my repo, mgag200-queue,

I'll try and review the rest soon, I'll probably split these up into obvious fixes for Linus, and leave the cleanups/features for -next if they aren't fixing any actual problems.

Thanks for looking into this so quickly!
Julia from Matrox also told me that she's
also going to look over the patches.

I'm going to look into
drm/mgag200: Initialize data needed to map fbdev memory
again.
You may already have seen that
drm/mgag200: Don't unreference when handle creation failed
should be dropped.

Thanks!
Egbert.