[PATCH] randr: Add 'set' fields to SetCrtcConfigs request
Aaron Plattner
aplattner at nvidia.com
Wed Feb 23 10:16:46 PST 2011
On Wed, Feb 16, 2011 at 10:59:22PM -0800, Keith Packard wrote:
> This permits clients to perform incremental configuration changes
> instead of requiring a complete new configuration to be written.
>
> Signed-off-by: Keith Packard <keithp at keithp.com>
> ---
> randr/rrcrtc.c | 332 ++++++++++++++++++++++++++++++++++----------------------
> 1 files changed, 202 insertions(+), 130 deletions(-)
>
> diff --git a/randr/rrcrtc.c b/randr/rrcrtc.c
> index 5fe6900..07e9019 100644
> --- a/randr/rrcrtc.c
> +++ b/randr/rrcrtc.c
> @@ -1483,63 +1483,82 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr screen,
> PixmapPtr pixmap;
> int rc, i, j;
> Rotation rotation;
> + int numOutputs;
>
> VERIFY_RR_CRTC(x->crtc, crtc, DixSetAttrAccess);
>
> - if (x->mode == None)
> - {
> - mode = NULL;
> - if (x->nOutput > 0)
> - return BadMatch;
> + mode = crtc->mode;
> + numOutputs = crtc->numOutputs;
> +
> + if (x->set & RR_SetCrtcOutputs)
> + numOutputs = x->nOutput;
> +
> + if (x->set & RR_SetCrtcMode) {
> + if (x->mode == None)
> + mode = NULL;
> + else
> + {
> + VERIFY_RR_MODE(x->mode, mode, DixSetAttrAccess);
> + if (x->nOutput == 0)
> + return BadMatch;
> + }
randrproto.txt says nothing about SetCrtcMode.
> }
> - else
> +
> + if (numOutputs)
> {
> - VERIFY_RR_MODE(x->mode, mode, DixSetAttrAccess);
> - if (x->nOutput == 0)
> + if (mode == NULL)
> return BadMatch;
> - }
> - if (x->nOutput)
> - {
> - outputs = malloc(x->nOutput * sizeof (RROutputPtr));
> + outputs = malloc(numOutputs * sizeof (RROutputPtr));
> if (!outputs)
> return BadAlloc;
> }
> - else
> + else {
> + if (mode != NULL)
> + return BadMatch;
randrproto.txt says nothing about SetCrtcOutputs. The interplay encoded
here probably ought to be described in the protocol doc.
> outputs = NULL;
> -
> - if (x->pixmap == None)
> - pixmap = NULL;
> - else if (x->pixmap == RR_CurrentScanoutPixmap)
> - pixmap = crtc->scanoutPixmap;
> - else
> - {
> - rc = dixLookupResourceByType((pointer *) &pixmap, x->pixmap,
> - RT_PIXMAP, client, DixWriteAccess);
> - if (rc != Success) {
> - free(outputs);
> - return rc;
> - }
> - /* XXX check to make sure this is a scanout pixmap */
> }
>
> - for (i = 0; i < x->nOutput; i++)
> - {
> - rc = dixLookupResourceByType((pointer *)(outputs + i), outputIds[i],
> - RROutputType, client, DixSetAttrAccess);
> - if (rc != Success)
> + if (x->set & RR_SetCrtcPixmap) {
> + if (x->pixmap == None)
> + pixmap = NULL;
> + else
> {
> - free(outputs);
> - return rc;
> + rc = dixLookupResourceByType((pointer *) &pixmap, x->pixmap,
> + RT_PIXMAP, client, DixWriteAccess);
> + if (rc != Success) {
> + free(outputs);
> + return rc;
> + }
> + /* XXX check to make sure this is a scanout pixmap */
> }
> - /* validate crtc for this output */
> - for (j = 0; j < outputs[i]->numCrtcs; j++)
> - if (outputs[i]->crtcs[j] == crtc)
> - break;
> - if (j == outputs[i]->numCrtcs)
> + } else
> + pixmap = crtc->scanoutPixmap;
> +
> + if (x->set & RR_SetCrtcOutputs) {
> + for (i = 0; i < numOutputs; i++)
> {
> - free(outputs);
> - return BadMatch;
> + rc = dixLookupResourceByType((pointer *)(outputs + i), outputIds[i],
> + RROutputType, client, DixSetAttrAccess);
> + if (rc != Success)
> + {
> + free(outputs);
> + return rc;
> + }
> + /* validate crtc for this output */
> + for (j = 0; j < outputs[i]->numCrtcs; j++)
> + if (outputs[i]->crtcs[j] == crtc)
> + break;
> + if (j == outputs[i]->numCrtcs)
> + {
> + free(outputs);
> + return BadMatch;
> + }
> }
> + } else
> + memcpy(outputs, crtc->outputs, numOutputs * sizeof (RROutputPtr));
> +
> + for (i = 0; i < numOutputs; i++)
> + {
> /* validate mode for this output */
> for (j = 0; j < outputs[i]->numModes + outputs[i]->numUserModes; j++)
> {
> @@ -1555,10 +1574,11 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr screen,
> return BadMatch;
> }
> }
> +
> /* validate clones */
> - for (i = 0; i < x->nOutput; i++)
> + for (i = 0; i < numOutputs; i++)
> {
> - for (j = 0; j < x->nOutput; j++)
> + for (j = 0; j < numOutputs; j++)
> {
> int k;
> if (i == j)
> @@ -1579,46 +1599,68 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr screen,
> if (crtc->pScreen != screen)
> return BadMatch;
>
> + rotation = crtc->rotation;
> + if (x->set & RR_SetCrtcRotation) {
> + /*
> + * Validate requested rotation
> + */
> + rotation = (Rotation) x->rotation;
> +
> + /* test the rotation bits only! */
> + switch (rotation & 0xf) {
> + case RR_Rotate_0:
> + case RR_Rotate_90:
> + case RR_Rotate_180:
> + case RR_Rotate_270:
> + break;
> + default:
> + /*
> + * Invalid rotation
> + */
> + client->errorValue = x->rotation;
> + free(outputs);
> + return BadValue;
> + }
> + }
> +
> scr_priv = rrGetScrPriv(screen);
>
> config->crtc = crtc;
> - config->x = x->x;
> - config->y = x->y;
> + config->x = crtc->x;
> + config->y = crtc->y;
> + if (x->set & RR_SetCrtcPosition) {
> + config->x = x->x;
> + config->y = x->y;
> + }
> config->mode = mode;
> - config->rotation = x->rotation;
> - config->numOutputs = x->nOutput;
> + config->rotation = rotation;
> + config->numOutputs = numOutputs;
> config->outputs = outputs;
> - PictTransform_from_xRenderTransform(&config->sprite_position_transform,
> - &x->spritePositionTransform);
> - PictTransform_from_xRenderTransform(&config->sprite_image_transform,
> - &x->spriteImageTransform);
> - pixman_f_transform_from_pixman_transform(&config->sprite_position_f_transform,
> - &config->sprite_position_transform);
> - pixman_f_transform_from_pixman_transform(&config->sprite_image_f_transform,
> - &config->sprite_image_transform);
> - config->pixmap = pixmap;
> - config->pixmap_x = x->xPixmap;
> - config->pixmap_y = x->yPixmap;
> -
> - /*
> - * Validate requested rotation
> - */
> - rotation = (Rotation) x->rotation;
>
> - /* test the rotation bits only! */
> - switch (rotation & 0xf) {
> - case RR_Rotate_0:
> - case RR_Rotate_90:
> - case RR_Rotate_180:
> - case RR_Rotate_270:
> - break;
> - default:
> - /*
> - * Invalid rotation
> - */
> - client->errorValue = x->rotation;
> - free(outputs);
> - return BadValue;
> + if (x->set & RR_SetCrtcSpritePositionTransform) {
> + PictTransform_from_xRenderTransform(&config->sprite_position_transform,
> + &x->spritePositionTransform);
> + pixman_f_transform_from_pixman_transform(&config->sprite_position_f_transform,
> + &config->sprite_position_transform);
> + } else {
> + config->sprite_position_transform = crtc->client_sprite_position_transform;
> + config->sprite_position_f_transform = crtc->client_sprite_f_position_transform;
> + }
> + if (x->set & RR_SetCrtcSpriteImageTransform) {
> + PictTransform_from_xRenderTransform(&config->sprite_image_transform,
> + &x->spriteImageTransform);
> + pixman_f_transform_from_pixman_transform(&config->sprite_image_f_transform,
> + &config->sprite_image_transform);
> + } else {
> + config->sprite_image_transform = crtc->client_sprite_image_transform;
> + config->sprite_image_f_transform = crtc->client_sprite_f_image_transform;
> + }
> + config->pixmap = pixmap;
> + config->pixmap_x = crtc->x;
> + config->pixmap_y = crtc->y;
> + if (x->set & RR_SetCrtcPixmapPosition) {
> + config->pixmap_x = x->xPixmap;
> + config->pixmap_y = x->yPixmap;
> }
>
> if (mode)
> @@ -1628,7 +1670,7 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr screen,
> /*
> * requested rotation or reflection not supported by screen
> */
> - client->errorValue = x->rotation;
> + client->errorValue = rotation;
> free(outputs);
> return BadMatch;
> }
> @@ -1639,13 +1681,13 @@ RRConvertCrtcConfig(ClientPtr client, ScreenPtr screen,
> */
> if (pixmap)
> {
> - if (x->xPixmap + mode->mode.width > pixmap->drawable.width) {
> - client->errorValue = x->xPixmap;
> + if (config->pixmap_x + mode->mode.width > pixmap->drawable.width) {
> + client->errorValue = config->pixmap_x;
> free(outputs);
> return BadValue;
> }
> - if (x->yPixmap + mode->mode.height > pixmap->drawable.height) {
> - client->errorValue = x->yPixmap;
> + if (config->pixmap_y + mode->mode.height > pixmap->drawable.height) {
> + client->errorValue = config->pixmap_y;
> free(outputs);
> return BadValue;
> }
> @@ -1687,13 +1729,15 @@ ProcRRSetCrtcConfigs (ClientPtr client)
> int num_configs = 0;
> int rc, i;
> int extra_len;
> - int num_output_ids;
> + int num_output_ids = 0;
>
> REQUEST_AT_LEAST_SIZE(xRRSetCrtcConfigsReq);
>
> extra_len = client->req_len - bytes_to_int32(sizeof(xRRSetCrtcConfigsReq));
>
> - num_configs = stuff->nConfigs;
> + num_configs = 0;
> + if (stuff->set & RR_SetScreenCrtcs)
> + num_configs = stuff->nConfigs;
>
> /* Check request length against number of configs specified */
> if (num_configs * (sizeof (xRRCrtcConfig) >> 2) > extra_len)
> @@ -1702,10 +1746,11 @@ ProcRRSetCrtcConfigs (ClientPtr client)
> extra_len -= num_configs * (sizeof (xRRCrtcConfig) >> 2);
> x_configs = (xRRCrtcConfig *) (stuff + 1);
>
> - /* Check remaining request length against number of outputs */
> - num_output_ids = 0;
> - for (i = 0; i < num_configs; i++)
> - num_output_ids += x_configs[i].nOutput;
> + if (stuff->set & RR_SetScreenCrtcs) {
> + /* Check remaining request length against number of outputs */
> + for (i = 0; i < num_configs; i++)
> + num_output_ids += x_configs[i].nOutput;
> + }
>
> if (extra_len != num_output_ids)
> return BadLength;
> @@ -1724,57 +1769,84 @@ ProcRRSetCrtcConfigs (ClientPtr client)
> goto sendReply;
> }
>
> - if (stuff->widthInMillimeters == 0 || stuff->heightInMillimeters == 0)
> - {
> - client->errorValue = 0;
> - return BadValue;
> + if (stuff->set & RR_SetScreenSizeInMillimeters) {
> + if (stuff->widthInMillimeters == 0 || stuff->heightInMillimeters == 0)
> + {
> + client->errorValue = 0;
> + return BadValue;
> + }
> + screen_config.mm_width = stuff->widthInMillimeters;
> + screen_config.mm_height = stuff->heightInMillimeters;
> + } else {
> + screen_config.mm_width = screen->mmWidth;
> + screen_config.mm_height = screen->mmHeight;
> }
>
> - if (stuff->screenPixmapWidth < scr_priv->minWidth ||
> - scr_priv->maxWidth < stuff->screenPixmapWidth)
> - {
> - client->errorValue = stuff->screenPixmapWidth;
> - return BadValue;
> - }
> - if (stuff->screenPixmapHeight < scr_priv->minHeight ||
> - scr_priv->maxHeight < stuff->screenPixmapHeight)
> - {
> - client->errorValue = stuff->screenPixmapHeight;
> - return BadValue;
> + if (stuff->set & RR_SetScreenPixmapSize) {
> + if (stuff->screenPixmapWidth < scr_priv->minWidth ||
> + scr_priv->maxWidth < stuff->screenPixmapWidth)
> + {
> + client->errorValue = stuff->screenPixmapWidth;
> + return BadValue;
> + }
> + if (stuff->screenPixmapHeight < scr_priv->minHeight ||
> + scr_priv->maxHeight < stuff->screenPixmapHeight)
> + {
> + client->errorValue = stuff->screenPixmapHeight;
> + return BadValue;
> + }
> + screen_config.screen_pixmap_width = stuff->screenPixmapWidth;
> + screen_config.screen_pixmap_height = stuff->screenPixmapHeight;
> + } else {
> + PixmapPtr screen_pixmap = screen->GetScreenPixmap(screen);
> + screen_config.screen_pixmap_width = screen_pixmap->drawable.width;
> + screen_config.screen_pixmap_height = screen_pixmap->drawable.height;
> + }
> +
> + if (stuff->set & RR_SetScreenSize) {
> + if (stuff->screenWidth <= 0 || stuff->screenWidth > 32767) {
> + client->errorValue = stuff->screenWidth;
> + return BadValue;
> + }
> + screen_config.screen_width = stuff->screenWidth;
> +
> + if (stuff->screenHeight <= 0 || stuff->screenHeight > 32767) {
> + client->errorValue = stuff->screenHeight;
> + return BadValue;
> + }
> + screen_config.screen_height = stuff->screenHeight;
> + } else {
> + WindowPtr root = screen->root;
> + screen_config.screen_width = root->drawable.width;
> + screen_config.screen_height = root->drawable.height;
> }
>
> - screen_config.screen_pixmap_width = stuff->screenPixmapWidth;
> - screen_config.screen_pixmap_height = stuff->screenPixmapHeight;
> - screen_config.screen_width = stuff->screenWidth;
> - screen_config.screen_height = stuff->screenHeight;
> - screen_config.mm_width = stuff->widthInMillimeters;
> - screen_config.mm_height = stuff->heightInMillimeters;
> + if (num_configs > 0) {
Existing bug, but it's not clear from randrproto.txt that RRSetCrtcConfigs
doesn't do anything if 'set' doesn't contain SetScreenCrtcs. From the
wording of the protocol doc, I would expect set = RR_SetScreenPixmapSize |
RR_SetScreenSizeInMillimeters | RR_SetScreenSize to change those three
things.
>
> - output_ids = (RROutput *) (x_configs + num_configs);
> + output_ids = (RROutput *) (x_configs + num_configs);
> + /*
> + * Convert protocol crtc configurations into
> + * server crtc configurations
> + */
> + configs = calloc(num_configs, sizeof (RRCrtcConfigRec));
> + if (configs == NULL)
> + return BadAlloc;
> + for (i = 0; i < num_configs; i++) {
> + rc = RRConvertCrtcConfig(client, screen, &screen_config,
> + &configs[i],
> + &x_configs[i], output_ids);
> + if (rc != Success) {
> + rep.status = RRSetConfigFailed;
> + goto sendReply;
> + }
> + output_ids += x_configs[i].nOutput;
> + }
>
> - /*
> - * Convert protocol crtc configurations into
> - * server crtc configurations
> - */
> - configs = calloc(num_configs, sizeof (RRCrtcConfigRec));
> - if (num_configs > 0 && configs == NULL)
> - return BadAlloc;
> - for (i = 0; i < num_configs; i++) {
> - rc = RRConvertCrtcConfig(client, screen, &screen_config,
> - &configs[i],
> - &x_configs[i], output_ids);
> - if (rc != Success) {
> + if (!RRSetCrtcConfigs (screen, &screen_config, configs, num_configs))
> + {
> rep.status = RRSetConfigFailed;
> goto sendReply;
> }
> - output_ids += x_configs[i].nOutput;
> - }
> -
> - if (num_configs &&
> - !RRSetCrtcConfigs (screen, &screen_config, configs, num_configs))
> - {
> - rep.status = RRSetConfigFailed;
> - goto sendReply;
> }
> rep.status = RRSetConfigSuccess;
> scr_priv->lastSetTime = currentTime;
> --
> 1.7.2.3
>
Other than the protocol doc comments and the existing bug noted above,
Reviewed-by: Aaron Plattner <aplattner at nvidia.com>
However, I do need to object strongly to such a major protocol change being
made after RC2 of a release cycle. We're supposed to be making only
critical bug fixes at this point! Planning fail.
-- Aaron
More information about the xorg-devel
mailing list