[PATCH 0/3] DRM/KMS framebuffer fixes

[PATCH 0/3] DRM/KMS framebuffer fixes

[Clemens Ladisch]

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

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

[Clemens Ladisch]

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 <clemens@ladisch.de>

--- linux-2.6/drivers/gpu/drm/drm_fb_helper.c
+++ linux-2.6/drivers/gpu/drm/drm_fb_helper.c
@@ -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 {

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

[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 <clemens@ladisch.de>

--- linux-2.6/drivers/gpu/drm/radeon/radeon_fb.c
+++ linux-2.6/drivers/gpu/drm/radeon/radeon_fb.c
@@ -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);
 	}
 

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

[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 <clemens@ladisch.de>
---
Untested.

--- linux-2.6/drivers/gpu/drm/i915/intel_fb.c
+++ linux-2.6/drivers/gpu/drm/i915/intel_fb.c
@@ -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);
 	}
 

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

[James Simmons]

> 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 <clemens@ladisch.de>
> ---
> Untested.
> 
> --- linux-2.6/drivers/gpu/drm/i915/intel_fb.c
> +++ linux-2.6/drivers/gpu/drm/i915/intel_fb.c
> @@ -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.

[PATCH v2 0/3] DRM/KMS framebuffer fixes

[Clemens Ladisch]

James Simmons wrote:
>> 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").
>> 
>> --- linux-2.6/drivers/gpu/drm/i915/intel_fb.c
>> +++ linux-2.6/drivers/gpu/drm/i915/intel_fb.c
>> @@ -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;
> 
> 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.

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

[Clemens Ladisch]

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 <clemens@ladisch.de>
---
No changes from v1.

 drivers/gpu/drm/drm_fb_helper.c |   6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-2.6/drivers/gpu/drm/drm_fb_helper.c
+++ linux-2.6/drivers/gpu/drm/drm_fb_helper.c
@@ -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 {

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

[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 <clemens@ladisch.de>
---
Needed for the next patch.

 include/drm/drm_crtc.h          |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6/include/drm/drm_crtc.h
+++ linux-2.6/include/drm/drm_crtc.h
@@ -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 */

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

[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 <clemens@ladisch.de>
---
v2: implemented suggestions by James Simmons

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

--- linux-2.6/drivers/gpu/drm/drm_fb_helper.c
+++ linux-2.6/drivers/gpu/drm/drm_fb_helper.c
@@ -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);
 

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

[James Simmons]

> 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 <clemens@ladisch.de>
> ---
> v2: implemented suggestions by James Simmons
> 
>  drivers/gpu/drm/drm_fb_helper.c |   4 ++++
>  1 file changed, 4 insertions(+)
> 
> --- linux-2.6/drivers/gpu/drm/drm_fb_helper.c
> +++ linux-2.6/drivers/gpu/drm/drm_fb_helper.c
> @@ -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);	
>  			return -EINVAL;
		}

Plug memory leak.


>  	} 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
> Dri-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/dri-devel
> 

[PATCH v3 0/3] DRM/KMS framebuffer fixes

[Clemens Ladisch]

James Simmons wrote:
> > @@ -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);	
> >  			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

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

[Clemens Ladisch]

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 <clemens@ladisch.de>
---
No changes from v1.

 drivers/gpu/drm/drm_fb_helper.c |   6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-2.6/drivers/gpu/drm/drm_fb_helper.c
+++ linux-2.6/drivers/gpu/drm/drm_fb_helper.c
@@ -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 {

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

[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 <clemens@ladisch.de>
---
Needed for the next patch; no changes from v2.

 include/drm/drm_crtc.h          |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- linux-2.6/include/drm/drm_crtc.h
+++ linux-2.6/include/drm/drm_crtc.h
@@ -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 */

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

[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 <clemens@ladisch.de>
---
v3: bugfix by James Simmons

 drivers/gpu/drm/drm_fb_helper.c |   7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

--- linux-2.6/drivers/gpu/drm/drm_fb_helper.c
+++ linux-2.6/drivers/gpu/drm/drm_fb_helper.c
@@ -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);