Re: [PATCH 02/12] drm/i915/guc: Allocate separate shared data object for GuC communication

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 09/10/17 15:35, Michel Thierry wrote:
On 10/9/2017 11:41 AM, Daniele Ceraolo Spurio wrote:


On 09/10/17 07:52, Michał Winiarski wrote:
We were using first page of kernel context render state for sharing data
with GuC. While it's justified by the fact that those pages are not used
(note, GuC still enforces this layout and refuses to work if we remove
the extra page in front), it's also confusing (why are we using this
particular page?). Let's allocate a separate object instead.

Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
Cc: Jeff McGee <jeff.mcgee@xxxxxxxxx>
Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>

+Michel (engine and watchdog reset with GuC use the shared page)

---
drivers/gpu/drm/i915/i915_guc_submission.c | 36 +++++++++++++++++++++++++++++-
  drivers/gpu/drm/i915/intel_guc.c           |  8 ++-----
  drivers/gpu/drm/i915/intel_guc.h           |  2 ++
  3 files changed, 39 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 8983d53af229..30f026566001 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -437,6 +437,33 @@ static void guc_stage_desc_fini(struct intel_guc *guc,
      memset(desc, 0, sizeof(*desc));
  }
+static int guc_shared_data_create(struct intel_guc *guc)
+{
+    struct i915_vma *vma;
+    void *vaddr;
+
+    vma = intel_guc_allocate_vma(guc, PAGE_SIZE);
+    if (IS_ERR(vma))
+        return PTR_ERR(vma);
+
+    vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+    if (IS_ERR(vaddr)) {
+        i915_vma_unpin_and_release(&vma);
+        return PTR_ERR(vaddr);
+    }
+
+    guc->shared_data = vma;
+    guc->shared_data_vaddr = vaddr;

Hi,

Allocating the shared_data until this point (i915_guc_submission_init) will be too late for GuC's watchdog.

GuC watchdog happens without i915 knowledge, so we have to pass this shared_data_offset during guc_params_init (in params[9] for the curious) instead of a h2g command; and the GuC parameters block has this note: "These parameters are read by the firmware on startup and cannot be changed thereafter".

Michał, if you plan to send another version of this, could you move it to guc_params_init? It isn't a big issue, I can just move it when we have an open source user and can upstream GuC watchdog.

Thanks,

-Michel


Ignore my previous reply, this is already being allocated before guc_params_init as it is.

Reviewed-by: Michel Thierry <michel.thierry@xxxxxxxxx>


I've noticed that this is now the 3rd place where we allocate and immediately pin a vma for guc (the other 2 being guc_stage_desc_pool_create and ctch_init). Maybe we can move the 2 operations to a more common helper (and do the same for the cleanup), but it's probably better to do it after all the ongoing GuC re-org has settled down. In the meantime, this patch is:

Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>

Daniele
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux