Skip to contentSkip to conversation

DRM/KMS framebuffer fixes

PATCH Superseded Clemens Ladisch 2 participants 14 messages Nov 2, 2009 – Nov 4, 2009

Subsystems (5)
DRM drivers DRM misc Intel i915 Radeon and amdgpu DRM drivers +1 more subsystems
lore
Share & tools
lore

Clemens Ladisch

author
#0 lore raw

cover DRM/KMS framebuffer fixes

These patches make the KMS framebuffer work with various programs like links, mplayer and X.

 drivers/gpu/drm/drm_fb_helper.c    |    6 +++---
 drivers/gpu/drm/i915/intel_fb.c    |    5 +++++
 drivers/gpu/drm/radeon/radeon_fb.c |    5 +++++
 3 files changed, 13 insertions(+), 3 deletions(-)
author
#1 lore raw

PATCH 1/3 drm/fb: fix FBIOGET/PUT_VSCREENINFO pixel clock handling

When the framebuffer driver does not publish detailed timing information for the current video mode, the correct value for the pixclock field is zero, not -1.

Since pixclock is actually unsigned, the value -1 would be interpreted as 4294967295 picoseconds (i.e., about 4 milliseconds) by register_framebuffer() and userspace programs.

This patch allows X.org's fbdev driver to work.

