[Intel-gfx] [Mesa-dev] [PATCH v3 4/4] drm/doc/rfc: i915 DG1 uAPI

[Jason Ekstrand]

On Wed, Apr 21, 2021 at 2:23 PM Daniel Vetter <daniel at ffwll.ch> wrote:
>
> On Wed, Apr 21, 2021 at 8:28 PM Tvrtko Ursulin
> <tvrtko.ursulin at linux.intel.com> wrote:
> > On 21/04/2021 18:17, Jason Ekstrand wrote:
> > > On Wed, Apr 21, 2021 at 9:25 AM Tvrtko Ursulin
> > > <tvrtko.ursulin at linux.intel.com> wrote:
> > >> On 21/04/2021 14:54, Jason Ekstrand wrote:
> > >>> On Wed, Apr 21, 2021 at 3:22 AM Tvrtko Ursulin
> > >>> <tvrtko.ursulin at linux.intel.com> wrote:
> > >>>> On 20/04/2021 18:00, Jason Ekstrand wrote:
> > >>>> I am not claiming to know memory region query will end up the same, and
> > >>>> I definitely agree we cannot guess the future. I am just saying rsvd
> > >>>> fields are inconsequential really in terms of maintenance burden and
> > >>>> have been proven useful in the past. So I disagree with the drive to
> > >>>> kick them all out.
> > >>>
> > >>> Sure, it doesn't cost anything to have extra zeros in the struct.
> > >>> However, if/when the API grows using rsvd fields, we end up with "if
> > >>> CAP_FOO is set, rsvd[5] means blah" which makes for a horribly
> > >>> confusing API.  As a userspace person who has to remember how to use
> > >>> this stuff, I'd rather make another call or chain in a struct than try
> > >>> to remember and/or figure out what all 8 rsvd fields mean.
> > >>
> > >> Well it's not called rsvd in the uapi which is aware of the new field
> > >> but has a new name.
> > >
> > > Are we allowed to do that?  This is a genuine question.  When I've
> > > tried in the past (cliprects), I was told we couldn't rename it even
> > > though literally no one had used it in code for years.
> >
> > Well we did the union for pad_to_size so I thought we are allowed that
> > trick at least. From my experience backward source level compatibility
> > is not always there even with things like glibc. Despite that, are we
> > generally required to stay backward source compatible I will not claim
> > either way.

I'm starting to lose the will to care about this particular bike shed.
I think I'm fine with keeping some RSVD fields as long as:

 1. We're very clear in the docs with flags and caps about what things
are inputs and what things are outputs and how they interact.
Preferably, everything is an output.
 2. If we already know that caps is useless without supported_caps,
let's either add supported_caps in now or throw out both and use some
rsvd for them when they begin to be needed.
 3. We have a plan for how we're going to use them in a
backwards-compatible way.

On 3, it sounds like we have a rough sketch of a plan but I'm still
unclear on some details.  In particular, we have an rsvd[8] at the end
but, if we're going to replace individual bits of it, we can't ever
shorten or re-name that array.  We could, potentially, do

union {
    __u32 rsvd[8];
    struct {
        __u32 new_field;
    };
};

and trust C to put all the fields of our anonymous struct at the top.
Otherwise, we've got to fill out our struct with more rsvd and that
gets to be a mess.

Another option would be to have

__u32 rsvd1;
__u32 rsvd2;
__u32 rsvd3;
/* etc... */

and we can replace them one at a time.

Again, my big point is that I want us to have a PLAN and not end up in
a scenario where we end up with rsvd[5] having magical meaning.  What
I see in DII does not give me confidence.  However, I do believe that
such a plan can exist.

--Jason

> I think the anonymous union with exactly same sized field is ok. We
> also try hard to be source compatible, but we have screwed up in the
> past and shrugged it off. The one example that comes to mind is
> extended structures at the bottom with new field, which the kernel
> automatically zero-extends for old userspace. But when you recompile,
> your new-old userspace might no longer clear the new fields because
> the ioctl code didn't start out by memset()ing the entire struct.

Also, we need to ensure that we memset everything to 0. :-)

--Jason