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

[David Herrmann]

Hi Marc

On Fri, Jun 21, 2013 at 2:17 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 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.
>>
> 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...

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.

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

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.

Cheers
David

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