Re: [PATCH v4 5/5] drm: Add decoding of DRM and KMS ioctls

[Patrik J. <pat...@li...>]

On Wed, Sep 09, 2015 at 01:50:40AM +0300, Dmitry V. Levin wrote:
> On Mon, Aug 24, 2015 at 02:42:50PM +0200, Patrik Jakobsson wrote:
> > First batch of drm / kms ioctls.
> 
> Several comments in addition to issues similar to 4/5 patch.
> 
> > +static int drm_mode_rm_fb(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	unsigned int handle;
> > +
> > +
> > +	if (entering(tcp)) {
> > +		if (umove(tcp, arg, &handle))
> > +			return RVAL_DECODED;
> > +
> > +		tprintf(", %u", handle);
> > +	}
> > +
> > +	return RVAL_DECODED | 1;
> > +}
> 
> Use printnum_int instead:
> 
> 	tprints(", ");
> 	printnum_int(tcp, arg, "%u");
> 	return RVAL_DECODED | 1;

Ok

> > +static int drm_mode_create_dumb(struct tcb *tcp, const unsigned int code, long arg)
> > +{
> > +	struct drm_mode_create_dumb dumb;
> > +
> > +	if (umove(tcp, arg, &dumb))
> > +		return RVAL_DECODED;
> > +
> > +	if (entering(tcp)) {
> > +		tprintf(", {width=%u, height=%u, bpp=%u, flags=0x%x",
> > +			dumb.width, dumb.height, dumb.bpp, dumb.flags);
> > +	} else if (exiting(tcp)) {
> > +		tprintf(", handle=%u, pitch=%u, size=%Lu}", dumb.handle,
> > +			dumb.pitch, dumb.size);
> > +	}
> > +
> > +	return RVAL_DECODED | 1;
> > +}
> 
> This generates a warning (which turns into an error with
> --enable-gcc-Werror) on x86_64 when using kernel drm headers:
> 
> drm.c: In function 'drm_mode_create_dumb':
> drm.c:521:11: error: format '%Lu' expects argument of type 'long long unsigned int', but argument 4 has type 'uint64_t {aka long unsigned int}' [-Werror=format=]

So this brings us back to whether to include drm kernel headers or not. If
-Werror is a requirement (which is already broken last time I checked) there
will need to be #ifdefs at various places in drm decoding. What would you
prefer. Both options are fine by me.

> > @@ -112,6 +577,84 @@ int drm_ioctl(struct tcb *tcp, const unsigned int code, long arg)
> >  	if (drm_is_priv(tcp->u_arg[1])) {
> >  		if (verbose(tcp) && drm_is_driver(tcp, "i915"))
> >  			ret = drm_i915_ioctl(tcp, code, arg);
> > +	} else {
> > +		switch (code) {
> > +		case DRM_IOCTL_VERSION:
> > +			ret = drm_version(tcp, code, arg);
> > +			break;
> 
> Looks like the return code can be forwarded without further delays.

Yes, this is a leftover from before the tcb priv interface. Will remove.
> 
> 
> -- 
> ldv