Re: [PATCH 2/2] drm/i915/guc: default to using GuC submission where possible

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Dave Gordon <david.s.gordon@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 2/2] drm/i915/guc: default to using GuC submission where possible
Date: Fri, 22 Apr 2016 19:45:15 +0100	[thread overview]
Message-ID: <20160422184515.GK17454@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1461349375-5394-2-git-send-email-david.s.gordon@intel.com>

On Fri, Apr 22, 2016 at 07:22:55PM +0100, Dave Gordon wrote:
> This patch simply changes the default value of "enable_guc_submission"
> from 0 (never) to -1 (auto). This means that GuC submission will be
> used if the platform has a GuC, the GuC supports the request submission
> protocol, and any required GuC firmwware was successfully loaded. If any
> of these conditions are not met, the driver will fall back to using
> execlist mode.

There are several shortcomings yet, in particular you've decided to add
new ABI...

i915_guc_wq_check_space(): Do not return ETIMEDOUT. This function's
return code is ABI.

Your choices are either EAGAIN if you think the hardware will catch up, or
more likely EIO and reset the hardware.

guc_add_workqueue_item: cannot fail

It must be fixed such that it is a void function without possibility of
failure. Not that you even successfully hooked up the failure paths in
the current code.

Same for guc_ring_doorbell.

And what exactly is that atomic64_cmpxchg() serialising with? There are
no other CPUs contending with the write, and neither does the GuC (and I
doubt it is taking any notice of the lock cmpxchg). Using cmpxchg where
a single WRITE_ONCE() of a 32bit value wins the perf prize for hotest
instruction and function in the kernel.
-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