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

[Sascha Hauer <s.hauer@pengutronix.de>]

On Tue, Dec 14, 2010 at 09:23:30PM +0800, Liu Ying wrote:
> 2010/12/14 Sascha Hauer <s.hauer@pengutronix.de>:
> > 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.

Yes, it will be fixed in the next series. It was the framebuffer driver
forcing yres_virtual to yres in set_par.

Sascha

> 
> >>
> >> Best Regards,
> >> Liu Ying
> >>
> >> 2010/12/13 Sascha Hauer <s.hauer@pengutronix.de>:
> >> > 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
> 

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