[PATCH 0/8] pxafb cleanup

[PATCH 0/8] pxafb cleanup

[eric miao]

The following series of patches try to address several issues of the
current PXA framebuffer driver. Some of these patches are already
submitted for review, here are more:

pxa: un-nest pxafb_parse_options() to cleanup the coding style issue
pxa: fix various coding style issues for pxafb
pxa: purge unnecessary pr_debug and comments from pxafb
pxa: sanitize the usage of #ifdef .. processing pxafb parameters
pxa: convert fb driver to use ioremap() and __raw_{readl, writel}
pxa: introduce "struct pxafb_dma_buff" for palette and dma descriptors
pxa: introduce register independent LCD connection type for pxafb
pxa: make lubbock/mainstone/zylonite/littleton to use new LCD connection type

Board maintainers are encouraged to use the new way for LCD connection
types.

To further clean-up the driver, I suggest to make the following changes:
1. removal of set_ctrlr_state(), the original code is written so because
the fb_blank() is called within interrupt context in 2.4 kernel, but this is
no longer the case in 2.6. And states can be simplified.

2. introduce a "vram" module parameter or boot option to indicate the
size of the video memory, so that frame buffer offset can be specified
within this video memory range, and PAN_DISPLAY can be done.
And the start offset of each overlay can also be specified.

3. a "pxafb_layer" structure describing each possible layers,
base, overlay1 and overlay2 for pxa27x can be described. From this
layer information, frame buffer device can be dynamically allocated
and registered. Palette manipulation, parameter checking and
layer activation code can be generalized.

-- 
Cheers
- eric

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 0/8] pxafb cleanup

[Alexandre Rusev]

eric miao wrote:
> The following series of patches try to address several issues of the
> current PXA framebuffer driver. Some of these patches are already
> submitted for review, here are more:
>
> pxa: un-nest pxafb_parse_options() to cleanup the coding style issue
> pxa: fix various coding style issues for pxafb
> pxa: purge unnecessary pr_debug and comments from pxafb
> pxa: sanitize the usage of #ifdef .. processing pxafb parameters
> pxa: convert fb driver to use ioremap() and __raw_{readl, writel}
> pxa: introduce "struct pxafb_dma_buff" for palette and dma descriptors
> pxa: introduce register independent LCD connection type for pxafb
> pxa: make lubbock/mainstone/zylonite/littleton to use new LCD connection type
>
> Board maintainers are encouraged to use the new way for LCD connection
> types.
>
> To further clean-up the driver, I suggest to make the following changes:
> 1. removal of set_ctrlr_state(), the original code is written so because
> the fb_blank() is called within interrupt context in 2.4 kernel, but this is
> no longer the case in 2.6. And states can be simplified.
>
> 2. introduce a "vram" module parameter or boot option to indicate the
> size of the video memory, so that frame buffer offset can be specified
> within this video memory range, and PAN_DISPLAY can be done.
> And the start offset of each overlay can also be specified.
>
> 3. a "pxafb_layer" structure describing each possible layers,
> base, overlay1 and overlay2 for pxa27x can be described. From this
>   
But what about support of DirectFB applications?
DirectFB as we know needs framebuffer memory to be allocated and mapped
only once
taking in account  the greatest values of colordeepth and resolution.

Furthermore for support of overlays DirectFB needs a capability to map
all overlays and baseplane
to be mapped continuously in memory (taking in account greatest bpp and
resolution values again)

I'd prefer to see DirectFB fixed against this, yet the fulfillment of
these  a bit strange rules
looks like to be the only way to make it possible the DirectFB to work
with pxafb driver :(
> layer information, frame buffer device can be dynamically allocated
> and registered. Palette manipulation, parameter checking and
> layer activation code can be generalized.
>
>   


-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 0/8] pxafb cleanup

[Alexandre Rusev]

