On 7/11/2017 5:54 PM, Chris Wilson wrote:
Quoting Michel Thierry (2017-07-12 01:14:46)
On 7/11/2017 5:07 PM, Chris Wilson wrote:
Quoting Chris Wilson (2017-07-12 01:00:02)
Also feel free to try and fixup the other confusion in intel_lrc.c.
Perhaps something like:
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b0738d2..f498aa6 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2031,8 +2031,9 @@ static int execlists_context_deferred_alloc(struct
i915_gem_context *ctx,
context_size = round_up(engine->context_size, I915_GTT_PAGE_SIZE);
- /* One extra page as the sharing data between driver and GuC */
- context_size += PAGE_SIZE * LRC_PPHWSP_PN;
+ /* Add an extra page at the start for sharing data with the GuC */
+ BUILD_BUG_ON(LRC_PPHWSP_PN != 1);
+ context_size += PAGE_SIZE;
ctx_obj = i915_gem_object_create(ctx->i915, context_size);
if (IS_ERR(ctx_obj)) {
Or better
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b0738d2..0b7fb38 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2031,8 +2031,8 @@ static int execlists_context_deferred_alloc(struct
i915_gem_context *ctx,
context_size = round_up(engine->context_size, I915_GTT_PAGE_SIZE);
- /* One extra page as the sharing data between driver and GuC */
- context_size += PAGE_SIZE * LRC_PPHWSP_PN;
+ /* Add an extra page at the start for sharing data with the GuC */
+ context_size += LRC_GUCSHR_PAGES * PAGE_SIZE;
One more thought about this, and with a little coaxing LRC_PPHWSP_PN may
be the most appropriate constant to use here.
/* Before the actual start of the context image, we insert a few pages
* for our own use and for sharing with the GuC.
*/
context_size += LRC_HEADER_PAGES * PAGE_SIZE;
ctx_obj = i915_gem_object_create(ctx->i915, context_size);
if (IS_ERR(ctx_obj)) {
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 52b3a1f..848ca21 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -72,7 +72,8 @@ int logical_xcs_ring_init(struct intel_engine_cs *engine);
/* One extra page is added before LRC for GuC as shared data */
#define LRC_GUCSHR_PN (0)
-#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1)
+#define LRC_GUCSHR_PAGES 1
+#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + LRC_GUCSHR_PAGES)
#define LRC_STATE_PN (LRC_PPHWSP_PN + 1)
but
/* We allocate a header at the start of the context image for our own
* use, therefore the actual location of the logical state is offset
* from the start of the VMA. The layout is
*
* | [guc] | [hwsp] [logical state] |
* |<- our header ->|<- context image ->|
*
*/
/* One extra page is used for sharing with the GuC */
#define LRC_GUCSHR_PN (0)
#define LRC_GUCSHR_PAGES 1
/* At the start of the context image is its per-process HWS page */
#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + LRC_GUCSHR_PAGES)
/* Finally we have the logical state for the context */
#define LRC_STATE_PN (LRC_PPHWSP_PN + 1)
/* Currently we include the PPHWSP in __intel_engine_context_size() so
* the size of the header is synonymous with the start of the PPHWSP.
*/
#define LRC_HEADER_PAGES LRC_PPHWSP_PN
It cannot be clearer now.
The only extra thing I was thinking about was also defining the 'size'
(length?) of the guc and pphwsp sections, in case those "+ 1" were not
clear:
#define LRC_GUCSHR_PN (0)
-#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1)
-#define LRC_STATE_PN (LRC_PPHWSP_PN + 1)
+#define LRC_GUCSHR_SZ (1)
+#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + LRC_GUCSHR_SZ)
+#define LRC_PPHWSP_SZ (1)
+#define LRC_STATE_PN (LRC_PPHWSP_PN + LRC_PPHWSP_SZ)
struct drm_i915_private;
Nowhere do we use LRC_GUCSHR_PN. Having it just gives us the mirage of
flexibility...
LRC_GUCSHR_PN is not used because people assumed it is always page 0 of
the kernel ctx :S
One more thing to fix, use LRC_GUCSHR_PN in all the places where this
_shared_ GuC page is used (guc_ggtt_offset(ctx->engine[RCS].state))
That may prove very handy.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
