Re: drm/i915: Watchdog timeout: DRM kernel interface to set the timeout

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Tvrtko Ursulin (2019-01-07 13:39:24)
> 
> On 07/01/2019 12:50, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2019-01-07 12:38:47)
> >> On 05/01/2019 02:39, Carlos Santa wrote:
> >>> +/* Return the timer count threshold in microseconds. */
> >>> +int i915_gem_context_get_watchdog(struct i915_gem_context *ctx,
> >>> +                               struct drm_i915_gem_context_param *args)
> >>> +     if (__copy_to_user(u64_to_user_ptr(args->value),
> >>> +                        &threshold_in_us,
> >>> +                        sizeof(threshold_in_us))) {
> >>
> >> I think user pointer hasn't been verified for write access yet so
> >> standard copy_to_user should be used.
> > 
> > The starting point here is that this bakes in OTHER_CLASS as uABI when
> > clearly it is not uABI. The array must be variable size and so the first
> > pass is to report the size to userspace (if they pass args->size = 0)
> > and then to copy up to the user requested size. Since it is a variable
> > array with uncertain indexing, the output should be along the lines of
> > [class, instance, watchdog]. But what about virtual engine? Good
> > question. (Remember set must follow the same pattern as get, so you can
> > always do a rmw cleanly.)
> > 
> > I guess we just have to accept class == -1 as meaning use instance as an
> > index into ctx->engines[]. Tvrtko? Virtual engine would be reported as
> > (-1, 0, watchdog), and settable similarly with the other engines being
> > addressable either by index or by class-instance.
> 
> Or use index based addressing mode if engine map is set? Index 0 only 
> being valid if load balancing is enabled on top.

I was trying to avoid tying ctx->engines[] into such an extensive set of
API changes, but given that it affects execbuffer_ioctl, the die is
cast. With that approach, every time we use class-instance (with respect
to a context) we should also swap it over to using an index when
ctx->engines[] is set.

The advantage to using a flag (in this case instance=-1) is that we can
then write igt (or library routines) oblivious to the state of
ctx->engines[].

From an application pov, they can ignore it, as they will be doing the
SET_ENGINES as part of their initial context construction (one presumes)
and so will always have ctx->engines[] known to be active.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx