Re: [PATCH 5/9] Add i.MX5 framebuffer driver

[Liu Ying]

2010/12/14 Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>:
> On Tue, Dec 14, 2010 at 02:40:09PM +0800, Liu Ying wrote:
>> Hello, Sascha,
>>
>> Please find my feedback with [LY] embedded.
>>
>> And, a bug related with this patch:
>> 1) Bootup the babbage board.
>> 2) echo U:640x480p-60 > /sys/class/graphics/fb0/mode
>> 3) cat /sys/class/graphics/fb0/virtual_size, it shows '640,480'.
>> 4) echo 640,960 > /sys/class/graphics/fb0/virtual_size, change virtual
>> yres to be 960.
>> 5) cat /sys/class/graphics/fb0/virtual_size, it still shows '640,480'.
>> I think this is caused by 'var->yres_virtual = var->yres;' in custom
>> fb_set_par() function(Sorry, I cannot make the comment inline this
>> time, as I don't find the original code within the mail I am
>> replying). This makes fb0 use only one block of memory whose size is
>> equal to screen size, so pan display can not really play her role and
>> screen tearing issue may happen.

Sascha,

Just would like to know if you've caught this bug.

>>
>> Best Regards,
>> Liu Ying
>>
>> 2010/12/13 Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>:
>> > On Sun, Dec 12, 2010 at 02:13:40PM +0800, Liu Ying wrote:
>> >> Hello, Sascha,
>> >>
>> >> I have following comments to this patch:
>> >> 1) Please modify the commit message, as IPUv3 is not embedded in i.MX50 SoC.
>> >
>> > ok.
>> >
>> >> 2) ADC is not supported yet in the framebuffer driver, so please
>> >> modify this comment:
>> >>    > + * Framebuffer Framebuffer Driver for SDC and ADC.
>> >
>> > ok.
>> >
>> >> 3) 'ipu_dp_set_window_pos()' is called only once in
>> >> imx_ipu_fb_set_par_overlay(). So, the framebuffer driver doesn't
>> >> support to change the overlay framebuffer position. Need a
>> >> mechanism/interface for users to change the overlay framebuffer
>> >> position.
>> >
>> > Yes. The overlay support is currently only present in the driver because
>> > I didn't want to break it in general. There is no generic overlay API in
>> > the kernel and I didn't want to invent the n+1 API. Currently the
>> > possible use cases of the overlay support are not clear to me. Number
>> > one use case would probably be to display video output, but the
>> > corresponding driver would be a v4l2 driver, so in this case we would
>> > only need an in-kernel API.
>> > Anyway, I have not the resources to implement complete overlay support.
>> > So either we keep it like it is and it more has an example character
>> > or we remove it completely from the driver for now.
>> >
>> >> 4) Need to make sure the framebuffer on DP-FG is blanked before the
>> >> framebuffer on DP-BG is blanked. Meanwhile, the framebuffer on DP-FG
>> >> should be unblanked after the framebuffer on DP-BG is unblanked
>> >> 5) Need to check the framebuffer on DP-FG doesn't run out of the range
>> >> of the framebuffer on DP-BG.
>> >
>> > I tend to remove the overlay support.
>> >
>> >> 6) I prefer to find the video mode in modedb first, and if we cannot
>> >> find the video mode in common video mode data base, we can find a
>> >> video mode in custom video mode data base which is defined in platform
>> >> data. In this way, we don't need to export common modefb.
>> >
>> > I exported the modedb to be able to show the different modes under
>> > /sys/class/graphics/fb0/modes and to set it with
>> > /sys/class/graphics/fb0/mode. If you want this there is no way around
>> > exporting it. The ioctl interface for mode setting is independent of the
>> > specific modes anyway.
>> [LY]  Just a concern. If a platform choose to add the generic video
>> mode data base to IPUv3 framebuffer modelist, 'cat
>> /sys/class/graphics/fb0/modes' will show all the video modes in the
>> generic data base to users. I am not sure whether showing them to
>> users means the IPUv3 framebuffer driver acknowledges to support all
>> of them or not. I tried to do 'echo U:1800x1440p-70 >
>> /sys/class/graphics/fb0/mode' with the kernel on ipuv3 branch, there
>> will be a kernel dump(seems it can not allocate enough memory).
>
> We'll have to increase the dma coherent size (and max zone order) for
> these resolutions to work. I have patches for this in my local tree but
> decided to wait until some contiguous memory allocator hits the tree.
> Also, the fb driver should sanity check the generic modes before adding
> them to the list. FOr example the IPU can't do 1920x1200@60. This code
> is missing currently.
>
> Sascha
>
>
> --
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
>



-- 
Best Regards,
Liu Ying
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/