2009-11-02 07:40:12

by Clemens Ladisch

[permalink] [raw]
Subject: [PATCH 0/3] 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(-)


2009-11-02 07:41:11

by Clemens Ladisch

[permalink] [raw]
Subject: [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]>

--- 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 {

2009-11-02 07:44:45

by Clemens Ladisch

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

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]>

--- 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);
}

2009-11-02 07:42:38

by Clemens Ladisch

[permalink] [raw]
Subject: [PATCH 3/3] drm/i915: allocate framebuffer cmap

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.

--- 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);
}

2009-11-02 14:34:01

by James Simmons

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



> 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.
>
> --- 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.

2009-11-03 12:17:56

by Clemens Ladisch

[permalink] [raw]
Subject: [PATCH v2 0/3] DRM/KMS framebuffer fixes

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.

2009-11-03 12:18:54

by Clemens Ladisch

[permalink] [raw]
Subject: [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(-)

--- 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 {

2009-11-03 12:19:30

by Clemens Ladisch

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

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

--- 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 */

2009-11-03 12:20:14

by Clemens Ladisch

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

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

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

2009-11-03 17:19:14

by James Simmons

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


> 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(+)
>
> --- 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
> [email protected]
> https://lists.sourceforge.net/lists/listinfo/dri-devel
>

2009-11-04 08:40:35

by Clemens Ladisch

[permalink] [raw]
Subject: [PATCH v3 0/3] DRM/KMS framebuffer fixes

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

2009-11-04 08:42:48

by Clemens Ladisch

[permalink] [raw]
Subject: [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(-)

--- 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 {

2009-11-04 08:42:54

by Clemens Ladisch

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

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

--- 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 */

2009-11-04 08:43:00

by Clemens Ladisch

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

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

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