[Intel-gfx] [PATCH 12/39] drm/i915: Allow a context to define its set of engines

[Chris Wilson]

Quoting Tvrtko Ursulin (2019-03-14 17:58:00)
> 
> On 14/03/2019 17:15, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-03-14 16:47:05)
> >>
> >> On 13/03/2019 14:43, Chris Wilson wrote:
> >>> +static int clone_engines(struct i915_gem_context *dst,
> >>> +                      struct i915_gem_context *src)
> >>> +{
> >>> +     struct intel_engine_cs **engines;
> >>> +     unsigned int nengine;
> >>> +
> >>> +     mutex_lock(&src->i915->drm.struct_mutex); /* serialise src->engine[] */
> >>> +     nengine = src->nengine;
> >>> +     if (!ZERO_OR_NULL_PTR(src->engines))
> >>
> >> A comment taking note of the reason for the ZERO_PTR trick.
> > 
> > I must have deleted /* XXX why kmemdup, why? */
> 
> Indeed why kmemdup? :) I thought it was about allowing count = 0 in the 
> set_engines and storing ZERO_PTR in ctx->engines in that case. In which 
> case it fall through the else branch and copies over the ZERO_PTR. 
> Unless you wanted to complain that kmemdup does not handle ZERO_PTR in 
> the source?

My first reaction was that kmemdup would indeed take care of the
ZERO_SIZE_PTR. I still think it should and should send a patch so that
it does.

> >>> @@ -110,6 +112,8 @@ struct i915_gem_context {
> >>>    #define CONTEXT_CLOSED                      1
> >>>    #define CONTEXT_FORCE_SINGLE_SUBMISSION     2
> >>>    
> >>> +     unsigned int nengine;
> >>
> >> Can I tempt you to pluralise this? I at least find it more obvious in
> >> plural.
> > 
> > Bah, historical precedent is
> > int   nengine;
> > void *engines;
> 
> Not everything historical is good! :)

I guess you want a num_ as well!

> >>> +static inline bool
> >>> +__check_struct_size(size_t base, size_t arr, size_t count, size_t *size)
> >>> +{
> >>> +     size_t sz;
> >>> +
> >>> +     if (check_mul_overflow(count, arr, &sz))
> >>> +             return false;
> >>> +
> >>> +     if (check_add_overflow(sz, base, &sz))
> >>> +             return false;
> >>> +
> >>> +     *size = sz;
> >>> +     return true;
> >>> +}
> >>> +
> >>> +#define check_struct_size(T, A, C, SZ) \
> >>> +     likely(__check_struct_size(sizeof(*(T)), \
> >>> +                                sizeof(*(T)->A) + __must_be_array((T)->A), \
> >>> +                                C, SZ))
> >>
> >> A comment explaining usage would be nice.
> > 
> > /* see struct_size() */
> 
> Come on, a bit more than that! Look at those self-documenting args, T, 
> A, C, SZ! It needs to say what it will calculate, where it stores it and 
> what it returns. If you want to have more hope people will actually use 
> the helpers you add.

That alternative is to use
	check_struct_size() (struct_size() < SIZE_MAX)

I didn't like that as it's conflating the overflow error and hope that
the compiler would be smarter with the overflow checking distinct.
-Chris