On Mon, 2019-02-25 at 13:34 +0000, Tvrtko Ursulin wrote:
> On 21/02/2019 02:58, Carlos Santa wrote:
> > From: Michel Thierry <michel.thierry@xxxxxxxxx>
> >
> > Users/tests relying on the total reset count will start seeing a
> > smaller
> > number since most of the hangs can be handled by engine reset.
> > Note that if reset engine x, context a running on engine y will be
> > unaware
> > and unaffected.
> >
> > To start the discussion, include just a total engine reset count.
> > If it
> > is deemed useful, it can be extended to report each engine
> > separately.
> >
> > Our igt's gem_reset_stats test will need changes to ignore the pad
> > field,
> > since it can now return reset_engine_count.
> >
> > v2: s/engine_reset/reset_engine/, use union in uapi to not break
> > compatibility.
> > v3: Keep rejecting attempts to use pad as input (Antonio)
> > v4: Rebased.
> > v5: Rebased.
> >
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Mika Kuoppala <mika.kuoppala@xxxxxxxxxxxxxxx>
> > Cc: Antonio Argenziano <antonio.argenziano@xxxxxxxxx>
> > Cc: Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> > Signed-off-by: Michel Thierry <michel.thierry@xxxxxxxxx>
> > Signed-off-by: Carlos Santa <carlos.santa@xxxxxxxxx>
> > ---
> > drivers/gpu/drm/i915/i915_gem_context.c | 12 ++++++++++--
> > include/uapi/drm/i915_drm.h | 6 +++++-
> > 2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_context.c
> > b/drivers/gpu/drm/i915/i915_gem_context.c
> > index 459f8eae1c39..cbfe8f2eb3f2 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> > @@ -1889,6 +1889,8 @@ int i915_gem_context_reset_stats_ioctl(struct
> > drm_device *dev,
> > struct drm_i915_private *dev_priv = to_i915(dev);
> > struct drm_i915_reset_stats *args = data;
> > struct i915_gem_context *ctx;
> > + struct intel_engine_cs *engine;
> > + enum intel_engine_id id;
> > int ret;
> >
> > if (args->flags || args->pad)
> > @@ -1907,10 +1909,16 @@ int
> > i915_gem_context_reset_stats_ioctl(struct drm_device *dev,
> > * we should wrap the hangstats with a seqlock.
> > */
> >
> > - if (capable(CAP_SYS_ADMIN))
> > + if (capable(CAP_SYS_ADMIN)) {
> > args->reset_count = i915_reset_count(&dev_priv-
> > >gpu_error);
> > - else
> > + for_each_engine(engine, dev_priv, id)
> > + args->reset_engine_count +=
> > + i915_reset_engine_count(&dev_priv-
> > >gpu_error,
> > + engine);
>
> If access to global GPU reset count is privileged, why is access to
> global engine reset count not? It seems to be fundamentally same
> level
> of data leakage.
But access to global engine reset count (i915_reset_engine_count) is
indeed priviledged. They both are inside if(CAP_SYS_ADMIN){...}, or
maybe I am missing something?
>
> If we wanted to provide some numbers to unprivileged users I think
> we
> would need to store some counters per file_priv/context and return
> those
> when !CAP_SYS_ADMIN.
The question would be why access to global GPU reset count is
priviledged then? I can't think of a reason why it should be
priviledged. I think the new counter (per engine) should fall in the
same category as the global GPU reset one, right? So, can we make them
both unpriviledged?
>
> > + } else {
> > args->reset_count = 0;
> > + args->reset_engine_count = 0;
> > + }
> >
> > args->batch_active = atomic_read(&ctx->guilty_count);
> > args->batch_pending = atomic_read(&ctx->active_count);
> > diff --git a/include/uapi/drm/i915_drm.h
> > b/include/uapi/drm/i915_drm.h
> > index cc03ef9f885f..3f2c89740b0e 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1642,7 +1642,11 @@ struct drm_i915_reset_stats {
> > /* Number of batches lost pending for execution, for this
> > context */
> > __u32 batch_pending;
> >
> > - __u32 pad;
> > + union {
> > + __u32 pad;
> > + /* Engine resets since boot/module reload, for all
> > contexts */
> > + __u32 reset_engine_count;
> > + };
>
> Chris pointed out in some other review that anonymous unions are not
> friendly towards C++ compilers.
>
> Not sure what is the best option here. Renaming the field could
> break
> old userspace building against newer headers. Is that acceptable?
>
I dug up some old comments from Chris and he stated that recycling the
union like that would be a bad idea since that would make the pad field
an output only parameter thus invalidating gem_reset_stats...
Why can't we simply add a new field __u32 reset_engine_count; as part
of the drm_i915_reset_stats struct?
Regards,
Carlos
> > };
> >
> > struct drm_i915_gem_userptr {
> >
>
> Regards,
>
> Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
