| Message ID | 1440570108-17354-2-git-send-email-human.hwang@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Hyungwon Hwang On 26 August 2015 at 07:21, Hyungwon Hwang <human.hwang@samsung.com> wrote: > This patch adds support for atomic modeset. Using -a option, user can > make modeset to use DRM_IOCTL_MODE_ATOMIC instead of legacy IOCTLs. > Also, by using -w option, user can set the property as before. > > Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com> > --- > tests/modetest/modetest.c | 221 +++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 187 insertions(+), 34 deletions(-) > > diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c > index b7f6d32..753a559 100644 > --- a/tests/modetest/modetest.c > +++ b/tests/modetest/modetest.c > @@ -1444,25 +1444,32 @@ static int parse_property(struct property_arg *p, const char *arg) > > static void usage(char *name) > { > - fprintf(stderr, "usage: %s [-cDdefMPpsCvw]\n", name); > + fprintf(stderr, "usage: %s [-acDdefMPpsCvw]\n", name); > + fprintf(stderr, "\tA: supported in atomic modeset\n"); > + fprintf(stderr, "\tL: supported in legacy modeset\n"); > > - fprintf(stderr, "\n Query options:\n\n"); > + fprintf(stderr, "\n Query options: [AL]\n\n"); > fprintf(stderr, "\t-c\tlist connectors\n"); > fprintf(stderr, "\t-e\tlist encoders\n"); > fprintf(stderr, "\t-f\tlist framebuffers\n"); > fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n"); > > - fprintf(stderr, "\n Test options:\n\n"); > + fprintf(stderr, "\n Common Test options: [AL]\n\n"); > + fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n"); > + > + fprintf(stderr, "\n Atomic Test options: [A]\n\n"); > + fprintf(stderr, "\t-a\tuse atomic modeset\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"); > fprintf(stderr, "\t-s <connector_id>[,<connector_id>][@<crtc_id>]:<mode>[-<vrefresh>][@<format>]\tset a mode\n"); > fprintf(stderr, "\t-C\ttest hw cursor\n"); > fprintf(stderr, "\t-v\ttest vsynced page flipping\n"); > - fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n"); > > fprintf(stderr, "\n Generic options:\n\n"); > - fprintf(stderr, "\t-d\tdrop master after mode set\n"); > - fprintf(stderr, "\t-M module\tuse the given driver\n"); > - fprintf(stderr, "\t-D device\tuse the given device\n"); > + fprintf(stderr, "\t-d\tdrop master after mode set [L]\n"); > + fprintf(stderr, "\t-M module\tuse the given driver [AL]\n"); > + fprintf(stderr, "\t-D device\tuse the given device [AL]\n"); > Readability is greatly impaired with this approach. Can I suggest splitting these into two sections - legacy and atomic. > fprintf(stderr, "\n\tDefault is to dump all info.\n"); > exit(0); > @@ -1495,7 +1502,132 @@ static int cursor_supported(void) > return 1; > } > > -static char optstr[] = "cdD:efM:P:ps:Cvw:"; > +static uint32_t is_obj_id_in_prop_args(struct property_arg *prop_args, > + unsigned int prop_count, uint32_t obj_id) Function name implies bool return value, as does the true/false below, yet you define it as "uint32_t" ? > +{ > + unsigned int i; > + > + for (i = 0; i < prop_count; i++) > + if (obj_id == prop_args[i].obj_id) > + return true; > + > + return false; > +} > + > +static int get_value_in_prop_args(struct property_arg *prop_args, > + unsigned int prop_count, uint32_t obj_id, > + const char *name) > +{ > + unsigned int i; > + > + for (i = 0; i < prop_count; i++) > + if (prop_args[i].obj_id == obj_id && > + !strcmp(prop_args[i].name, name)) Indentation seems off. Align prop_args... with !strcmp... > + return (int)prop_args[i].value; > + > + return -1; > +} > + > +static uint32_t get_atomic_plane_prop_id(struct resources *res, uint32_t obj_id, > + const char *name) > +{ > + drmModePropertyRes *props_info; > + struct plane *plane; > + unsigned int i, j; > + > + for (i = 0; i < res->plane_res->count_planes; i++) { > + plane = &res->planes[i]; > + if (plane->plane->plane_id != obj_id) > + continue; > + > + for (j = 0; j < plane->props->count_props; j++) { > + props_info = plane->props_info[j]; > + if (!strcmp(props_info->name, name)) > + return props_info->prop_id; > + } > + } > + > + return 0; > +} > + > +static int allocate_fb(struct device *dev, drmModeAtomicReqPtr req, struct resources *res, > + uint32_t width, uint32_t height, uint32_t pixel_format, > + int pattern, struct bo **bo, uint32_t *fb_id) > +{ > + uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0}; > + int ret; > + > + *bo = bo_create(dev->fd, pixel_format, width, height, > + handles, pitches, offsets, pattern); > + if (*bo == NULL) { > + fprintf(stderr, "failed to create bo (%ux%u): %s\n", > + width, height, strerror(errno)); > + return -1; > + } > + > + ret = drmModeAddFB2(dev->fd, width, height, pixel_format, > + handles, pitches, offsets, fb_id, 0); > + if (ret) { > + fprintf(stderr, "failed to add fb (%ux%u): %s\n", > + width, height, strerror(errno)); > + bo_destroy(*bo); > + return ret; > + } > + > + return 0; > +} > + > +static int allocate_fbs(struct device *dev, drmModeAtomicReqPtr req, > + struct resources *res, struct property_arg *prop_args, > + unsigned int prop_count) > +{ > + uint32_t plane_id, fb_obj_id, fb_id, i, width, height, pixel_format; > + struct bo *bo; > + int ret; > + > + for (i = 0; i < res->plane_res->count_planes; i++) { > + plane_id = res->planes[i].plane->plane_id; > + if (!is_obj_id_in_prop_args(prop_args, prop_count, plane_id)) > + continue; > + > + 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"); > + pixel_format = DRM_FORMAT_XRGB8888; > + > + ret = allocate_fb(dev, req, res, width, height, pixel_format, > + PATTERN_SMPTE, &bo, &fb_id); > + if (ret < 0) > + return ret; > + > + dev->mode.bo = bo; > + dev->mode.fb_id = fb_id; > + > + fb_obj_id = get_atomic_plane_prop_id(res, plane_id, "FB_ID"); > + if (!fb_obj_id) Teardown the bo or error. > + return -1; > + > + ret = drmModeAtomicAddProperty(req, plane_id, fb_obj_id, fb_id); > + if (ret < 0) Ditto. > + return ret; > + } > + > + return 0; > +} > + > +static void atomic_modeset(struct device *dev, drmModeAtomicReqPtr req, > + struct resources *res, struct property_arg *prop_args, > + unsigned int prop_count) > +{ > + uint32_t flags = 0; > + > + allocate_fbs(dev, req, res, prop_args, prop_count); > + > + drmModeAtomicCommit(dev->fd, req, flags, NULL); Check if these succeed, teardown when needed and return the result ? > +} > + > +static char optstr[] = "acdD:efM:P:ps:Cvw:"; > > int main(int argc, char **argv) > { > @@ -1515,6 +1647,8 @@ int main(int argc, char **argv) > struct pipe_arg *pipe_args = NULL; > struct plane_arg *plane_args = NULL; > struct property_arg *prop_args = NULL; > + drmModeAtomicReqPtr req = NULL; > + char *obj_type = NULL; > unsigned int args = 0; > int ret; > > @@ -1525,6 +1659,11 @@ int main(int argc, char **argv) > args++; > > switch (c) { > + case 'a': > + if (!req) > + req = drmModeAtomicAlloc(); Currently you'll silently fall back to legacy if this fails. This sounds like a bad idea, imho. Check for failure and error out ? > + args--; > + break; > case 'c': > connectors = 1; > break; > @@ -1646,6 +1785,9 @@ int main(int argc, char **argv) > return -1; > } > > + if (req) > + drmSetClientCap(dev.fd, DRM_CLIENT_CAP_ATOMIC, 1); > + Again check for error and bail out ? > dev.resources = get_resources(&dev); > if (!dev.resources) { > drmClose(dev.fd); > @@ -1660,46 +1802,57 @@ int main(int argc, char **argv) > dump_resource(&dev, planes); > dump_resource(&dev, framebuffers); > > - for (i = 0; i < prop_count; ++i) > - set_property(&dev, &prop_args[i]); > + if (req) { > + for (i = 0; i < prop_count; ++i) { > + get_prop_info(dev.resources, &prop_args[i], obj_type); > + drmModeAtomicAddProperty(req, prop_args[i].obj_id, > + prop_args[i].prop_id, prop_args[i].value); Check for failure and warn/error accordingly ? > + } > > - if (count || plane_count) { > - uint64_t cap = 0; > + atomic_modeset(&dev, req, dev.resources, prop_args, prop_count); > Ditto ? Maybe I'm confused by the diffstat, but I cannot see where you cleanup all the state calloc'd in allocate_fbs (and further down). Fwiw most of this can be left unchanged if you opt for the following approach. if (req) { do_atomic_stuff() cleanup() return X } existing legacy code. Last but not least: for good measure check this with valgrind. Cheers Emil
On 2 September 2015 at 01:11, Emil Velikov <emil.l.velikov@gmail.com> wrote: > Hi Hyungwon Hwang > > On 26 August 2015 at 07:21, Hyungwon Hwang <human.hwang@samsung.com> wrote: >> This patch adds support for atomic modeset. Using -a option, user can >> make modeset to use DRM_IOCTL_MODE_ATOMIC instead of legacy IOCTLs. >> Also, by using -w option, user can set the property as before. >> >> Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com> >> --- >> tests/modetest/modetest.c | 221 +++++++++++++++++++++++++++++++++++++++------- >> 1 file changed, 187 insertions(+), 34 deletions(-) >> >> diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c >> index b7f6d32..753a559 100644 >> --- a/tests/modetest/modetest.c >> +++ b/tests/modetest/modetest.c >> @@ -1444,25 +1444,32 @@ static int parse_property(struct property_arg *p, const char *arg) >> >> static void usage(char *name) >> { >> - fprintf(stderr, "usage: %s [-cDdefMPpsCvw]\n", name); >> + fprintf(stderr, "usage: %s [-acDdefMPpsCvw]\n", name); >> + fprintf(stderr, "\tA: supported in atomic modeset\n"); >> + fprintf(stderr, "\tL: supported in legacy modeset\n"); >> >> - fprintf(stderr, "\n Query options:\n\n"); >> + fprintf(stderr, "\n Query options: [AL]\n\n"); >> fprintf(stderr, "\t-c\tlist connectors\n"); >> fprintf(stderr, "\t-e\tlist encoders\n"); >> fprintf(stderr, "\t-f\tlist framebuffers\n"); >> fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n"); >> >> - fprintf(stderr, "\n Test options:\n\n"); >> + fprintf(stderr, "\n Common Test options: [AL]\n\n"); >> + fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n"); >> + >> + fprintf(stderr, "\n Atomic Test options: [A]\n\n"); >> + fprintf(stderr, "\t-a\tuse atomic modeset\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"); >> fprintf(stderr, "\t-s <connector_id>[,<connector_id>][@<crtc_id>]:<mode>[-<vrefresh>][@<format>]\tset a mode\n"); >> fprintf(stderr, "\t-C\ttest hw cursor\n"); >> fprintf(stderr, "\t-v\ttest vsynced page flipping\n"); >> - fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n"); >> >> fprintf(stderr, "\n Generic options:\n\n"); >> - fprintf(stderr, "\t-d\tdrop master after mode set\n"); >> - fprintf(stderr, "\t-M module\tuse the given driver\n"); >> - fprintf(stderr, "\t-D device\tuse the given device\n"); >> + fprintf(stderr, "\t-d\tdrop master after mode set [L]\n"); >> + fprintf(stderr, "\t-M module\tuse the given driver [AL]\n"); >> + fprintf(stderr, "\t-D device\tuse the given device [AL]\n"); >> > Readability is greatly impaired with this approach. Can I suggest > splitting these into two sections - legacy and atomic. > A bit tired and I've misread the above. Please ignore this comment. -Emil
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index b7f6d32..753a559 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -1444,25 +1444,32 @@ static int parse_property(struct property_arg *p, const char *arg) static void usage(char *name) { - fprintf(stderr, "usage: %s [-cDdefMPpsCvw]\n", name); + fprintf(stderr, "usage: %s [-acDdefMPpsCvw]\n", name); + fprintf(stderr, "\tA: supported in atomic modeset\n"); + fprintf(stderr, "\tL: supported in legacy modeset\n"); - fprintf(stderr, "\n Query options:\n\n"); + fprintf(stderr, "\n Query options: [AL]\n\n"); fprintf(stderr, "\t-c\tlist connectors\n"); fprintf(stderr, "\t-e\tlist encoders\n"); fprintf(stderr, "\t-f\tlist framebuffers\n"); fprintf(stderr, "\t-p\tlist CRTCs and planes (pipes)\n"); - fprintf(stderr, "\n Test options:\n\n"); + fprintf(stderr, "\n Common Test options: [AL]\n\n"); + fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n"); + + fprintf(stderr, "\n Atomic Test options: [A]\n\n"); + fprintf(stderr, "\t-a\tuse atomic modeset\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"); fprintf(stderr, "\t-s <connector_id>[,<connector_id>][@<crtc_id>]:<mode>[-<vrefresh>][@<format>]\tset a mode\n"); fprintf(stderr, "\t-C\ttest hw cursor\n"); fprintf(stderr, "\t-v\ttest vsynced page flipping\n"); - fprintf(stderr, "\t-w <obj_id>:<prop_name>:<value>\tset property\n"); fprintf(stderr, "\n Generic options:\n\n"); - fprintf(stderr, "\t-d\tdrop master after mode set\n"); - fprintf(stderr, "\t-M module\tuse the given driver\n"); - fprintf(stderr, "\t-D device\tuse the given device\n"); + fprintf(stderr, "\t-d\tdrop master after mode set [L]\n"); + fprintf(stderr, "\t-M module\tuse the given driver [AL]\n"); + fprintf(stderr, "\t-D device\tuse the given device [AL]\n"); fprintf(stderr, "\n\tDefault is to dump all info.\n"); exit(0); @@ -1495,7 +1502,132 @@ static int cursor_supported(void) return 1; } -static char optstr[] = "cdD:efM:P:ps:Cvw:"; +static uint32_t is_obj_id_in_prop_args(struct property_arg *prop_args, + unsigned int prop_count, uint32_t obj_id) +{ + unsigned int i; + + for (i = 0; i < prop_count; i++) + if (obj_id == prop_args[i].obj_id) + return true; + + return false; +} + +static int get_value_in_prop_args(struct property_arg *prop_args, + unsigned int prop_count, uint32_t obj_id, + const char *name) +{ + unsigned int i; + + for (i = 0; i < prop_count; i++) + if (prop_args[i].obj_id == obj_id && + !strcmp(prop_args[i].name, name)) + return (int)prop_args[i].value; + + return -1; +} + +static uint32_t get_atomic_plane_prop_id(struct resources *res, uint32_t obj_id, + const char *name) +{ + drmModePropertyRes *props_info; + struct plane *plane; + unsigned int i, j; + + for (i = 0; i < res->plane_res->count_planes; i++) { + plane = &res->planes[i]; + if (plane->plane->plane_id != obj_id) + continue; + + for (j = 0; j < plane->props->count_props; j++) { + props_info = plane->props_info[j]; + if (!strcmp(props_info->name, name)) + return props_info->prop_id; + } + } + + return 0; +} + +static int allocate_fb(struct device *dev, drmModeAtomicReqPtr req, struct resources *res, + uint32_t width, uint32_t height, uint32_t pixel_format, + int pattern, struct bo **bo, uint32_t *fb_id) +{ + uint32_t handles[4] = {0}, pitches[4] = {0}, offsets[4] = {0}; + int ret; + + *bo = bo_create(dev->fd, pixel_format, width, height, + handles, pitches, offsets, pattern); + if (*bo == NULL) { + fprintf(stderr, "failed to create bo (%ux%u): %s\n", + width, height, strerror(errno)); + return -1; + } + + ret = drmModeAddFB2(dev->fd, width, height, pixel_format, + handles, pitches, offsets, fb_id, 0); + if (ret) { + fprintf(stderr, "failed to add fb (%ux%u): %s\n", + width, height, strerror(errno)); + bo_destroy(*bo); + return ret; + } + + return 0; +} + +static int allocate_fbs(struct device *dev, drmModeAtomicReqPtr req, + struct resources *res, struct property_arg *prop_args, + unsigned int prop_count) +{ + uint32_t plane_id, fb_obj_id, fb_id, i, width, height, pixel_format; + struct bo *bo; + int ret; + + for (i = 0; i < res->plane_res->count_planes; i++) { + plane_id = res->planes[i].plane->plane_id; + if (!is_obj_id_in_prop_args(prop_args, prop_count, plane_id)) + continue; + + 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"); + pixel_format = DRM_FORMAT_XRGB8888; + + ret = allocate_fb(dev, req, res, width, height, pixel_format, + PATTERN_SMPTE, &bo, &fb_id); + if (ret < 0) + return ret; + + dev->mode.bo = bo; + dev->mode.fb_id = fb_id; + + fb_obj_id = get_atomic_plane_prop_id(res, plane_id, "FB_ID"); + if (!fb_obj_id) + return -1; + + ret = drmModeAtomicAddProperty(req, plane_id, fb_obj_id, fb_id); + if (ret < 0) + return ret; + } + + return 0; +} + +static void atomic_modeset(struct device *dev, drmModeAtomicReqPtr req, + struct resources *res, struct property_arg *prop_args, + unsigned int prop_count) +{ + uint32_t flags = 0; + + allocate_fbs(dev, req, res, prop_args, prop_count); + + drmModeAtomicCommit(dev->fd, req, flags, NULL); +} + +static char optstr[] = "acdD:efM:P:ps:Cvw:"; int main(int argc, char **argv) { @@ -1515,6 +1647,8 @@ int main(int argc, char **argv) struct pipe_arg *pipe_args = NULL; struct plane_arg *plane_args = NULL; struct property_arg *prop_args = NULL; + drmModeAtomicReqPtr req = NULL; + char *obj_type = NULL; unsigned int args = 0; int ret; @@ -1525,6 +1659,11 @@ int main(int argc, char **argv) args++; switch (c) { + case 'a': + if (!req) + req = drmModeAtomicAlloc(); + args--; + break; case 'c': connectors = 1; break; @@ -1646,6 +1785,9 @@ int main(int argc, char **argv) return -1; } + if (req) + drmSetClientCap(dev.fd, DRM_CLIENT_CAP_ATOMIC, 1); + dev.resources = get_resources(&dev); if (!dev.resources) { drmClose(dev.fd); @@ -1660,46 +1802,57 @@ int main(int argc, char **argv) dump_resource(&dev, planes); dump_resource(&dev, framebuffers); - for (i = 0; i < prop_count; ++i) - set_property(&dev, &prop_args[i]); + if (req) { + for (i = 0; i < prop_count; ++i) { + get_prop_info(dev.resources, &prop_args[i], obj_type); + drmModeAtomicAddProperty(req, prop_args[i].obj_id, + prop_args[i].prop_id, prop_args[i].value); + } - if (count || plane_count) { - uint64_t cap = 0; + atomic_modeset(&dev, req, dev.resources, prop_args, prop_count); - ret = drmGetCap(dev.fd, DRM_CAP_DUMB_BUFFER, &cap); - if (ret || cap == 0) { - fprintf(stderr, "driver doesn't support the dumb buffer API\n"); - return 1; - } + getchar(); + } else { + for (i = 0; i < prop_count; ++i) + set_property(&dev, &prop_args[i]); - if (count) - set_mode(&dev, pipe_args, count); + if (count || plane_count) { + uint64_t cap = 0; - if (plane_count) - set_planes(&dev, plane_args, plane_count); + ret = drmGetCap(dev.fd, DRM_CAP_DUMB_BUFFER, &cap); + if (ret || cap == 0) { + fprintf(stderr, "driver doesn't support the dumb buffer API\n"); + return 1; + } - if (test_cursor) - set_cursors(&dev, pipe_args, count); + if (count) + set_mode(&dev, pipe_args, count); - if (test_vsync) - test_page_flip(&dev, pipe_args, count); + if (plane_count) + set_planes(&dev, plane_args, plane_count); - if (drop_master) - drmDropMaster(dev.fd); + if (test_cursor) + set_cursors(&dev, pipe_args, count); - getchar(); + if (test_vsync) + test_page_flip(&dev, pipe_args, count); - if (test_cursor) - clear_cursors(&dev); + if (drop_master) + drmDropMaster(dev.fd); - if (plane_count) - clear_planes(&dev, plane_args, plane_count); + getchar(); - if (count) - clear_mode(&dev); + if (test_cursor) + clear_cursors(&dev); + + if (plane_count) + clear_planes(&dev, plane_args, plane_count); + } } + clear_mode(&dev); free_resources(dev.resources); + drmFree(req); return 0; }
This patch adds support for atomic modeset. Using -a option, user can make modeset to use DRM_IOCTL_MODE_ATOMIC instead of legacy IOCTLs. Also, by using -w option, user can set the property as before. Signed-off-by: Hyungwon Hwang <human.hwang@samsung.com> --- tests/modetest/modetest.c | 221 +++++++++++++++++++++++++++++++++++++++------- 1 file changed, 187 insertions(+), 34 deletions(-)