Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature

From: Daniel Vetter <daniel@ffwll.ch>
To: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: dri-devel@lists.freedesktop.org, cphealy@gmail.com
Subject: Re: [PATCH 01/10] drm/etnaviv: add uapi for register read feature
Date: Tue, 31 Jan 2017 08:40:02 +0100	[thread overview]
Message-ID: <20170131074002.pgv7wg6g4jseaeev@phenom.ffwll.local> (raw)
In-Reply-To: <20170130234559.GN27312@n2100.armlinux.org.uk>

On Mon, Jan 30, 2017 at 11:45:59PM +0000, Russell King - ARM Linux wrote:
> On Mon, Jan 30, 2017 at 09:18:25PM +0100, Thierry Reding wrote:
> > On Fri, Dec 09, 2016 at 12:21:22PM +0100, Christian Gmeiner wrote:
> > > @@ -167,6 +174,9 @@ struct drm_etnaviv_gem_submit {
> > >  	__u64 bos;            /* in, ptr to array of submit_bo's */
> > >  	__u64 relocs;         /* in, ptr to array of submit_reloc's */
> > >  	__u64 stream;         /* in, ptr to cmdstream */
> > > +	__u64 readbacks;      /* in, ptr to array of submit_readback's */
> > > +	__u32 nr_readbacks;   /* in, number of submit_readback's */
> > > +	__u32 padding;
> > >  };
> > 
> > What about ABI stability? How's this going to work when userspace uses
> > old headers but runs against a new kernel? What about userspace using
> > newer headers than the kernel?
> 
> +1, this is unacceptable.  We went through a period of making sure
> that the ABI was going to be stable once we merged the driver into
> the kernel, and the rule is that we don't ever break userspace.  This
> does exactly that, so it's not permissible.
> 
> If this change is necessary, it needs to be a new ioctl number for the
> call with the new structure, and the old ioctl has to keep working.
> 
> There are other users of etnaviv other than mesa that will break.

As long as you don't do it wrong (i.e. struct not used in arrays, and some
flags to indicate when the new fields should be looked at) then it's
perfectly safe to extend drm ioctl structs at the end. The core drm ioctl
functions zero-pad length mismatches in both directions if needed.

The only additional recommendation is to still do a new #define (the ioctl
number has changed anyway since it encodes the size), to make sure that
old userspace still uses the old structs even with upgraded headers.
Otherwise they might not properly clear the new fields, which is the one
case that breaks backwards/forwards compat (been there, done that, not
fun).

We should probably have a chapter in the drm-uapi.rst docs about this,
with a link to the botching-up-ioctls.txt file with the more general
recommendatations.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel