On Tue, Jun 16, 2015 at 06:32:22PM +0100, Tomas Elf wrote: > On 16/06/2015 17:55, Chris Wilson wrote: > >On Tue, Jun 16, 2015 at 04:43:55PM +0100, Tomas Elf wrote: > >>On 16/06/2015 14:43, Daniel Vetter wrote: > >>>On Mon, Jun 08, 2015 at 06:03:20PM +0100, Tomas Elf wrote: > >>>>The TDR ULT used to validate this patch series requires a special uevent for > >>>>full GPU resets in order to distinguish between different kinds of resets. > >>>> > >>>>Signed-off-by: Tomas Elf <tomas.elf@xxxxxxxxx> > >>> > >>>Why duplicate the uevent we send out from i915_reset_and_wakeup? At least > >>>I can't spot what this gets us in addition to the existing one. > >>>-Daniel > >> > >>Look at this line: > >>>>+ reset_event[0] = kasprintf(GFP_KERNEL, "%s", "GPU RESET=0"); > >> > >>It doesn't exist in reset_and_wakeup (specifically, the "GPU > >>RESET=0" part). It's a uevent that happens at the time of the actual > >>GPU reset (GDRST register write). In the subsequent TDR commit we > >>add another one to the point of the actual engine reset, which also > >>includes information about what exact engine was reset. > >> > >>The uevents in reset_and_wakeup only tell the user that an error has > >>been detected and that some kind of reset has happened, these new > >>uevents specify exactly what kind of reset has happened. This > >>particular one on its own it's not very meaningful since there is > >>only one supported form of reset at this point but once we add > >>engine reset support it's useful to be able to discern the types of > >>resets from each other (GPU reset, RCS engine reset, VCS engine > >>reset, VCS2 engine reset, BCS engine reset, VECS engine reset). > >> > >>Does that make sense? > > > >The ultimate question is how do you envisage these uevents being used? > > > >At present, we have abrtd listening out for when to grab the > >/sys/drm/cardX/error and maybe for GL robustness (though I would imagine > >if they thought such events useful we would have had demands for a DRM > >event on the guilty/victim fd). > > > >Does it really make sense to send uevents for both hang, partial-reset, > >and full-reset? > >-Chris > > > > The reason we have such a detailed set of uevents is primarily for > testing purposes. Our internal VPG tests check for these uevents to > make sure that the expected recovery mode is actually being used. > Which makes sense, because the TDR driver code contains reset > promotion logic to decide what recovery mode to use and if that > logic somehow gets broken the driver might go with the wrong > recovery mode. Thus, it's worth testing and therefore those uevents > need to be there. Of course, I guess the argument "our internal VPG > tests do this" might not hold water since the tests haven't been > upstreamed? If that's the case then I guess I don't have any opinion > about what uevent goes where and we could go with whatever set of > uevents you prefer. uevents aren't free, so if we can find a way to get coverage without waking up the buses, I'd be happy (though we shouldn't need that many reset uevents, it is mainly the principle of trying to use the right tool for the job and not just the first one that comes to hand). Also uevents are by their very nature userspace ABI so we should plan carefully how we expect them to be used. I would have thought tracepoints would have provided better level of detail. Or just a debugfs file with each stage passed - polling that throughout the test would give the same edge events. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
