[PATCH 00/16] drm/mgag200: Fixes and enhancements for the MGAG200 KMS driver

[PATCH 00/16] drm/mgag200: Fixes and enhancements for the MGAG200 KMS driver

[Egbert Eich]

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

-- 
1.8.1.4

[PATCH 01/16] drm/mgag200: Fix memleaks in error path in mgag200_fb_create()

[Egbert Eich]

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 <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 964f58c..6453e4c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -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;
 }
 
-- 
1.8.1.4

[PATCH 02/16] drm/mgag200: Fix memleak in error path in mgag200_bo_create()

[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 <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 3acb2b0..6461fd2 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -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)
-- 
1.8.1.4

[PATCH 03/16] drm/mgag200: Free container instead of member in mga_user_framebuffer_destroy()

[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 <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 9fa5685..43ebf8d 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -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 = {
-- 
1.8.1.4

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

[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 <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index 43ebf8d..d51096c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -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;
 }
-- 
1.8.1.4

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

[Chris Wilson]

On Wed, Jul 17, 2013 at 03:07:17PM +0200, Egbert Eich wrote:
> drm_gem_handle_create() should not have referenced an object when it's
> creation has failed for some reason.
> 
> Signed-off-by: Egbert Eich <eich@suse.com>

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Egbert Eich]

Chris Wilson writes:
 > On Wed, Jul 17, 2013 at 03:07:17PM +0200, Egbert Eich wrote:
 > > drm_gem_handle_create() should not have referenced an object when it's
 > > creation has failed for some reason.
 > > 
 > > Signed-off-by: Egbert Eich <eich@suse.com>
 > 
 > 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.

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

[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 <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 6453e4c..371f3eb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -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;
 
-- 
1.8.1.4

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

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 22 +++++++++-------------
 1 file changed, 9 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 6461fd2..2606031 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -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

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

[Dave Airlie]

On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
> Signed-off-by: Egbert Eich <eich@suse.de>

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

Dave.

> ---
>  drivers/gpu/drm/mgag200/mgag200_ttm.c | 22 +++++++++-------------
>  1 file changed, 9 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
> index 6461fd2..2606031 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
> @@ -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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Egbert Eich]

Add missing 'return 0;'.

v2: Simplified patch as suggested by Dave Airlie <airlied@redhat.com>

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 6461fd2..1e8746c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -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);
-- 
1.8.1.4

[PATCH 07/16] drm/mgag200: Make local function mgag200_gem_init_object() static

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_main.c b/drivers/gpu/drm/mgag200/mgag200_main.c
index d51096c..fe8ed66 100644
--- a/drivers/gpu/drm/mgag200/mgag200_main.c
+++ b/drivers/gpu/drm/mgag200/mgag200_main.c
@@ -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;
 
-- 
1.8.1.4

[PATCH 08/16] drm/mgag200: Simplify function mgag200_bo_unpin()

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 2606031..3a2e5e2 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -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)
-- 
1.8.1.4

[PATCH 09/16] drm/mgag200: Add an crtc_disable callback to the crtc helper funcs

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 251784a..2fe1f64 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -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,
-- 
1.8.1.4

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

[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 <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 3a2e5e2..8ceeb0c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -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);
-- 
1.8.1.4

[PATCH V2] drm/mgag200: Invalidate page tables when pinning a BO

[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.
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 <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_ttm.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ttm.c b/drivers/gpu/drm/mgag200/mgag200_ttm.c
index 3a2e5e2..6aa8fad 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ttm.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ttm.c
@@ -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);
 
-- 
1.8.1.4

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

[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 <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_fb.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c
index 371f3eb..94c64de 100644
--- a/drivers/gpu/drm/mgag200/mgag200_fb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_fb.c
@@ -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,
-- 
1.8.1.4

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

[Dave Airlie]

On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
> 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.

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

[Egbert Eich]

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 2fe1f64..020a623 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -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");
-- 
1.8.1.4

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

[Egbert Eich]

From: Takashi Iwai <tiwai@suse.com>

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

Signed-off-by: Egbert Eich <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 020a623..c8983f9 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -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);
 
-- 
1.8.1.4

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

[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 <eich@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index c8983f9..503a414 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -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]);
-- 
1.8.1.4

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

[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 <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 503a414..0bb0e1e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -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)
-- 
1.8.1.4

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

[Dave Airlie]

On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
> 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 <eich@suse.com>

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

Dave.

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

[Adam Jackson]

On Tue, 2013-07-23 at 15:46 +1000, Dave Airlie wrote:
> On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
> > 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 <eich@suse.com>
> 
> 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

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

[Daniel Vetter]

On Tue, Jul 23, 2013 at 5:30 PM, Adam Jackson <ajax@redhat.com> wrote:
> On Tue, 2013-07-23 at 15:46 +1000, Dave Airlie wrote:
>> On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
>> > 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 <eich@suse.com>
>>
>> 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

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

[Egbert Eich]

This code was ported from the xorg mga driver.

Signed-off-by: Egbert Eich <eich@suse.com>
---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index 0bb0e1e..3faacbe 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -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);
 
-- 
1.8.1.4

Re: [PATCH 00/16] drm/mgag200: Fixes and enhancements for the MGAG200 KMS driver

[Dave Airlie]

On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
> 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.

>
> 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
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/16] drm/mgag200: Fixes and enhancements for the MGAG200 KMS driver

[Egbert Eich]

Hi Dave,

Dave Airlie writes:
 > On Wed, Jul 17, 2013 at 11:07 PM, Egbert Eich <eich@suse.com> wrote:
 > > 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.