eric miao wrote:
> The following series of patches try to address several issues of the
> current PXA framebuffer driver. Some of these patches are already
> submitted for review, here are more:
>
> pxa: un-nest pxafb_parse_options() to cleanup the coding style issue
> pxa: fix various coding style issues for pxafb
> pxa: purge unnecessary pr_debug and comments from pxafb
> pxa: sanitize the usage of #ifdef .. processing pxafb parameters
> pxa: convert fb driver to use ioremap() and __raw_{readl, writel}
> pxa: introduce "struct pxafb_dma_buff" for palette and dma descriptors
> pxa: introduce register independent LCD connection type for pxafb
> pxa: make lubbock/mainstone/zylonite/littleton to use new LCD connection type
>
> Board maintainers are encouraged to use the new way for LCD connection
> types.
>
> To further clean-up the driver, I suggest to make the following changes:
> 1. removal of set_ctrlr_state(), the original code is written so because
> the fb_blank() is called within interrupt context in 2.4 kernel, but this is
> no longer the case in 2.6. And states can be simplified.
>
> 2. introduce a "vram" module parameter or boot option to indicate the
> size of the video memory, so that frame buffer offset can be specified
> within this video memory range, and PAN_DISPLAY can be done.
> And the start offset of each overlay can also be specified.
>
> 3. a "pxafb_layer" structure describing each possible layers,
> base, overlay1 and overlay2 for pxa27x can be described. From this
> layer information, frame buffer device can be dynamically allocated
> and registered. Palette manipulation, parameter checking and
> layer activation code can be generalized.
>   
Some considerations on dynamic allocation of framebuffer/overlys memory
(if applicable).
Early implementation of framebuffer/overlays for pxa270 contained the
following bug:
when resolution of framebuffer/overlay or bpp  was changed and that
framebuffer/overlay
(or some part of it) was mapped by userspace application the driver just
invoke kfree/dma_free_writecombine,
with the memory area still leaving mapped to userspace!
This could cause the corruption of kernel memory.
(for example DirectFB just tried to draw at the previously mapped area
so no more new picture appeared at screen;) )

