Re: [PATCH 2/3] modetest: add atomic modeset support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Hyungwon Hwang

On 26 August 2015 at 07:21, Hyungwon Hwang <human.hwang@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> ---
>  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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux