[PATCH] Make pm2's xv driver collect options like all other drivers.

Matt Turner mattst88 at gmail.com
Sat Dec 4 08:33:49 PST 2010


On Fri, Nov 12, 2010 at 3:48 AM, Jesse Adkins <jesserayadkins at gmail.com> wrote:
> The current method of argument collection is to collect options from different
> ports of a VideoAdaptor record. Specifically, the ports had to be named
> 'Input' for input options, and 'Output' for output options.
>
> This resulted in three groups of options, requiring people to know what
> VideoAdaptor does, both of which were not documented in the man page. If you
> forgot to create a VideoAdaptor port, then the xv driver would just not work.
>
> This patch makes the xv driver collect options from the screen, like every
> single other driver. Input and Output prefixes are used for options where the
> input and output ports have the same args (FramesPerSec, for example).
> Documentation added for the change.
>
> This is a step toward getting rid of VideoAdaptor, since only glint uses it
> (and is probably the only one to have used it).
>
> Signed-off-by: Jesse Adkins <jesserayadkins at gmail.com>
> ---
>  man/glint.man   |   24 +++++++++++
>  src/pm2_video.c |  115 ++++++++++++++++++++++---------------------------------
>  2 files changed, 70 insertions(+), 69 deletions(-)
>
> diff --git a/man/glint.man b/man/glint.man
> index 3bafcf4..3505e94 100644
> --- a/man/glint.man
> +++ b/man/glint.man
> @@ -102,6 +102,30 @@ acceleration in general, but disables it for some special cases.  Default: off.
>  .BI "Option \*qFireGL3000\*q \*q" boolean \*q
>  If you have a card of the same name, turn this on.  Default: off.
>  .TP
> +The Permedia 2 xv driver supports some additional options:
> +.TP
> +.BI "Option \*qDevice\*q \*q" string \*q
> +A path to the Permedia 2 kernel driver. This is required for Xv support.

This looks like it needs to be changed. I don't know if this is a
relic of the days when we had glint DRM or what.