When we do NOTallocate maximal possible sized piece of memory for
baseplane and overlay in advance
this problem seems still to be an issue :(
I suppose that driver-specific vm_area_ops to be attached by driver to
each mapping did by userland
due to avoid kernel memory corruption at unmap.

Yet the problem is that if userland applicatein frequently use/unuse and
change resolution/bpp of overlay
and forget to unmap it each time, then all consistent/writecombine
memory would be locked by this application.


Allocating maximal possible size (and not using it) is quite adversary
against writecombine/consistent memory pool.
With current definition of CONSISTENT_SIZE for architecture it would be
even impossible to allocate
all the needed memory for all overlays! :(

















-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 0/8] pxafb cleanup

[Alexandre Rusev]

eric miao wrote:
> On Wed, Feb 27, 2008 at 4:58 PM, Alexandre Rusev <arusev@ru.mvista.com> wrote:
>   
>> eric miao wrote:
>> ...
>>
>>  I'd prefer to see DirectFB fixed against this, yet the fulfillment of
>>  these  a bit strange rules
>>  looks like to be the only way to make it possible the DirectFB to work
>>  with pxafb driver :(
>>
>>     
>
> Actually, I don't quite like what the current overlay driver is doing.
>   
I agree at this point!
> I'd prefer some frame buffer APIs so the upper application can
> enable/disable and adjust the offset of overlay within the whole
> video memory. E.g.
>
> Instead of
>     overlay1_fd = open("/dev/fb1", O_RDWR);
>     mmap(overlay1_fd, ...)
>     ....
>
> I'd prefer:
>
>     fd = open("/dev/fb0", O_RDWR);
>     video_mem = mmap(fd, ....)
>     /* the whole video memory is mapped, say 4MB */
>
>     offset = claim_overlay_memory_within( video_mem, overlay_mem_size )
>     ioctl(fd, FBIOSET_OVERLAY, offset)
>
> Or something like this, thus treating overlay as a sub-feature of the whole
> frame buffer driver, instead of an independent frame buffer device.
>
> But it is not impossible to create another frame buffer device for the
> overlay, you can still avoid multiple mapping by:
>
>     base_fd = open("/dev/fb0", O_RDWR);
>     video_mem = mmap(base_fd, ....)
>
>     overlay1_fd = open("/dev/fb1", O_RDWR);
>     ioctl(overlay1_fd, FBIOGET_VSCREENINFO, &var)
>
>     (x_off, y_off) = allocate_rect_area_within( video_mem, x_size, y_size );
>     var.x_offset = x_off;
>     var.y_offset = y_off;
>
>     ioctl(overlay1_fd, FBIOPUT_VSCREENINFO, &var);
>
> Which is better, I cannot say. Maybe it depends on upper application's
> usage. I don't see such kind of overlays being well supported either
> in DirectFB or X window server, :(
>   
The second approach is more universal (I think), but really NOT supported by
userland, especially DirectFB.

Having /dev/bfN for each overlay makes possible to use overlays with
not-overlay-aware
applications and that's a big advantage from point of view of applied
programming.

Yet I also can't for sure which approach is better now...

I think that changing DirectFB would be nice solution of the problem 
(yet it's quite beyond my control ;) )
It's model of using overlays is quite restrictive and I don't see the
reason why it doing so.
Imean not only assumption that base ans overlays are mapped consistently
and only once but
also the assumption that multi-buffering is implemented through panning
feature only (without no abstraction layer).









-------------------------------------------------------------------
List admin: http://lists.arm.linux.org.uk/mailman/listinfo/linux-arm-kernel
FAQ:        http://www.arm.linux.org.uk/mailinglists/faq.php
Etiquette:  http://www.arm.linux.org.uk/mailinglists/etiquette.php

Re: [PATCH 0/8] pxafb cleanup

[eric miao]

On Wed, Feb 27, 2008 at 4:58 PM, Alexandre Rusev <arusev@ru.mvista.com> wrote:
> eric miao wrote:
>  > The following series of patches try to address several issues of the
>  > current PXA framebuffer driver. Some of these patches are already
>  > submitted for review, here are more:
>  >
>  > pxa: un-nest pxafb_parse_options() to cleanup the coding style issue
>  > pxa: fix various coding style issues for pxafb
>  > pxa: purge unnecessary pr_debug and comments from pxafb
>  > pxa: sanitize the usage of #ifdef .. processing pxafb parameters
>  > pxa: convert fb driver to use ioremap() and __raw_{readl, writel}
>  > pxa: introduce "struct pxafb_dma_buff" for palette and dma descriptors
>  > pxa: introduce register independent LCD connection type for pxafb
>  > pxa: make lubbock/mainstone/zylonite/littleton to use new LCD connection type
>  >
>  > Board maintainers are encouraged to use the new way for LCD connection
>  > types.
>  >
>  > To further clean-up the driver, I suggest to make the following changes:
>  > 1. removal of set_ctrlr_state(), the original code is written so because
>  > the fb_blank() is called within interrupt context in 2.4 kernel, but this is
>  > no longer the case in 2.6. And states can be simplified.
>  >
>  > 2. introduce a "vram" module parameter or boot option to indicate the
>  > size of the video memory, so that frame buffer offset can be specified
>  > within this video memory range, and PAN_DISPLAY can be done.
>  > And the start offset of each overlay can also be specified.
>  >
>  > 3. a "pxafb_layer" structure describing each possible layers,
>  > base, overlay1 and overlay2 for pxa27x can be described. From this
>  >
>  But what about support of DirectFB applications?
>  DirectFB as we know needs framebuffer memory to be allocated and mapped
>  only once
>  taking in account  the greatest values of colordeepth and resolution.
>
>  Furthermore for support of overlays DirectFB needs a capability to map
>  all overlays and baseplane
>  to be mapped continuously in memory (taking in account greatest bpp and
>  resolution values again)
>
>  I'd prefer to see DirectFB fixed against this, yet the fulfillment of
>  these  a bit strange rules
>  looks like to be the only way to make it possible the DirectFB to work
>  with pxafb driver :(
>

Actually, I don't quite like what the current overlay driver is doing.
I'd prefer some frame buffer APIs so the upper application can
enable/disable and adjust the offset of overlay within the whole
video memory. E.g.

Instead of
    overlay1_fd = open("/dev/fb1", O_RDWR);
    mmap(overlay1_fd, ...)
    ....

I'd prefer:

    fd = open("/dev/fb0", O_RDWR);
    video_mem = mmap(fd, ....)
    /* the whole video memory is mapped, say 4MB */

    offset = claim_overlay_memory_within( video_mem, overlay_mem_size )
    ioctl(fd, FBIOSET_OVERLAY, offset)

Or something like this, thus treating overlay as a sub-feature of the whole
frame buffer driver, instead of an independent frame buffer device.

But it is not impossible to create another frame buffer device for the
overlay, you can still avoid multiple mapping by:

    base_fd = open("/dev/fb0", O_RDWR);
    video_mem = mmap(base_fd, ....)

    overlay1_fd = open("/dev/fb1", O_RDWR);
    ioctl(overlay1_fd, FBIOGET_VSCREENINFO, &var)

    (x_off, y_off) = allocate_rect_area_within( video_mem, x_size, y_size );
    var.x_offset = x_off;
    var.y_offset = y_off;

    ioctl(overlay1_fd, FBIOPUT_VSCREENINFO, &var);

Which is better, I cannot say. Maybe it depends on upper application's
usage. I don't see such kind of overlays being well supported either
in DirectFB or X window server, :(


>
> > layer information, frame buffer device can be dynamically allocated
>  > and registered. Palette manipulation, parameter checking and
>  > layer activation code can be generalized.
>  >
>  >
>
>



-- 
Cheers
- eric

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/

Re: [PATCH 0/8] pxafb cleanup

[eric miao]

On Wed, Feb 27, 2008 at 5:25 PM, Alexandre Rusev <arusev@ru.mvista.com> wrote:
>
> eric miao wrote:
>  > The following series of patches try to address several issues of the
>  > current PXA framebuffer driver. Some of these patches are already
>  > submitted for review, here are more:
>  >
>  > pxa: un-nest pxafb_parse_options() to cleanup the coding style issue
>  > pxa: fix various coding style issues for pxafb
>  > pxa: purge unnecessary pr_debug and comments from pxafb
>  > pxa: sanitize the usage of #ifdef .. processing pxafb parameters
>  > pxa: convert fb driver to use ioremap() and __raw_{readl, writel}
>  > pxa: introduce "struct pxafb_dma_buff" for palette and dma descriptors
>  > pxa: introduce register independent LCD connection type for pxafb
>  > pxa: make lubbock/mainstone/zylonite/littleton to use new LCD connection type
>  >
>  > Board maintainers are encouraged to use the new way for LCD connection
>  > types.
>  >
>  > To further clean-up the driver, I suggest to make the following changes:
>  > 1. removal of set_ctrlr_state(), the original code is written so because
>  > the fb_blank() is called within interrupt context in 2.4 kernel, but this is
>  > no longer the case in 2.6. And states can be simplified.
>  >
>  > 2. introduce a "vram" module parameter or boot option to indicate the
>  > size of the video memory, so that frame buffer offset can be specified
>  > within this video memory range, and PAN_DISPLAY can be done.
>  > And the start offset of each overlay can also be specified.
>  >
>  > 3. a "pxafb_layer" structure describing each possible layers,
>  > base, overlay1 and overlay2 for pxa27x can be described. From this
>  > layer information, frame buffer device can be dynamically allocated
>  > and registered. Palette manipulation, parameter checking and
>  > layer activation code can be generalized.
>  >
>  Some considerations on dynamic allocation of framebuffer/overlys memory
>  (if applicable).
>  Early implementation of framebuffer/overlays for pxa270 contained the
>  following bug:
>  when resolution of framebuffer/overlay or bpp  was changed and that
>  framebuffer/overlay
>  (or some part of it) was mapped by userspace application the driver just
>  invoke kfree/dma_free_writecombine,
>  with the memory area still leaving mapped to userspace!
>  This could cause the corruption of kernel memory.
>  (for example DirectFB just tried to draw at the previously mapped area
>  so no more new picture appeared at screen;) )
>
>  When we do NOTallocate maximal possible sized piece of memory for
>  baseplane and overlay in advance
>  this problem seems still to be an issue :(
>  I suppose that driver-specific vm_area_ops to be attached by driver to
>  each mapping did by userland
>  due to avoid kernel memory corruption at unmap.
>

Userland app should really unmap and dereference the previously
mapped area for the overlay(s), but a fix in the driver is also necessary.

>  Yet the problem is that if userland applicatein frequently use/unuse and
>  change resolution/bpp of overlay
>  and forget to unmap it each time, then all consistent/writecombine
>  memory would be locked by this application.
>
>
>  Allocating maximal possible size (and not using it) is quite adversary
>  against writecombine/consistent memory pool.
>  With current definition of CONSISTENT_SIZE for architecture it would be
>  even impossible to allocate
>  all the needed memory for all overlays! :(
>
>

Not necessary true. We don't have to allocate the video memory from
consistent pool, which is mainly for things like DMA descriptor and
small FIFO areas. Normal memory can be allocated and reserved
for video usage.

>
>
>
>
>
>
>
>
>
>
>
>
>
>
>



-- 
Cheers
- eric

-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/