Signed-off-by Clemens Ladisch <[email protected]>
drivers/gpu/drm/drm_fb_helper.c +3 -3
@@ -583,7 +583,7 @@ int drm_fb_helper_check_var(struct fb_va
 	struct drm_framebuffer *fb = fb_helper->fb;
 	int depth;
 
-	if (var->pixclock == -1 || !var->pixclock)
+	if (var->pixclock != 0)
 		return -EINVAL;
 
 	/* Need to resize the fb object !!! */

@@ -675,7 +675,7 @@ int drm_fb_helper_set_par(struct fb_info
 	int ret;
 	int i;
 
-	if (var->pixclock != -1) {
+	if (var->pixclock != 0) {
 		DRM_ERROR("PIXEL CLCOK SET\n");
 		return -EINVAL;
 	}

@@ -888,7 +888,7 @@ int drm_fb_helper_single_fb_probe(struct
 	fb_helper->fb = fb;
 
 	if (new_fb) {
-		info->var.pixclock = -1;
+		info->var.pixclock = 0;
 		if (register_framebuffer(info) < 0)
 			return -EINVAL;
 	} else {
author
#2 lore raw

PATCH 2/3 drm/radeon/kms: allocate framebuffer cmap

In reply to Clemens Ladisch

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

Signed-off-by Clemens Ladisch <[email protected]>
drivers/gpu/drm/radeon/radeon_fb.c +5 -0
@@ -242,6 +242,10 @@ int radeonfb_create(struct drm_device *d
 		goto out_unref;
 	}
 
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret)
+		goto out_unref;
+
 	memset_io(fbptr, 0, aligned_size);
 
 	strcpy(info->fix.id, "radeondrmfb");

@@ -341,6 +345,7 @@ int radeonfb_remove(struct drm_device *d
 		radeon_object_kunmap(robj);
 		radeon_object_unpin(robj);
 		drm_fb_helper_free(&rfbdev->helper);
+		fb_dealloc_cmap(&info->cmap);
 		framebuffer_release(info);
 	}
author
#3 lore raw

PATCH 3/3 drm/i915: allocate framebuffer cmap

In reply to Clemens Ladisch

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

Signed-off-by Clemens Ladisch <[email protected]>

---
Untested.

drivers/gpu/drm/i915/intel_fb.c +5 -0
@@ -227,6 +227,10 @@ static int intelfb_create(struct drm_dev
 
 	fb->fbdev = info;
 
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret)
+		goto out_unpin;
+
 	par->intel_fb = intel_fb;
 
 	/* To allow resizeing without swapping buffers */

@@ -270,6 +274,7 @@ int intelfb_remove(struct drm_device *de
 		iounmap(info->screen_base);
 		if (info->par)
 			drm_fb_helper_free(&par->helper);
+		fb_dealloc_cmap(&info->cmap);
 		framebuffer_release(info);
 	}
#4 lore raw

Re: [PATCH 3/3] drm/i915: allocate framebuffer cmap

Quoted 13 lines

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

Signed-off-by Clemens Ladisch <[email protected]>

---
Untested.

drivers/gpu/drm/i915/intel_fb.c static int intelfb_create(struct drm_dev · line 236

Quoted 10 lines
drivers/gpu/drm/i915/intel_fb.c +4 -0
@@ -227,6 +227,10 @@ static int intelfb_create(struct drm_dev
 
 	fb->fbdev = info;
 
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret)
+		goto out_unpin;
+
 	par->intel_fb = intel_fb;
 
 	/* To allow resizeing without swapping buffers */

It would be better to place that code in drm_fb_helper_single_fb_probe. Also instead of 256 I would recommend using crtc->gamma_size.

author
#5 lore raw

v2 cover DRM/KMS framebuffer fixes

Quoted 10 lines · James Simmons
Quoted 10 lines

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

drivers/gpu/drm/i915/intel_fb.c +0 -0
@@ -227,6 +227,10 @@ static int intelfb_create(struct drm_dev
 
 	fb->fbdev = info;
Quoted 7 lines

drivers/gpu/drm/i915/intel_fb.c static int intelfb_create(struct drm_dev · line 234

Quoted 5 lines
drivers/gpu/drm/i915/intel_fb.c +4 -0
+	ret = fb_alloc_cmap(&info->cmap, 256, 0);
+	if (ret)
+		goto out_unpin;
+
 	par->intel_fb = intel_fb;

It would be better to place that code in drm_fb_helper_single_fb_probe.

Thanks, I wasn't quite able to untangle the indirections between the four framebuffer structures.

New patch set follows.

author
#6 lore raw

PATCH v2 1/3 drm/fb: fix FBIOGET/PUT_VSCREENINFO pixel clock handling

When the framebuffer driver does not publish detailed timing information for the current video mode, the correct value for the pixclock field is zero, not -1.

Since pixclock is actually unsigned, the value -1 would be interpreted as 4294967295 picoseconds (i.e., about 4 milliseconds) by register_framebuffer() and userspace programs.

This patch allows X.org's fbdev driver to work.

Signed-off-by Clemens Ladisch <[email protected]>

---
No changes from v1.

 drivers/gpu/drm/drm_fb_helper.c |   6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +3 -3
@@ -599,7 +599,7 @@ int drm_fb_helper_check_var(struct fb_va
 	struct drm_framebuffer *fb = fb_helper->fb;
 	int depth;
 
-	if (var->pixclock == -1 || !var->pixclock)
+	if (var->pixclock != 0)
 		return -EINVAL;
 
 	/* Need to resize the fb object !!! */

@@ -691,7 +691,7 @@ int drm_fb_helper_set_par(struct fb_info
 	int ret;
 	int i;
 
-	if (var->pixclock != -1) {
+	if (var->pixclock != 0) {
 		DRM_ERROR("PIXEL CLCOK SET\n");
 		return -EINVAL;
 	}

@@ -904,7 +904,7 @@ int drm_fb_helper_single_fb_probe(struct
 	fb_helper->fb = fb;
 
 	if (new_fb) {
-		info->var.pixclock = -1;
+		info->var.pixclock = 0;
 		if (register_framebuffer(info) < 0)
 			return -EINVAL;
 	} else {
author
#7 lore raw

PATCH v2 2/3 drm: set the type of the drm_framebuffer::fbdev field

In reply to Clemens Ladisch

The fbdev field of the drm_framebuffer structure is always used to store a pointer to a fb_info, so there is no reason for it to be void*.

Signed-off-by Clemens Ladisch <[email protected]>

---
Needed for the next patch.

 include/drm/drm_crtc.h          |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/drm/drm_crtc.h +1 -1
@@ -256,7 +256,7 @@ struct drm_framebuffer {
 	unsigned int depth;
 	int bits_per_pixel;
 	int flags;
-	void *fbdev;
+	struct fb_info *fbdev;
 	u32 pseudo_palette[17];
 	struct list_head filp_head;
 	/* if you are using the helper */
author
#8 lore raw

PATCH v2 3/3 drm/kms: allocate framebuffer cmap

In reply to Clemens Ladisch

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

Signed-off-by Clemens Ladisch <[email protected]>

---
v2: implemented suggestions by James Simmons

 drivers/gpu/drm/drm_fb_helper.c |   4 ++++
 1 file changed, 4 insertions(+)
drivers/gpu/drm/drm_fb_helper.c +4 -0
@@ -905,6 +905,9 @@ int drm_fb_helper_single_fb_probe(struct
 
 	if (new_fb) {
 		info->var.pixclock = 0;
+		ret = fb_alloc_cmap(&info->cmap, crtc->gamma_size, 0);
+		if (ret)
+			return ret;
 		if (register_framebuffer(info) < 0)
 			return -EINVAL;
 	} else {

@@ -936,6 +939,7 @@ void drm_fb_helper_free(struct drm_fb_he
 		unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 	drm_fb_helper_crtc_free(helper);
+	fb_dealloc_cmap(&helper->fb->fbdev->cmap);
 }
 EXPORT_SYMBOL(drm_fb_helper_free);
#9 lore raw

Re: [PATCH v2 3/3] drm/kms: allocate framebuffer cmap

Quoted 15 lines

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

Signed-off-by Clemens Ladisch <[email protected]>

---
v2: implemented suggestions by James Simmons

 drivers/gpu/drm/drm_fb_helper.c |   4 ++++
 1 file changed, 4 insertions(+)

drivers/gpu/drm/drm_fb_helper.c int drm_fb_helper_single_fb_probe(struct · line 913

Quoted 9 lines
drivers/gpu/drm/drm_fb_helper.c +3 -0
@@ -905,6 +905,9 @@ int drm_fb_helper_single_fb_probe(struct
 
 	if (new_fb) {
 		info->var.pixclock = 0;
+		ret = fb_alloc_cmap(&info->cmap, crtc->gamma_size, 0);
+		if (ret)
+			return ret;
 
 
 		if (register_framebuffer(info) < 0) {

			fb_dealloc_cmap(info->cmap);
Quoted 1 line

return -EINVAL;

		}

Plug memory leak.

Quoted 23 lines
drivers/gpu/drm/drm_fb_helper.c +1 -1
 	} else {

@@ -936,6 +939,7 @@ void drm_fb_helper_free(struct drm_fb_he
 		unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 	drm_fb_helper_crtc_free(helper);
+	fb_dealloc_cmap(&helper->fb->fbdev->cmap);
 }
 EXPORT_SYMBOL(drm_fb_helper_free);
 
 
 
------------------------------------------------------------------------------

Come build with us! The BlackBerry(R) Developer Conference in SF, CA is the only developer event you need to attend this year. Jumpstart your developing skills, take BlackBerry mobile applications to market and stay ahead of the curve. Join us from November 9 - 12, 2009. Register now!
http://p.sf.net/sfu/devconference
--
_______________________________________________
Dri-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/dri-devel

author
#10 lore raw

v3 cover DRM/KMS framebuffer fixes

Quoted 8 lines · James Simmons
Quoted 7 lines
drivers/gpu/drm/drm_fb_helper.c +3 -0
@@ -905,6 +905,9 @@ int drm_fb_helper_single_fb_probe(struct
 
 	if (new_fb) {
 		info->var.pixclock = 0;
+		ret = fb_alloc_cmap(&info->cmap, crtc->gamma_size, 0);
+		if (ret)
+			return ret;
Quoted 6 lines

Review int drm_fb_helper_single_fb_probe(struct · line 911

Quoted 1 line
(quoted diff) +0 -0
 		if (register_framebuffer(info) < 0) {

			fb_dealloc_cmap(info->cmap);

drivers/gpu/drm/drm_fb_helper.c int drm_fb_helper_single_fb_probe(struct · line 912

Quoted 1 line
drivers/gpu/drm/drm_fb_helper.c +0 -0
 			return -EINVAL;

		}

Plug memory leak.

Oops. Thanks again.
New patch set follows:

 include/drm/drm_crtc.h          |    2 +-
 drivers/gpu/drm/drm_fb_helper.c |   14 ++++++++++----
 2 files changed, 11 insertions(+), 5 deletions(-)

v2: incorporated suggestions by James Simmons
v3: bugfix by James Simmons

author
#11 lore raw

PATCH v3 1/3 drm/fb: fix FBIOGET/PUT_VSCREENINFO pixel clock handling

When the framebuffer driver does not publish detailed timing information for the current video mode, the correct value for the pixclock field is zero, not -1.

Since pixclock is actually unsigned, the value -1 would be interpreted as 4294967295 picoseconds (i.e., about 4 milliseconds) by register_framebuffer() and userspace programs.

This patch allows X.org's fbdev driver to work.

Signed-off-by Clemens Ladisch <[email protected]>

---
No changes from v1.

 drivers/gpu/drm/drm_fb_helper.c |   6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/gpu/drm/drm_fb_helper.c +3 -3
@@ -599,7 +599,7 @@ int drm_fb_helper_check_var(struct fb_va
 	struct drm_framebuffer *fb = fb_helper->fb;
 	int depth;
 
-	if (var->pixclock == -1 || !var->pixclock)
+	if (var->pixclock != 0)
 		return -EINVAL;
 
 	/* Need to resize the fb object !!! */

@@ -691,7 +691,7 @@ int drm_fb_helper_set_par(struct fb_info
 	int ret;
 	int i;
 
-	if (var->pixclock != -1) {
+	if (var->pixclock != 0) {
 		DRM_ERROR("PIXEL CLCOK SET\n");
 		return -EINVAL;
 	}

@@ -904,7 +904,7 @@ int drm_fb_helper_single_fb_probe(struct
 	fb_helper->fb = fb;
 
 	if (new_fb) {
-		info->var.pixclock = -1;
+		info->var.pixclock = 0;
 		if (register_framebuffer(info) < 0)
 			return -EINVAL;
 	} else {
author
#12 lore raw

PATCH v3 2/3 drm: set the type of the drm_framebuffer::fbdev field

In reply to Clemens Ladisch

The fbdev field of the drm_framebuffer structure is always used to store a pointer to a fb_info, so there is no reason for it to be void*.

Signed-off-by Clemens Ladisch <[email protected]>

---
Needed for the next patch; no changes from v2.

 include/drm/drm_crtc.h          |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/drm/drm_crtc.h +1 -1
@@ -256,7 +256,7 @@ struct drm_framebuffer {
 	unsigned int depth;
 	int bits_per_pixel;
 	int flags;
-	void *fbdev;
+	struct fb_info *fbdev;
 	u32 pseudo_palette[17];
 	struct list_head filp_head;
 	/* if you are using the helper */
author
#13 lore raw

PATCH v3 3/3 drm/kms: allocate framebuffer cmap

In reply to Clemens Ladisch

Without an allocated colormap, FBIOGETCMAP fails. This would make programs restore an all-black colormap ("links -g") or fail to work altogether ("mplayer -vo fbdev2").

Signed-off-by Clemens Ladisch <[email protected]>

---
v3: bugfix by James Simmons

 drivers/gpu/drm/drm_fb_helper.c |   7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/gpu/drm/drm_fb_helper.c +7 -1
@@ -905,8 +905,13 @@ int drm_fb_helper_single_fb_probe(struct
 
 	if (new_fb) {
 		info->var.pixclock = 0;
-		if (register_framebuffer(info) < 0)
+		ret = fb_alloc_cmap(&info->cmap, crtc->gamma_size, 0);
+		if (ret)
+			return ret;
+		if (register_framebuffer(info) < 0) {
+			fb_dealloc_cmap(&info->cmap);
 			return -EINVAL;
+		}
 	} else {
 		drm_fb_helper_set_par(info);
 	}

@@ -936,6 +941,7 @@ void drm_fb_helper_free(struct drm_fb_he
 		unregister_sysrq_key('v', &sysrq_drm_fb_helper_restore_op);
 	}
 	drm_fb_helper_crtc_free(helper);
+	fb_dealloc_cmap(&helper->fb->fbdev->cmap);
 }
 EXPORT_SYMBOL(drm_fb_helper_free);