[PATCH 05/18] fbdev: initialize yoffset of varinfo

[David Herrmann]

Hi

On Fri, Jun 21, 2013 at 12:05 PM, Marc Chalain <marc.chalain at gmail.com> wrote:
>
>
> 2013/6/21 David Herrmann <dh.herrmann at gmail.com>
>>
>> Hi
>>
>> On Fri, Jun 21, 2013 at 11:43 AM, Marc Chalain <marc.chalain at gmail.com>
>> wrote:
>> >
>> > 2013/6/21 David Herrmann <dh.herrmann at gmail.com>
>> >>
>> >> Hi
>> >>
>> >> On Fri, Jun 21, 2013 at 10:49 AM, mchalain [marc.chalain at gmail.com]
>> >> <marc.chalain at gmail.com> wrote:
>> >> > From: mchalain <marc.chalain at gmail.com>
>> >> >
>> >> >  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.

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

Cheers
David