[virglrenderer-devel] [PATCH 1/1] vtest: command dispatch now uses a jump-table
Marc-André Lureau
marcandre.lureau at redhat.com
Mon Jun 4 13:54:01 UTC 2018
Hi
On Mon, Jun 4, 2018 at 2:42 PM, Nathan Gauër <nathan.gauer at lse.epita.fr> wrote:
> removed the switch and goto based loop.
> To add a command, create the corresponding define in vtest_protocol.h and
> an entry in the jump-table (vrend_server.c).
>
looks good, minor nits below
> Signed-off-by: Nathan Gauër <nathan.gauer at lse.epita.fr>
> ---
> vtest/vtest.h | 11 ++---
> vtest/vtest_renderer.c | 17 +++++--
> vtest/vtest_server.c | 108 ++++++++++++++++++++++-------------------
> 3 files changed, 75 insertions(+), 61 deletions(-)
>
> diff --git a/vtest/vtest.h b/vtest/vtest.h
> index deb6618..6b1d91d 100644
> --- a/vtest/vtest.h
> +++ b/vtest/vtest.h
> @@ -27,10 +27,10 @@
> #include <errno.h>
> int vtest_create_renderer(int in_fd, int out_fd, uint32_t length);
>
> -int vtest_send_caps(void);
> +int vtest_send_caps(uint32_t length_dw);
>
> -int vtest_create_resource(void);
> -int vtest_resource_unref(void);
> +int vtest_create_resource(uint32_t length_dw);
> +int vtest_resource_unref(uint32_t length_dw);
> int vtest_submit_cmd(uint32_t length_dw);
>
> int vtest_transfer_get(uint32_t length_dw);
> @@ -38,10 +38,9 @@ int vtest_transfer_put(uint32_t length_dw);
>
> int vtest_block_read(int fd, void *buf, int size);
>
> -int vtest_resource_busy_wait(void);
> -int vtest_renderer_create_fence(void);
> +int vtest_resource_busy_wait(uint32_t length_dw);
> +int vtest_renderer_create_fence(uint32_t length_dw);
> int vtest_poll(void);
>
> void vtest_destroy_renderer(void);
> #endif
> -
> diff --git a/vtest/vtest_renderer.c b/vtest/vtest_renderer.c
> index 3b8fe1a..fb34955 100644
> --- a/vtest/vtest_renderer.c
> +++ b/vtest/vtest_renderer.c
> @@ -35,6 +35,8 @@
> #include "vtest_protocol.h"
> #include "util.h"
>
> +#define UNUSED_PARAMETER(Param) (void)Param
> +
> static int ctx_id = 1;
> static int fence_id = 1;
>
> @@ -153,8 +155,9 @@ void vtest_destroy_renderer(void)
> renderer.out_fd = -1;
> }
>
> -int vtest_send_caps(void)
> +int vtest_send_caps(uint32_t length_dw)
> {
> + UNUSED_PARAMETER(length_dw);
> uint32_t max_ver, max_size;
> void *caps_buf;
> uint32_t hdr_buf[2];
> @@ -182,8 +185,9 @@ end:
> return 0;
> }
>
> -int vtest_create_resource(void)
> +int vtest_create_resource(uint32_t length_dw)
> {
> + UNUSED_PARAMETER(length_dw);
> uint32_t res_create_buf[VCMD_RES_CREATE_SIZE];
> struct virgl_renderer_resource_create_args args;
> int ret;
> @@ -211,8 +215,9 @@ int vtest_create_resource(void)
> return ret;
> }
>
> -int vtest_resource_unref(void)
> +int vtest_resource_unref(uint32_t length_dw)
> {
> + UNUSED_PARAMETER(length_dw);
> uint32_t res_unref_buf[VCMD_RES_UNREF_SIZE];
> int ret;
> uint32_t handle;
> @@ -347,8 +352,9 @@ int vtest_transfer_put(uint32_t length_dw)
> return 0;
> }
>
> -int vtest_resource_busy_wait(void)
> +int vtest_resource_busy_wait(uint32_t length_dw)
> {
> + UNUSED_PARAMETER(length_dw);
> uint32_t bw_buf[VCMD_BUSY_WAIT_SIZE];
> int ret, fd;
> int flags;
> @@ -392,8 +398,9 @@ int vtest_resource_busy_wait(void)
> return 0;
> }
>
> -int vtest_renderer_create_fence(void)
> +int vtest_renderer_create_fence(uint32_t length_dw)
> {
> + UNUSED_PARAMETER(length_dw);
> virgl_renderer_create_fence(fence_id++, ctx_id);
> return 0;
> }
> diff --git a/vtest/vtest_server.c b/vtest/vtest_server.c
> index 918639b..d9b8077 100644
> --- a/vtest/vtest_server.c
> +++ b/vtest/vtest_server.c
> @@ -86,67 +86,75 @@ static int wait_for_socket_accept(int sock)
> return -1;
> }
>
> +static int (*_commands_ftable[])(uint32_t length_dw) = {
> + NULL /* CMD ids starts at 1 */,
> + vtest_send_caps,
> + vtest_create_resource,
> + vtest_resource_unref,
> + vtest_transfer_get,
> + vtest_transfer_put,
> + vtest_submit_cmd,
> + vtest_resource_busy_wait,
> + NULL /* vtest_create_renderer */,
> +};
> +
I would name the array simply "vtest_commands", make it const.
> static int run_renderer(int in_fd, int out_fd)
> {
> +#define COMMAND_COUNT (sizeof(_commands_ftable) / sizeof(*_commands_ftable))
> +
I suggest to have the define next to the array, named after the
variable name VTEST_COMMANDS_SIZE.
> int ret;
> uint32_t header[VTEST_HDR_SIZE];
> bool inited = false;
> -again:
> - ret = vtest_wait_for_fd_read(in_fd);
> - if (ret < 0)
> - goto fail;
> -
> - ret = vtest_block_read(in_fd, &header, sizeof(header));
> -
> - if (ret == 8) {
> - if (!inited) {
> - if (header[1] != VCMD_CREATE_RENDERER)
> - goto fail;
> - ret = vtest_create_renderer(in_fd, out_fd, header[0]);
> - inited = true;
> - }
> - vtest_poll();
> - switch (header[1]) {
> - case VCMD_GET_CAPS:
> - ret = vtest_send_caps();
> - break;
> - case VCMD_RESOURCE_CREATE:
> - ret = vtest_create_resource();
> - break;
> - case VCMD_RESOURCE_UNREF:
> - ret = vtest_resource_unref();
> - break;
> - case VCMD_SUBMIT_CMD:
> - ret = vtest_submit_cmd(header[0]);
> - break;
> - case VCMD_TRANSFER_GET:
> - ret = vtest_transfer_get(header[0]);
> - break;
> - case VCMD_TRANSFER_PUT:
> - ret = vtest_transfer_put(header[0]);
> - break;
> - case VCMD_RESOURCE_BUSY_WAIT:
> - vtest_renderer_create_fence();
> - ret = vtest_resource_busy_wait();
> - break;
> - default:
> - break;
> - }
>
> - if (ret < 0) {
> - goto fail;
> - }
> + do {
> + ret = vtest_wait_for_fd_read(in_fd);
> + if (ret < 0) {
> + break;
> + }
> +
> + ret = vtest_block_read(in_fd, &header, sizeof(header));
> + if (ret != 8) {
> + break;
> + }
> +
> + if (!inited) {
> + /* The first command MUST be VCMD_CREATE_RENDERER */
> + if (header[1] != VCMD_CREATE_RENDERER) {
> + break;
> + }
> +
> + ret = vtest_create_renderer(in_fd, out_fd, header[0]);
> + inited = true;
> + vtest_poll();
> + continue;
> + }
> +
> + vtest_poll();
> + if (header[1] <= 0 || header[1] > COMMAND_COUNT) {
> + break;
> + }
> +
> + /* only specific case to handle */
> + if (header[1] == VCMD_RESOURCE_BUSY_WAIT) {
> + vtest_renderer_create_fence(header[0]);
why that specific case? Couldn't the create_fence() be moved to
vtest_resource_busy_wait() ?
> + }
> +
> + if (_commands_ftable[header[1]] == NULL) {
> + break;
> + }
> +
> + ret = _commands_ftable[header[1]](header[0]);
> + if (ret < 0) {
> + break;
> + }
> + } while (1);
>
> - goto again;
> - }
> - if (ret <= 0) {
> - goto fail;
> - }
> -fail:
> fprintf(stderr, "socket failed - closing renderer\n");
> +
> vtest_destroy_renderer();
> close(in_fd);
> return 0;
> +#undef COMMAND_COUNT
no need to undefine it imho
> }
>
> int main(int argc, char **argv)
> --
> 2.17.0
>
> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel
More information about the virglrenderer-devel
mailing list