[PATCH 2/5] xfree86: Remove xf86EnterServerState

Tiago Vignatti tiago.vignatti at nokia.com
Fri Dec 17 05:10:37 PST 2010


On Thu, Dec 16, 2010 at 03:31:37PM -0500, ext Adam Jackson wrote:
> Back when we had RAC this was a vaguely meaningful thing.  Since then
> it's been a glorified (and confusing) wrapper around xf86BlockSIGIO.
> 
> Note that the APM and VT switch code are unusual relative to other code
> that cares about SIGIO state.  Most callers push a SIGIO disable to
> create a critical section for the duration of the caller's stack frame,
> but those two effectively disable SIGIO after their return and re-enable
> on their next entry.
> 
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
>  hw/xfree86/common/xf86.h       |    1 -
>  hw/xfree86/common/xf86Bus.c    |   33 ---------------------------------
>  hw/xfree86/common/xf86Cursor.c |    6 +++---
>  hw/xfree86/common/xf86Events.c |   13 ++++---------
>  hw/xfree86/common/xf86Init.c   |   11 ++++++-----
>  hw/xfree86/common/xf86PM.c     |   20 ++++++++------------
>  hw/xfree86/common/xf86Priv.h   |    1 -
>  hw/xfree86/common/xf86str.h    |    7 -------
>  8 files changed, 21 insertions(+), 71 deletions(-)
> 
> diff --git a/hw/xfree86/common/xf86.h b/hw/xfree86/common/xf86.h
> index b29ec51..769466c 100644
> --- a/hw/xfree86/common/xf86.h
> +++ b/hw/xfree86/common/xf86.h
> @@ -136,7 +136,6 @@ extern _X_EXPORT EntityInfoPtr xf86GetEntityInfo(int entityIndex);
>  extern _X_EXPORT Bool xf86SetEntityFuncs(int entityIndex, EntityProc init,
>                         EntityProc enter, EntityProc leave, pointer);
>  extern _X_EXPORT Bool xf86IsEntityPrimary(int entityIndex);
> -extern _X_EXPORT void xf86EnterServerState(xf86State state);
>  extern _X_EXPORT ScrnInfoPtr xf86FindScreenForEntity(int entityIndex);
> 
>  extern _X_EXPORT int xf86GetLastScrnFlag(int entityIndex);
> diff --git a/hw/xfree86/common/xf86Bus.c b/hw/xfree86/common/xf86Bus.c
> index 623b130..3b08968 100644
> --- a/hw/xfree86/common/xf86Bus.c
> +++ b/hw/xfree86/common/xf86Bus.c
> @@ -497,32 +497,6 @@ xf86AccessLeave(void)
>  }
> 
>  /*
> - * xf86EnterServerState() -- set state the server is in.
> - */
> -
> -typedef enum { TRI_UNSET, TRI_TRUE, TRI_FALSE } TriState;
> -
> -void
> -xf86EnterServerState(xf86State state)
> -{
> -    static int sigio_state;
> -    static TriState sigio_blocked = TRI_UNSET;
> -
> -    /*
> -     * This is a good place to block SIGIO during SETUP state. SIGIO should be
> -     * blocked in SETUP state otherwise (u)sleep() might get interrupted
> -     * early. We take care not to call xf86BlockSIGIO() twice.
> -     */
> -    if ((state == SETUP) && (sigio_blocked != TRI_TRUE)) {
> -        sigio_state = xf86BlockSIGIO();
> -       sigio_blocked = TRI_TRUE;
> -    } else if ((state == OPERATING) && (sigio_blocked != TRI_UNSET)) {
> -        xf86UnblockSIGIO(sigio_state);
> -        sigio_blocked = TRI_FALSE;
> -    }
> -}
> -
> -/*
>   * xf86PostProbe() -- Allocate all non conflicting resources
>   * This function gets called by xf86Init().
>   */
> @@ -544,13 +518,6 @@ xf86PostProbe(void)
>             xf86Entities[i]->entityInit(i,xf86Entities[i]->private);
>  }
> 
> -void
> -xf86PostScreenInit(void)
> -{
> -    xf86VGAarbiterWrapFunctions();
> -    xf86EnterServerState(OPERATING);
> -}
> -
>  int
>  xf86GetLastScrnFlag(int entityIndex)
>  {
> diff --git a/hw/xfree86/common/xf86Cursor.c b/hw/xfree86/common/xf86Cursor.c
> index 0d27fd5..929f047 100644
> --- a/hw/xfree86/common/xf86Cursor.c
> +++ b/hw/xfree86/common/xf86Cursor.c
> @@ -202,7 +202,7 @@ xf86SwitchMode(ScreenPtr pScreen, DisplayModePtr mode)
>    ScrnInfoPtr pScr = XF86SCRNINFO(pScreen);
>    ScreenPtr   pCursorScreen;
>    Bool        Switched;
> -  int         px, py;
> +  int         px, py, was_blocked;
>    DeviceIntPtr dev, it;
> 
>    if (!pScr->vtSema || !mode || !pScr->SwitchMode)
> @@ -232,7 +232,7 @@ xf86SwitchMode(ScreenPtr pScreen, DisplayModePtr mode)
>    if (pScreen == pCursorScreen)
>      miPointerGetPosition(dev, &px, &py);
> 
> -  xf86EnterServerState(SETUP);
> +  was_blocked = xf86BlockSIGIO();
>    Switched = (*pScr->SwitchMode)(pScr->scrnIndex, mode, 0);
>    if (Switched) {
>      pScr->currentMode = mode;
> @@ -269,7 +269,7 @@ xf86SwitchMode(ScreenPtr pScreen, DisplayModePtr mode)
>        pScr->frameY1 = pScr->virtualY - 1;
>      }
>    }
> -  xf86EnterServerState(OPERATING);
> +  xf86UnblockSIGIO(was_blocked);