> +.TP
> +.BI "Option \*qInputBuffers\*q \*q" integer \*q
> +Sets the number of buffers for incoming data. Minimum of 1, max of 2.
> +.TP
> +.BI "Option \*qInputFramesPerSec\*q \*q" integer \*q
> +Expected frames per second for incoming data.
> +.TP
> +.BI "Option \*qInputEncoding\*q \*q" string \*q
> +The encoding that input data will have.
> +.TP
> +.BI "Option \*qOutputBuffers\*q \*q" integer \*q
> +This should probably set the number of buffers for outgoing data. It actually
> +does nothing.
> +.TP
> +.BI "Option \*qOutputFramesPerSec\*q \*q" integer \*q
> +Expected frames per second for outgoing data.
> +.TP
> +.BI "Option \*qOutputEncoding\*q \*q" string \*q
> +The encoding to put output data in.
> +.TP
>  .SH "SEE ALSO"
>  __xservername__(__appmansuffix__), __xconfigfile__(__filemansuffix__), Xserver(__appmansuffix__), X(__miscmansuffix__)
>  .SH AUTHORS
> diff --git a/src/pm2_video.c b/src/pm2_video.c
> index df10bea..82eab32 100644
> --- a/src/pm2_video.c
> +++ b/src/pm2_video.c
> @@ -199,28 +199,22 @@ static const Bool ColorBars = FALSE;
>
>  typedef enum {
>     OPTION_DEVICE,
> -    OPTION_FPS,
> -    OPTION_BUFFERS,
> -    OPTION_ENCODING,
> -    OPTION_EXPOSE      /* obsolete, ignored */
> +    OPTION_IN_FPS,
> +    OPTION_IN_BUFFERS,
> +    OPTION_IN_ENCODING,
> +    OPTION_OUT_FPS,
> +    OPTION_OUT_BUFFERS,
> +    OPTION_OUT_ENCODING,
>  } OptToken;
>
> -/* XXX These should be made const, and per-screen/adaptor copies processed. */
> -static OptionInfoRec AdaptorOptions[] = {
> +static const OptionInfoRec pm2Options[] = {
>     { OPTION_DEVICE,           "Device",       OPTV_STRING,    {0}, FALSE },
> -    { -1,                      NULL,           OPTV_NONE,      {0}, FALSE }
> -};
> -static OptionInfoRec InputOptions[] = {
> -    { OPTION_BUFFERS,          "Buffers",      OPTV_INTEGER,   {0}, FALSE },
> -    { OPTION_FPS,              "FramesPerSec", OPTV_INTEGER,   {0}, FALSE },
> -    { OPTION_ENCODING,         "Encoding",     OPTV_STRING,    {0}, FALSE },
> -    { -1,                      NULL,           OPTV_NONE,      {0}, FALSE }
> -};
> -static OptionInfoRec OutputOptions[] = {
> -    { OPTION_BUFFERS,          "Buffers",      OPTV_INTEGER,   {0}, FALSE },
> -    { OPTION_EXPOSE,           "Expose",       OPTV_BOOLEAN,   {0}, FALSE },
> -    { OPTION_FPS,              "FramesPerSec", OPTV_INTEGER,   {0}, FALSE },
> -    { OPTION_ENCODING,         "Encoding",     OPTV_STRING,    {0}, FALSE },
> +    { OPTION_IN_BUFFERS,               "InputBuffers", OPTV_INTEGER,   {0}, FALSE },
> +    { OPTION_IN_FPS,           "InputFramesPerSec", OPTV_INTEGER,      {0}, FALSE },
> +    { OPTION_IN_ENCODING,              "InputEncoding",        OPTV_STRING,    {0}, FALSE },
> +    { OPTION_OUT_BUFFERS,              "OutputBuffers",        OPTV_INTEGER,   {0}, FALSE },
> +    { OPTION_OUT_FPS,          "OutputFramesPerSec", OPTV_INTEGER,     {0}, FALSE },
> +    { OPTION_OUT_ENCODING,             "OutputEncoding",       OPTV_STRING,    {0}, FALSE },
>     { -1,                      NULL,           OPTV_NONE,      {0}, FALSE }
>  };
>
> @@ -2964,7 +2958,6 @@ Permedia2VideoInit(ScreenPtr pScreen)
>     ScrnInfoPtr pScrn = xf86Screens[pScreen->myNum];
>     GLINTPtr pGlint = GLINTPTR(pScrn);
>     AdaptorPrivPtr pAPriv;
> -    pointer options[3];
>     DevUnion Private[PORTS];
>     XF86VideoAdaptorRec VAR[ADAPTORS];
>     XF86VideoAdaptorPtr VARPtrs[ADAPTORS];
> @@ -2981,64 +2974,48 @@ Permedia2VideoInit(ScreenPtr pScreen)
>         return;
>     }
>
> -    options[0] = NULL; /* VideoAdaptor "input" subsection options */
> -    options[1] = NULL; /* VideoAdaptor "output" subsection options */
> -    options[2] = NULL; /* VideoAdaptor options */
> -
> -    for (i = 0;; i++) {
> -       char *adaptor = NULL; /* receives VideoAdaptor section identifier */
> -
> -       if (!options[0])
> -           options[0] = xf86FindXvOptions(pScreen->myNum, i, "input", &adaptor, options[2] ? NULL : &options[2]);
> -
> -       if (!options[1])
> -           options[1] = xf86FindXvOptions(pScreen->myNum, i, "output", &adaptor, options[2] ? NULL : &options[2]);
> -
> -       if (!adaptor) {
> -           if (!i) /* VideoAdaptor reference enables Xv vio driver */
> -               VideoIO = FALSE;
> -           break;
> -       } else if (options[0] && options[1])
> -           break;
> -    }
> -
> -    if (VideoIO) {
> -      unsigned int temp;
> -      PCI_READ_LONG(pGlint->PciInfo, &temp, PCI_SUBSYSTEM_ID_REG);
> -      switch (temp) {
> -       case PCI_SUBSYSTEM_ID_WINNER_2000_P2A:
> -       case PCI_SUBSYSTEM_ID_WINNER_2000_P2C:
> -       case PCI_SUBSYSTEM_ID_GLORIA_SYNERGY_P2A:
> -       case PCI_SUBSYSTEM_ID_GLORIA_SYNERGY_P2C:
> -           break;
> -
> -       default:
> -           xf86DrvMsgVerb(pScrn->scrnIndex, X_PROBED, 1, "No Xv vio support for this board\n");
> -           VideoIO = FALSE;
> +    {
> +           /* Check for vio support. Input+Output options will be ignored when
> +              there's no vio support. */
> +           unsigned int temp;
> +           PCI_READ_LONG(pGlint->PciInfo, &temp, PCI_SUBSYSTEM_ID_REG);
> +           switch (temp) {
> +                   case PCI_SUBSYSTEM_ID_WINNER_2000_P2A:
> +                   case PCI_SUBSYSTEM_ID_WINNER_2000_P2C:
> +                   case PCI_SUBSYSTEM_ID_GLORIA_SYNERGY_P2A:
> +                   case PCI_SUBSYSTEM_ID_GLORIA_SYNERGY_P2C:
> +                           break;
> +                   default:
> +                           VideoIO = FALSE;
> +                           xf86DrvMsgVerb(pScrn->scrnIndex, X_PROBED, 1, "No Xv vio support for this board\n");
>        }
> -    }
> +
>     if (pGlint->NoAccel && !VideoIO)
>        return;
>
>     xf86DrvMsgVerb(pScrn->scrnIndex, X_INFO, 1, "Initializing Xv driver rev. 4\n");
>
>     if (VideoIO) {
> -       for (i = 0; i <= 2; i++) {
> -           xf86ProcessOptions(pScrn->scrnIndex, options[i],
> -               (i == 0) ? InputOptions :
> -               (i == 1) ? OutputOptions :
> -                          AdaptorOptions);
> +           xf86CollectOptions(pScrn, NULL);
> +           /* Process the options */
> +           if (!(pGlint->Options = malloc(sizeof(pm2Options))))
> +               return;

We have a malloc here.

> +
> +           memcpy(pGlint->Options, pm2Options, sizeof(pm2Options));
>
> -           xf86ShowUnusedOptions(pScrn->scrnIndex, options[i]);
> +           xf86ProcessOptions(pScrn->scrnIndex, pScrn->options, pGlint->Options);
> +           xf86ShowUnusedOptions(pScrn->scrnIndex, pGlint->Options);
>        }
>
> -       if (xf86IsOptionSet(AdaptorOptions, OPTION_DEVICE)) {
> -           if (!xvipcOpen(xf86GetOptValString(AdaptorOptions, OPTION_DEVICE), pScrn))
> +       if (xf86IsOptionSet(pm2Options, OPTION_DEVICE)) {
> +           if (!xvipcOpen(xf86GetOptValString(pm2Options, OPTION_DEVICE), pScrn))
>                VideoIO = FALSE;
>        }
>     }
>
>     if (!(pAPriv = NewAdaptorPriv(pScrn, VideoIO))) {
> +       if (VideoIO)
> +               free(pGlint->Options);

And a conditional free. Are we missing another free somewhere else?

>        xf86DrvMsg(pScrn->scrnIndex, X_ERROR, "Xv driver initialization failed\n");
>        return;
>     }
> @@ -3046,14 +3023,14 @@ Permedia2VideoInit(ScreenPtr pScreen)
>     if (VideoIO) {
>        int n;
>
> -       if (xf86GetOptValInteger(InputOptions, OPTION_BUFFERS, &n))
> +       if (xf86GetOptValInteger(pm2Options, OPTION_IN_BUFFERS, &n))
>            pAPriv->Port[0].BuffersRequested = CLAMP(n, 1, 2);
> -       if (xf86GetOptValInteger(InputOptions, OPTION_FPS, &n))
> +       if (xf86GetOptValInteger(pm2Options, OPTION_IN_FPS, &n))
>            pAPriv->Port[0].FramesPerSec = CLAMP(n, 1, 30);
>
> -       if (xf86GetOptValInteger(OutputOptions, OPTION_BUFFERS, &n))
> +       if (xf86GetOptValInteger(pm2Options, OPTION_OUT_BUFFERS, &n))
>            pAPriv->Port[1].BuffersRequested = 1;
> -       if (xf86GetOptValInteger(OutputOptions, OPTION_FPS, &n))
> +       if (xf86GetOptValInteger(pm2Options, OPTION_OUT_FPS, &n))
>            pAPriv->Port[1].FramesPerSec = CLAMP(n, 1, 30);
>     }
>
> @@ -3178,14 +3155,14 @@ Permedia2VideoInit(ScreenPtr pScreen)
>        xf86DrvMsg(pScrn->scrnIndex, X_INFO, "Xv frontend scaler enabled\n");
>
>        if (VideoIO) {
> -           if ((s = xf86GetOptValString(InputOptions, OPTION_ENCODING)))
> +           if ((s = xf86GetOptValString(pm2Options, OPTION_IN_ENCODING)))
>                for (i = 0; i < ENTRIES(InputVideoEncodings); i++)
>                    if (!strncmp(s, InputVideoEncodings[i].name, strlen(s))) {
>                        Permedia2SetPortAttribute(pScrn, xvEncoding, i, (pointer) &pAPriv->Port[0]);
>                        break;
>                    }
>
> -           if ((s = xf86GetOptValString(OutputOptions, OPTION_ENCODING)))
> +           if ((s = xf86GetOptValString(pm2Options, OPTION_OUT_ENCODING)))
>                for (i = 0; i < ENTRIES(OutputVideoEncodings); i++)
>                    if (!strncmp(s, OutputVideoEncodings[i].name, strlen(s))) {
>                        Permedia2SetPortAttribute(pScrn, xvEncoding, i, (pointer) &pAPriv->Port[1]);
> --


More information about the xorg-devel mailing list