Dear Emil,
On Wed, 02 Sep 2015 01:43:41 +0100
Emil Velikov <emil.l.velikov@xxxxxxxxx> wrote:
> On 26 August 2015 at 07:21, Hyungwon Hwang <human.hwang@xxxxxxxxxxx>
> wrote:
> > This patch adds support for atomic page flip. User can specify -V
> > option with the plane id for testing atomic page flipping.
> > ---
> > tests/modetest/modetest.c | 153
> > ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 149
> > insertions(+), 4 deletions(-)
> >
> > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c
> > index 753a559..9bffa98 100644
> > --- a/tests/modetest/modetest.c
> > +++ b/tests/modetest/modetest.c
> > @@ -719,6 +719,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 {
> > @@ -1444,7 +1448,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");
> >
> > @@ -1459,6 +1463,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");
> > @@ -1627,7 +1632,138 @@ static void atomic_modeset(struct device
> > *dev, drmModeAtomicReqPtr req, drmModeAtomicCommit(dev->fd, req,
> > flags, NULL); }
> >
> > -static char optstr[] = "acdD:efM:P:ps:Cvw:";
> > +static void
> > +atomic_page_flip_handler(int fd, unsigned int frame,
> > + unsigned int sec, unsigned int usec, void *data)
> > +{
> > + static drmModeAtomicReqPtr req = NULL;
> > + unsigned int new_fb_id;
> > + struct timeval end;
> > + struct pipe_arg *pipe;
> > + double t;
> > + uint32_t flags = 0;
> > +
> > + pipe = (struct pipe_arg *)(unsigned long)data;
> > +
> > + 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++;
> > +
> > + req = drmModeAtomicAlloc();
> > +
> > + drmModeAtomicAddProperty(req, pipe->plane_id,
> > pipe->fb_obj_id, new_fb_id); +
> We'll crash badly if req is NULL here. I guess we can smoke test the
> API, but that is no excuse for the missing null check.
>
> > + flags = DRM_MODE_PAGE_FLIP_EVENT;
> > + drmModeAtomicCommit(fd, req, flags, pipe);
> > +
> > + if (pipe->swap_count == 60) {
> > + 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 int atomic_test_page_flip(struct device *dev,
> > drmModeAtomicReqPtr req,
> > + struct resources *res, struct property_arg
> > *prop_args,
> > + unsigned int prop_count,unsigned int
> > plane_id) +{
> > + struct bo *other_bo;
> > + unsigned int other_fb_id;
> > + struct pipe_arg *pipe = NULL;
> > + drmEventContext evctx;
> > + uint32_t flags = 0, fb_obj_id = 0, pixel_format;
> > + uint64_t width, height;
> > + int ret;
> > +
> > + if (!is_obj_id_in_prop_args(prop_args, prop_count,
> > plane_id))
> > + return -1;
> > +
> > + fb_obj_id = get_atomic_plane_prop_id(res, plane_id,
> > "FB_ID");
> > + if (!fb_obj_id)
> > + return -1;
> > +
> > + width = get_value_in_prop_args(prop_args, prop_count,
> > plane_id,
> > + "SRC_W");
> > + height = get_value_in_prop_args(prop_args, prop_count,
> > plane_id,
> > + "SRC_H");
> Here and in the previous patch you assume that always succeeds.
> Perhaps one should check it otherwise all sort of crazy stuff will
> happen in allocate_fb() ?
>
> > + pixel_format = DRM_FORMAT_XRGB8888;
> > +
> For the future we might consider making this user configurable,
> although as is it looks like a good default.
>
> > + ret = allocate_fb(dev, req, res, width, height,
> > pixel_format,
> > + PATTERN_TILES, &other_bo, &other_fb_id);
> > + if (ret < 0)
> > + return ret;
> > +
> > + ret = drmModeAtomicAddProperty(req, plane_id, fb_obj_id,
> > + other_fb_id);
> > + if (ret < 0)
> Why do you return 'success' here ?
>
> > + goto err;
> > +
> > + if (!fb_obj_id) {
> Here (and in previous patch) we're missing RmFB()
> Leaking other_bo.
>
> > + fprintf(stderr, "page flipping requires at least
> > one plane setting.\n");
> > + return -1;
> > + }
> > +
> > + pipe = drmMalloc(sizeof *pipe);
> Please use calloc.
>
> > +
> > + gettimeofday(&pipe->start, NULL);
> > + pipe->swap_count = 0;
> > + pipe->plane_id = 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)
> Should you return the error code rather than 0 ?
>
> > + goto err_rmfb;
> > +
> > + memset(&evctx, 0, sizeof evctx);
> > + evctx.version = DRM_EVENT_CONTEXT_VERSION;
> This is very bad, please don't do it.
>
> DRM_EVENT_CONTEXT_VERSION defines the version currently present not
> the one you implement/use. They might match now, but as one bumps the
> define modetest build against it will end up very badly.
>
> Seems issue is present in the 'legacy' modeset path and vbltest :'(
Yes. It could be a problem in future. Then do you think new defines
must be set for the versions? or will it be OK just use a constant in
the code?
Thank you for your review.
Best regards,
Hyungwon Hwang
>
> > + 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;
> > + int ret;
> > +
> > + 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_rmfb:
> > + drmFree(pipe);
> > + drmModeRmFB(dev->fd, other_fb_id);
> > +err:
> > + bo_destroy(other_bo);
> > +
> > + return 0;
> > +}
> > +
> > +static char optstr[] = "acdD:efM:P:ps:CvV:w:";
> >
> > int main(int argc, char **argv)
> > {
> > @@ -1641,7 +1777,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;
> > int count = 0, plane_count = 0;
> > unsigned int prop_count = 0;
> > struct pipe_arg *pipe_args = NULL;
> > @@ -1723,6 +1859,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); @@ -1775,7 +1916,7 @@ int main(int argc, char
> > **argv) return -1;
> > }
> >
> > - if (test_vsync && !count) {
> > + if (test_vsync && (!count && !req)) {
> > fprintf(stderr, "page flipping requires at least
> > one -s option.\n");
> Something doesn't match here - the top level (help) description, the
> conditional and/or the error printf.
>
> Cheers,
> Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
