<div class="gmail_quote">2013/6/21 David Herrmann <<a href="mailto:dh.herrmann@gmail.com" target="_blank">dh.herrmann@gmail.com</a>> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Hi Marc <div><div class="h5"> On Fri, Jun 21, 2013 at 2:17 PM, Marc Chalain <<a href="mailto:marc.chalain@gmail.com">marc.chalain@gmail.com</a>> wrote: > > > 2013/6/21 David Herrmann <<a href="mailto:dh.herrmann@gmail.com">dh.herrmann@gmail.com</a>> >> >> Hi >> >> On Fri, Jun 21, 2013 at 12:05 PM, Marc Chalain <<a href="mailto:marc.chalain@gmail.com">marc.chalain@gmail.com</a>> >> wrote: >> > >> > >> > 2013/6/21 David Herrmann <<a href="mailto:dh.herrmann@gmail.com">dh.herrmann@gmail.com</a>> >> >> >> >> Hi >> >> >> >> On Fri, Jun 21, 2013 at 11:43 AM, Marc Chalain <<a href="mailto:marc.chalain@gmail.com">marc.chalain@gmail.com</a>> >> >> wrote: >> >> > >> >> > 2013/6/21 David Herrmann <<a href="mailto:dh.herrmann@gmail.com">dh.herrmann@gmail.com</a>> >> >> >> >> >> >> Hi >> >> >> >> >> >> On Fri, Jun 21, 2013 at 10:49 AM, mchalain [<a href="mailto:marc.chalain@gmail.com">marc.chalain@gmail.com</a>] >> >> >> <<a href="mailto:marc.chalain@gmail.com">marc.chalain@gmail.com</a>> wrote: >> >> >> > From: mchalain <<a href="mailto:marc.chalain@gmail.com">marc.chalain@gmail.com</a>> >> >> >> > >> >> >> > it initializes varinfo.yoffset. varinfo.yoffset has to >> >> >> > point on the beginning of the video memory. >> >> >> > The card uses this value to push on the screen a part of >> >> >> > the video memory when this one is larger than the screen. >> >> >> > >> >> >> > --- >> >> >> > weston/src/compositor-fbdev.c | 6 ++++++ >> >> >> > 1 file changed, 6 insertions(+) >> >> >> > >> >> >> > diff --git a/weston/src/compositor-fbdev.c >> >> >> > b/weston/src/compositor-fbdev.c >> >> >> > index adfb67a..d2aee9b 100644 >> >> >> > --- a/weston/src/compositor-fbdev.c >> >> >> > +++ b/weston/src/compositor-fbdev.c >> >> >> > @@ -368,6 +368,11 @@ fbdev_query_screen_info(struct fbdev_output >> >> >> > *output, int fd, >> >> >> > return -1; >> >> >> > } >> >> >> > >> >> >> > + if (varinfo.yoffset != 0) { >> >> >> > + varinfo.yoffset = 0; >> >> >> > + if (ioctl(fd, FBIOPAN_DISPLAY, &varinfo) < 0) >> >> >> > + return -1; >> >> >> > + } >> >> >> >> >> >> Why do you need this? It's unnecessary. We call FBIOPUT_VSCREENINFO >> >> >> after fbdev_query_screen_info(), anyway. Furthermore, not all >> >> >> drivers >> >> >> support panning even though the yoffset may be non-zero (you need >> >> >> FBIOPUT_VSCREENINFO then). >> >> >> >> >> >> I think you can just drop this here but keep the yoffset=0 below. >> >> >> >> >> > >> >> > where do you use FBIOPUT_VSCREENINFO ? I only find inside >> >> > fbdev_set_screen_info but this function is called only when the >> >> > output >> >> > is >> >> > reenable not at the startup. >> >> >> >> My bad! >> >> But still, you now call FBIOPAN_DISPLAY on _every_ >> >> fbdev_query_screen_info() while it is only needed during initial setup >> >> as VT_ENTER calls fbdev_set_screen_info(), anyway. So instead, I'd >> >> recommend to just call fbdev_set_screen_info() during initial setup as >> >> well. >> >> >> > >> > No I call FBIOPAN_DISPLAY only at startup after yoffset is set to 0 >> >> You call it everytime yoffset isn't 0. So if you VT_LEAVE, another >> application changes the yoffset for whatever reason and we VT_ENTER, >> you will end up calling FBIOPAN_DISPLAY again followed by >> fbdev_set_screen_info() if the screen-info changed. >> >> Don't expect applications on other VTs to behave as expected. They may >> do anything! They may even use some acceleration which we in weston >> aren't aware of. >> > If another application set yoffset, with VT_ENTER we pass inside > fbdev_set_screen_info > and yoffset is set to 0. > After fbdev_set_screen_info is buggy and this patch is not a bugfix... > the bug is you set varinfo.bits_per_pixel with a value that could be > different of 32 and > after you set the colors as A8R8G8B8 ... > I don't want to use it, because I can't check all cases of this function, my > card is too simple... </div></div>Sorry, I cannot follow. It's the following scenario that I think can be handled better: WESTON-START => fbdev initialization => FBIOPAN_DISPLAY VT_LEAVE (some other app changes the fbdev state and offsets) VT_ENTER => fbdev_output_reenable() => fbdev_frame_buffer_open() => fbdev_query_screen_info() => FBIOPAN_DISPLAY => fbdev_set_screen_info() => FBIOPUT_VSCREENINFO So after a VT_ENTER, we call FBIOPAN_DISPLAY even though we reset the screeninfo later (because it changed). So why not modify "compare_screen_info" to return 1 if the offsets are not 0? This avoids any additional call to FBIOPAN_DISPLAY. As you noticed before, this would require fbdev_output_create to call fbdev_set_screen_info() as we do not reset the offsets during fbdev output setup. <div class="im"> >> >> > It seems strange that FBIOPAN_DISPLAY is not supported by some >> >> > devices. >> >> > I >> >> > used this one to not set all the varinfo on the device and to be >> >> > faster. >> >> >> >> See ./drivers/video/fbmem.c fb_pan_display(). Trivial fbdev drivers >> >> might not support it, but user-space can still set the offset via >> >> FBIOPUT_VSCREENINFO. If you want, you can call FBIOPAN_DISPLAY and if >> >> it fails, you use FBIOPUT_VSCREENINFO. >> >> >> > I read the yoffset hasn't to be null in FB_VMODE_YWRAP . But yoffset is >> > only used for panning. >> > It's useless to set it if it doesn't use (fbmem.c uses yoffset only for >> > panning in your example too) >> >> A driver may not support FBIOPAN_DISPLAY but still support panning via >> FBIOPUT_VSCREENINFO. If any framebuffer parameter change requires a >> full modeset, there is no reason to support FBIOPAN_DISPLAY. But >> panning may still be supported via yoffset and FBIOPUT_VSCREENINFO. >> > You tell that kernel bug must be corrected by kernel developers in the > thread about SYN_DROPPED > and now you tell me that we have to check that the driver should not be > completed ??? </div>It's not a kernel bug. Every driver is free to implement FBIOPAN_DISPLAY. But a driver is also free to not implement it but still support panning via a full modeset-cicle (FBIOPUT_VSCREENINFO). Also note that fbdev drivers (due to their inherently broken design) tend to allow all kinds of driver-dependent acceleration ioctls which may cause _any_ kind of state-change in the driver that may be reflected through weird screen-states. Hence, it's always good to perform a full modeset on initialization and VT_ENTER. </blockquote><div>I agree with you, but the current fb_set_screeninfo is broken. It runs fine only for few card, and set the registers with constant value without check the result after is not a good idea. Normally we should do STARTUP get fixinfo get varinfo set varinfo with configuration get varinfo to retrieve the values used store varinfo some where VT_LEAVE some body change varinfo VT_ENTER restore varinfo I just want to give you a bugfix about yoffset, that I did to continue my job. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Cheers David <div class="HOEnZb"><div class="h5"> > I don't understand the need to add useless code. I will change > FBIOPAN_DISPLAY by > FBIOPUT_VSCREENINFO but it's only to make you happy. > >> >> Cheers >> David > > </div></div></blockquote></div>