Re: [PATCH v3 3/3] modetest: add atomic page flip support

On 23 September 2015 at 03:45, Hyungwon Hwang human.hwang@samsung.com wrote:

This patch adds support for atomic page flip. User can specify -V option with the plane id for testing atomic page flipping.

Signed-off-by: Hyungwon Hwang human.hwang@samsung.com

tests/modetest/modetest.c | 195 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 187 insertions(+), 8 deletions(-)

diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index bc5a227..418acaa 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -747,6 +747,10 @@ struct pipe_arg { struct timeval start;

    int swap_count;

•   /* for atomic modeset */
  

•   uint32_t plane_id;
  

•   uint32_t fb_obj_id;
  

};

struct plane_arg { @@ -1477,7 +1481,7 @@ static int parse_property(struct property_arg *p, const char *arg)

static void usage(char *name) {

•   fprintf(stderr, "usage: %s [-acDdefMPpsCvw]\n", name);
  

•   fprintf(stderr, "usage: %s [-acDdefMPpsCvVw]\n", name);
    fprintf(stderr, "\tA: supported in atomic modeset\n");
    fprintf(stderr, "\tL: supported in legacy modeset\n");
  
  

@@ -1492,6 +1496,7 @@ static void usage(char *name)

    fprintf(stderr, "\n Atomic Test options: [A]\n\n");
    fprintf(stderr, "\t-a\tuse atomic modeset\n");

•   fprintf(stderr, "\t-V <flipping_plane_id>\ttest vsynced page flipping\n");
  
    fprintf(stderr, "\n Legacy test options: [L]\n\n");
    fprintf(stderr, "\t-P <crtc_id>:<w>x<h>[+<x>+<y>][*<scale>][@<format>]\tset a plane\n");
  

@@ -1641,7 +1646,8 @@ static int allocate_fb(int fd, drmModeAtomicReqPtr req, struct resources *res,

static int allocate_fbs(struct device *dev, drmModeAtomicReqPtr req, struct resources *res, struct property_arg *prop_args,

•                   unsigned int prop_count, struct bo **bo, uint32_t *fb_id)
  

•                   unsigned int prop_count, struct bo **bo, uint32_t *fb_id,
  

•                   uint32_t flip_plane_id)
  

{ uint32_t plane_id, fb_obj_id, pixel_format; uint64_t width, height; @@ -1652,6 +1658,9 @@ static int allocate_fbs(struct device *dev, drmModeAtomicReqPtr req, if (!is_obj_id_in_prop_args(prop_args, prop_count, plane_id)) continue;

•           if (flip_plane_id == plane_id)
  

•                   dev->mode.fb_id = fb_id[i];
  

•           fb_obj_id = get_plane_prop_id(res, plane_id, "FB_ID");
            if (!fb_obj_id) {
                    fprintf(stderr, "plane(%u) does not exist\n", plane_id);
  

@@ -1714,8 +1723,162 @@ static void deallocate_fbs(int fd, int num_planes, uint32_t *fb_id, struct bo ** } }

+static void atomic_page_flip_handler(int fd, unsigned int frame, unsigned int sec,

•                                   unsigned int usec, void *data)
  

+{

•   static drmModeAtomicReqPtr req = NULL;
  

Initialisation not needed

•   unsigned int new_fb_id;
  

•   struct timeval end;
  

•   struct pipe_arg *pipe;
  

•   double t;
  

•   uint32_t flags = 0;
  

Feed 0 directly into the function ?

•   int ret;
  

•   pipe = (struct pipe_arg *)(unsigned long)data;
  

•   req = drmModeAtomicAlloc();
  

•   if (!req) {
  

•           fprintf(stderr, "failed to allocate drmModeAtomicReqPtr\n");
  

•           return;
  

•   }
  

•   if (pipe->current_fb_id == pipe->fb_id[0])
  

•           new_fb_id = pipe->fb_id[1];
  

•   else
  

•           new_fb_id = pipe->fb_id[0];
  

•   pipe->current_fb_id = new_fb_id;
  

•   pipe->swap_count++;
  

•   ret = drmModeAtomicAddProperty(req, pipe->plane_id, pipe->fb_obj_id, new_fb_id);
  

•   if (ret < 0) {
  

•           fprintf(stderr, "failed to add atomic property in pageflip handler\n");
  

Use a goto label ?

•           drmModeAtomicFree(req);
  

•           return;
  

•   }
  

•   flags = DRM_MODE_PAGE_FLIP_EVENT;
  

•   ret = drmModeAtomicCommit(fd, req, flags, pipe);
  

•   if (ret < 0) {
  

•           fprintf(stderr, "failed to commit in pageflip handler\n");
  

•           drmModeAtomicFree(req);
  

•           return;
  

•   }
  

•   if (pipe->swap_count == 60) {
  

Use vrefresh here ?

•           gettimeofday(&end, NULL);
  

•           t = end.tv_sec + end.tv_usec * 1e-6 -
  

•                   (pipe->start.tv_sec + pipe->start.tv_usec * 1e-6);
  

•           fprintf(stderr, "freq: %.02fHz\n", pipe->swap_count / t);
  

•           pipe->swap_count = 0;
  

•           pipe->start = end;
  

•   }
  

•   drmModeAtomicFree(req);
  

+}

+static void atomic_test_page_flip(struct device *dev, drmModeAtomicReqPtr req,

•                   struct resources *res, struct property_arg *prop_args,
  

constify the structs ?

•                   unsigned int prop_count, unsigned int flip_plane_id)
  

+{

•   struct bo *other_bo;
  

•   unsigned int other_fb_id;
  

•   struct pipe_arg pipe;
  

•   drmEventContext evctx;
  

•   uint32_t flags = 0, fb_obj_id = 0, pixel_format;
  

Variable and initialisation isn't needed.

•   uint64_t width, height;
  

•   int ret;
  

•   fb_obj_id = get_plane_prop_id(res, flip_plane_id, "FB_ID");
  

•   if (!fb_obj_id) {
  

•           fprintf(stderr, "plane(%u) does not exist\n", flip_plane_id);
  

•           return;
  

•   }
  

•   if (!is_obj_id_in_prop_args(prop_args, prop_count, flip_plane_id)) {
  

•           fprintf(stderr, "plane (%u) must be set for pageflip\n", flip_plane_id);
  

•           return;
  

•   }
  

•   ret = get_value_in_prop_args(prop_args, prop_count, flip_plane_id,
  

•                                   "SRC_W", &width);
  

•   if (ret < 0) {
  

•           fprintf(stderr, "SRC_W for plane (%u) must be set\n", flip_plane_id);
  

•           return;
  

•   }
  

•   ret = get_value_in_prop_args(prop_args, prop_count, flip_plane_id,
  

•                                   "SRC_H", &height);
  

•   if (ret < 0) {
  

•           fprintf(stderr, "SRC_H for plane (%u) must be set\n", flip_plane_id);
  

•           return;
  

•   }
  

•   pixel_format = DRM_FORMAT_XRGB8888;
  

•   ret = allocate_fb(dev->fd, req, res, width, height, pixel_format,
  

•                   PATTERN_TILES, &other_bo, &other_fb_id);
  

•   if (ret < 0)
  

•           return;
  

•   ret = drmModeAtomicAddProperty(req, flip_plane_id, fb_obj_id,
  

•                   other_fb_id);
  

•   if (ret < 0) {
  

•           fprintf(stderr, "failed to add atomic property for pageflip");
  

•           goto err;
  

•   }
  

•   gettimeofday(&pipe.start, NULL);
  

•   pipe.swap_count = 0;
  

•   pipe.plane_id = flip_plane_id;
  

•   pipe.fb_obj_id = fb_obj_id;
  

•   pipe.fb_id[0] = dev->mode.fb_id;
  

•   pipe.fb_id[1] = other_fb_id;
  

•   pipe.current_fb_id = other_fb_id;
  

•   flags = DRM_MODE_PAGE_FLIP_EVENT;
  

•   ret = drmModeAtomicCommit(dev->fd, req, flags, &pipe);
  

•   if (ret < 0) {
  

•           fprintf(stderr, "failed to commit for pageflip\n");
  

•           goto err;
  

•   }
  

•   memset(&evctx, 0, sizeof evctx);
  

•   evctx.version = 2;
  

•   evctx.vblank_handler = NULL;
  

•   evctx.page_flip_handler = atomic_page_flip_handler;
  

•   while (1) {
  

•           struct timeval timeout = { .tv_sec = 3, .tv_usec = 0 };
  

•           fd_set fds;
  

•           FD_ZERO(&fds);
  

•           FD_SET(0, &fds);
  

•           FD_SET(dev->fd, &fds);
  

•           ret = select(dev->fd + 1, &fds, NULL, NULL, &timeout);
  

•           if (ret <= 0) {
  

•                   fprintf(stderr, "select timed out or error (ret %d)\n",
  

•                                   ret);
  

•                   continue;
  

•           } else if (FD_ISSET(0, &fds)) {
  

•                   break;
  

•           }
  

•           drmHandleEvent(dev->fd, &evctx);
  

•   }
  

+err:

•   drmModeRmFB(dev->fd, other_fb_id);
  

•   bo_destroy(other_bo);
  

•   return;
  

+}

static int atomic_modeset(struct device *dev, drmModeAtomicReqPtr req,

•                   struct property_arg *prop_args, unsigned int prop_count)
  

•                   struct property_arg *prop_args, unsigned int prop_count,
  

•                   int test_vsync, int flip_plane_id)
  

{ unsigned int num_planes; const char *obj_type = NULL; @@ -1753,7 +1916,8 @@ static int atomic_modeset(struct device *dev, drmModeAtomicReqPtr req, return -1; }

•   ret = allocate_fbs(dev, req, dev->resources, prop_args, prop_count, bo, fb_id);
  

•   ret = allocate_fbs(dev, req, dev->resources, prop_args, prop_count, bo,
  

•                           fb_id, flip_plane_id);
    if (ret < 0) {
            free(fb_id);
            free(bo);
  

@@ -1769,6 +1933,10 @@ static int atomic_modeset(struct device *dev, drmModeAtomicReqPtr req, return ret; }

•   if (test_vsync)
  

•           atomic_test_page_flip(dev, req, dev->resources,
  

•                           prop_args, prop_count, flip_plane_id);
  

•   getchar();
  
    deallocate_fbs(dev->fd, num_planes, fb_id, bo);
  

@@ -1778,7 +1946,7 @@ static int atomic_modeset(struct device *dev, drmModeAtomicReqPtr req, return 0; }

-static char optstr[] = "acdD:efM:P:ps:Cvw:"; +static char optstr[] = "acdD:efM:P:ps:CvV:w:";

int main(int argc, char **argv) { @@ -1792,7 +1960,7 @@ int main(int argc, char **argv) const char *modules[] = { "i915", "radeon", "nouveau", "vmwgfx", "omapdrm", "exynos", "tilcdc", "msm", "sti", "tegra", "imx-drm", "rockchip", "atmel-hlcdc" }; char *device = NULL; char *module = NULL;

•   unsigned int i;
  

•   unsigned int i, flip_plane_id = 0;
    unsigned int count = 0, plane_count = 0;
    unsigned int prop_count = 0;
    struct pipe_arg *pipe_args = NULL;
  

@@ -1873,6 +2041,11 @@ int main(int argc, char **argv) case 'v': test_vsync = 1; break;

•           case 'V':
  

•                   if (sscanf(optarg, "%u", &flip_plane_id) != 1)
  

•                           usage(argv[0]);
  

•                   test_vsync = 1;
  

•                   break;
            case 'w':
                    prop_args = realloc(prop_args,
                                       (prop_count + 1) * sizeof *prop_args);
  

@@ -1925,11 +2098,16 @@ int main(int argc, char **argv) return -1; }

•   if (test_vsync && !count) {
  

•   if (test_vsync && (!count && !is_atomic)) {
            fprintf(stderr, "page flipping requires at least one -s option.\n");
            return -1;
    }
  
  

•   if (test_vsync && is_atomic && flip_plane_id == 0) {
  

•           fprintf(stderr, "fliping plane id must be set for atomic pageflip\n");
  

And how does average Joe find out the ID ? Something feels quite funny with this approach.

Hyungwon, despite my nitpicking your work looks quite good. The above question is rather important and I take care of everything else -either before pushing or as a follow up patch-es, depending on your preference. Let me know how you feel about it.

Thanks Emil