[RFC PATCH v3 2/3] drm: implement decoding of DRM ioctls
Zhibin Li
haoyouab at gmail.com
Wed Jul 31 06:49:45 UTC 2019
On Mon, Jul 29, 2019 at 9:58 AM Eugene Syromiatnikov <esyr at redhat.com>
wrote:
> This is just a cursory review, since I haven't looked at in-kernel
> DRM ioctl implementation yet.
>
> On Sat, Jul 27, 2019 at 11:57:16PM +0800, Zhibin Li wrote:
>
> > (drm_version, drm_unique, drm_get_map, drm_get_client, drm_get_stats,
> > drm_add_map, drm_add_bufs, drm_mark_bufs, drm_info_bufs, drm_map_bufs,
> > drm_free_bufs, drm_rm_map, drm_sarea_ctx, drm_res_ctx, drm_agp_enable,
> > drm_agp_info, drm_agp_alloc, drm_agp_free, drm_mode_get_connector,
> > drm_scatter_gather, drm_wait_vblank, drm_mode_getplaneresources,
> > drm_mode_add_fb2, drm_mode_obj_getproperties, drm_mode_obj_setproperty
> > drm_agp_bind): New functions.
> > (drm_ioctl_mpers): Use them.
>
> I don't thing the list of functions is required for a new file GNU-style
> changelog entry.
May I ask when this is changed? I guess I'm not following up.
> (print_drm_iowr, drm_decode_number, drm_get_magic, drm_irq_busid,
> > drm_set_version, drm_modeset_ctl, drm_gem_close, drm_gem_flink,
> > drm_gem_open, drm_get_cap, drm_set_client_cap, drm_auth_magic,
> > drm_noop, drm_control, drm_ctx, drm_rm_ctx, drm_get_ctx, drm_lock,
> > drm_prime_handle_to_fd, drm_prime_fd_to_handle, drm_crtc_get_sequence,
> > drm_crtc_queue_sequence, drm_mode_get_resources, drm_mode_crtc,
> > drm_mode_print_modeinfo, drm_mode_cursor, drm_mode_get_encoder,
> > drm_mode_gamma, drm_mode_get_property, drm_mode_set_property,
> > drm_mode_get_prop_blob, drm_mode_get_fb, drm_mode_add_fb,
> > drm_mode_rm_fb, drm_mode_page_flip, drm_mode_create_dumb,
> > drm_mode_dirty_fb, drm_mode_map_dumb, drm_mode_destroy_dumb,
> > drm_mode_getplane, drm_mode_setplane, drm_mode_cursor2,
> > drm_mode_atomic, drm_mode_createpropblob, drm_mode_destroypropblob,
> > drm_syncobj_create, drm_syncobj_destroy, drm_syncobj_handle_fd,
> > drm_syncobj_wait, drm_syncobj_reset_or_signal, drm_mode_create_lease,
> > drm_mode_list_lessees, drm_mode_get_lease, drm_mode_revoke_lease,
> > drm_syncobj_timeline_wait, drm_syncobj_query_or_timeline_signal,
> > drm_syncobj_transfer): New functions.
> > (drm_ioctl): Use them and use drm_ioctl_mpers.
>
> Ditto.
>
> > --- /dev/null
> > +++ b/drm.c
> > @@ -0,0 +1,1531 @@
> > +/*
> > + * Copyright (c) 2019 Patrik Jakobsson <pjakobsson at suse.de>
> The copyright of the original patch dates back to (at least) 2015[1],
> and it was claimed by Intel back then.
>
> Your copyright attribution is also missing (adding generic "The strace
> developers" attribution would be sufficient as well).
>
> [1]
> https://lists.freedesktop.org/archives/intel-gfx/2015-August/074252.html
Sure. Will do.
>
>
> + *
> > + * SPDX-License-Identifier: LGPL-2.1-or-later
> > + */
> > +
> > +#include "defs.h"
> > +#include "print_fields.h"
> > +
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +
>
> > +# ifdef HAVE_DRM_H
> > +# include <drm.h>
> > +# else
> > +# include <drm/drm.h>
> > +# endif
> I wonder what happens if neither of those is found.
> Note also that this file can be rather old and do not contain all the
> ioctl constants, so it is likely also worth having an xlat with all
> related DRM_IOC constants.
>
OK, I will give it a try. And does this also mean those ifdefs around
ioctl constants which you said are unreliable could be removed
because all DRM_IOC constants can be found in this case?
> +
> > +static void
> > +print_drm_iowr(const unsigned int nr, const unsigned int size,
> > + const char *str)
> > +{
> > + tprintf("DRM_IOWR(%#x, %#x) /* %s */", nr, size, str);
> This likely needs xlat style support.
>
Any example or reference?
> > +}
> > +
> > +int
> > +drm_decode_number(struct tcb *const tcp, const unsigned int code)
> > +{
> > + const unsigned int nr = _IOC_NR(code);
> > +
> > + if (_IOC_DIR(code) == (_IOC_READ | _IOC_WRITE)) {
> > + switch (nr) {
>
> > + case 0xa7:
> Unnecessary indentation.
>
Got it.
>
> > + /* In Linux commit v3.12-rc7~26^2~2 a u32
> padding was added */
> > + /* to struct drm_mode_get_connector so in
> old kernel headers */
> > + /* the size of this structure is 0x4c
> instead of 0x50. */
> drm argument sizes are constantly changing (please take a look at Gleb's
> ioctl-related commits, they can provide a rough impression; I'm not sure
> to which extent it is applicable to core DRM ioctls, however), a more
> robust way of decoding drm ioctl number have to be implemented.
>
> Yes I've seen Gleb updating ioctl entries according to the kernel release.
The only thing that is changing is the size. Would _IOC_NR() be enough?
Considering the code below, it may be the case for
> DRM_IOCTL_MODE_GETPLANERESOURCES, DRM_IOCTL_MODE_OBJ_GETPROPERTIES,
> and DRM_IOCTL_MODE_OBJ_SETPROPERTY as well.
>
>
It seems the corresponding structure of these three ioctl constants are not
changing.
The sizes of them are different among personalities because of
byte-alignment. I will
check git history to confirm that.
> + if (_IOC_SIZE(code) == 0x4c) {
> > + print_drm_iowr(nr, _IOC_SIZE(code),
> > +
> "DRM_IOCTL_MODE_GETCONNECTOR");
> > + return IOCTL_NUMBER_STOP_LOOKUP;
> > + } else {
> > + return 0;
> > + }
> > + }
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +drm_get_magic(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_auth auth;
> > +
> > + if (exiting(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &auth))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_U("{", auth, magic);
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > + }
> > +
> > + return 0;
>
>
> I'd rewrite exiting-only decoding as
>
> if (entering(tcp)) {
> tprints(", ");
> return 0;
> }
>
> /* exiting-only code */
>
> As it allows remove one indentation level. It's slightly less readable,
> though, but it is also common enough among strace decoders to sacrifice
> a bit of readability.
>
> The other thing is printing comma on exiting vs. on entering elsewhere,
> which is somewhat inconsistent.
>
> > +}
>
> > +# include "xlat/drm_modeset_cmd.h"
> I'd prefer to have all the includes at the beginning of the file, it
> simplifies maintenance of the code in the long run.
>
OK. I will move them all to the beginning right away. The code below
is similar so I rather not to reply all the "inline #include" one by one.
>
> > +# include "xlat/drm_capability.h"
> Ditto.
>
> > +
> > +static int
> > +drm_get_cap(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_get_cap cap;
> > +
> > + if (entering(tcp)) {
>
> > + tprints(", ");
> Okay, now I feel that this comma printing can well be re-factored out of
> the specific argument printing functions; moreover, that would make
> these printing functions structure printing funtions, ergo more
> re-usable (in case printing of the same structures will be needed
> somewhere else in the code).
>
> > + if (umove_or_printaddr(tcp, arg, &cap))
> > + return RVAL_IOCTL_DECODED;
>
> > + tprints("{capability=");
> > + printxval(drm_capability, cap.capability, "DRM_CAP_???");
> Why not "PRINT_FIELD_XVAL("{", cap, capability, drm_capability,
> "DRM_CAP_???")"?
>
>
Sorry I didn't notice there is a PRINT_FIELD_* macro for this.
> +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &cap)) {
> Adding a separate umove_no_syserror() macro might be a good idea.
>
>
You mean umove_nor_syserror() ? Should this macro be added to defs.h
or just in this file scope? I see some other code uses this, too.
> > + PRINT_FIELD_U(", ", cap, value);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
>
> > +# include "xlat/drm_client_capability.h"
> Inline include.
>
> > +
> > +static int
> > +drm_set_client_cap(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_set_client_cap cap;
> > +
> > + tprints(", ");
>
> > + if (!umove_or_printaddr(tcp, arg, &cap)) {
>
> I'd stick to
>
> if (umove_or_printaddr(tcp, arg, &cap))
> return RVAL_IOCTL_DECODED;
>
> /* umove success path */
>
> Again, for the sake of reducing indentation.
>
Sure, will follow this style in my code.
>
> > + tprints("{capability=");
> > + printxval(drm_client_capability, cap.capability,
> "DRM_CLIENT_CAP_???");
> Why not PRINT_FIELD_XVAL?
>
> > + PRINT_FIELD_U(", ", cap, value);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_auth_magic(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_auth auth;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &auth)) {
> > + PRINT_FIELD_U("{", auth, magic);
> I suspect that "magic" is expected to be hexadecimal.
>
I see that in the kernel code drivers/gpu/drm/drm_auth.c, there is a
DRM_DEBUG() invocation which prints magic using "%u" so I simply
follow that. Should magic numbers always be hexadecimal?
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
>
> > +# include "xlat/drm_control_func.h"
> Inline include.
>
> > +
> > +static int
> > +drm_control(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_control control;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &control)) {
>
> > + tprints("{func=");
> > + printxval(drm_control_func, control.func, "DRM_???");
> Why not PRINT_FIELD_XVAL?
>
> > + PRINT_FIELD_D(", ", control, irq);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_ctx(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_ctx ctx;
> > +
> > + if (exiting(tcp)) {
> Exiting-only decoding.
>
> > + tprints(", ");
> Comma on exiting.
>
> > + if (umove_or_printaddr(tcp, arg, &ctx))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", ctx, handle);
> Are handles sequential or random in nature? In the latter case, please
> consider hexadecimal output format.
>
> > + tprints("}");
>
> This code can be factored out into a separate function that is called by
> drm_ctx/drm_rm_ctx (and optionally drm_get_ctx).
>
Sure, will check it later.
>
> > +
> > + return RVAL_IOCTL_DECODED;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +drm_rm_ctx(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_ctx ctx;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
>
> > + if (umove_or_printaddr(tcp, arg, &ctx))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", ctx, handle);
> > + tprints("}");
> Can be factored out.
>
Sorry for so many redundant codes. This week I will spend part of
my time re-factoring codes.
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_ctx_flags.h"
> > +
> > +static int
> > +drm_get_ctx(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_ctx ctx;
> > +
> > + if (exiting(tcp)) {
> Exiting-only decoding.
>
> > + tprints(", ");
> Comma on exiting.
>
> > + if (umove_or_printaddr(tcp, arg, &ctx))
> > + return RVAL_IOCTL_DECODED;
>
> > + tprints("{flags=");
> > + printxval(drm_ctx_flags, ctx.flags, "_DRM_CONTEXT_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
Well that's what I was confused with. I thought printxval is enough for all
those printing so I didn't take a look into printflags. Obviously I was
wrong
and I should check all the flags in the code and use PRINT_FIELD_FLAGS
as you mentioned a lot of them in the comments. I'd not to bother replying
all of them because they are similar and I've already mark them down for
revision.
>
> Why handle field is not printed here? I suspect it has to be printed on
> entering, hasn't it?
>
I took reference from the updated kernel code. In
drivers/gpu/drm/drm_context.c: drm_legacy_getctx(), I couldn't see any use
of handle field and the flags is also marked to zero. Actually I'm not sure
what
is going on here. Any thoughts?
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > + }
> > +
> > + return 0;
> > +}
>
> > +# include "xlat/drm_lock_flags.h"
> Inline include.
>
> > +static int
> > +drm_lock(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_lock lock;
> > +
> > + if (exiting(tcp)) {
> Exiting-only decoding.
>
> > + tprints(", ");
> Comma on exiting.
>
> > + if (umove_or_printaddr(tcp, arg, &lock))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_D("{", lock, context);
>
> > + tprints(", flags=");
> > + printxval(drm_lock_flags, lock.flags, "_DRM_LOCK_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_XVAL?
>
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +drm_prime_handle_to_fd(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_prime_handle handle;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &handle))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", handle, handle);
>
> > + PRINT_FIELD_X(", ", handle, flags);
> The flags field can have DRM_CLOEXEC and DRM_RDWR flags.
>
OK. Seems I've missed some of the flags decoding.
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &handle)) {
>
> > + PRINT_FIELD_U(", ", handle, fd);
> Why not PRINT_FIELD_FD?
>
Will use PRINT_FIELD_FD.
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_prime_fd_to_handle(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_prime_handle handle;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &handle))
> > + return RVAL_IOCTL_DECODED;
>
> > + PRINT_FIELD_U("{", handle, fd);
> Why not PRINT_FIELD_FD?
>
> This also makes order of printing different comparing to order of fields
> in the structure.
>
It seems this one is using the fd the get a handle, shouldn't it be printed
in such order?
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &handle)) {
>
> > + PRINT_FIELD_U(", ", handle, handle);
> I feel that drm_handle_t likely needs a separate PRINT_FIELD_* macro.
>
You mean like others in print_fields.h?
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +
> > +}
> > +
>
> > +# ifdef DRM_IOCTL_CRTC_GET_SEQUENCE
> Ugh, those ifdef's make strace behaviour depend on the version of drm
> headers it is built against, which is quite unreliable.
>
That's true. Will adding xlat for all of the DRM_IOC constants avoid this?
> > +static int
> > +drm_crtc_get_sequence(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_crtc_get_sequence seq;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &seq))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", seq, crtc_id);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &seq)) {
> > + PRINT_FIELD_U(", ", seq, active);
> > + PRINT_FIELD_U(", ", seq, sequence);
> > + PRINT_FIELD_D(", ", seq, sequence_ns);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
> > +
> > +# include "xlat/drm_crtc_sequence_flags.h"
> > +
> > +static int
> > +drm_crtc_queue_sequence(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_crtc_queue_sequence seq;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &seq))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{");
> It's probably better to put this opening brace printing into respective
> PRINT_FIELD_* macros.
>
Indeed.
>
> > + if (entering(tcp)) {
> > + PRINT_FIELD_U("", seq, crtc_id);
>
> > + tprints(", flags=");
> > + printxval(drm_crtc_sequence_flags, seq.flags,
> "DRM_CRTC_SEQUENCE_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_XVAL?
>
> > + PRINT_FIELD_U(", ", seq, user_data);
> user_data is opaque and likely has to be printed as hexadecimal.
>
OK.
> > + PRINT_FIELD_U(", ", seq, sequence);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + PRINT_FIELD_U("", seq, sequence);
> ... or at least put this into "else" block, just to improve readability a
> bit.
>
There are some other similar parts in the code. Should I use else block
for all of them or just because this is only one line?
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +static int
> > +drm_mode_get_resources(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_card_res res;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &res))
> > + return RVAL_IOCTL_DECODED;
>
> > + PRINT_FIELD_PTR("{", res, fb_id_ptr);
> > + PRINT_FIELD_PTR(", ", res, crtc_id_ptr);
> > + PRINT_FIELD_PTR(", ", res, connector_id_ptr);
> > + PRINT_FIELD_PTR(", ", res, encoder_id_ptr);
> Why PRINT_FIELD_PTR if this is a non-mpers ioctl? Should PRINT_FIELD_ADDR
> or PRINT_FIELD_ADDR64 be used, for example?
>
Oh sorry. I didn't notice that there is a mpers_ptr_t type casting in
PRINT_FIELD_PTR and I just simply use it as its name imply.
BTW, I think a short comment or description of this PRINT_FIELD_*
part would make it more clear considering some of them are similar.
>
> Those are arrays of count_* elements (which are R/W, by the way), they
> likely
> has to be treated as such.
>
Directly print all the array elements, you mean? Any elegant way
to do this? I mean putting a lot of for loop makes a lot of redundant
code. Will a macro or function be better?
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &res)) {
> > + PRINT_FIELD_U(", ", res, count_fbs);
> > + PRINT_FIELD_U(", ", res, count_crtcs);
> > + PRINT_FIELD_U(", ", res, count_connectors);
> > + PRINT_FIELD_U(", ", res, count_encoders);
> > + PRINT_FIELD_U(", ", res, min_width);
> > + PRINT_FIELD_U(", ", res, max_width);
> > + PRINT_FIELD_U(", ", res, min_height);
> > + PRINT_FIELD_U(", ", res, max_height);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_mode_type.h"
> > +# include "xlat/drm_mode_flags.h"
> Inline include.
>
> > +
> > +static void
> > +drm_mode_print_modeinfo(struct drm_mode_modeinfo *info)
> > +{
> Why curly braces are not printed here?
>
They are printed in the caller of this function but seems printing
them here is better. Will change this.
>
> > + PRINT_FIELD_U("", *info, clock);
> > + PRINT_FIELD_U(", ", *info, hdisplay);
> > + PRINT_FIELD_U(", ", *info, hsync_start);
> > + PRINT_FIELD_U(", ", *info, hsync_end);
> > + PRINT_FIELD_U(", ", *info, htotal);
> > + PRINT_FIELD_U(", ", *info, hskew);
> > + PRINT_FIELD_U(", ", *info, vdisplay);
> > + PRINT_FIELD_U(", ", *info, vsync_start);
> > + PRINT_FIELD_U(", ", *info, vsync_end);
> > + PRINT_FIELD_U(", ", *info, vtotal);
> > + PRINT_FIELD_U(", ", *info, vscan);
> > + PRINT_FIELD_U(", ", *info, vrefresh);
>
> > + tprints(", flags=");
> > + printxval(drm_mode_flags, info->flags, "DRM_MODE_FLAG_PIC_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > + tprints(", type=");
> > + printxval(drm_mode_type, info->type, "DRM_MODE_TYPE_???");
> Why not PRINT_FIELD_XVAL?
>
> > + PRINT_FIELD_CSTRING(", ", *info, name);
> > +}
> > +
> > +static int
> > +drm_mode_crtc(struct tcb *const tcp, const kernel_ulong_t arg,
> > + bool is_get)
> > +{
> > + struct drm_mode_crtc crtc;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &crtc))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", crtc, crtc_id);
> > + if (is_get)
> > + return 0;
> > + PRINT_FIELD_PTR(", ", crtc, set_connectors_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> It is treated as a pointer to u32 array containing count_connectors
> elements in kernel, why not printing it like that?
>
> > + PRINT_FIELD_U(", ", crtc, count_connectors);
> This one is R/W.
>
Oh my fault. As I see, most of the count_* fields are R/W and
that's what I missed. Will double-check them.
>
> > + PRINT_FIELD_U(", ", crtc, fb_id);
> > + PRINT_FIELD_U(", ", crtc, x);
> > + PRINT_FIELD_U(", ", crtc, y);
> > + PRINT_FIELD_U(", ", crtc, gamma_size);
> > + PRINT_FIELD_U(", ", crtc, mode_valid);
> > + tprints(", mode={");
> > +
> > + drm_mode_print_modeinfo(&crtc.mode);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &crtc)) {
> Why not check for "is_get" first here and avoid nested "if" at all?
>
OK.
> > + if (is_get) {
>
> > + PRINT_FIELD_PTR(", ", crtc, set_connectors_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> It is treated as a pointer to u32 array containing count_connectors
> elements in kernel, why not printing it like that?
>
> > + PRINT_FIELD_U(", ", crtc, count_connectors);
> > + PRINT_FIELD_U(", ", crtc, fb_id);
> > + PRINT_FIELD_U(", ", crtc, x);
> > + PRINT_FIELD_U(", ", crtc, y);
> > + PRINT_FIELD_U(", ", crtc, gamma_size);
> > + PRINT_FIELD_U(", ", crtc, mode_valid);
> > + tprints(", mode={");
> > +
> > + drm_mode_print_modeinfo(&crtc.mode);
> > + tprints("}");
> This code looks the same for entering and exiting, it probably can be
> factored
> out in a separate function (print_drm_mode_crtc_tail or something like
> that).
>
OK will try.
>
> > + }
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_cursor(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_cursor cursor;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &cursor))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_X("{", cursor, flags);
> It can have DRM_MODE_CURSOR_BO and DRM_MODE_CURSOR_MOVE.
>
> + PRINT_FIELD_U(", ", cursor, crtc_id);
> I feel like crtc_id needs a separate PRINT_FIELD_* macro.
OK.
> > + PRINT_FIELD_D(", ", cursor, x);
> > + PRINT_FIELD_D(", ", cursor, y);
> > + PRINT_FIELD_U(", ", cursor, width);
> > + PRINT_FIELD_U(", ", cursor, height);
> > + PRINT_FIELD_U(", ", cursor, handle);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_gamma(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_crtc_lut lut;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &lut))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + /* We don't print the entire table, just the pointers */
> > + PRINT_FIELD_U("{", lut, crtc_id);
> > + PRINT_FIELD_U(", ", lut, gamma_size);
>
> > + PRINT_FIELD_PTR(", ", lut, red);
> > + PRINT_FIELD_PTR(", ", lut, green);
> > + PRINT_FIELD_PTR(", ", lut, blue);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64? And why not retrieve and
> print these arrays?
>
OK will do.
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_mode_encoder_type.h"
> Inline include.
>
> > +
> > +static int
> > +drm_mode_get_encoder(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_get_encoder enc;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &enc))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_U("{", enc, encoder_id);
> > +
> > + if (entering(tcp)) {
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + tprints(", encoder_type=");
> > + printxval(drm_mode_encoder_type, enc.encoder_type,
> "DRM_MODE_ENCODER_???");
> Why not PRINT_FIELD_XVAL?
>
> > + PRINT_FIELD_U(", ", enc, crtc_id);
> > + PRINT_FIELD_X(", ", enc, possible_crtcs);
> > + PRINT_FIELD_X(", ", enc, possible_clones);
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_get_property(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_get_property prop;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &prop))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", prop, prop_id);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &prop)) {
>
> > + PRINT_FIELD_PTR(", ", prop, values_ptr);
> > + PRINT_FIELD_PTR(", ", prop, enum_blob_ptr);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> enum_blob_ptr is an array of count_enum_blobs elements of struct
> drm_mode_property_enum type, why not print it as such?
>
> > + PRINT_FIELD_X(", ", prop, flags);
> This can have DRM_MODE_PROP_PENDING, DRM_MODE_PROP_RANGE,
> DRM_MODE_PROP_IMMUTABLE, DRM_MODE_PROP_ENUM, DRM_MODE_PROP_BLOB,
> DRM_MODE_PROP_BITMASK as flags, and DRM_MODE_PROP_EXTENDED_TYPE in
> higher bits.
>
>
Seems I've missed a big part.
> > + PRINT_FIELD_CSTRING(", ", prop, name);
>
> > + PRINT_FIELD_U(", ", prop, count_values);
> > + PRINT_FIELD_U(", ", prop, count_enum_blobs);
> These are R/W.
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_set_property(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_connector_set_property prop;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &prop)) {
> > + PRINT_FIELD_U("{", prop, value);
> > + PRINT_FIELD_U(", ", prop, prop_id);
> > + PRINT_FIELD_U(", ", prop, connector_id);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_get_prop_blob(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_get_blob blob;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &blob))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", blob, blob_id);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &blob)) {
>
> > + PRINT_FIELD_U(", ", blob, length);
> This one is R/W.
>
> > + PRINT_FIELD_U(", ", blob, data);
> Data is a pointer to blob, it likely has to be printed using printstr
> with appropriate flags.
>
Will try it later.
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_get_fb(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_fb_cmd cmd;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &cmd))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", cmd, fb_id);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &cmd)) {
>
> > + PRINT_FIELD_U(", ", cmd, width);
> > + PRINT_FIELD_U(", ", cmd, height);
> > + PRINT_FIELD_U(", ", cmd, pitch);
> > + PRINT_FIELD_U(", ", cmd, bpp);
> > + PRINT_FIELD_U(", ", cmd, depth);
> > + PRINT_FIELD_U(", ", cmd, handle);
> This code is duplicated between drm_mode_get_fb and drm_mode_add_fb,
> it can be factored out in a separate function.
>
OK.
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_add_fb(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_fb_cmd cmd;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &cmd))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", cmd, width);
> > + PRINT_FIELD_U(", ", cmd, height);
> > + PRINT_FIELD_U(", ", cmd, pitch);
> > + PRINT_FIELD_U(", ", cmd, bpp);
> > + PRINT_FIELD_U(", ", cmd, depth);
> > + PRINT_FIELD_U(", ", cmd, handle);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove_or_printaddr(tcp, arg, &cmd)) {
> > + PRINT_FIELD_U(", ", cmd, fb_id);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_rm_fb(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + unsigned int handle;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &handle))
> > + tprintf("%u", handle);
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_mode_page_flip_flags.h"
> Inline include.
>
> > +
> > +static int
> > +drm_mode_page_flip(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_crtc_page_flip flip;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &flip)) {
> > + PRINT_FIELD_U("{", flip, crtc_id);
> > + PRINT_FIELD_U(", ", flip, fb_id);
>
> > + tprints(", flags=");
> > + printxval(drm_mode_page_flip_flags, flip.flags,
> "DRM_MODE_PAGE_FLIP_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > + PRINT_FIELD_X(", ", flip, user_data);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_dirty_fb(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_fb_dirty_cmd cmd;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &cmd)) {
> > + PRINT_FIELD_U("{", cmd, fb_id);
>
> > + PRINT_FIELD_X(", ", cmd, flags);
> This one can have DRM_MODE_FB_DIRTY_ANNOTATE_COPY and
> DRM_MODE_FB_DIRTY_ANNOTATE_FILL as flags.
>
> > + PRINT_FIELD_X(", ", cmd, color);
> > + PRINT_FIELD_U(", ", cmd, num_clips);
>
> > + PRINT_FIELD_PTR(", ", cmd, clips_ptr);
> Why not PRINT_FIELD_ADDr/PRINT_FIELD_ADDR64?
>
> clips_ptr is a pointer to an array of num_clips elements of struct
> drm_clip_rect
> type, why not print it as such?
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_destroy_dumb(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_destroy_dumb dumb;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &dumb)) {
> > + PRINT_FIELD_U("{", dumb, handle);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_getplane(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_get_plane plane;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &plane))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{");
> > + if (entering(tcp)) {
> > + PRINT_FIELD_U("", plane, plane_id);
>
> > + PRINT_FIELD_PTR(", ", plane, format_type_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> This is a pointer to array of count_format_types elements of type
> uint32_t, and likely has to be treated as such.
>
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + PRINT_FIELD_PTR("", plane, format_type_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> This is a pointer to array of count_format_types elements of type
> uint32_t, and likely has to be treated as such.
>
> > + PRINT_FIELD_U(", ", plane, crtc_id);
> > + PRINT_FIELD_U(", ", plane, fb_id);
> > + PRINT_FIELD_U(", ", plane, possible_crtcs);
> > + PRINT_FIELD_U(", ", plane, gamma_size);
>
> > + PRINT_FIELD_U(", ", plane, count_format_types);
> This one is R/W.
>
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_setplane(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_set_plane plane;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &plane))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", plane, plane_id);
> > + PRINT_FIELD_U(", ", plane, crtc_id);
> > + PRINT_FIELD_U(", ", plane, fb_id);
>
> > + PRINT_FIELD_X(", ", plane, flags);
> Seems like it can have DRM_MODE_PRESENT_TOP_FIELD and
> DRM_MODE_PRESENT_BOTTOM_FIELD flags.
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &plane)) {
> > + PRINT_FIELD_D(", ", plane, crtc_x);
> > + PRINT_FIELD_D(", ", plane, crtc_y);
>
> > + PRINT_FIELD_D(", ", plane, crtc_w);
> > + PRINT_FIELD_D(", ", plane, crtc_h);
> These are unsigned.
My fault.
>
>
> + PRINT_FIELD_D(", ", plane, src_x);
> > + PRINT_FIELD_D(", ", plane, src_y);
> > + PRINT_FIELD_D(", ", plane, src_h);
> > + PRINT_FIELD_D(", ", plane, src_w);
> These are unsigned and are in 16.16 fixed point format.
Well this is new to me. I should search for information.
>
>
> + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_cursor2(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_cursor2 cursor;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &cursor))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_X("{", cursor, flags);
> This one employs the same DRM_MODE_CURSOR_BO and DRM_MODE_CURSOR_MOVE
> flags as the flags field in drm_mode_cursor.
>
> > + PRINT_FIELD_U(", ", cursor, crtc_id);
> > + PRINT_FIELD_D(", ", cursor, x);
> > + PRINT_FIELD_D(", ", cursor, y);
> > + PRINT_FIELD_U(", ", cursor, width);
> > + PRINT_FIELD_U(", ", cursor, height);
> > + PRINT_FIELD_U(", ", cursor, handle);
> > + PRINT_FIELD_D(", ", cursor, hot_x);
> > + PRINT_FIELD_D(", ", cursor, hot_y);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_atomic(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_atomic atomic;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &atomic)) {
> > + PRINT_FIELD_X("{", atomic, flags);
> > + PRINT_FIELD_U(", ", atomic, count_objs);
>
> > + PRINT_FIELD_PTR(", ", atomic, objs_ptr);
> > + PRINT_FIELD_PTR(", ", atomic, count_props_ptr);
> > + PRINT_FIELD_PTR(", ", atomic, props_ptr);
> > + PRINT_FIELD_PTR(", ", atomic, prop_values_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> Those are arrays and likely hast to be treated as such.
>
> > + PRINT_FIELD_U(", ", atomic, reserved);
> It usually makes sense to print reserved fields only in case they are
> non-zero.
>
OK, padding field is similar right? Will check them all.
>
> > + PRINT_FIELD_U(", ", atomic, user_data);
> user_data is opaque and likely has to be printed as hexadecimal.
>
>
OK
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_createpropblob(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_create_blob blob;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &blob))
> > + return RVAL_IOCTL_DECODED;
> > +
>
> > + PRINT_FIELD_PTR("{", blob, data);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> Since it is a blob, it makes sense to print it using printstr with
> appropriate flags.
>
> > + PRINT_FIELD_U(", ", blob, length);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &blob)) {
> > + PRINT_FIELD_U(", ", blob, blob_id);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_destroypropblob(struct tcb *const tcp, const kernel_ulong_t
> arg)
> > +{
> > + struct drm_mode_destroy_blob blob;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &blob)) {
> > + PRINT_FIELD_U("{", blob, blob_id);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_CREATE
> > +
> > +# include "xlat/drm_syncobj_flags.h"
> > +
> > +static int
> > +drm_syncobj_create(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_syncobj_create create;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &create))
> > + return RVAL_IOCTL_DECODED;
> > +
>
> > + tprints("{flags=");
> > + printxval(drm_syncobj_flags, create.flags,
> "DRM_SYNCOJB_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &create)) {
> > + PRINT_FIELD_U(", ", create, handle);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_DESTROY
> > +static int
> > +drm_syncobj_destroy(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_syncobj_destroy destroy;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &destroy)) {
> > + PRINT_FIELD_U("{", destroy, handle);
>
> > + PRINT_FIELD_U(", ", destroy, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD
> > +static int
> > +drm_syncobj_handle_fd(struct tcb *const tcp, const kernel_ulong_t arg,
> > + bool is_handle_to_fd)
> > +{
> > + struct drm_syncobj_handle handle;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &handle))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + if (is_handle_to_fd)
> > + PRINT_FIELD_U("{", handle, handle);
> > + else
>
> > + PRINT_FIELD_D("{", handle, fd);
> Why not PRINT_FIELD_FD?
>
> > + PRINT_FIELD_X(", ", handle, flags);
> This one can have DRM_SYNCOBJ_FD_TO_HANDLE_FLAGS_IMPORT_SYNC_FILE or
> DRM_SYNCOBJ_HANDLE_TO_FD_FLAGS_EXPORT_SYNC_FILE flag.
>
> > + PRINT_FIELD_U(", ", handle, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &handle)) {
> > + if (is_handle_to_fd)
>
> > + PRINT_FIELD_D(", ", handle, fd);
> Why not PRINT_FIELD_FD?
>
> > + else
> > + PRINT_FIELD_U(", ", handle, handle);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_WAIT
> > +static int
> > +drm_syncobj_wait(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_syncobj_wait wait;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &wait))
> > + return RVAL_IOCTL_DECODED;
>
> > + PRINT_FIELD_PTR("{", wait, handles);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> This is an array of count_handles elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_D(", ", wait, timeout_nsec);
> If this timeout is absolute, it might make sense to print a time stamp in
> ISO 8601 format as well.
>
This one is also new to me. I will search for info later.
>
> > + PRINT_FIELD_U(", ", wait, count_handles);
>
> > + PRINT_FIELD_X(", ", wait, flags);
> This one can have DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL,
> ant DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT flags.
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &wait)) {
> > + PRINT_FIELD_U(", ", wait, first_signaled);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_RESET
> > +static int
> > +drm_syncobj_reset_or_signal(struct tcb *const tcp, const kernel_ulong_t
> arg)
> > +{
> > + struct drm_syncobj_array array;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &array)) {
>
> > + PRINT_FIELD_PTR("{", array, handles);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> This is an array of count_handles elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_U(", ", array, count_handles);
>
> > + PRINT_FIELD_U(", ", array, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_MODE_CREATE_LEASE
> > +static int
> > +drm_mode_create_lease(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_create_lease lease;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &lease))
> > + return RVAL_IOCTL_DECODED;
> > +
>
> > + PRINT_FIELD_PTR("{", lease, object_ids);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> This is an array of object_count elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_U(", ", lease, object_count);
>
> > + PRINT_FIELD_X(", ", lease, flags);
> It can have O_CLOEXEC and O_NONBLOCK, at least.
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &lease)) {
> > + PRINT_FIELD_U(", ", lease, lessee_id);
>
> > + PRINT_FIELD_U(", ", lease, fd);
> Why not PRINT_FIELD_FD?
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_MODE_LIST_LESSEES
> > +static int
> > +drm_mode_list_lessees(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_list_lessees lessees;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &lessees))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{");
> > + if (entering(tcp)) {
>
> > + PRINT_FIELD_U("", lessees, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > + PRINT_FIELD_PTR(", ",lessees, lessees_ptr);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> This is an array of count_lessees elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_U(", ", lessees, count_lessees);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + PRINT_FIELD_U("", lessees, count_lessees);
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_MODE_GET_LEASE
> > +static int
> > +drm_mode_get_lease(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_get_lease lease;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &lease))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{");
> > + if (entering(tcp)) {
>
> > + PRINT_FIELD_U("", lease, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > + PRINT_FIELD_PTR(", ", lease, objects_ptr);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> This is an array of count_object elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_U(", ", lease, count_objects);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + PRINT_FIELD_U("", lease, count_objects);
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_MODE_REVOKE_LEASE
> > +static int
> > +drm_mode_revoke_lease(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_mode_revoke_lease lease;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &lease)) {
> > + PRINT_FIELD_U("{", lease, lessee_id);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT
> > +
> > +# include "xlat/drm_syncobj_wait_flags.h"
> > +
> > +static int
> > +drm_syncobj_timeline_wait(struct tcb *const tcp, const kernel_ulong_t
> arg)
> > +{
> > + struct drm_syncobj_timeline_wait wait;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &wait))
> > + return RVAL_IOCTL_DECODED;
>
> > + PRINT_FIELD_PTR("{", wait, handles);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> This is an array of count_handles elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_D(", ", wait, timeout_nsec);
> If this timeout is absolute, it might make sense to print a time stamp in
> ISO 8601 format as well.
>
> > + PRINT_FIELD_U(", ", wait, count_handles);
>
> > + tprints(", flags=");
> > + printxval(drm_syncobj_wait_flags, wait.flags,
> "DRM_SYNCOBJ_WAIT_FLAGS_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &wait)) {
> > + PRINT_FIELD_U(", ", wait, first_signaled);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_QUERY
> > +static int
> > +drm_syncobj_query_or_timeline_signal(struct tcb *const tcp, const
> kernel_ulong_t arg)
> > +{
> > + struct drm_syncobj_timeline_array array;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &array)) {
>
> > + PRINT_FIELD_PTR("{", array, handles);
> > + PRINT_FIELD_PTR(", ", array, points);
> Why not PRINT_FIELD_ADDR/PRINT_FIELD_ADDR64?
>
> "handles" is an array of count_handles elements of uint32_t type, and
> likely
> has to be treated as such.
>
> "points" is an array of count_handles elements of uint64_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_U(", ", array, count_handles);
>
> > + PRINT_FIELD_U(", ", array, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +# ifdef DRM_IOCTL_SYNCOBJ_TRANSFER
> > +static int
> > +drm_syncobj_transfer(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct drm_syncobj_transfer transfer;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &transfer)) {
> > + PRINT_FIELD_U("{", transfer, src_handle);
> > + PRINT_FIELD_U(", ", transfer, dst_handle);
> > + PRINT_FIELD_U(", ", transfer, src_point);
> > + PRINT_FIELD_U(", ", transfer, dst_point);
>
> > + tprints(", flags=");
> > + printxval(drm_syncobj_wait_flags, transfer.flags,
> "DRM_SYNCOBJ_WAIT_FLAGS_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > + PRINT_FIELD_U(", ", transfer, pad);
> It likely makes sense to print this one only in case it is non-zero.
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +# endif
> > +
> > +int
> > +drm_ioctl(struct tcb *const tcp, const unsigned int code,
> > + const kernel_ulong_t arg)
> > +{
>
> > + switch (code) {
> I'd rather perform dispatch against _IOC_NR(code) if there's nothing
> preventing that.
>
Why? I saw that other decoders use switch(code) directly.
But it makes sense here because the size of arguments are
always changing as we've discussed above. If I do so, does
that mean I have to add _IOC_NR() to every case DRM_IOCTL
constants? Would that look redundant?
>
> > + case DRM_IOCTL_GET_MAGIC: /* R */
> > + return drm_get_magic(tcp, arg);
> > + case DRM_IOCTL_IRQ_BUSID: /* RW */
> > + return drm_irq_busid(tcp, arg);
> > + case DRM_IOCTL_SET_VERSION: /* RW */
> > + return drm_set_version(tcp, arg);
> > + case DRM_IOCTL_MODESET_CTL: /* W */
> > + return drm_modeset_ctl(tcp, arg);
> > + case DRM_IOCTL_GEM_CLOSE: /* W */
> > + return drm_gem_close(tcp, arg);
> > + case DRM_IOCTL_GEM_FLINK: /* RW */
> > + return drm_gem_flink(tcp, arg);
> > + case DRM_IOCTL_GEM_OPEN: /* RW */
> > + return drm_gem_open(tcp, arg);
> > + case DRM_IOCTL_GET_CAP: /* RW */
> > + return drm_get_cap(tcp, arg);
> > + case DRM_IOCTL_SET_CLIENT_CAP: /* W */
> > + return drm_set_client_cap(tcp, arg);
> > + case DRM_IOCTL_AUTH_MAGIC: /* W */
> > + return drm_auth_magic(tcp, arg);
> > + case DRM_IOCTL_BLOCK: /* RW */
> > + case DRM_IOCTL_UNBLOCK: /* RW */
> > + case DRM_IOCTL_MOD_CTX: /* W */
> > + case DRM_IOCTL_ADD_DRAW: /* RW */
> > + case DRM_IOCTL_RM_DRAW: /* RW */
> > + case DRM_IOCTL_FINISH: /* W */
> > + case DRM_IOCTL_MODE_ATTACHMODE: /* RW */
> > + case DRM_IOCTL_MODE_DETACHMODE: /* RW */
> > + return drm_noop(tcp, arg);
> > + case DRM_IOCTL_CONTROL: /* W */
> > + return drm_control(tcp, arg);
> > + case DRM_IOCTL_ADD_CTX: /* RW */
> > + case DRM_IOCTL_SWITCH_CTX: /* W */
> > + case DRM_IOCTL_NEW_CTX: /* W */
> > + return drm_ctx(tcp, arg);
> > + case DRM_IOCTL_RM_CTX: /* RW */
> > + return drm_rm_ctx(tcp, arg);
> > + case DRM_IOCTL_GET_CTX: /* RW */
> > + return drm_get_ctx(tcp, arg);
> > + case DRM_IOCTL_LOCK: /* W */
> > + case DRM_IOCTL_UNLOCK: /* W */
> > + return drm_lock(tcp, arg);
> > + case DRM_IOCTL_PRIME_HANDLE_TO_FD: /* RW */
> > + return drm_prime_handle_to_fd(tcp, arg);
> > + case DRM_IOCTL_PRIME_FD_TO_HANDLE: /* RW */
> > + return drm_prime_fd_to_handle(tcp, arg);
> > +# ifdef DRM_IOCTL_CRTC_GET_SEQUENCE
> > + case DRM_IOCTL_CRTC_GET_SEQUENCE: /* RW */
> > + return drm_crtc_get_sequence(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_CRTC_QUEUE_SEQUENCE
> > + case DRM_IOCTL_CRTC_QUEUE_SEQUENCE: /* RW */
> > + return drm_crtc_queue_sequence(tcp, arg);
> > +# endif
> > + case DRM_IOCTL_MODE_GETRESOURCES: /* RW */
> > + return drm_mode_get_resources(tcp, arg);
> > + case DRM_IOCTL_MODE_GETCRTC: /* RW */
> > + case DRM_IOCTL_MODE_SETCRTC: /* RW */
> > + return drm_mode_crtc(tcp, arg, code ==
> DRM_IOCTL_MODE_GETCRTC);
> > + case DRM_IOCTL_MODE_CURSOR: /* RW */
> > + return drm_mode_cursor(tcp, arg);
> > + case DRM_IOCTL_MODE_GETGAMMA: /* RW */
> > + case DRM_IOCTL_MODE_SETGAMMA: /* RW */
> > + return drm_mode_gamma(tcp, arg);
> > + case DRM_IOCTL_MODE_GETENCODER: /* RW */
> > + return drm_mode_get_encoder(tcp, arg);
> > + case DRM_IOCTL_MODE_GETPROPERTY: /* RW */
> > + return drm_mode_get_property(tcp, arg);
> > + case DRM_IOCTL_MODE_SETPROPERTY: /* RW */
> > + return drm_mode_set_property(tcp, arg);
> > + case DRM_IOCTL_MODE_GETPROPBLOB: /* RW */
> > + return drm_mode_get_prop_blob(tcp, arg);
> > + case DRM_IOCTL_MODE_GETFB: /* RW */
> > + return drm_mode_get_fb(tcp, arg);
> > + case DRM_IOCTL_MODE_ADDFB: /* RW */
> > + return drm_mode_add_fb(tcp, arg);
> > + case DRM_IOCTL_MODE_RMFB: /* RW */
> > + return drm_mode_rm_fb(tcp, arg);
> > + case DRM_IOCTL_MODE_PAGE_FLIP: /* RW */
> > + return drm_mode_page_flip(tcp, arg);
> > + case DRM_IOCTL_MODE_DIRTYFB: /* RW */
> > + return drm_mode_dirty_fb(tcp, arg);
> > + case DRM_IOCTL_MODE_CREATE_DUMB: /* RW */
> > + return drm_mode_create_dumb(tcp, arg);
> > + case DRM_IOCTL_MODE_MAP_DUMB: /* RW */
> > + return drm_mode_map_dumb(tcp, arg);
> > + case DRM_IOCTL_MODE_DESTROY_DUMB: /* RW */
> > + return drm_mode_destroy_dumb(tcp, arg);
> > + case DRM_IOCTL_MODE_GETPLANE: /* RW */
> > + return drm_mode_getplane(tcp, arg);
> > + case DRM_IOCTL_MODE_SETPLANE: /* RW */
> > + return drm_mode_setplane(tcp, arg);
> > + case DRM_IOCTL_MODE_CURSOR2: /* RW */
> > + return drm_mode_cursor2(tcp, arg);
> > + case DRM_IOCTL_MODE_ATOMIC: /* RW */
> > + return drm_mode_atomic(tcp, arg);//
> > + case DRM_IOCTL_MODE_CREATEPROPBLOB: /* RW */
> > + return drm_mode_createpropblob(tcp, arg);
> > + case DRM_IOCTL_MODE_DESTROYPROPBLOB: /* RW */
> > + return drm_mode_destroypropblob(tcp, arg);
> > +# ifdef DRM_IOCTL_SYNCOBJ_CREATE
> > + case DRM_IOCTL_SYNCOBJ_CREATE: /* RW */
> > + return drm_syncobj_create(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_DESTROY
> > + case DRM_IOCTL_SYNCOBJ_DESTROY: /* RW */
> > + return drm_syncobj_destroy(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD
> > + case DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD: /* RW */
> > + case DRM_IOCTL_SYNCOBJ_FD_TO_HANDLE: /* RW */
> > + return drm_syncobj_handle_fd(tcp, arg, code ==
> DRM_IOCTL_SYNCOBJ_HANDLE_TO_FD);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_WAIT
> > + case DRM_IOCTL_SYNCOBJ_WAIT: /* RW */
> > + return drm_syncobj_wait(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_RESET
> > + case DRM_IOCTL_SYNCOBJ_RESET: /* RW */
> > + case DRM_IOCTL_SYNCOBJ_SIGNAL: /* RW */
> > + return drm_syncobj_reset_or_signal(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_MODE_CREATE_LEASE
> > + case DRM_IOCTL_MODE_CREATE_LEASE: /* RW */
> > + return drm_mode_create_lease(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_MODE_LIST_LESSEES
> > + case DRM_IOCTL_MODE_LIST_LESSEES: /* RW */
> > + return drm_mode_list_lessees(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_MODE_GET_LEASE
> > + case DRM_IOCTL_MODE_GET_LEASE: /* RW */
> > + return drm_mode_get_lease(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_MODE_REVOKE_LEASE
> > + case DRM_IOCTL_MODE_REVOKE_LEASE: /* RW */
> > + return drm_mode_revoke_lease(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT
> > + case DRM_IOCTL_SYNCOBJ_TIMELINE_WAIT: /* RW */
> > + return drm_syncobj_timeline_wait(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_QUERY
> > + case DRM_IOCTL_SYNCOBJ_QUERY: /* RW */
> > + case DRM_IOCTL_SYNCOBJ_TIMELINE_SIGNAL: /* RW */
> > + return drm_syncobj_query_or_timeline_signal(tcp, arg);
> > +# endif
> > +# ifdef DRM_IOCTL_SYNCOBJ_TRANSFER
> > + case DRM_IOCTL_SYNCOBJ_TRANSFER: /* RW */
> > + return drm_syncobj_transfer(tcp, arg);
> > +# endif
> > + default:
> > + return drm_ioctl_mpers(tcp, code, arg);
> > + }
> > +}
> > +
> > +#endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
> > diff --git a/drm_mpers.c b/drm_mpers.c
> > new file mode 100644
> > index 00000000..1b685a52
> > --- /dev/null
> > +++ b/drm_mpers.c
> > @@ -0,0 +1,815 @@
> > +#include "defs.h"
> > +
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +
> > +#ifdef HAVE_DRM_H
> > +# include <drm.h>
> > +#else
> > +# include <drm/drm.h>
> > +#endif
> > +
> > +#include DEF_MPERS_TYPE(struct_drm_version)
> > +#include DEF_MPERS_TYPE(struct_drm_unique)
> > +#include DEF_MPERS_TYPE(struct_drm_map)
> > +#include DEF_MPERS_TYPE(struct_drm_client)
> > +#include DEF_MPERS_TYPE(struct_drm_stats)
> > +#include DEF_MPERS_TYPE(struct_drm_buf_desc)
> > +#include DEF_MPERS_TYPE(struct_drm_buf_info)
> > +#include DEF_MPERS_TYPE(struct_drm_buf_map)
> > +#include DEF_MPERS_TYPE(struct_drm_buf_free)
> > +#include DEF_MPERS_TYPE(struct_drm_ctx_priv_map)
> > +#include DEF_MPERS_TYPE(struct_drm_ctx_res)
> > +#include DEF_MPERS_TYPE(struct_drm_agp_mode)
> > +#include DEF_MPERS_TYPE(struct_drm_agp_info)
> > +#include DEF_MPERS_TYPE(struct_drm_agp_buffer)
> > +#include DEF_MPERS_TYPE(struct_drm_agp_binding)
> > +#include DEF_MPERS_TYPE(struct_drm_scatter_gather)
> > +#include DEF_MPERS_TYPE(union_drm_wait_vblank)
> > +#include DEF_MPERS_TYPE(struct_drm_mode_get_connector)
> > +#include DEF_MPERS_TYPE(struct_drm_mode_fb_cmd2)
> > +#include DEF_MPERS_TYPE(struct_drm_mode_get_plane_res)
> > +#include DEF_MPERS_TYPE(struct_drm_mode_obj_get_properties)
> > +#include DEF_MPERS_TYPE(struct_drm_mode_obj_set_property)
> > +
> > +typedef struct drm_version struct_drm_version;
> > +typedef struct drm_unique struct_drm_unique;
> > +typedef struct drm_map struct_drm_map;
> > +typedef struct drm_client struct_drm_client;
> > +typedef struct drm_stats struct_drm_stats;
> > +typedef struct drm_buf_desc struct_drm_buf_desc;
> > +typedef struct drm_buf_info struct_drm_buf_info;
> > +typedef struct drm_buf_map struct_drm_buf_map;
> > +typedef struct drm_buf_free struct_drm_buf_free;
> > +typedef struct drm_ctx_priv_map struct_drm_ctx_priv_map;
> > +typedef struct drm_ctx_res struct_drm_ctx_res;
> > +typedef struct drm_agp_mode struct_drm_agp_mode;
> > +typedef struct drm_agp_info struct_drm_agp_info;
> > +typedef struct drm_agp_buffer struct_drm_agp_buffer;
> > +typedef struct drm_agp_binding struct_drm_agp_binding;
> > +typedef struct drm_scatter_gather struct_drm_scatter_gather;
> > +typedef union drm_wait_vblank union_drm_wait_vblank;
> > +typedef struct drm_mode_get_connector struct_drm_mode_get_connector;
> > +typedef struct drm_mode_fb_cmd2 struct_drm_mode_fb_cmd2;
> > +typedef struct drm_mode_get_plane_res struct_drm_mode_get_plane_res;
> > +typedef struct drm_mode_obj_get_properties
> struct_drm_mode_obj_get_properties;
> > +typedef struct drm_mode_obj_set_property
> struct_drm_mode_obj_set_property;
> > +
> > +#endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
> > +
> > +#include MPERS_DEFS
> > +
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > +
> > +# include "print_fields.h"
> > +
> > +static int
> > +drm_version(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_version ver;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > + if (umove_or_printaddr(tcp, arg, &ver))
> > + return RVAL_IOCTL_DECODED;
> > + if (entering(tcp)) {
> > + PRINT_FIELD_U("{", ver, name_len);
> > + PRINT_FIELD_U(", ", ver, date_len);
> > + PRINT_FIELD_U(", ", ver, desc_len);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + PRINT_FIELD_D("{", ver, version_major);
> > + PRINT_FIELD_D(", ", ver, version_minor);
> > + PRINT_FIELD_D(", ", ver, version_patchlevel);
> > + PRINT_FIELD_U(", ", ver, name_len);
>
> > + tprints(", name=");
> > + printstrn(tcp, ptr_to_kulong(ver.name), ver.name_len);
> Why not PRINT_FIELD_STRN?
>
In include/uapi/drm/drm.h, the ver.name field is a char * type. Seems
in PRINT_FIELD_STRN macro we have to cast char * to kernel_ulong_t
which is not achievable in this macro. Am I missing anything here?
Some other comments below are similar.
> > + PRINT_FIELD_U(", ", ver, date_len);
>
> > + tprints(", date=");
> > + printstrn(tcp, ptr_to_kulong(ver.date), ver.date_len);
> Why not PRINT_FIELD_STRN?
>
> > + PRINT_FIELD_U(", ", ver, desc_len);
>
> > + tprints(", desc=");
> > + printstrn(tcp, ptr_to_kulong(ver.desc), ver.desc_len);
> Why not PRINT_FIELD_STRN?
>
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_unique(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_unique unique;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > + if (umove_or_printaddr(tcp, arg, &unique))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_U("{", unique, unique_len);
> > + if (entering(tcp)) {
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + tprints(", unique=");
> > + printstrn(tcp, ptr_to_kulong(unique.unique), unique.unique_len);
> Why not PRINT_FIELD_STRN?
>
> I suspect that a cast to uintptr_t is sufficient here.
>
Yes you're right. BTW what's the difference? I mean when
to use ptr_to_kulong and when casting to uintptr_t?
>
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
>
> > +# include "xlat/drm_map_type.h"
> > +# include "xlat/drm_map_flags.h"
> Inline include.
>
> > +
> > +static int
> > +drm_get_map(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_map map;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &map))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_X("{", map, offset);
> > + if (entering(tcp)) {
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + PRINT_FIELD_U(", ", map, size);
>
> > + tprints(", type=");
> > + printxval(drm_map_type, map.type, "_DRM_???");
> Why not PRINT_FIELD_XVAL?
>
> > + tprints(", flags=");
> > + printxval(drm_map_flags, map.flags, "_DRM_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > + PRINT_FIELD_PTR(", ", map, handle);
> > + PRINT_FIELD_D(", ", map, mtrr);
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_get_client(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_client client;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &client))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_D("{", client, idx);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &client)) {
> > + PRINT_FIELD_D(", ", client, auth);
> > + PRINT_FIELD_U(", ", client, pid);
>
> > + PRINT_FIELD_U(", ", client, uid);
> Why not PRINT_FIELD_UID?
>
OK.
>
> > + PRINT_FIELD_U(", ", client, magic);
> > + PRINT_FIELD_U(", ", client, iocs);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_stat_type.h"
> > +
> > +static int
> > +drm_get_stats(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_stats stats;
> > +
> > + if (exiting(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &stats))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", stats, count);
> > + tprints(", data=[");
> > +
>
> > + for (unsigned int i = 0; i < 15; i++) {
> Why is "15" hard-coded?
>
> Also, it seems that it should go up to "count".
>
My fault.
>
> > + tprintf("%s%lu", i > 0 ? ", {value=" : "{value=",
> (unsigned long) stats.data[i].value);
> Overly long line.
>
> > + tprints(", type=");
>
> > + printxval(drm_stat_type, stats.data[i].type,
> "_DRM_STAT_???");
> Overly long line.
>
My fault.
>
> Why not PRINT_FIELD_XVAL?
>
> > + tprints("}");
> > + }
>
> It could be worth considering incorporating something like
> print_local_array from "Add support for printing local arrays to
> print_array" commit from esyr/evdev_decode_bitset branch instead
> of open-coding it.
>
>
Will this work for all those arrays pointed by *_ptr fields
in some structures above and below?
> > + tprints("]}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +drm_add_map(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_map map;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &map)) {
> > + PRINT_FIELD_X("{", map, offset);
> > + PRINT_FIELD_U(", ", map, size);
>
> > + tprints(", type=");
> > + printxval(drm_map_type, map.type, "_DRM_???");
> Why not PRINT_FIELD_XVAL?
>
> > + tprints(", flags");
> > + printxval(drm_map_flags, map.flags, "_DRM_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_buf_desc_flags.h"
> > +
> > +static int
> > +drm_add_bufs(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_buf_desc desc;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > + if (umove_or_printaddr(tcp, arg, &desc))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_D("{", desc, count);
> > + PRINT_FIELD_D(", ", desc, size);
> > + if (entering(tcp)) {
>
> > + tprints(", flags=");
> > + printxval(drm_buf_desc_flags, desc.flags, "_DRM_???");
> Why printxval and not printflags?
>
> Why not PRINT_FIELD_FLAGS?
>
> > + PRINT_FIELD_X(", ", desc, agp_start);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mark_bufs(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_buf_desc desc;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &desc)) {
> > + PRINT_FIELD_D("{", desc, size);
> > + PRINT_FIELD_D(", ", desc, low_mark);
> > + PRINT_FIELD_D(", ", desc, high_mark);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_info_bufs(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_buf_info info;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > + if (umove_or_printaddr(tcp, arg, &info))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{");
> > + if (entering(tcp)) {
>
> > + PRINT_FIELD_PTR("", info, list);
> It is an array of count elements of struct drm_buf_desc type, and
> likely has to be treated as such.
>
> > + PRINT_FIELD_D(", ", info, count);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + PRINT_FIELD_D("", info, count);
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_map_bufs(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_buf_map map;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &map))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_D("{", map, count);
> > + PRINT_FIELD_PTR(", ", map, virtual);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &map)) {
>
> > + PRINT_FIELD_PTR(", ", map, list);
> It is an array of count elements of struct drm_buf_pub type, and
> likely has to be treated as such.
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_free_bufs(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_buf_free free;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &free)) {
> > + PRINT_FIELD_D("{", free, count);
>
> > + PRINT_FIELD_PTR(", ", free, list);
> It's an array of count elements of int type, and likely has to be
> treated as such.
>
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_rm_map(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_map map;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &map)) {
> > + PRINT_FIELD_PTR("{", map, handle);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_sarea_ctx(struct tcb *const tcp, const kernel_ulong_t arg,
> > + const bool is_get)
> > +{
> > + struct_drm_ctx_priv_map map;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &map))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", map, ctx_id);
> > + if (!is_get)
> > + PRINT_FIELD_PTR(", ", map, handle);
> > + return 0;
> > + }
> > +
>
> > + if (!syserror(tcp) && !umove(tcp, arg, &map)) {
> > + if (is_get)
> "if" nesting can be avoided here.
>
Sure.
>
> > + PRINT_FIELD_PTR(", ", map, handle);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_res_ctx(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_ctx_res ctx;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > + if (umove_or_printaddr(tcp, arg, &ctx))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_D("{", ctx, count);
> > +
> > + if (entering(tcp)) {
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + PRINT_FIELD_PTR(", ", ctx, contexts);
> It's an array of count elements of struct drm_ctx type, and likely
> has to be treated as such.
>
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +
> > +static int
> > +drm_agp_enable(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_agp_mode mode;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &mode)) {
> > + PRINT_FIELD_U("{", mode, mode);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_agp_info(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_agp_info info;
> > +
> > + if (exiting(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &info))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_D("{", info, agp_version_major);
> > + PRINT_FIELD_D(", ", info, agp_version_minor);
> > + PRINT_FIELD_U(", ", info, mode);
>
> > + PRINT_FIELD_U(", ", info, aperture_base);
> This is likely better to be printed as hexadecimal.
>
>
OK.
> > + PRINT_FIELD_U(", ", info, aperture_size);
> > + PRINT_FIELD_U(", ", info, memory_allowed);
> > + PRINT_FIELD_U(", ", info, memory_used);
>
> > + PRINT_FIELD_U(", ", info, id_vendor);
> > + PRINT_FIELD_U(", ", info, id_device);
> These are likely better to be printed as hexadecimal.
>
>
OK.
> + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int
> > +drm_agp_alloc(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_agp_buffer buffer;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &buffer))
> > + return RVAL_IOCTL_DECODED;
> > + PRINT_FIELD_U("{", buffer, size);
> > + PRINT_FIELD_U(", ", buffer, type);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &buffer)) {
> > + PRINT_FIELD_U(", ", buffer, handle);
> > + PRINT_FIELD_U(", ", buffer, physical);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_agp_free(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_agp_buffer buffer;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &buffer)) {
> > + PRINT_FIELD_U("{", buffer, handle);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_agp_bind(struct tcb *const tcp, const kernel_ulong_t arg, bool
> is_bind)
> > +{
> > + struct_drm_agp_binding binding;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &binding)) {
> > + PRINT_FIELD_U("{", binding, handle);
> > + if (is_bind)
> > + PRINT_FIELD_X(", ", binding, offset);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_scatter_gather(struct tcb *const tcp, const kernel_ulong_t arg,
> > + bool is_alloc)
> > +{
> > + struct_drm_scatter_gather sg;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &sg))
> > + return RVAL_IOCTL_DECODED;
> > + if (is_alloc)
> > + PRINT_FIELD_U("{", sg, size);
> > + else
> > + PRINT_FIELD_U("{", sg, handle);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &sg)) {
> > + if (is_alloc)
> > + PRINT_FIELD_U(", ", sg, handle);
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +# include "xlat/drm_vblank_seq_type.h"
> > +
> > +static int
> > +drm_wait_vblank(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + union_drm_wait_vblank vblank;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &vblank))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{request={type=");
> > + printxval(drm_vblank_seq_type, vblank.request.type,
> "_DRM_VBLANK_???");
> Overly long line.
>
My fault.
>
> Seems like the value consists of three parts (_DRM_VBLANK_TYPES_MASK,
> _DRM_VBLANK_HIGH_CRTC_MASK, and _DRM_VBLANK_FLAGS_MASK; the latter omits
> _DRM_VBLANK_FLIP for unknown reason), and only bits covered by
> _DRM_VBLANK_TYPES_MASK and _DRM_VBLANK_HIGH_CRTC_MASK has to be decoded
> as xval,
> the rest is flags.
You're right. Somewhere else in the code may has the same problem.
Will double-check.
> > + PRINT_FIELD_U(", ", vblank.request, sequence);
> > + PRINT_FIELD_U(", ", vblank.request, signal);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &vblank)) {
>
> > + PRINT_FIELD_U(", reply={", vblank.reply, type);
> This field has the same "enum drm_vblank_seq_type" type.
>
> > + PRINT_FIELD_U(", ", vblank.reply, sequence);
>
> > + PRINT_FIELD_U(", ", vblank.reply, tval_sec);
> > + PRINT_FIELD_U(", ", vblank.reply, tval_usec);
> They both are signed.
>
> > + tprints("}");
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_get_connector(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_mode_get_connector con;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &con))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_U("{", con, connector_id);
> > + PRINT_FIELD_U(", ", con, count_encoders);
> > +
> > + if (entering(tcp)) {
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + PRINT_FIELD_PTR(", ", con, encoders_ptr);
> It's an array of count_encoders elements of uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_PTR(", ", con, modes_ptr);
> It's an array of count_modes elements of struct drm_mode_modeinfo type,
> and likely has to be treated as such.
>
> > + PRINT_FIELD_PTR(", ", con, props_ptr);
> It's an array of count_props elements of struct uint32_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_PTR(", ", con, prop_values_ptr);
> It's an array of count_props elements of struct uint64_t type, and likely
> has to be treated as such.
>
> > + PRINT_FIELD_U(", ", con, count_modes);
> > + PRINT_FIELD_U(", ", con, count_props);
> These seems to be R/W.
>
> > + PRINT_FIELD_U(", ", con, encoder_id);
> > + PRINT_FIELD_U(", ", con, connector_type);
> > + PRINT_FIELD_U(", ", con, connector_type_id);
> > + PRINT_FIELD_U(", ", con, connection);
> > + PRINT_FIELD_U(", ", con, mm_width);
> > + PRINT_FIELD_U(", ", con, mm_height);
> > + PRINT_FIELD_U(", ", con, subpixel);
>
> There's also "pad" field that may be worth printing in case it is
> non-zero.
>
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_add_fb2(struct tcb *const tcp, const kernel_ulong_t arg)
> > +{
> > + struct_drm_mode_fb_cmd2 cmd;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &cmd))
> > + return RVAL_IOCTL_DECODED;
>
> > + tprintf("{width=%u, height=%u, pixel_format=%#x, flags=%u,
> "
> It may be worth considering rewriting this using PRINT_FIELDS_*.
>
Indeed.
>
> "flags" field can contain DRM_MODE_FB_INTERLACED and DRM_MODE_FB_MODIFIERS
> flags.
>
> > + "handles=[%u, %u, %u, %u], "
> > + "pitches=[%u, %u, %u, %u], "
> > + "offsets=[%u, %u, %u, %u]",
> > + cmd.width, cmd.height, cmd.pixel_format, cmd.flags,
>
> > + cmd.handles[0], cmd.handles[1], cmd.handles[2],
> > + cmd.handles[3], cmd.pitches[0], cmd.pitches[1],
> > + cmd.pitches[2], cmd.pitches[3], cmd.offsets[0],
> > + cmd.offsets[1], cmd.offsets[2], cmd.offsets[3]);
> This part is quite difficult to read.
>
>
I should use for loop for this or perhaps print_local_array?
> > +#ifdef HAVE_STRUCT_DRM_MODE_FB_CMD2_MODIFIER
> > + tprintf(", modifiers=[%" PRIu64 ", %" PRIu64 ", %" PRIu64
> ", %" PRIu64 "]",
> > + (uint64_t) cmd.modifier[0], (uint64_t)
> cmd.modifier[1],
> > + (uint64_t) cmd.modifier[2], (uint64_t)
> cmd.modifier[3]);
> > +#endif
> Again, wrapping part of decoding in ifdef's seems fragile.
Well, modifier field in struct drm_mode_fb_cmd2 is added in some
newer version of libdrm. It's not like add xlat for DRM_IOC constants.
What's your suggestion on handling this?
>
>
> +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &cmd))
> > + PRINT_FIELD_U(", ", cmd, fb_id);
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_getplaneresources(struct tcb *const tcp, const kernel_ulong_t
> arg)
> > +{
> > + struct_drm_mode_get_plane_res res;
> > +
> > + if (entering(tcp))
> > + tprints(", ");
> > + else if (syserror(tcp))
> > + return RVAL_IOCTL_DECODED;
> > + else
> > + tprints(" => ");
> > +
> > + if (umove_or_printaddr(tcp, arg, &res))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + tprints("{");
> > + if (entering(tcp)) {
>
> > + PRINT_FIELD_PTR("", res, plane_id_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> "plane_id_ptr" is an array of count_planes elements of uint32_t type,
> and likely has to be treated as such.
>
> > + PRINT_FIELD_U(", ", res, count_planes);
> > + tprints("}");
> > +
> > + return 0;
> > + }
> > +
>
> > + PRINT_FIELD_U("", res, count_planes);
> This one seems to be R/W.
>
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_obj_getproperties(struct tcb *const tcp, const kernel_ulong_t
> arg)
> > +{
> > + struct_drm_mode_obj_get_properties prop;
> > +
> > + if (entering(tcp)) {
> > + tprints(", ");
> > + if (umove_or_printaddr(tcp, arg, &prop))
> > + return RVAL_IOCTL_DECODED;
> > +
> > + PRINT_FIELD_PTR("{", prop, props_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> "props_ptr" is an array of count_props elements of uint32_t type,
> and likely has to be treated as such.
>
> > + PRINT_FIELD_PTR(", ", prop, prop_values_ptr);
> Why not PRINT_FIELD_ADDR64?
>
> "prop_values_ptr" is an array of count_props elements of uint64_t type,
> and likely has to be treated as such.
>
> > + PRINT_FIELD_U(", ", prop, obj_id);
> > + PRINT_FIELD_U(", ", prop, obj_type);
> > +
> > + return 0;
> > + }
> > +
> > + if (!syserror(tcp) && !umove(tcp, arg, &prop)) {
>
> > + PRINT_FIELD_U(", ", prop, count_props);
> This seems to be R/W.
>
> > + }
> > +
> > + tprints("}");
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +static int
> > +drm_mode_obj_setproperty(struct tcb *const tcp, const kernel_ulong_t
> arg)
> > +{
> > + struct_drm_mode_obj_set_property prop;
> > +
> > + tprints(", ");
> > + if (!umove_or_printaddr(tcp, arg, &prop)) {
> > + PRINT_FIELD_U("{", prop, value);
> > + PRINT_FIELD_U(", ", prop, prop_id);
> > + PRINT_FIELD_U(", ", prop, obj_id);
> > + PRINT_FIELD_U(", ", prop, obj_type);
> > + tprints("}");
> > + }
> > +
> > + return RVAL_IOCTL_DECODED;
> > +}
> > +
> > +MPERS_PRINTER_DECL(int, drm_ioctl_mpers, struct tcb *const tcp,
> > + const unsigned int code, const kernel_ulong_t arg)
> > +{
> > + switch (code) {
> I'd rather perform dispatch against _IOC_NR(code) if there's nothing
> preventing that.
> > + case DRM_IOCTL_VERSION: /* RW */
> > + return drm_version(tcp, arg);
> > + case DRM_IOCTL_GET_UNIQUE: /* RW */
>
> > + //case DRM_IOCTL_SET_UNIQUE: /* W */ /* invalid op */
> Leftover comment.
>
>
Will remove.
> > + return drm_unique(tcp, arg);
> > + case DRM_IOCTL_GET_MAP: /* RW */
> > + return drm_get_map(tcp, arg);
> > + case DRM_IOCTL_GET_CLIENT: /* RW */
> > + return drm_get_client(tcp, arg);
> > + case DRM_IOCTL_GET_STATS: /* R */
> > + return drm_get_stats(tcp, arg);
> > + case DRM_IOCTL_ADD_MAP: /* RW */
> > + return drm_add_map(tcp, arg);
> > + case DRM_IOCTL_ADD_BUFS: /* RW */
> > + return drm_add_bufs(tcp, arg);
> > + case DRM_IOCTL_MARK_BUFS: /* W */
> > + return drm_mark_bufs(tcp, arg);
> > + case DRM_IOCTL_INFO_BUFS: /* RW */
> > + return drm_info_bufs(tcp, arg);
> > + case DRM_IOCTL_MAP_BUFS: /* RW */
> > + return drm_map_bufs(tcp, arg);
> > + case DRM_IOCTL_FREE_BUFS: /* W */
> > + return drm_free_bufs(tcp, arg);
> > + case DRM_IOCTL_RM_MAP: /* W */
> > + return drm_rm_map(tcp, arg);
> > + case DRM_IOCTL_SET_SAREA_CTX:
> > + case DRM_IOCTL_GET_SAREA_CTX:
> > + return drm_sarea_ctx(tcp, arg, code ==
> DRM_IOCTL_GET_SAREA_CTX);
> > + case DRM_IOCTL_RES_CTX: /* RW */
> > + return drm_res_ctx(tcp, arg);
> > + case DRM_IOCTL_AGP_ENABLE: /* W */
> > + return drm_agp_enable(tcp, arg);
> > + case DRM_IOCTL_AGP_INFO: /* R */
> > + return drm_agp_info(tcp, arg);
> > + case DRM_IOCTL_AGP_ALLOC: /* RW */
> > + return drm_agp_alloc(tcp, arg);
> > + case DRM_IOCTL_AGP_FREE: /* W */
> > + return drm_agp_free(tcp, arg);
> > + case DRM_IOCTL_AGP_BIND: /* W */
> > + case DRM_IOCTL_AGP_UNBIND: /* W */
> > + return drm_agp_bind(tcp, arg, code == DRM_IOCTL_AGP_BIND);
> > + case DRM_IOCTL_SG_ALLOC: /* RW */
> > + case DRM_IOCTL_SG_FREE: /* W */
> > + return drm_scatter_gather(tcp, arg, code ==
> DRM_IOCTL_SG_ALLOC);
> > + case DRM_IOCTL_WAIT_VBLANK: /* RW */
> > + return drm_wait_vblank(tcp, arg);
> > + case DRM_IOCTL_MODE_ADDFB2: /* RW */
> > + return drm_mode_add_fb2(tcp, arg);
> > + }
> > + /* variable length, so we can't use switch(code) to identify
> DRM_IOCTL_MODE_GETCONNECTOR */
> > + if (_IOC_NR(code) == _IOC_NR(DRM_IOCTL_MODE_GETCONNECTOR)) {
> > + return drm_mode_get_connector(tcp, arg);
> > + }
> > +
> > + if (_IOC_NR(code) == _IOC_NR(DRM_IOCTL_MODE_GETPLANERESOURCES)) {
> > + return drm_mode_getplaneresources(tcp, arg);
> > + }
> > +
> > + if (_IOC_NR(code) == _IOC_NR(DRM_IOCTL_MODE_OBJ_GETPROPERTIES)) {
> > + return drm_mode_obj_getproperties(tcp, arg);
> > + }
> > +
> > + if (_IOC_NR(code) == _IOC_NR(DRM_IOCTL_MODE_OBJ_SETPROPERTY)) {
> > + return drm_mode_obj_setproperty(tcp, arg);
> > + }
> > +
> > + return RVAL_DECODED;
> > +}
> > +
> > +#endif /* HAVE_DRM_H || HAVE_DRM_DRM_H */
> > diff --git a/ioctl.c b/ioctl.c
> > index b80292cb..b9aa9833 100644
> > --- a/ioctl.c
> > +++ b/ioctl.c
> > @@ -195,6 +195,10 @@ ioctl_decode_command_number(struct tcb *tcp)
> > return 1;
> > }
> > return 0;
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > + case 'd':
> > + return drm_decode_number(tcp, code);
> > +#endif
> I'd prefer having this code unconditional here and put a stub
> drm_decode_number
> in drm_ioctl.c, but it's more up to Dmitry.
>
>
Can you explain this part a little bit? I'm not sure I'm following. Since
the
whole bulk of code in drm_ioctl.c is ifdefed, would that be a problem if
this
part is used unconditionally?
> default:
> > return 0;
> > }
> > @@ -264,6 +268,10 @@ ioctl_decode(struct tcb *tcp)
> > return fs_x_ioctl(tcp, code, arg);
> > case 0x22:
> > return scsi_ioctl(tcp, code, arg);
>
> > +#if defined(HAVE_DRM_H) || defined(HAVE_DRM_DRM_H)
> > + case 'd':
> > + return drm_ioctl(tcp, code, arg);
> > +#endif
> Same here regarding drm_ioctl.
>
> > case 'L':
> > return loop_ioctl(tcp, code, arg);
> > #ifdef HAVE_STRUCT_MTD_WRITE_REQ
> > diff --git a/xlat/drm_buf_desc_flags.in b/xlat/drm_buf_desc_flags.in
> > new file mode 100644
> > index 00000000..64c41fa9
> > --- /dev/null
> > +++ b/xlat/drm_buf_desc_flags.in
> > @@ -0,0 +1,5 @@
> "#sorted" is (probably) missing here.
>
OK, I didn't notice this before. I will check all the files in xlat/
to make sure the comments are in place.
>
> > +_DRM_PAGE_ALIGN 0x01
> > +_DRM_AGP_BUFFER 0x02
> > +_DRM_SG_BUFFER 0x04
> > +_DRM_FB_BUFFER 0x08
> > +_DRM_PCI_BUFFER_RO 0x10
> > diff --git a/xlat/drm_capability.in b/xlat/drm_capability.in
> > new file mode 100644
> > index 00000000..ac30d02f
> > --- /dev/null
> > +++ b/xlat/drm_capability.in
> > @@ -0,0 +1,14 @@
> "#value_indexed" is missing here.
>
> > +DRM_CAP_DUMB_BUFFER 0x1
> > +DRM_CAP_VBLANK_HIGH_CRTC 0x2
> > +DRM_CAP_DUMB_PREFERRED_DEPTH 0x3
> > +DRM_CAP_DUMB_PREFER_SHADOW 0x4
> > +DRM_CAP_PRIME 0x5
> > +DRM_CAP_TIMESTAMP_MONOTONIC 0x6
> > +DRM_CAP_ASYNC_PAGE_FLIP 0x7
> > +DRM_CAP_CURSOR_WIDTH 0x8
> > +DRM_CAP_CURSOR_HEIGHT 0x9
> > +DRM_CAP_ADDFB2_MODIFIERS 0x10
> > +DRM_CAP_PAGE_FLIP_TARGET 0x11
> > +DRM_CAP_CRTC_IN_VBLANK_EVENT 0x12
> > +DRM_CAP_SYNCOBJ 0x13
> > +DRM_CAP_SYNCOBJ_TIMELINE 0x14
> > diff --git a/xlat/drm_client_capability.in b/xlat/
> drm_client_capability.in
> > new file mode 100644
> > index 00000000..7c3d867c
> > --- /dev/null
> > +++ b/xlat/drm_client_capability.in
> > @@ -0,0 +1,5 @@
> "#value_indexed" is missing here.
>
> > +DRM_CLIENT_CAP_STEREO_3D 1
> > +DRM_CLIENT_CAP_UNIVERSAL_PLANES 2
> > +DRM_CLIENT_CAP_ATOMIC 3
> > +DRM_CLIENT_CAP_ASPECT_RATIO 4
> > +DRM_CLIENT_CAP_WRITEBACK_CONNECTORS 5
> > diff --git a/xlat/drm_control_func.in b/xlat/drm_control_func.in
> > new file mode 100644
> > index 00000000..c270bc62
> > --- /dev/null
> > +++ b/xlat/drm_control_func.in
> > @@ -0,0 +1,4 @@
> "#value_indexed" is missing here.
>
> > +DRM_ADD_COMMAND 0
> > +DRM_RM_COMMAND 1
> > +DRM_INST_HANDLER 2
> > +DRM_UNINST_HANDLER 3
> > diff --git a/xlat/drm_crtc_sequence_flags.in b/xlat/
> drm_crtc_sequence_flags.in
> > new file mode 100644
> > index 00000000..8283287d
> > --- /dev/null
> > +++ b/xlat/drm_crtc_sequence_flags.in
> > @@ -0,0 +1,2 @@
> "#sorted" is (probably) missing here.
>
> > +DRM_CRTC_SEQUENCE_RELATIVE 0x00000001
> > +DRM_CRTC_SEQUENCE_NEXT_ON_MISS 0x00000002
> > diff --git a/xlat/drm_ctx_flags.in b/xlat/drm_ctx_flags.in
> > new file mode 100644
> > index 00000000..4225a19b
> > --- /dev/null
> > +++ b/xlat/drm_ctx_flags.in
> > @@ -0,0 +1,2 @@
> "#sorted" is (probably) missing here.
>
> > +_DRM_CONTEXT_PRESERVED 0x01
> > +_DRM_CONTEXT_2DONLY 0x02
> > diff --git a/xlat/drm_lock_flags.in b/xlat/drm_lock_flags.in
> > new file mode 100644
> > index 00000000..960d946f
> > --- /dev/null
> > +++ b/xlat/drm_lock_flags.in
> > @@ -0,0 +1,6 @@
> "#sorted" is (probably) missing here.
>
> > +_DRM_LOCK_READY 0x01
> > +_DRM_LOCK_QUIESCENT 0x02
> > +_DRM_LOCK_FLUSH 0x04
> > +_DRM_LOCK_FLUSH_ALL 0x08
> > +_DRM_HALT_ALL_QUEUES 0x10
> > +_DRM_HALT_CUR_QUEUES 0x20
> > diff --git a/xlat/drm_map_flags.in b/xlat/drm_map_flags.in
> > new file mode 100644
> > index 00000000..9cd46901
> > --- /dev/null
> > +++ b/xlat/drm_map_flags.in
> "#sorted" is (probably) missing here.
>
> > @@ -0,0 +1,8 @@
> > +_DRM_RESTRICTED 0x01
> > +_DRM_READ_ONLY 0x02
> > +_DRM_LOCKED 0x04
> > +_DRM_KERNEL 0x08
> > +_DRM_WRITE_COMBINING 0x10
> > +_DRM_CONTAINS_LOCK 0x20
> > +_DRM_REMOVABLE 0x40
> > +_DRM_DRIVER 0x80
> > diff --git a/xlat/drm_map_type.in b/xlat/drm_map_type.in
> > new file mode 100644
> > index 00000000..6a51a66a
> > --- /dev/null
> > +++ b/xlat/drm_map_type.in
> > @@ -0,0 +1,6 @@
> "#value_indexed" is missing here.
>
> > +_DRM_FRAME_BUFFER 0
> > +_DRM_REGISTERS 1
> > +_DRM_SHM 2
> > +_DRM_AGP 3
> > +_DRM_SCATTER_GATHER 4
> > +_DRM_CONSISTENT 5
> > diff --git a/xlat/drm_mode_encoder_type.in b/xlat/
> drm_mode_encoder_type.in
> > new file mode 100644
> > index 00000000..cbd67918
> > --- /dev/null
> > +++ b/xlat/drm_mode_encoder_type.in
> > @@ -0,0 +1,9 @@
> "#value_indexed" is missing here.
>
> > +DRM_MODE_ENCODER_NONE 0
> > +DRM_MODE_ENCODER_DAC 1
> > +DRM_MODE_ENCODER_TMDS 2
> > +DRM_MODE_ENCODER_LVDS 3
> > +DRM_MODE_ENCODER_TVDAC 4
> > +DRM_MODE_ENCODER_VIRTUAL 5
> > +DRM_MODE_ENCODER_DSI 6
> > +DRM_MODE_ENCODER_DPMST 7
> > +DRM_MODE_ENCODER_DPI 8
> > diff --git a/xlat/drm_mode_flags.in b/xlat/drm_mode_flags.in
> > new file mode 100644
> > index 00000000..d862689c
> > --- /dev/null
> > +++ b/xlat/drm_mode_flags.in
> > @@ -0,0 +1,6 @@
> "#sorted" is (probably) missing here.
>
> > +DRM_MODE_FLAG_PIC_AR_MASK (0x0F << 19)
> I doubt this one should be here.
>
> > +DRM_MODE_FLAG_PIC_AR_NONE (0 << 19)
> > +DRM_MODE_FLAG_PIC_AR_4_3 (1 << 19)
> > +DRM_MODE_FLAG_PIC_AR_16_9 (2 << 19)
> > +DRM_MODE_FLAG_PIC_AR_64_27 (3 << 19)
> > +DRM_MODE_FLAG_PIC_AR_256_135 (4 << 19)
> > diff --git a/xlat/drm_mode_page_flip_flags.in b/xlat/
> drm_mode_page_flip_flags.in
> > new file mode 100644
> > index 00000000..b0006d84
> > --- /dev/null
> > +++ b/xlat/drm_mode_page_flip_flags.in
> > @@ -0,0 +1,6 @@
>
> > +DRM_MODE_PAGE_FLIP_EVENT
> > +DRM_MODE_PAGE_FLIP_ASYNC
> > +DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE
> > +DRM_MODE_PAGE_FLIP_TARGET_RELATIVE
> Those doesn't feel to be arch-specific, so their values can be provided
> here.
>
> > +DRM_MODE_PAGE_FLIP_TARGET
> It should be before DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE and
> DRM_MODE_PAGE_FLIP_TARGET_RELATIVE in order to be decoded by printflags.
>
> > +DRM_MODE_PAGE_FLIP_FLAGS
> It should go first in order to be decoded bt printflags. I'm not sure,
> however, whether this is a part of kernel API.
>
> > diff --git a/xlat/drm_mode_type.in b/xlat/drm_mode_type.in
> > new file mode 100644
> > index 00000000..3e9a9094
> > --- /dev/null
> > +++ b/xlat/drm_mode_type.in
> > @@ -0,0 +1,8 @@
> "#sorted" is (probably) missing here.
>
> > +DRM_MODE_TYPE_BUILTIN (1 << 0)
> > +DRM_MODE_TYPE_CLOCK_C (1 << 1 | DRM_MODE_TYPE_BUILTIN)
> > +DRM_MODE_TYPE_CRTC_C (1 << 2 | DRM_MODE_TYPE_BUILTIN)
> > +DRM_MODE_TYPE_PREFERRED (1 << 3)
> > +DRM_MODE_TYPE_DEFAULT (1 << 4)
> > +DRM_MODE_TYPE_USERDEF (1 << 5)
> > +DRM_MODE_TYPE_DRIVER (1 << 6)
> > +DRM_MODE_TYPE_ALL (DRM_MODE_TYPE_PREFERRED |
> DRM_MODE_TYPE_USERDEF | DRM_MODE_TYPE_DRIVER)
> > diff --git a/xlat/drm_modeset_cmd.in b/xlat/drm_modeset_cmd.in
> > new file mode 100644
> > index 00000000..a58a9076
> > --- /dev/null
> > +++ b/xlat/drm_modeset_cmd.in
> > @@ -0,0 +1,2 @@
> "#value_indexed" is missing here.
>
> > +_DRM_PRE_MODESET 1
> > +_DRM_POST_MODESET 2
>
> > diff --git a/xlat/drm_stat_type.in b/xlat/drm_stat_type.in
> > new file mode 100644
> > index 00000000..b15f18d5
> > --- /dev/null
> > +++ b/xlat/drm_stat_type.in
> > @@ -0,0 +1,15 @@
> "#value_indexed" is missing here.
>
> > +_DRM_STAT_LOCK 0
> > +_DRM_STAT_OPENS 1
>
> > diff --git a/xlat/drm_syncobj_flags.in b/xlat/drm_syncobj_flags.in
> > new file mode 100644
> > index 00000000..b047f1c0
> > --- /dev/null
> > +++ b/xlat/drm_syncobj_flags.in
> > @@ -0,0 +1 @@
> "#sorted" is missing here.
>
> > +DRM_SYNCOBJ_CREATE_SIGNALED (1 << 0)
> > diff --git a/xlat/drm_syncobj_wait_flags.in b/xlat/
> drm_syncobj_wait_flags.in
> > new file mode 100644
> > index 00000000..185f94b1
> > --- /dev/null
> > +++ b/xlat/drm_syncobj_wait_flags.in
> > @@ -0,0 +1,3 @@
> "#sorted" is missing here.
>
> > +DRM_SYNCOBJ_WAIT_FLAGS_WAIT_ALL (1 << 0)
> > +DRM_SYNCOBJ_WAIT_FLAGS_WAIT_FOR_SUBMIT (1 << 1)
> > +DRM_SYNCOBJ_WAIT_FLAGS_WAIT_AVAILABLE (1 << 2)
>
>
> > diff --git a/xlat/drm_vblank_seq_type.in b/xlat/drm_vblank_seq_type.in
> > new file mode 100644
> > index 00000000..6460cf55
> > --- /dev/null
> > +++ b/xlat/drm_vblank_seq_type.in
> > @@ -0,0 +1,8 @@
> "#sorted" is missing here.
>
> > +_DRM_VBLANK_ABSOLUTE 0x0
> > +_DRM_VBLANK_RELATIVE 0x1
>
> > +_DRM_VBLANK_HIGH_CRTC_MASK 0x0000003e
> I'm not sure about leading zeroes here, it complicates reading.
>
> > +_DRM_VBLANK_EVENT 0x4000000
> > +_DRM_VBLANK_FLIP 0x8000000
> > +_DRM_VBLANK_NEXTONMISS 0x10000000
> > +_DRM_VBLANK_SECONDARY 0x20000000
> > +_DRM_VBLANK_SIGNAL 0x40000000
>
>
> It's also likely worth considering abbreviating output for some
> informational ioctls.
>
You mean something like -v option? I've been thinking about this and
planing to add support for this in the next patchset.
--
> Strace-devel mailing list
> Strace-devel at lists.strace.io
> https://lists.strace.io/mailman/listinfo/strace-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20190731/41995607/attachment.html>
More information about the Strace-devel
mailing list