[PATCH] xserver: add monitor Option "ZoomModes"
Aaron Plattner
aplattner at nvidia.com
Tue Mar 19 14:07:47 PDT 2013
On 03/07/13 07:07, vdb at picaros.org wrote:
>> On 11/21/2012 04:12 AM, vdb at picaros.org wrote:
>>> Add support for the Option "ZoomModes" in a monitor section:
>>>
>>> Section "Monitor"
>>> Identifier "a21inch"
>>> Option "PreferredMode" "1600x1200"
>>> Option "ZoomModes" "1600x1200 1280x1024 1280x1024 640x480"
>>> EndSection
>>
>> "ZoomModes" seems like an unfortunate name to me, but there's precedent
>> for it in the "DontZoom" option so I won't object to it too strongly.
>
> Well, I settled on "ZoomModes" since xserver/hw/xfree86 functions with
> 'Zoom' in their name relate to the Ctrl+Alt+Keypad-{Plus,Minus} mode
> switch. The 'Modes' part comes from the Section "Screen"/Subsection
> "Display"/Modes statement.
That's fair.
>>> diff --git a/hw/xfree86/man/xorg.conf.man b/hw/xfree86/man/xorg.conf.man
>>> index 5d92bbe..729d301 100644
>>> --- a/hw/xfree86/man/xorg.conf.man
>>> +++ b/hw/xfree86/man/xorg.conf.man
>>> @@ -1676,6 +1676,16 @@ This optional entry specifies a mode to be marked as the preferred initial mode
>>> of the monitor.
>>> (RandR 1.2-supporting drivers only)
>>> .TP 7
>>> +.BI "Option \*qZoomModes\*q \*q" name " " name " " ... \*q
>>> +This optional entry specifies modes to be marked as zoom modes.
>>> +It is possible to switch to the next and previous mode via
>>> +.BR Ctrl+Alt+Keypad\-Plus " and " Ctrl+Alt+Keypad\-Minus .
>>> +All these keypad available modes are selected from the screen mode
>>> +list. This list is a copy of the compatibility output monitor mode
>>
>> roff requires each sentence to begin on its own line, though you can add
>> additional line breaks in the middle of sentences for readability.
>>
>>> +list. Since this output is the output connected to the lowest
>>> +dot\-area monitor, as determined from its largest size mode, that
>>
>> Should this be a hyphen ('-') rather than a literal dash ('\-')?
>>
>>> +monitor defines the available zoom modes.
>>
>> This only applies to RandR 1.2 drivers, so it should probably get the
>> "RandR 1.2-supporting drivers only" text.
>
> Indeed, text modified per suggestions, thank you.
>
>>> diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
>>> index 154f684..2e46885 100644
>>> --- a/hw/xfree86/modes/xf86Crtc.c
>>> +++ b/hw/xfree86/modes/xf86Crtc.c
>
>>> @@ -1424,6 +1426,88 @@ preferredMode(ScrnInfoPtr pScrn, xf86OutputPtr output)
>>> return preferred_mode;
>>> }
>>>
>>> +/** identify a token
>>> + * args
>>> + * *src a string with zero or more tokens, e.g. "tok0 tok1",
>>> + * **token stores a pointer to the first token character,
>>> + * *len stores the token length.
>>> + * return
>>> + * a pointer into src[] at the token terminating character, or
>>> + * NULL if no token is found.
>>> + */
>>> +static const char *
>>> +gettoken(const char *src, const char **token, int *len)
>>> +{
>>> + const char *next, *delim = " \t";
>>> + int skip;
>>> +
>>> + if (!src)
>>> + return NULL;
>>> +
>>> + skip = strspn(src, delim);
>>> + *token = &src[skip];
>>> +
>>> + *len = strcspn(*token, delim);
>>> + /* Support for backslash escaped delimiters could be implemented
>>> + * here.
>>> + */
>>> +
>>> + /* (*token)[0] != '\0' <==> *len > 0 */
>>> + next = *len > 0 ? &(*token)[*len] : NULL;
>>
>> This would probably be clearer written
>>
>> if (*len > 0)
>> return &(*token)[*len];
>> else
>> return NULL;
>>
>>> +
>>> + return next;
>>> +}
>
> Agreed, code changed per suggestion.
>
>> It seems surprising that there isn't already a function to do this.
>
> There used to be a function in xf86-video-ati/ to process the
> Option "MetaModes" for the driver's internal multiple output support.
>
> I checked xserver/hw/xfree86/{ common, parser} but couldn't find
> anything reusable. An option statement with a list value, for example
>
> Option "ZoomModes" "1600x1200" "1280x1024" "1280x1024" "640x480"
>
> would be ideal. It's almost there: in common/xf86Opt.h ValueUnion
> add a pointer to a list, add support in parser/Flag.c
> xf86parseOption() for list values and extend common/xf86Option.c
> ParseOptionValue().
>
> But this added complexity doesn't seem worthwhile for a single Option
> user. So I settled for a string of mode names and a gettoken()
> function.
Sounds fine.
>>> +/** Check for a user configured zoom mode list, Option "ZoomModes":
>>> + *
>>> + * Section "Monitor"
>>> + * Identifier "a21inch"
>>> + * Option "ZoomModes" "1600x1200 1280x1024 1280x1024 640x480"
>>> + * EndSection
>>> + *
>>> + * Each user mode name is searched for independently so the list
>>> + * specification order is free. An output mode is matched at most
>>> + * once, a mode with an already set M_T_USERDEF type bit is skipped.
>>> + * Thus a repeat mode name specificaton matches the next output mode.
>>
>> s/specificaton/specification/
>>
>> This took me a few reads to make sense. Maybe add "with the same name"
>> to the end of this sentence?
>
> Indeed, text amended per suggestion, thank you.
>
>>> + * Ctrl+Alt+Keypad-{Plus,Minus} zooms {in,out} by selecting the
>>> + * {next,previous} M_T_USERDEF mode in the screen modes list, itself
>>> + * sorted toward lower dot area or lower dot clock frequency, see
>>> + * modes/xf86Crtc.c: xf86SortModes() xf86SetScrnInfoModes(), and
>>> + * common/xf86Cursor.c: xf86ZoomViewport().
>>> + */
>>> +static int
>>> +processZoomModes(xf86OutputPtr output)
>>> +{
>>> + const char *zoom_modes;
>>> + int count = 0;
>>> +
>>> + zoom_modes = xf86GetOptValString(output->options, OPTION_ZOOM_MODES);
>>> +
>>> + if (zoom_modes) {
>>> + const char *token, *next;
>>> + int len;
>>> +
>>> + next = gettoken(zoom_modes, &token, &len);
>>> + while (next) {
>>> + DisplayModePtr mode;
>>> +
>>> + for (mode = output->probed_modes; mode; mode = mode->next)
>>> + if (!strncmp(token, mode->name, len) /* prefix match */
>>> + && mode->name[len] == '\0' /* equal length */
>>
>> This should probably use xf86NameCmp for consistency with the rest of
>> the server's parsing code. I know that means using strndup or similar;
>> sorry.
>
> It seems a design choice to match mode names with plain strcmp()
> or strncmp(). Examples in xserver/hw/xfree86:
>
> common/xf86Config.c modeIsPresent(),
> fbdevhw/fbdevhw.c fbdevHWSetVideoModes(),
> modes/xf86Crtc.c xf86ProbeOutputModes() Option "PreferredMode".
>
> Also the now unused modes/xf86Modes.c xf86ValidateModesUserConfig()
> uses a plain prefix match to filter for modes whose name matches a
> listed "Screen"/"Display"/Modes name.
Got it. Thanks for checking that.
>>> + && !(mode->type & M_T_USERDEF)) { /* no rematch */
>>> + mode->type |= M_T_USERDEF;
>>> + break;
>>> + }
>>> +
>>> + count++;
>>> + next = gettoken(next, &token, &len);
>>> + }
>>> + }
>>> +
>>> + return count;
>>> +}
>
> The revised patch v2 is here below. Servaas.
Thanks for making these changes. Version 2 looks good to me:
Reviewed-by: Aaron Plattner <aplattner at nvidia.com>
> ---
> hw/xfree86/man/xorg.conf.man | 11 +++++
> hw/xfree86/modes/xf86Crtc.c | 89 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 100 insertions(+), 0 deletions(-)
>
> diff --git a/hw/xfree86/man/xorg.conf.man b/hw/xfree86/man/xorg.conf.man
> index 5d92bbe..f9f98dc 100644
> --- a/hw/xfree86/man/xorg.conf.man
> +++ b/hw/xfree86/man/xorg.conf.man
> @@ -1676,6 +1676,17 @@ This optional entry specifies a mode to be marked as the preferred initial mode
> of the monitor.
> (RandR 1.2-supporting drivers only)
> .TP 7
> +.BI "Option \*qZoomModes\*q \*q" name " " name " " ... \*q
> +This optional entry specifies modes to be marked as zoom modes.
> +It is possible to switch to the next and previous mode via
> +.BR Ctrl+Alt+Keypad\-Plus " and " Ctrl+Alt+Keypad\-Minus .
> +All these keypad available modes are selected from the screen mode list.
> +This list is a copy of the compatibility output monitor mode list.
> +Since this output is the output connected to the lowest
> +dot-area monitor, as determined from its largest size mode, that
> +monitor defines the available zoom modes.
> +(RandR 1.2-supporting drivers only)
> +.TP 7
> .BI "Option \*qPosition\*q \*q" x " " y \*q
> This optional entry specifies the position of the monitor within the X
> screen.
> diff --git a/hw/xfree86/modes/xf86Crtc.c b/hw/xfree86/modes/xf86Crtc.c
> index 154f684..a49dc3e 100644
> --- a/hw/xfree86/modes/xf86Crtc.c
> +++ b/hw/xfree86/modes/xf86Crtc.c
> @@ -427,6 +427,7 @@ extern XF86ConfigPtr xf86configptr;
>
> typedef enum {
> OPTION_PREFERRED_MODE,
> + OPTION_ZOOM_MODES,
> OPTION_POSITION,
> OPTION_BELOW,
> OPTION_RIGHT_OF,
> @@ -445,6 +446,7 @@ typedef enum {
>
> static OptionInfoRec xf86OutputOptions[] = {
> {OPTION_PREFERRED_MODE, "PreferredMode", OPTV_STRING, {0}, FALSE},
> + {OPTION_ZOOM_MODES, "ZoomModes", OPTV_STRING, {0}, FALSE },
> {OPTION_POSITION, "Position", OPTV_STRING, {0}, FALSE},
> {OPTION_BELOW, "Below", OPTV_STRING, {0}, FALSE},
> {OPTION_RIGHT_OF, "RightOf", OPTV_STRING, {0}, FALSE},
> @@ -1424,6 +1426,90 @@ preferredMode(ScrnInfoPtr pScrn, xf86OutputPtr output)
> return preferred_mode;
> }
>
> +/** identify a token
> + * args
> + * *src a string with zero or more tokens, e.g. "tok0 tok1",
> + * **token stores a pointer to the first token character,
> + * *len stores the token length.
> + * return
> + * a pointer into src[] at the token terminating character, or
> + * NULL if no token is found.
> + */
> +static const char *
> +gettoken(const char *src, const char **token, int *len)
> +{
> + const char *delim = " \t";
> + int skip;
> +
> + if (!src)
> + return NULL;
> +
> + skip = strspn(src, delim);
> + *token = &src[skip];
> +
> + *len = strcspn(*token, delim);
> + /* Support for backslash escaped delimiters could be implemented
> + * here.
> + */
> +
> + /* (*token)[0] != '\0' <==> *len > 0 */
> + if (*len > 0)
> + return &(*token)[*len];
> + else
> + return NULL;
> +}
> +
> +/** Check for a user configured zoom mode list, Option "ZoomModes":
> + *
> + * Section "Monitor"
> + * Identifier "a21inch"
> + * Option "ZoomModes" "1600x1200 1280x1024 1280x1024 640x480"
> + * EndSection
> + *
> + * Each user mode name is searched for independently so the list
> + * specification order is free. An output mode is matched at most
> + * once, a mode with an already set M_T_USERDEF type bit is skipped.
> + * Thus a repeat mode name specification matches the next output mode
> + * with the same name.
> + *
> + * Ctrl+Alt+Keypad-{Plus,Minus} zooms {in,out} by selecting the
> + * {next,previous} M_T_USERDEF mode in the screen modes list, itself
> + * sorted toward lower dot area or lower dot clock frequency, see
> + * modes/xf86Crtc.c: xf86SortModes() xf86SetScrnInfoModes(), and
> + * common/xf86Cursor.c: xf86ZoomViewport().
> + */
> +static int
> +processZoomModes(xf86OutputPtr output)
> +{
> + const char *zoom_modes;
> + int count = 0;
> +
> + zoom_modes = xf86GetOptValString(output->options, OPTION_ZOOM_MODES);
> +
> + if (zoom_modes) {
> + const char *token, *next;
> + int len;
> +
> + next = gettoken(zoom_modes, &token, &len);
> + while (next) {
> + DisplayModePtr mode;
> +
> + for (mode = output->probed_modes; mode; mode = mode->next)
> + if (!strncmp(token, mode->name, len) /* prefix match */
> + && mode->name[len] == '\0' /* equal length */
> + && !(mode->type & M_T_USERDEF)) { /* no rematch */
> + mode->type |= M_T_USERDEF;
> + break;
> + }
> +
> + count++;
> + next = gettoken(next, &token, &len);
> + }
> + }
> +
> + return count;
> +}
> +
> static void
> GuessRangeFromModes(MonPtr mon, DisplayModePtr mode)
> {
> @@ -1719,6 +1805,9 @@ xf86ProbeOutputModes(ScrnInfoPtr scrn, int maxX, int maxY)
> }
> }
>
> + /* Ctrl+Alt+Keypad-{Plus,Minus} zoom mode: M_T_USERDEF mode type */
> + processZoomModes(output);
> +
> output->initial_rotation = xf86OutputInitialRotation(output);
>
> if (debug_modes) {
> --
> 1.7.4.5
>
More information about the xorg-devel
mailing list