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

[Marc Chalain]

2013/6/21 David Herrmann <dh.herrmann at gmail.com>

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


> 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
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20130622/2e6bad49/attachment-0001.html>