On Tue, Apr 18, 2017 at 02:36:14PM -0700, Michel Thierry wrote:
>
> On 18/04/17 14:20, Chris Wilson wrote:
> >On Tue, Apr 18, 2017 at 01:23:32PM -0700, Michel Thierry wrote:
> >>@@ -1329,10 +1331,29 @@ static int gen8_emit_bb_start(struct
> >>drm_i915_gem_request *req,
> >> req->ctx->ppgtt->pd_dirty_rings &=
> >> ~intel_engine_flag(req->engine);
> >> }
> >>
> >>- cs = intel_ring_begin(req, 4);
> >>+ /* bb_start only */
> >>+ num_dwords = 4;
> >>+
> >>+ /* check if watchdog will be required */
> >>+ if (req->ctx->engine[req->engine->id].watchdog_threshold != 0) {
> >>+ if (!req->engine->emit_start_watchdog ||
> >>+ !req->engine->emit_stop_watchdog)
> >>+ return -EINVAL;
> >
> >This is still a bug in the context setparam to get to this point without
> >a watchdog.
> >
>
> This can't happen (threshold != 0 && no emit_watchdog func),
> i915_gem_context_set_watchdog returns -ENODEV if vcs's
> emit_start_watchdog is not defined (the assumption is if the vcs has
> it, rcs does too).
>
> I can remove it, if that's what you mean.
Yes, we shouldn't be setting the watchdog threshold if the watchdog is
not available. GEM_BUG_ON() would be fine. Throwing a very, very late
EINVAL is disconcerting.
> But re i915_gem_context_set_watchdog, I think maybe it should return
> ENODEV when there's no watchdog and the user is trying to get the
> array size (args->size == 0), and don't give false hopes.
Seems reasonable.
> >>+
> >>+ /* + start_watchdog (6) + stop_watchdog (4) */
> >>+ num_dwords += 10;
> >>+ watchdog_running = true;
> >>+ }
> >>+static u32 *gen8_emit_stop_watchdog(struct drm_i915_gem_request *req, u32
> >>*cs)
> >>+{
> >>+ struct intel_engine_cs *engine = req->engine;
> >>+
> >>+ /* XXX: no watchdog support in BCS engine */
> >>+ GEM_BUG_ON(engine->id == BCS);
> >>+
> >>+ *cs++ = MI_LOAD_REGISTER_IMM(2);
> >>+ *cs++ = i915_mmio_reg_offset(RING_CNTR(engine->mmio_base));
> >>+ *cs++ = get_watchdog_disable(engine);
> >>+ *cs++ = MI_NOOP;
> >
> >Oops.
>
> _context_set_watchdog also rejects if threshold[BCS] != 0.
LRI(2), but only setting one register not two.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