Apart from your modification, it's bit weird to have SwitchMode stuff inside
xf86Cursor. I guess you could move this somewhere else before apply your
changes.

 
>    if (pScr->AdjustFrame)
>      (*pScr->AdjustFrame)(pScr->scrnIndex, pScr->frameX0, pScr->frameY0, 0);
> diff --git a/hw/xfree86/common/xf86Events.c b/hw/xfree86/common/xf86Events.c
> index fdd908a..84c0d18 100644
> --- a/hw/xfree86/common/xf86Events.c
> +++ b/hw/xfree86/common/xf86Events.c
> @@ -414,7 +414,8 @@ xf86ReleaseKeys(DeviceIntPtr pDev)
>  static void
>  xf86VTSwitch(void)
>  {
> -  int i, prevSIGIO;
> +  int i;
> +  static int prevSIGIO;

AFAIU we don't need to use local static here. It will have the same behaviour.


>    InputInfoPtr pInfo;
>    IHPtr ih;
> 
> @@ -456,7 +457,8 @@ xf86VTSwitch(void)
>            DisableDevice(pInfo->dev, TRUE);
>        }
>      }
> -    xf86EnterServerState(SETUP);
> +
> +    prevSIGIO = xf86BlockSIGIO();
>      for (i = 0; i < xf86NumScreens; i++)
>         xf86Screens[i]->LeaveVT(i, 0);
> 
> @@ -468,14 +470,11 @@ xf86VTSwitch(void)
>         */
> 
>        DebugF("xf86VTSwitch: Leave failed\n");
> -      prevSIGIO = xf86BlockSIGIO();
>        xf86AccessEnter();
> -      xf86EnterServerState(SETUP);
>        for (i = 0; i < xf86NumScreens; i++) {
>         if (!xf86Screens[i]->EnterVT(i, 0))
>           FatalError("EnterVT failed for screen %d\n", i);
>        }
> -      xf86EnterServerState(OPERATING);
>        if (!(dispatchException & DE_TERMINATE)) {
>         for (i = 0; i < xf86NumScreens; i++) {
>           if (xf86Screens[i]->EnableDisableFBAccess)
> @@ -513,11 +512,9 @@ xf86VTSwitch(void)
>             xf86DisableIO();
>      }
>    } else {
> -
>      DebugF("xf86VTSwitch: Entering\n");
>      if (!xf86VTSwitchTo()) return;
> 
> -    prevSIGIO = xf86BlockSIGIO();
>  #ifdef XF86PM
>      xf86OSPMClose = xf86OSPMOpen();
>  #endif
> @@ -525,13 +522,11 @@ xf86VTSwitch(void)
>      if (xorgHWAccess)
>         xf86EnableIO();
>      xf86AccessEnter();
> -    xf86EnterServerState(SETUP);
>      for (i = 0; i < xf86NumScreens; i++) {
>        xf86Screens[i]->vtSema = TRUE;
>        if (!xf86Screens[i]->EnterVT(i, 0))
>           FatalError("EnterVT failed for screen %d\n", i);
>      }
> -    xf86EnterServerState(OPERATING);
>      for (i = 0; i < xf86NumScreens; i++) {
>        if (xf86Screens[i]->EnableDisableFBAccess)
>         (*xf86Screens[i]->EnableDisableFBAccess)(i, TRUE);
> diff --git a/hw/xfree86/common/xf86Init.c b/hw/xfree86/common/xf86Init.c
> index ef90fa5..c1596d6 100644
> --- a/hw/xfree86/common/xf86Init.c
> +++ b/hw/xfree86/common/xf86Init.c
> @@ -330,7 +330,7 @@ InstallSignalHandlers(void)
>  void
>  InitOutput(ScreenInfo *pScreenInfo, int argc, char **argv)
>  {
> -  int                    i, j, k, scr_index;
> +  int                    i, j, k, scr_index, was_blocked = 0;
>    char                   **modulelist;
>    pointer                *optionlist;
>    Pix24Flags            screenpix24, pix24;
> @@ -709,7 +709,7 @@ InitOutput(ScreenInfo *pScreenInfo, int argc, char **argv)
>        ioctl(xf86Info.consoleFd, VT_RELDISP, VT_ACKACQ);
>  #endif
>        xf86AccessEnter();
> -      xf86EnterServerState(SETUP);
> +      was_blocked = xf86BlockSIGIO();
>      }
>    }
>  #ifdef SCO325
> @@ -794,7 +794,8 @@ InitOutput(ScreenInfo *pScreenInfo, int argc, char **argv)
>  #endif
>    }
> 
> -  xf86PostScreenInit();
> +  xf86VGAarbiterWrapFunctions();
> +  xf86UnblockSIGIO(was_blocked);
> 
>    xf86InitOrigins();
> 
> @@ -943,6 +944,8 @@ AbortDDX(void)
>  {
>    int i;
> 
> +  xf86BlockSIGIO();
> +
>    /*
>     * try to restore the original video state
>     */
> @@ -951,8 +954,6 @@ AbortDDX(void)
>        DPMSSet(serverClient, DPMSModeOn);
>  #endif
>    if (xf86Screens) {
> -      if (xf86Screens[0]->vtSema)
> -         xf86EnterServerState(SETUP);
>        for (i = 0; i < xf86NumScreens; i++)
>           if (xf86Screens[i]->vtSema) {
>               /*
> diff --git a/hw/xfree86/common/xf86PM.c b/hw/xfree86/common/xf86PM.c
> index 7af89b5..7566b72 100644
> --- a/hw/xfree86/common/xf86PM.c
> +++ b/hw/xfree86/common/xf86PM.c
> @@ -33,6 +33,7 @@
>  #include "xf86.h"
>  #include "xf86Priv.h"
>  #include "xf86Xinput.h"
> +#include "xf86_OSproc.h"
> 
>  int (*xf86PMGetEventFromOs)(int fd,pmEvent *events,int num) = NULL;
>  pmWait (*xf86PMConfirmEventToOs)(int fd,pmEvent event) = NULL;
> @@ -61,6 +62,8 @@ eventName(pmEvent event, char **str)
>      }
>  }
> 
> +static int sigio_blocked_for_suspend;
> +
>  static void
>  suspend (pmEvent event, Bool undo)
>  {
> @@ -78,7 +81,7 @@ suspend (pmEvent event, Bool undo)
>         DisableDevice(pInfo->dev, TRUE);
>         pInfo = pInfo->next;
>      }
> -    xf86EnterServerState(SETUP);
> +    sigio_blocked_for_suspend = xf86BlockSIGIO();
>      for (i = 0; i < xf86NumScreens; i++) {
>         if (xf86Screens[i]->PMEvent)
>             xf86Screens[i]->PMEvent(i,event,undo);
> @@ -98,7 +101,6 @@ resume(pmEvent event, Bool undo)
>      InputInfoPtr pInfo;
> 
>      xf86AccessEnter();
> -    xf86EnterServerState(SETUP);
>      for (i = 0; i < xf86NumScreens; i++) {
>         if (xf86Screens[i]->PMEvent)
>             xf86Screens[i]->PMEvent(i,event,undo);
> @@ -107,7 +109,7 @@ resume(pmEvent event, Bool undo)
>             xf86Screens[i]->EnterVT(i, 0);
>         }
>      }
> -    xf86EnterServerState(OPERATING);
> +    xf86UnblockSIGIO(sigio_blocked_for_suspend);
>      for (i = 0; i < xf86NumScreens; i++) {
>         if (xf86Screens[i]->EnableDisableFBAccess)
>             (*xf86Screens[i]->EnableDisableFBAccess) (i, TRUE);
> @@ -124,12 +126,7 @@ resume(pmEvent event, Bool undo)
>  static void
>  DoApmEvent(pmEvent event, Bool undo)
>  {
> -    /*
> -     * we leave that as a global function for now. I don't know if
> -     * this might cause problems in the future. It is a global server
> -     * variable therefore it needs to be in a server info structure
> -     */
> -    int i, setup = 0;
> +    int i, was_blocked;
> 
>      switch(event) {
>  #if 0
> @@ -159,14 +156,13 @@ DoApmEvent(pmEvent event, Bool undo)
>         }
>         break;
>      default:
> +       was_blocked = xf86BlockSIGIO();
>         for (i = 0; i < xf86NumScreens; i++) {
>             if (xf86Screens[i]->PMEvent) {
> -               if (!setup) xf86EnterServerState(SETUP);
> -               setup = 1;
>                 xf86Screens[i]->PMEvent(i,event,undo);
>             }
>         }
> -       if (setup) xf86EnterServerState(OPERATING);
> +       xf86UnblockSIGIO(was_blocked);
>         break;
>      }
>  }
> diff --git a/hw/xfree86/common/xf86Priv.h b/hw/xfree86/common/xf86Priv.h
> index 08c0fa9..7137a53 100644
> --- a/hw/xfree86/common/xf86Priv.h
> +++ b/hw/xfree86/common/xf86Priv.h
> @@ -114,7 +114,6 @@ extern _X_EXPORT void xf86AccessLeave(void);
>  extern _X_EXPORT void xf86PostProbe(void);
>  extern _X_EXPORT void xf86ClearEntityListForScreen(int scrnIndex);
>  extern _X_EXPORT void xf86AddDevToEntity(int entityIndex, GDevPtr dev);
> -extern _X_EXPORT void xf86PostScreenInit(void);
> 
>  /* xf86Config.c */
> 
> diff --git a/hw/xfree86/common/xf86str.h b/hw/xfree86/common/xf86str.h
> index a65237a..0493dc5 100644
> --- a/hw/xfree86/common/xf86str.h
> +++ b/hw/xfree86/common/xf86str.h
> @@ -581,13 +581,6 @@ typedef struct _entityInfo {
>      DriverPtr driver;
>  } EntityInfoRec, *EntityInfoPtr;
> 
> -/* server states */
> -
> -typedef enum {
> -    SETUP,
> -    OPERATING
> -} xf86State;
> -
>  /* DGA */
> 
>  typedef struct {
> --
> 1.7.3.2

After reading this I've been wondering if we should need a return value on
Block/UnblockSIGIO. Anyway, it's quite okay for me:

Reviewed-by: Tiago Vignatti <tiago.vignatti at nokia.com>

             Tiago


More information about the xorg-devel mailing list