* [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
@ 2017-02-09 10:24 Oscar Mateo
2017-02-09 19:22 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-02-10 12:04 ` [PATCH] " Chris Wilson
0 siblings, 2 replies; 10+ messages in thread
From: Oscar Mateo @ 2017-02-09 10:24 UTC (permalink / raw)
To: intel-gfx
There is other stuff that potentially needs cleaning, even if we didn't get to the point of
creating an execbuf_client.
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 0065b38..e277105 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -963,10 +963,8 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
struct i915_guc_client *client;
client = fetch_and_zero(&guc->execbuf_client);
- if (!client)
- return;
-
- guc_client_free(dev_priv, client);
+ if (client)
+ guc_client_free(dev_priv, client);
i915_vma_unpin_and_release(&guc->ads_vma);
i915_vma_unpin_and_release(&guc->log.vma);
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
2017-02-09 10:24 [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client Oscar Mateo
@ 2017-02-09 19:22 ` Patchwork
2017-02-10 12:04 ` [PATCH] " Chris Wilson
1 sibling, 0 replies; 10+ messages in thread
From: Patchwork @ 2017-02-09 19:22 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
URL : https://patchwork.freedesktop.org/series/19393/
State : success
== Summary ==
Series 19393v1 drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
https://patchwork.freedesktop.org/api/1.0/series/19393/revisions/1/mbox/
fi-bdw-5557u total:252 pass:238 dwarn:0 dfail:0 fail:0 skip:14
fi-bsw-n3050 total:252 pass:213 dwarn:0 dfail:0 fail:0 skip:39
fi-bxt-j4205 total:252 pass:230 dwarn:0 dfail:0 fail:0 skip:22
fi-bxt-t5700 total:83 pass:70 dwarn:0 dfail:0 fail:0 skip:12
fi-byt-j1900 total:252 pass:225 dwarn:0 dfail:0 fail:0 skip:27
fi-byt-n2820 total:252 pass:221 dwarn:0 dfail:0 fail:0 skip:31
fi-hsw-4770 total:252 pass:233 dwarn:0 dfail:0 fail:0 skip:19
fi-hsw-4770r total:252 pass:233 dwarn:0 dfail:0 fail:0 skip:19
fi-ilk-650 total:252 pass:199 dwarn:0 dfail:0 fail:0 skip:53
fi-ivb-3520m total:252 pass:231 dwarn:0 dfail:0 fail:0 skip:21
fi-ivb-3770 total:252 pass:231 dwarn:0 dfail:0 fail:0 skip:21
fi-kbl-7500u total:252 pass:229 dwarn:0 dfail:0 fail:2 skip:21
fi-skl-6260u total:252 pass:239 dwarn:0 dfail:0 fail:0 skip:13
fi-skl-6700hq total:252 pass:232 dwarn:0 dfail:0 fail:0 skip:20
fi-skl-6700k total:252 pass:227 dwarn:4 dfail:0 fail:0 skip:21
fi-skl-6770hq total:252 pass:239 dwarn:0 dfail:0 fail:0 skip:13
fi-snb-2520m total:252 pass:221 dwarn:0 dfail:0 fail:0 skip:31
fi-snb-2600 total:252 pass:220 dwarn:0 dfail:0 fail:0 skip:32
8ca0c2e06c85344b8fee2bd5c48d6f3571fc870a drm-tip: 2017y-02m-09d-13h-35m-52s UTC integration manifest
7227a92 drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
== Logs ==
For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3758/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
2017-02-09 10:24 [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client Oscar Mateo
2017-02-09 19:22 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2017-02-10 12:04 ` Chris Wilson
2017-02-13 15:55 ` Oscar Mateo
1 sibling, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2017-02-10 12:04 UTC (permalink / raw)
To: Oscar Mateo; +Cc: intel-gfx
On Thu, Feb 09, 2017 at 02:24:25AM -0800, Oscar Mateo wrote:
> There is other stuff that potentially needs cleaning, even if we didn't get to the point of
> creating an execbuf_client.
Just because the allocator doesn't employ onion unwinding?
Or is there more to come?
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.oc.uk>
-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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
2017-02-10 12:04 ` [PATCH] " Chris Wilson
@ 2017-02-13 15:55 ` Oscar Mateo
2017-02-14 14:20 ` Joonas Lahtinen
0 siblings, 1 reply; 10+ messages in thread
From: Oscar Mateo @ 2017-02-13 15:55 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On 02/10/2017 04:04 AM, Chris Wilson wrote:
> On Thu, Feb 09, 2017 at 02:24:25AM -0800, Oscar Mateo wrote:
>> There is other stuff that potentially needs cleaning, even if we didn't get to the point of
>> creating an execbuf_client.
> Just because the allocator doesn't employ onion unwinding?
> Or is there more to come?
No, nothing more to come. I just saw this was wrong in passing and
decided to send a patch. But I see Joonas is spearheading a bigger
cleanup, so maybe this can be fixed there.
>> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.oc.uk>
> -Chris
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client
2017-02-13 15:55 ` Oscar Mateo
@ 2017-02-14 14:20 ` Joonas Lahtinen
2017-02-16 14:18 ` [PATCH] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
0 siblings, 1 reply; 10+ messages in thread
From: Joonas Lahtinen @ 2017-02-14 14:20 UTC (permalink / raw)
To: Oscar Mateo, Chris Wilson; +Cc: intel-gfx
On ma, 2017-02-13 at 07:55 -0800, Oscar Mateo wrote:
>
> On 02/10/2017 04:04 AM, Chris Wilson wrote:
> >
> > On Thu, Feb 09, 2017 at 02:24:25AM -0800, Oscar Mateo wrote:
> > >
> > > There is other stuff that potentially needs cleaning, even if we didn't get to the point of
> > > creating an execbuf_client.
> > Just because the allocator doesn't employ onion unwinding?
> > Or is there more to come?
> No, nothing more to come. I just saw this was wrong in passing and
> decided to send a patch. But I see Joonas is spearheading a bigger
> cleanup, so maybe this can be fixed there.
I was actually inspired by this change (I had it in my TODO list) which
I completed while going through the code. Would be great if you can do
a similar fixup of the code to include proper onion teardown to
submission_init and make submission_fini without condition checks.
In the guc_client case it brought up quite good fixes and
irregularities in phasing the init and teardown.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH] drm/i915/guc: Add onion teardown to the GuC setup
2017-02-14 14:20 ` Joonas Lahtinen
@ 2017-02-16 14:18 ` Oscar Mateo
2017-02-16 14:21 ` Oscar Mateo
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Oscar Mateo @ 2017-02-16 14:18 UTC (permalink / raw)
To: intel-gfx
Starting with intel_guc_loader, down to intel_guc_submission
and finally to intel_guc_log.
Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
---
drivers/gpu/drm/i915/i915_guc_submission.c | 94 +++++----
drivers/gpu/drm/i915/intel_guc_loader.c | 19 +-
drivers/gpu/drm/i915/intel_guc_log.c | 309 +++++++++++++++--------------
drivers/gpu/drm/i915/intel_uc.h | 5 +-
4 files changed, 229 insertions(+), 198 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index f7d9897..4147674 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -609,8 +609,14 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
return vma;
}
-static void guc_client_free(struct i915_guc_client *client)
+static void guc_client_free(struct i915_guc_client **p_client)
{
+ struct i915_guc_client *client;
+
+ client = fetch_and_zero(p_client);
+ if (!client)
+ return;
+
/*
* XXX: wait for any outstanding submissions before freeing memory.
* Be sure to drop any locks
@@ -818,7 +824,7 @@ static void guc_policies_init(struct guc_policies *policies)
policies->is_valid = 1;
}
-static void guc_addon_create(struct intel_guc *guc)
+static int guc_addon_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct i915_vma *vma;
@@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
sizeof(struct guc_mmio_reg_state) +
GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
- vma = guc->ads_vma;
- if (!vma) {
- vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
- if (IS_ERR(vma))
- return;
+ vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
+ if (IS_ERR(vma))
+ return PTR_ERR(vma);
- guc->ads_vma = vma;
- }
+ guc->ads_vma = vma;
page = i915_vma_first_page(vma);
ads = kmap(page);
@@ -885,6 +888,13 @@ static void guc_addon_create(struct intel_guc *guc)
sizeof(struct guc_mmio_reg_state);
kunmap(page);
+
+ return 0;
+}
+
+static void guc_addon_destroy(struct intel_guc *guc)
+{
+ i915_vma_unpin_and_release(&guc->ads_vma);
}
/*
@@ -899,6 +909,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
struct intel_guc *guc = &dev_priv->guc;
struct i915_vma *vma;
void *vaddr;
+ int ret;
if (!HAS_GUC_SCHED(dev_priv))
return 0;
@@ -919,15 +930,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
guc->ctx_pool = vma;
- vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
- if (IS_ERR(vaddr))
- goto err;
+ vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
+ if (IS_ERR(vaddr)) {
+ ret = PTR_ERR(vaddr);
+ goto err_vma;
+ }
guc->ctx_pool_vaddr = vaddr;
+ ret = intel_guc_log_create(guc);
+ if (ret < 0)
+ goto err_vaddr;
+
+ ret = guc_addon_create(guc);
+ if (ret < 0)
+ goto err_log;
+
ida_init(&guc->ctx_ids);
- intel_guc_log_create(guc);
- guc_addon_create(guc);
guc->execbuf_client = guc_client_alloc(dev_priv,
INTEL_INFO(dev_priv)->ring_mask,
@@ -935,14 +954,33 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
dev_priv->kernel_context);
if (IS_ERR(guc->execbuf_client)) {
DRM_ERROR("Failed to create GuC client for execbuf!\n");
- goto err;
+ ret = PTR_ERR(guc->execbuf_client);
+ goto err_ads;
}
return 0;
+err_ads:
+ guc_addon_destroy(guc);
+err_log:
+ intel_guc_log_destroy(guc);
+err_vaddr:
+ i915_gem_object_unpin_map(guc->ctx_pool->obj);
+err_vma:
+ i915_vma_unpin_and_release(&guc->ctx_pool);
-err:
- i915_guc_submission_fini(dev_priv);
- return -ENOMEM;
+ return ret;
+}
+
+void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
+{
+ struct intel_guc *guc = &dev_priv->guc;
+
+ guc_client_free(&guc->execbuf_client);
+ ida_destroy(&guc->ctx_ids);
+ guc_addon_destroy(guc);
+ intel_guc_log_destroy(guc);
+ i915_gem_object_unpin_map(guc->ctx_pool->obj);
+ i915_vma_unpin_and_release(&guc->ctx_pool);
}
static void guc_reset_wq(struct i915_guc_client *client)
@@ -1004,26 +1042,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
intel_execlists_enable_submission(dev_priv);
}
-void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
-{
- struct intel_guc *guc = &dev_priv->guc;
- struct i915_guc_client *client;
-
- client = fetch_and_zero(&guc->execbuf_client);
- if (client && !IS_ERR(client))
- guc_client_free(client);
-
- i915_vma_unpin_and_release(&guc->ads_vma);
- i915_vma_unpin_and_release(&guc->log.vma);
-
- if (guc->ctx_pool_vaddr) {
- ida_destroy(&guc->ctx_ids);
- i915_gem_object_unpin_map(guc->ctx_pool->obj);
- }
-
- i915_vma_unpin_and_release(&guc->ctx_pool);
-}
-
/**
* intel_guc_suspend() - notify GuC entering suspend state
* @dev_priv: i915 device private
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
index 22f882d..0b48ad8 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -489,7 +489,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
err = i915_guc_submission_init(dev_priv);
if (err)
- goto fail;
+ goto err_guc;
/*
* WaEnableuKernelHeaderValidFix:skl,bxt
@@ -504,7 +504,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
*/
err = guc_hw_reset(dev_priv);
if (err)
- goto fail;
+ goto err_submission;
intel_huc_load(dev_priv);
err = guc_ucode_xfer(dev_priv);
@@ -512,7 +512,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
break;
if (--retries == 0)
- goto fail;
+ goto err_submission;
DRM_INFO("GuC fw load failed: %d; will reset and "
"retry %d more time(s)\n", err, retries);
@@ -528,7 +528,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
err = i915_guc_submission_enable(dev_priv);
if (err)
- goto fail;
+ goto err_interrupts;
guc_interrupts_capture(dev_priv);
}
@@ -539,15 +539,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
return 0;
+err_interrupts:
+ gen9_disable_guc_interrupts(dev_priv);
+err_submission:
+ i915_guc_submission_fini(dev_priv);
+err_guc:
+ i915_ggtt_disable_guc(dev_priv);
fail:
if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
- guc_interrupts_release(dev_priv);
- i915_guc_submission_disable(dev_priv);
- i915_guc_submission_fini(dev_priv);
- i915_ggtt_disable_guc(dev_priv);
-
/*
* We've failed to load the firmware :(
*
diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
index 5c0f9a4..c1365e6 100644
--- a/drivers/gpu/drm/i915/intel_guc_log.c
+++ b/drivers/gpu/drm/i915/intel_guc_log.c
@@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
return intel_guc_send(guc, action, ARRAY_SIZE(action));
}
-
/*
* Sub buffer switch callback. Called whenever relay has to switch to a new
* sub buffer, relay stays on the same sub buffer if 0 is returned.
@@ -139,12 +138,7 @@ static int remove_buf_file_callback(struct dentry *dentry)
.remove_buf_file = remove_buf_file_callback,
};
-static void guc_log_remove_relay_file(struct intel_guc *guc)
-{
- relay_close(guc->log.relay_chan);
-}
-
-static int guc_log_create_relay_channel(struct intel_guc *guc)
+static int guc_log_relay_channel_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct rchan *guc_log_relay_chan;
@@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
return 0;
}
-static int guc_log_create_relay_file(struct intel_guc *guc)
+static void guc_log_relay_channel_destroy(struct intel_guc *guc)
+{
+ relay_close(guc->log.relay_chan);
+ guc->log.relay_chan = NULL;
+}
+
+static int guc_log_relay_file_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
struct dentry *log_dir;
int ret;
+ if (i915.guc_log_level < 0)
+ return 0; /* nothing to do */
+
/* For now create the log file in /sys/kernel/debug/dri/0 dir */
log_dir = dev_priv->drm.primary->debugfs_root;
@@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
}
ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
- if (ret) {
+ if (ret == -EEXIST) {
+ /* Ignore "file already exists" */
+ }
+ else if (ret < 0) {
DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
return ret;
}
@@ -371,31 +377,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
}
}
-static void guc_log_cleanup(struct intel_guc *guc)
-{
- struct drm_i915_private *dev_priv = guc_to_i915(guc);
-
- lockdep_assert_held(&dev_priv->drm.struct_mutex);
-
- /* First disable the flush interrupt */
- gen9_disable_guc_interrupts(dev_priv);
-
- if (guc->log.flush_wq)
- destroy_workqueue(guc->log.flush_wq);
-
- guc->log.flush_wq = NULL;
-
- if (guc->log.relay_chan)
- guc_log_remove_relay_file(guc);
-
- guc->log.relay_chan = NULL;
-
- if (guc->log.buf_addr)
- i915_gem_object_unpin_map(guc->log.vma->obj);
-
- guc->log.buf_addr = NULL;
-}
-
static void capture_logs_work(struct work_struct *work)
{
struct intel_guc *guc =
@@ -404,120 +385,84 @@ static void capture_logs_work(struct work_struct *work)
guc_log_capture_logs(guc);
}
-static int guc_log_create_extras(struct intel_guc *guc)
+static int guc_log_extras_create(struct intel_guc *guc)
{
struct drm_i915_private *dev_priv = guc_to_i915(guc);
void *vaddr;
- int ret;
+ int ret = 0;
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- /* Nothing to do */
if (i915.guc_log_level < 0)
- return 0;
+ return 0; /* nothing to do */
- if (!guc->log.buf_addr) {
- /* Create a WC (Uncached for read) vmalloc mapping of log
- * buffer pages, so that we can directly get the data
- * (up-to-date) from memory.
- */
- vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
- if (IS_ERR(vaddr)) {
- ret = PTR_ERR(vaddr);
- DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
- return ret;
- }
+ if (guc->log.buf_addr)
+ return 0; /* already allocated */
- guc->log.buf_addr = vaddr;
+ /* Create a WC (Uncached for read) vmalloc mapping of log
+ * buffer pages, so that we can directly get the data
+ * (up-to-date) from memory.
+ */
+ vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
+ if (IS_ERR(vaddr)) {
+ DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
+ return PTR_ERR(vaddr);
}
- if (!guc->log.relay_chan) {
- /* Create a relay channel, so that we have buffers for storing
- * the GuC firmware logs, the channel will be linked with a file
- * later on when debugfs is registered.
- */
- ret = guc_log_create_relay_channel(guc);
- if (ret)
- return ret;
- }
+ guc->log.buf_addr = vaddr;
- if (!guc->log.flush_wq) {
- INIT_WORK(&guc->log.flush_work, capture_logs_work);
-
- /*
- * GuC log buffer flush work item has to do register access to
- * send the ack to GuC and this work item, if not synced before
- * suspend, can potentially get executed after the GFX device is
- * suspended.
- * By marking the WQ as freezable, we don't have to bother about
- * flushing of this work item from the suspend hooks, the pending
- * work item if any will be either executed before the suspend
- * or scheduled later on resume. This way the handling of work
- * item can be kept same between system suspend & rpm suspend.
- */
- guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
- WQ_HIGHPRI | WQ_FREEZABLE);
- if (guc->log.flush_wq == NULL) {
- DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
- return -ENOMEM;
- }
+ /* Create a relay channel, so that we have buffers for storing
+ * the GuC firmware logs, the channel will be linked with a file
+ * later on when debugfs is registered.
+ */
+ ret = guc_log_relay_channel_create(guc);
+ if (ret < 0)
+ goto err_vaddr;
+
+ INIT_WORK(&guc->log.flush_work, capture_logs_work);
+
+ /*
+ * GuC log buffer flush work item has to do register access to
+ * send the ack to GuC and this work item, if not synced before
+ * suspend, can potentially get executed after the GFX device is
+ * suspended.
+ * By marking the WQ as freezable, we don't have to bother about
+ * flushing of this work item from the suspend hooks, the pending
+ * work item if any will be either executed before the suspend
+ * or scheduled later on resume. This way the handling of work
+ * item can be kept same between system suspend & rpm suspend.
+ */
+ guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
+ WQ_HIGHPRI | WQ_FREEZABLE);
+ if (guc->log.flush_wq == NULL) {
+ DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
+ ret = -ENOMEM;
+ goto err_relaychan;
}
return 0;
+err_relaychan:
+ guc_log_relay_channel_destroy(guc);
+err_vaddr:
+ i915_gem_object_unpin_map(guc->log.vma->obj);
+
+ return ret;
}
-void intel_guc_log_create(struct intel_guc *guc)
+static void guc_log_extras_destroy(struct intel_guc *guc)
{
- struct i915_vma *vma;
- unsigned long offset;
- uint32_t size, flags;
-
- if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
- i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
-
- /* The first page is to save log buffer state. Allocate one
- * extra page for others in case for overlap */
- size = (1 + GUC_LOG_DPC_PAGES + 1 +
- GUC_LOG_ISR_PAGES + 1 +
- GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
-
- vma = guc->log.vma;
- if (!vma) {
- /* We require SSE 4.1 for fast reads from the GuC log buffer and
- * it should be present on the chipsets supporting GuC based
- * submisssions.
- */
- if (WARN_ON(!i915_has_memcpy_from_wc())) {
- /* logging will not be enabled */
- i915.guc_log_level = -1;
- return;
- }
-
- vma = intel_guc_allocate_vma(guc, size);
- if (IS_ERR(vma)) {
- /* logging will be off */
- i915.guc_log_level = -1;
- return;
- }
-
- guc->log.vma = vma;
-
- if (guc_log_create_extras(guc)) {
- guc_log_cleanup(guc);
- i915_vma_unpin_and_release(&guc->log.vma);
- i915.guc_log_level = -1;
- return;
- }
- }
-
- /* each allocated unit is a page */
- flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
- (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
- (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
- (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+ /*
+ * It's possible that extras were never allocated because guc_log_level
+ * was < 0 at the time
+ **/
+ if (!guc->log.buf_addr)
+ return;
- offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
- guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+ destroy_workqueue(guc->log.flush_wq);
+ guc->log.flush_wq = NULL;
+ guc_log_relay_channel_destroy(guc);
+ i915_gem_object_unpin_map(guc->log.vma->obj);
+ guc->log.buf_addr = NULL;
}
static int guc_log_late_setup(struct intel_guc *guc)
@@ -527,26 +472,25 @@ static int guc_log_late_setup(struct intel_guc *guc)
lockdep_assert_held(&dev_priv->drm.struct_mutex);
- if (i915.guc_log_level < 0)
- return -EINVAL;
-
/* If log_level was set as -1 at boot time, then setup needed to
* handle log buffer flush interrupts would not have been done yet,
* so do that now.
*/
- ret = guc_log_create_extras(guc);
+ ret = guc_log_extras_create(guc);
if (ret)
goto err;
- ret = guc_log_create_relay_file(guc);
+ ret = guc_log_relay_file_create(guc);
if (ret)
- goto err;
+ goto err_extras;
return 0;
+err_extras:
+ guc_log_extras_destroy(guc);
err:
- guc_log_cleanup(guc);
/* logging will remain off */
i915.guc_log_level = -1;
+
return ret;
}
@@ -586,6 +530,68 @@ static void guc_flush_logs(struct intel_guc *guc)
guc_log_capture_logs(guc);
}
+int intel_guc_log_create(struct intel_guc *guc)
+{
+ struct i915_vma *vma;
+ unsigned long offset;
+ uint32_t size, flags;
+ int ret;
+
+ if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
+ i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
+
+ /* The first page is to save log buffer state. Allocate one
+ * extra page for others in case for overlap */
+ size = (1 + GUC_LOG_DPC_PAGES + 1 +
+ GUC_LOG_ISR_PAGES + 1 +
+ GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
+
+ /* We require SSE 4.1 for fast reads from the GuC log buffer and
+ * it should be present on the chipsets supporting GuC based
+ * submisssions.
+ */
+ if (WARN_ON(!i915_has_memcpy_from_wc())) {
+ ret = -EINVAL;
+ goto err;
+ }
+
+ vma = intel_guc_allocate_vma(guc, size);
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ goto err;
+ }
+
+ guc->log.vma = vma;
+
+ ret = guc_log_extras_create(guc);
+ if (ret < 0)
+ goto err_vma;
+
+ /* each allocated unit is a page */
+ flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
+ (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
+ (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
+ (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
+
+ offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
+ guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
+
+ return 0;
+err_vma:
+ i915_vma_unpin_and_release(&guc->log.vma);
+err:
+ /* logging will be off */
+ i915.guc_log_level = -1;
+
+ return ret;
+}
+
+void intel_guc_log_destroy(struct intel_guc *guc)
+{
+ guc_log_extras_destroy(guc);
+ i915_vma_unpin_and_release(&guc->log.vma);
+}
+
int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
{
struct intel_guc *guc = &dev_priv->guc;
@@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
return ret;
}
- i915.guc_log_level = log_param.verbosity;
+ if (log_param.logging_enabled)
+ {
+ i915.guc_log_level = log_param.verbosity;
- /* If log_level was set as -1 at boot time, then the relay channel file
- * wouldn't have been created by now and interrupts also would not have
- * been enabled.
- */
- if (!dev_priv->guc.log.relay_chan) {
+ /* If log_level was set as -1 at boot time, then the relay channel file
+ * wouldn't have been created by now and interrupts also would not have
+ * been enabled. Try again now, just in case.
+ */
ret = guc_log_late_setup(guc);
- if (!ret)
- gen9_enable_guc_interrupts(dev_priv);
- } else if (!log_param.logging_enabled) {
+ if (ret < 0) {
+ DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
+ return ret;
+ }
+
+ gen9_enable_guc_interrupts(dev_priv);
+ }
+ else
+ {
/* Once logging is disabled, GuC won't generate logs & send an
* interrupt. But there could be some data in the log buffer
* which is yet to be captured. So request GuC to update the log
@@ -629,9 +642,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
/* As logging is disabled, update log level to reflect that */
i915.guc_log_level = -1;
- } else {
- /* In case interrupts were disabled, enable them now */
- gen9_enable_guc_interrupts(dev_priv);
}
return ret;
@@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
return;
mutex_lock(&dev_priv->drm.struct_mutex);
- guc_log_cleanup(&dev_priv->guc);
+ gen9_disable_guc_interrupts(dev_priv);
+ intel_guc_log_destroy(&dev_priv->guc);
mutex_unlock(&dev_priv->drm.struct_mutex);
}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 511b96b..f34448b 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -217,10 +217,11 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
/* intel_guc_log.c */
-void intel_guc_log_create(struct intel_guc *guc);
+int intel_guc_log_create(struct intel_guc *guc);
+void intel_guc_log_destroy(struct intel_guc *guc);
+int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
void i915_guc_log_register(struct drm_i915_private *dev_priv);
void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
-int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
static inline u32 guc_ggtt_offset(struct i915_vma *vma)
{
--
1.9.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: Add onion teardown to the GuC setup
2017-02-16 14:18 ` [PATCH] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
@ 2017-02-16 14:21 ` Oscar Mateo
2017-02-17 23:06 ` Daniele Ceraolo Spurio
2017-02-20 10:52 ` Joonas Lahtinen
2 siblings, 0 replies; 10+ messages in thread
From: Oscar Mateo @ 2017-02-16 14:21 UTC (permalink / raw)
To: intel-gfx
This goes on top of "Keep the ctx_pool_vaddr mapped, for easy access",
which is in turn goes on top of Joonas' "Sanitize GuC client
initialization". If the reviews go well and once Joonas finishes his
patch, I can resend everything as a series so that merging is easier.
-- Oscar
On 02/16/2017 06:18 AM, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 94 +++++----
> drivers/gpu/drm/i915/intel_guc_loader.c | 19 +-
> drivers/gpu/drm/i915/intel_guc_log.c | 309 +++++++++++++++--------------
> drivers/gpu/drm/i915/intel_uc.h | 5 +-
> 4 files changed, 229 insertions(+), 198 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index f7d9897..4147674 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -609,8 +609,14 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
> return vma;
> }
>
> -static void guc_client_free(struct i915_guc_client *client)
> +static void guc_client_free(struct i915_guc_client **p_client)
> {
> + struct i915_guc_client *client;
> +
> + client = fetch_and_zero(p_client);
> + if (!client)
> + return;
> +
> /*
> * XXX: wait for any outstanding submissions before freeing memory.
> * Be sure to drop any locks
> @@ -818,7 +824,7 @@ static void guc_policies_init(struct guc_policies *policies)
> policies->is_valid = 1;
> }
>
> -static void guc_addon_create(struct intel_guc *guc)
> +static int guc_addon_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct i915_vma *vma;
> @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
> sizeof(struct guc_mmio_reg_state) +
> GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
>
> - vma = guc->ads_vma;
> - if (!vma) {
> - vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> - if (IS_ERR(vma))
> - return;
> + vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
>
> - guc->ads_vma = vma;
> - }
> + guc->ads_vma = vma;
>
> page = i915_vma_first_page(vma);
> ads = kmap(page);
> @@ -885,6 +888,13 @@ static void guc_addon_create(struct intel_guc *guc)
> sizeof(struct guc_mmio_reg_state);
>
> kunmap(page);
> +
> + return 0;
> +}
> +
> +static void guc_addon_destroy(struct intel_guc *guc)
> +{
> + i915_vma_unpin_and_release(&guc->ads_vma);
> }
>
> /*
> @@ -899,6 +909,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> struct intel_guc *guc = &dev_priv->guc;
> struct i915_vma *vma;
> void *vaddr;
> + int ret;
>
> if (!HAS_GUC_SCHED(dev_priv))
> return 0;
> @@ -919,15 +930,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>
> guc->ctx_pool = vma;
>
> - vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> - if (IS_ERR(vaddr))
> - goto err;
> + vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + goto err_vma;
> + }
>
> guc->ctx_pool_vaddr = vaddr;
>
> + ret = intel_guc_log_create(guc);
> + if (ret < 0)
> + goto err_vaddr;
> +
> + ret = guc_addon_create(guc);
> + if (ret < 0)
> + goto err_log;
> +
> ida_init(&guc->ctx_ids);
> - intel_guc_log_create(guc);
> - guc_addon_create(guc);
>
> guc->execbuf_client = guc_client_alloc(dev_priv,
> INTEL_INFO(dev_priv)->ring_mask,
> @@ -935,14 +954,33 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> dev_priv->kernel_context);
> if (IS_ERR(guc->execbuf_client)) {
> DRM_ERROR("Failed to create GuC client for execbuf!\n");
> - goto err;
> + ret = PTR_ERR(guc->execbuf_client);
> + goto err_ads;
> }
>
> return 0;
> +err_ads:
> + guc_addon_destroy(guc);
> +err_log:
> + intel_guc_log_destroy(guc);
> +err_vaddr:
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> +err_vma:
> + i915_vma_unpin_and_release(&guc->ctx_pool);
>
> -err:
> - i915_guc_submission_fini(dev_priv);
> - return -ENOMEM;
> + return ret;
> +}
> +
> +void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> +{
> + struct intel_guc *guc = &dev_priv->guc;
> +
> + guc_client_free(&guc->execbuf_client);
> + ida_destroy(&guc->ctx_ids);
> + guc_addon_destroy(guc);
> + intel_guc_log_destroy(guc);
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> + i915_vma_unpin_and_release(&guc->ctx_pool);
> }
>
> static void guc_reset_wq(struct i915_guc_client *client)
> @@ -1004,26 +1042,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
> intel_execlists_enable_submission(dev_priv);
> }
>
> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> -{
> - struct intel_guc *guc = &dev_priv->guc;
> - struct i915_guc_client *client;
> -
> - client = fetch_and_zero(&guc->execbuf_client);
> - if (client && !IS_ERR(client))
> - guc_client_free(client);
> -
> - i915_vma_unpin_and_release(&guc->ads_vma);
> - i915_vma_unpin_and_release(&guc->log.vma);
> -
> - if (guc->ctx_pool_vaddr) {
> - ida_destroy(&guc->ctx_ids);
> - i915_gem_object_unpin_map(guc->ctx_pool->obj);
> - }
> -
> - i915_vma_unpin_and_release(&guc->ctx_pool);
> -}
> -
> /**
> * intel_guc_suspend() - notify GuC entering suspend state
> * @dev_priv: i915 device private
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 22f882d..0b48ad8 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -489,7 +489,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>
> err = i915_guc_submission_init(dev_priv);
> if (err)
> - goto fail;
> + goto err_guc;
>
> /*
> * WaEnableuKernelHeaderValidFix:skl,bxt
> @@ -504,7 +504,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> */
> err = guc_hw_reset(dev_priv);
> if (err)
> - goto fail;
> + goto err_submission;
>
> intel_huc_load(dev_priv);
> err = guc_ucode_xfer(dev_priv);
> @@ -512,7 +512,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> break;
>
> if (--retries == 0)
> - goto fail;
> + goto err_submission;
>
> DRM_INFO("GuC fw load failed: %d; will reset and "
> "retry %d more time(s)\n", err, retries);
> @@ -528,7 +528,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>
> err = i915_guc_submission_enable(dev_priv);
> if (err)
> - goto fail;
> + goto err_interrupts;
> guc_interrupts_capture(dev_priv);
> }
>
> @@ -539,15 +539,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>
> return 0;
>
> +err_interrupts:
> + gen9_disable_guc_interrupts(dev_priv);
> +err_submission:
> + i915_guc_submission_fini(dev_priv);
> +err_guc:
> + i915_ggtt_disable_guc(dev_priv);
> fail:
> if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
> guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
>
> - guc_interrupts_release(dev_priv);
> - i915_guc_submission_disable(dev_priv);
> - i915_guc_submission_fini(dev_priv);
> - i915_ggtt_disable_guc(dev_priv);
> -
> /*
> * We've failed to load the firmware :(
> *
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 5c0f9a4..c1365e6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
> return intel_guc_send(guc, action, ARRAY_SIZE(action));
> }
>
> -
> /*
> * Sub buffer switch callback. Called whenever relay has to switch to a new
> * sub buffer, relay stays on the same sub buffer if 0 is returned.
> @@ -139,12 +138,7 @@ static int remove_buf_file_callback(struct dentry *dentry)
> .remove_buf_file = remove_buf_file_callback,
> };
>
> -static void guc_log_remove_relay_file(struct intel_guc *guc)
> -{
> - relay_close(guc->log.relay_chan);
> -}
> -
> -static int guc_log_create_relay_channel(struct intel_guc *guc)
> +static int guc_log_relay_channel_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct rchan *guc_log_relay_chan;
> @@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
> return 0;
> }
>
> -static int guc_log_create_relay_file(struct intel_guc *guc)
> +static void guc_log_relay_channel_destroy(struct intel_guc *guc)
> +{
> + relay_close(guc->log.relay_chan);
> + guc->log.relay_chan = NULL;
> +}
> +
> +static int guc_log_relay_file_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct dentry *log_dir;
> int ret;
>
> + if (i915.guc_log_level < 0)
> + return 0; /* nothing to do */
> +
> /* For now create the log file in /sys/kernel/debug/dri/0 dir */
> log_dir = dev_priv->drm.primary->debugfs_root;
>
> @@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
> }
>
> ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
> - if (ret) {
> + if (ret == -EEXIST) {
> + /* Ignore "file already exists" */
> + }
> + else if (ret < 0) {
> DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> return ret;
> }
> @@ -371,31 +377,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> }
> }
>
> -static void guc_log_cleanup(struct intel_guc *guc)
> -{
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> - lockdep_assert_held(&dev_priv->drm.struct_mutex);
> -
> - /* First disable the flush interrupt */
> - gen9_disable_guc_interrupts(dev_priv);
> -
> - if (guc->log.flush_wq)
> - destroy_workqueue(guc->log.flush_wq);
> -
> - guc->log.flush_wq = NULL;
> -
> - if (guc->log.relay_chan)
> - guc_log_remove_relay_file(guc);
> -
> - guc->log.relay_chan = NULL;
> -
> - if (guc->log.buf_addr)
> - i915_gem_object_unpin_map(guc->log.vma->obj);
> -
> - guc->log.buf_addr = NULL;
> -}
> -
> static void capture_logs_work(struct work_struct *work)
> {
> struct intel_guc *guc =
> @@ -404,120 +385,84 @@ static void capture_logs_work(struct work_struct *work)
> guc_log_capture_logs(guc);
> }
>
> -static int guc_log_create_extras(struct intel_guc *guc)
> +static int guc_log_extras_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> void *vaddr;
> - int ret;
> + int ret = 0;
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> - /* Nothing to do */
> if (i915.guc_log_level < 0)
> - return 0;
> + return 0; /* nothing to do */
>
> - if (!guc->log.buf_addr) {
> - /* Create a WC (Uncached for read) vmalloc mapping of log
> - * buffer pages, so that we can directly get the data
> - * (up-to-date) from memory.
> - */
> - vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> - return ret;
> - }
> + if (guc->log.buf_addr)
> + return 0; /* already allocated */
>
> - guc->log.buf_addr = vaddr;
> + /* Create a WC (Uncached for read) vmalloc mapping of log
> + * buffer pages, so that we can directly get the data
> + * (up-to-date) from memory.
> + */
> + vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> + if (IS_ERR(vaddr)) {
> + DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> + return PTR_ERR(vaddr);
> }
>
> - if (!guc->log.relay_chan) {
> - /* Create a relay channel, so that we have buffers for storing
> - * the GuC firmware logs, the channel will be linked with a file
> - * later on when debugfs is registered.
> - */
> - ret = guc_log_create_relay_channel(guc);
> - if (ret)
> - return ret;
> - }
> + guc->log.buf_addr = vaddr;
>
> - if (!guc->log.flush_wq) {
> - INIT_WORK(&guc->log.flush_work, capture_logs_work);
> -
> - /*
> - * GuC log buffer flush work item has to do register access to
> - * send the ack to GuC and this work item, if not synced before
> - * suspend, can potentially get executed after the GFX device is
> - * suspended.
> - * By marking the WQ as freezable, we don't have to bother about
> - * flushing of this work item from the suspend hooks, the pending
> - * work item if any will be either executed before the suspend
> - * or scheduled later on resume. This way the handling of work
> - * item can be kept same between system suspend & rpm suspend.
> - */
> - guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> - WQ_HIGHPRI | WQ_FREEZABLE);
> - if (guc->log.flush_wq == NULL) {
> - DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> - return -ENOMEM;
> - }
> + /* Create a relay channel, so that we have buffers for storing
> + * the GuC firmware logs, the channel will be linked with a file
> + * later on when debugfs is registered.
> + */
> + ret = guc_log_relay_channel_create(guc);
> + if (ret < 0)
> + goto err_vaddr;
> +
> + INIT_WORK(&guc->log.flush_work, capture_logs_work);
> +
> + /*
> + * GuC log buffer flush work item has to do register access to
> + * send the ack to GuC and this work item, if not synced before
> + * suspend, can potentially get executed after the GFX device is
> + * suspended.
> + * By marking the WQ as freezable, we don't have to bother about
> + * flushing of this work item from the suspend hooks, the pending
> + * work item if any will be either executed before the suspend
> + * or scheduled later on resume. This way the handling of work
> + * item can be kept same between system suspend & rpm suspend.
> + */
> + guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> + WQ_HIGHPRI | WQ_FREEZABLE);
> + if (guc->log.flush_wq == NULL) {
> + DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> + ret = -ENOMEM;
> + goto err_relaychan;
> }
>
> return 0;
> +err_relaychan:
> + guc_log_relay_channel_destroy(guc);
> +err_vaddr:
> + i915_gem_object_unpin_map(guc->log.vma->obj);
> +
> + return ret;
> }
>
> -void intel_guc_log_create(struct intel_guc *guc)
> +static void guc_log_extras_destroy(struct intel_guc *guc)
> {
> - struct i915_vma *vma;
> - unsigned long offset;
> - uint32_t size, flags;
> -
> - if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
> - i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
> -
> - /* The first page is to save log buffer state. Allocate one
> - * extra page for others in case for overlap */
> - size = (1 + GUC_LOG_DPC_PAGES + 1 +
> - GUC_LOG_ISR_PAGES + 1 +
> - GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> -
> - vma = guc->log.vma;
> - if (!vma) {
> - /* We require SSE 4.1 for fast reads from the GuC log buffer and
> - * it should be present on the chipsets supporting GuC based
> - * submisssions.
> - */
> - if (WARN_ON(!i915_has_memcpy_from_wc())) {
> - /* logging will not be enabled */
> - i915.guc_log_level = -1;
> - return;
> - }
> -
> - vma = intel_guc_allocate_vma(guc, size);
> - if (IS_ERR(vma)) {
> - /* logging will be off */
> - i915.guc_log_level = -1;
> - return;
> - }
> -
> - guc->log.vma = vma;
> -
> - if (guc_log_create_extras(guc)) {
> - guc_log_cleanup(guc);
> - i915_vma_unpin_and_release(&guc->log.vma);
> - i915.guc_log_level = -1;
> - return;
> - }
> - }
> -
> - /* each allocated unit is a page */
> - flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> - (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> - (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> - (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> + /*
> + * It's possible that extras were never allocated because guc_log_level
> + * was < 0 at the time
> + **/
> + if (!guc->log.buf_addr)
> + return;
>
> - offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> - guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> + destroy_workqueue(guc->log.flush_wq);
> + guc->log.flush_wq = NULL;
> + guc_log_relay_channel_destroy(guc);
> + i915_gem_object_unpin_map(guc->log.vma->obj);
> + guc->log.buf_addr = NULL;
> }
>
> static int guc_log_late_setup(struct intel_guc *guc)
> @@ -527,26 +472,25 @@ static int guc_log_late_setup(struct intel_guc *guc)
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> - if (i915.guc_log_level < 0)
> - return -EINVAL;
> -
> /* If log_level was set as -1 at boot time, then setup needed to
> * handle log buffer flush interrupts would not have been done yet,
> * so do that now.
> */
> - ret = guc_log_create_extras(guc);
> + ret = guc_log_extras_create(guc);
> if (ret)
> goto err;
>
> - ret = guc_log_create_relay_file(guc);
> + ret = guc_log_relay_file_create(guc);
> if (ret)
> - goto err;
> + goto err_extras;
>
> return 0;
> +err_extras:
> + guc_log_extras_destroy(guc);
> err:
> - guc_log_cleanup(guc);
> /* logging will remain off */
> i915.guc_log_level = -1;
> +
> return ret;
> }
>
> @@ -586,6 +530,68 @@ static void guc_flush_logs(struct intel_guc *guc)
> guc_log_capture_logs(guc);
> }
>
> +int intel_guc_log_create(struct intel_guc *guc)
> +{
> + struct i915_vma *vma;
> + unsigned long offset;
> + uint32_t size, flags;
> + int ret;
> +
> + if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
> + i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
> +
> + /* The first page is to save log buffer state. Allocate one
> + * extra page for others in case for overlap */
> + size = (1 + GUC_LOG_DPC_PAGES + 1 +
> + GUC_LOG_ISR_PAGES + 1 +
> + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> +
> + /* We require SSE 4.1 for fast reads from the GuC log buffer and
> + * it should be present on the chipsets supporting GuC based
> + * submisssions.
> + */
> + if (WARN_ON(!i915_has_memcpy_from_wc())) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + vma = intel_guc_allocate_vma(guc, size);
> + if (IS_ERR(vma)) {
> + ret = PTR_ERR(vma);
> + goto err;
> + }
> +
> + guc->log.vma = vma;
> +
> + ret = guc_log_extras_create(guc);
> + if (ret < 0)
> + goto err_vma;
> +
> + /* each allocated unit is a page */
> + flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> + (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> + (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> + (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> +
> + offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> + guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> +
> + return 0;
> +err_vma:
> + i915_vma_unpin_and_release(&guc->log.vma);
> +err:
> + /* logging will be off */
> + i915.guc_log_level = -1;
> +
> + return ret;
> +}
> +
> +void intel_guc_log_destroy(struct intel_guc *guc)
> +{
> + guc_log_extras_destroy(guc);
> + i915_vma_unpin_and_release(&guc->log.vma);
> +}
> +
> int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> {
> struct intel_guc *guc = &dev_priv->guc;
> @@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> return ret;
> }
>
> - i915.guc_log_level = log_param.verbosity;
> + if (log_param.logging_enabled)
> + {
> + i915.guc_log_level = log_param.verbosity;
>
> - /* If log_level was set as -1 at boot time, then the relay channel file
> - * wouldn't have been created by now and interrupts also would not have
> - * been enabled.
> - */
> - if (!dev_priv->guc.log.relay_chan) {
> + /* If log_level was set as -1 at boot time, then the relay channel file
> + * wouldn't have been created by now and interrupts also would not have
> + * been enabled. Try again now, just in case.
> + */
> ret = guc_log_late_setup(guc);
> - if (!ret)
> - gen9_enable_guc_interrupts(dev_priv);
> - } else if (!log_param.logging_enabled) {
> + if (ret < 0) {
> + DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> + return ret;
> + }
> +
> + gen9_enable_guc_interrupts(dev_priv);
> + }
> + else
> + {
> /* Once logging is disabled, GuC won't generate logs & send an
> * interrupt. But there could be some data in the log buffer
> * which is yet to be captured. So request GuC to update the log
> @@ -629,9 +642,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>
> /* As logging is disabled, update log level to reflect that */
> i915.guc_log_level = -1;
> - } else {
> - /* In case interrupts were disabled, enable them now */
> - gen9_enable_guc_interrupts(dev_priv);
> }
>
> return ret;
> @@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> - guc_log_cleanup(&dev_priv->guc);
> + gen9_disable_guc_interrupts(dev_priv);
> + intel_guc_log_destroy(&dev_priv->guc);
> mutex_unlock(&dev_priv->drm.struct_mutex);
> }
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 511b96b..f34448b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -217,10 +217,11 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>
> /* intel_guc_log.c */
> -void intel_guc_log_create(struct intel_guc *guc);
> +int intel_guc_log_create(struct intel_guc *guc);
> +void intel_guc_log_destroy(struct intel_guc *guc);
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
> void i915_guc_log_register(struct drm_i915_private *dev_priv);
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>
> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> {
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: Add onion teardown to the GuC setup
2017-02-16 14:18 ` [PATCH] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
2017-02-16 14:21 ` Oscar Mateo
@ 2017-02-17 23:06 ` Daniele Ceraolo Spurio
2017-02-20 10:39 ` Joonas Lahtinen
2017-02-20 10:52 ` Joonas Lahtinen
2 siblings, 1 reply; 10+ messages in thread
From: Daniele Ceraolo Spurio @ 2017-02-17 23:06 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
On 16/02/17 06:18, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
> ---
> drivers/gpu/drm/i915/i915_guc_submission.c | 94 +++++----
> drivers/gpu/drm/i915/intel_guc_loader.c | 19 +-
> drivers/gpu/drm/i915/intel_guc_log.c | 309 +++++++++++++++--------------
> drivers/gpu/drm/i915/intel_uc.h | 5 +-
> 4 files changed, 229 insertions(+), 198 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index f7d9897..4147674 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -609,8 +609,14 @@ struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size)
> return vma;
> }
>
> -static void guc_client_free(struct i915_guc_client *client)
> +static void guc_client_free(struct i915_guc_client **p_client)
> {
> + struct i915_guc_client *client;
> +
> + client = fetch_and_zero(p_client);
> + if (!client)
> + return;
> +
This works now, but it might become risky if we have multiple clients in
the future because we might do something like the following:
for_each_guc_client(client, guc->client_list, ...)
guc_client_free(&client);
> /*
> * XXX: wait for any outstanding submissions before freeing memory.
> * Be sure to drop any locks
> @@ -818,7 +824,7 @@ static void guc_policies_init(struct guc_policies *policies)
> policies->is_valid = 1;
> }
>
> -static void guc_addon_create(struct intel_guc *guc)
> +static int guc_addon_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct i915_vma *vma;
> @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
> sizeof(struct guc_mmio_reg_state) +
> GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
>
> - vma = guc->ads_vma;
> - if (!vma) {
I believe the if was here to avoid re-allocating the vma if this
function was called after a GPU reset. I agree that the check should be
outside this function (and it already is), but we might want to still
add here something like:
if (WARN_ON(guc->ads_vma))
return 0;
> - vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> - if (IS_ERR(vma))
> - return;
> + vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
>
> - guc->ads_vma = vma;
> - }
> + guc->ads_vma = vma;
>
> page = i915_vma_first_page(vma);
> ads = kmap(page);
> @@ -885,6 +888,13 @@ static void guc_addon_create(struct intel_guc *guc)
> sizeof(struct guc_mmio_reg_state);
>
> kunmap(page);
> +
> + return 0;
> +}
> +
> +static void guc_addon_destroy(struct intel_guc *guc)
> +{
> + i915_vma_unpin_and_release(&guc->ads_vma);
> }
>
> /*
> @@ -899,6 +909,7 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> struct intel_guc *guc = &dev_priv->guc;
> struct i915_vma *vma;
> void *vaddr;
> + int ret;
>
> if (!HAS_GUC_SCHED(dev_priv))
> return 0;
> @@ -919,15 +930,23 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
>
> guc->ctx_pool = vma;
>
> - vaddr = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
> - if (IS_ERR(vaddr))
> - goto err;
> + vaddr = i915_gem_object_pin_map(guc->ctx_pool->obj, I915_MAP_WB);
> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + goto err_vma;
> + }
>
> guc->ctx_pool_vaddr = vaddr;
>
> + ret = intel_guc_log_create(guc);
> + if (ret < 0)
> + goto err_vaddr;
> +
> + ret = guc_addon_create(guc);
> + if (ret < 0)
> + goto err_log;
> +
> ida_init(&guc->ctx_ids);
> - intel_guc_log_create(guc);
> - guc_addon_create(guc);
>
> guc->execbuf_client = guc_client_alloc(dev_priv,
> INTEL_INFO(dev_priv)->ring_mask,
> @@ -935,14 +954,33 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> dev_priv->kernel_context);
> if (IS_ERR(guc->execbuf_client)) {
> DRM_ERROR("Failed to create GuC client for execbuf!\n");
> - goto err;
> + ret = PTR_ERR(guc->execbuf_client);
> + goto err_ads;
> }
>
> return 0;
> +err_ads:
> + guc_addon_destroy(guc);
> +err_log:
> + intel_guc_log_destroy(guc);
> +err_vaddr:
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> +err_vma:
> + i915_vma_unpin_and_release(&guc->ctx_pool);
>
> -err:
> - i915_guc_submission_fini(dev_priv);
> - return -ENOMEM;
> + return ret;
> +}
> +
> +void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> +{
> + struct intel_guc *guc = &dev_priv->guc;
> +
if I'm not missing anything, this function is called from
intel_guc_fini(), which can in turn be called from the onion unwinding
of i915_load_modeset_init() before intel_guc_init() is ever called, so
we need to either fix that unwinding or add some kind of guard here.
> + guc_client_free(&guc->execbuf_client);
> + ida_destroy(&guc->ctx_ids);
> + guc_addon_destroy(guc);
> + intel_guc_log_destroy(guc);
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> + i915_vma_unpin_and_release(&guc->ctx_pool);
> }
>
> static void guc_reset_wq(struct i915_guc_client *client)
> @@ -1004,26 +1042,6 @@ void i915_guc_submission_disable(struct drm_i915_private *dev_priv)
> intel_execlists_enable_submission(dev_priv);
> }
>
> -void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> -{
> - struct intel_guc *guc = &dev_priv->guc;
> - struct i915_guc_client *client;
> -
> - client = fetch_and_zero(&guc->execbuf_client);
> - if (client && !IS_ERR(client))
> - guc_client_free(client);
> -
> - i915_vma_unpin_and_release(&guc->ads_vma);
> - i915_vma_unpin_and_release(&guc->log.vma);
> -
> - if (guc->ctx_pool_vaddr) {
> - ida_destroy(&guc->ctx_ids);
> - i915_gem_object_unpin_map(guc->ctx_pool->obj);
> - }
> -
> - i915_vma_unpin_and_release(&guc->ctx_pool);
> -}
> -
> /**
> * intel_guc_suspend() - notify GuC entering suspend state
> * @dev_priv: i915 device private
> diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c b/drivers/gpu/drm/i915/intel_guc_loader.c
> index 22f882d..0b48ad8 100644
> --- a/drivers/gpu/drm/i915/intel_guc_loader.c
> +++ b/drivers/gpu/drm/i915/intel_guc_loader.c
> @@ -489,7 +489,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>
> err = i915_guc_submission_init(dev_priv);
> if (err)
> - goto fail;
> + goto err_guc;
>
> /*
> * WaEnableuKernelHeaderValidFix:skl,bxt
> @@ -504,7 +504,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> */
> err = guc_hw_reset(dev_priv);
> if (err)
> - goto fail;
> + goto err_submission;
>
> intel_huc_load(dev_priv);
> err = guc_ucode_xfer(dev_priv);
> @@ -512,7 +512,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
> break;
>
> if (--retries == 0)
> - goto fail;
> + goto err_submission;
>
> DRM_INFO("GuC fw load failed: %d; will reset and "
> "retry %d more time(s)\n", err, retries);
> @@ -528,7 +528,7 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>
> err = i915_guc_submission_enable(dev_priv);
> if (err)
> - goto fail;
> + goto err_interrupts;
> guc_interrupts_capture(dev_priv);
> }
>
> @@ -539,15 +539,16 @@ int intel_guc_setup(struct drm_i915_private *dev_priv)
>
> return 0;
>
> +err_interrupts:
> + gen9_disable_guc_interrupts(dev_priv);
> +err_submission:
> + i915_guc_submission_fini(dev_priv);
> +err_guc:
> + i915_ggtt_disable_guc(dev_priv);
> fail:
> if (guc_fw->load_status == INTEL_UC_FIRMWARE_PENDING)
> guc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
>
> - guc_interrupts_release(dev_priv);
> - i915_guc_submission_disable(dev_priv);
> - i915_guc_submission_fini(dev_priv);
> - i915_ggtt_disable_guc(dev_priv);
> -
> /*
> * We've failed to load the firmware :(
> *
> diff --git a/drivers/gpu/drm/i915/intel_guc_log.c b/drivers/gpu/drm/i915/intel_guc_log.c
> index 5c0f9a4..c1365e6 100644
> --- a/drivers/gpu/drm/i915/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/intel_guc_log.c
> @@ -66,7 +66,6 @@ static int guc_log_control(struct intel_guc *guc, u32 control_val)
> return intel_guc_send(guc, action, ARRAY_SIZE(action));
> }
>
> -
> /*
> * Sub buffer switch callback. Called whenever relay has to switch to a new
> * sub buffer, relay stays on the same sub buffer if 0 is returned.
> @@ -139,12 +138,7 @@ static int remove_buf_file_callback(struct dentry *dentry)
> .remove_buf_file = remove_buf_file_callback,
> };
>
> -static void guc_log_remove_relay_file(struct intel_guc *guc)
> -{
> - relay_close(guc->log.relay_chan);
> -}
> -
> -static int guc_log_create_relay_channel(struct intel_guc *guc)
> +static int guc_log_relay_channel_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct rchan *guc_log_relay_chan;
> @@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
> return 0;
> }
>
> -static int guc_log_create_relay_file(struct intel_guc *guc)
> +static void guc_log_relay_channel_destroy(struct intel_guc *guc)
> +{
> + relay_close(guc->log.relay_chan);
> + guc->log.relay_chan = NULL;
> +}
> +
> +static int guc_log_relay_file_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct dentry *log_dir;
> int ret;
>
> + if (i915.guc_log_level < 0)
> + return 0; /* nothing to do */
> +
> /* For now create the log file in /sys/kernel/debug/dri/0 dir */
> log_dir = dev_priv->drm.primary->debugfs_root;
>
> @@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
> }
>
> ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
> - if (ret) {
> + if (ret == -EEXIST) {
> + /* Ignore "file already exists" */
> + }
> + else if (ret < 0) {
> DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> return ret;
> }
> @@ -371,31 +377,6 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> }
> }
>
> -static void guc_log_cleanup(struct intel_guc *guc)
> -{
> - struct drm_i915_private *dev_priv = guc_to_i915(guc);
> -
> - lockdep_assert_held(&dev_priv->drm.struct_mutex);
> -
> - /* First disable the flush interrupt */
> - gen9_disable_guc_interrupts(dev_priv);
> -
> - if (guc->log.flush_wq)
> - destroy_workqueue(guc->log.flush_wq);
> -
> - guc->log.flush_wq = NULL;
> -
> - if (guc->log.relay_chan)
> - guc_log_remove_relay_file(guc);
> -
> - guc->log.relay_chan = NULL;
> -
> - if (guc->log.buf_addr)
> - i915_gem_object_unpin_map(guc->log.vma->obj);
> -
> - guc->log.buf_addr = NULL;
> -}
> -
> static void capture_logs_work(struct work_struct *work)
> {
> struct intel_guc *guc =
> @@ -404,120 +385,84 @@ static void capture_logs_work(struct work_struct *work)
> guc_log_capture_logs(guc);
> }
>
> -static int guc_log_create_extras(struct intel_guc *guc)
> +static int guc_log_extras_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> void *vaddr;
> - int ret;
> + int ret = 0;
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> - /* Nothing to do */
> if (i915.guc_log_level < 0)
> - return 0;
> + return 0; /* nothing to do */
>
> - if (!guc->log.buf_addr) {
> - /* Create a WC (Uncached for read) vmalloc mapping of log
> - * buffer pages, so that we can directly get the data
> - * (up-to-date) from memory.
> - */
> - vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> - return ret;
> - }
> + if (guc->log.buf_addr)
> + return 0; /* already allocated */
>
> - guc->log.buf_addr = vaddr;
> + /* Create a WC (Uncached for read) vmalloc mapping of log
> + * buffer pages, so that we can directly get the data
> + * (up-to-date) from memory.
> + */
> + vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> + if (IS_ERR(vaddr)) {
> + DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> + return PTR_ERR(vaddr);
> }
>
> - if (!guc->log.relay_chan) {
> - /* Create a relay channel, so that we have buffers for storing
> - * the GuC firmware logs, the channel will be linked with a file
> - * later on when debugfs is registered.
> - */
> - ret = guc_log_create_relay_channel(guc);
> - if (ret)
> - return ret;
> - }
> + guc->log.buf_addr = vaddr;
>
> - if (!guc->log.flush_wq) {
> - INIT_WORK(&guc->log.flush_work, capture_logs_work);
> -
> - /*
> - * GuC log buffer flush work item has to do register access to
> - * send the ack to GuC and this work item, if not synced before
> - * suspend, can potentially get executed after the GFX device is
> - * suspended.
> - * By marking the WQ as freezable, we don't have to bother about
> - * flushing of this work item from the suspend hooks, the pending
> - * work item if any will be either executed before the suspend
> - * or scheduled later on resume. This way the handling of work
> - * item can be kept same between system suspend & rpm suspend.
> - */
> - guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> - WQ_HIGHPRI | WQ_FREEZABLE);
> - if (guc->log.flush_wq == NULL) {
> - DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> - return -ENOMEM;
> - }
> + /* Create a relay channel, so that we have buffers for storing
> + * the GuC firmware logs, the channel will be linked with a file
> + * later on when debugfs is registered.
> + */
> + ret = guc_log_relay_channel_create(guc);
> + if (ret < 0)
> + goto err_vaddr;
> +
> + INIT_WORK(&guc->log.flush_work, capture_logs_work);
> +
> + /*
> + * GuC log buffer flush work item has to do register access to
> + * send the ack to GuC and this work item, if not synced before
> + * suspend, can potentially get executed after the GFX device is
> + * suspended.
> + * By marking the WQ as freezable, we don't have to bother about
> + * flushing of this work item from the suspend hooks, the pending
> + * work item if any will be either executed before the suspend
> + * or scheduled later on resume. This way the handling of work
> + * item can be kept same between system suspend & rpm suspend.
> + */
> + guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> + WQ_HIGHPRI | WQ_FREEZABLE);
> + if (guc->log.flush_wq == NULL) {
> + DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> + ret = -ENOMEM;
> + goto err_relaychan;
> }
>
> return 0;
> +err_relaychan:
> + guc_log_relay_channel_destroy(guc);
> +err_vaddr:
> + i915_gem_object_unpin_map(guc->log.vma->obj);
> +
guc->log.buf_addr = NULL here would be good because you use it to check
if the extras have been created.
> + return ret;
> }
>
> -void intel_guc_log_create(struct intel_guc *guc)
> +static void guc_log_extras_destroy(struct intel_guc *guc)
> {
> - struct i915_vma *vma;
> - unsigned long offset;
> - uint32_t size, flags;
> -
> - if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
> - i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
> -
> - /* The first page is to save log buffer state. Allocate one
> - * extra page for others in case for overlap */
> - size = (1 + GUC_LOG_DPC_PAGES + 1 +
> - GUC_LOG_ISR_PAGES + 1 +
> - GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> -
> - vma = guc->log.vma;
> - if (!vma) {
> - /* We require SSE 4.1 for fast reads from the GuC log buffer and
> - * it should be present on the chipsets supporting GuC based
> - * submisssions.
> - */
> - if (WARN_ON(!i915_has_memcpy_from_wc())) {
> - /* logging will not be enabled */
> - i915.guc_log_level = -1;
> - return;
> - }
> -
> - vma = intel_guc_allocate_vma(guc, size);
> - if (IS_ERR(vma)) {
> - /* logging will be off */
> - i915.guc_log_level = -1;
> - return;
> - }
> -
> - guc->log.vma = vma;
> -
> - if (guc_log_create_extras(guc)) {
> - guc_log_cleanup(guc);
> - i915_vma_unpin_and_release(&guc->log.vma);
> - i915.guc_log_level = -1;
> - return;
> - }
> - }
> -
> - /* each allocated unit is a page */
> - flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> - (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> - (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> - (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> + /*
> + * It's possible that extras were never allocated because guc_log_level
> + * was < 0 at the time
> + **/
> + if (!guc->log.buf_addr)
> + return;
>
> - offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> - guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> + destroy_workqueue(guc->log.flush_wq);
> + guc->log.flush_wq = NULL;
> + guc_log_relay_channel_destroy(guc);
> + i915_gem_object_unpin_map(guc->log.vma->obj);
> + guc->log.buf_addr = NULL;
> }
>
> static int guc_log_late_setup(struct intel_guc *guc)
> @@ -527,26 +472,25 @@ static int guc_log_late_setup(struct intel_guc *guc)
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> - if (i915.guc_log_level < 0)
> - return -EINVAL;
> -
> /* If log_level was set as -1 at boot time, then setup needed to
> * handle log buffer flush interrupts would not have been done yet,
> * so do that now.
> */
> - ret = guc_log_create_extras(guc);
> + ret = guc_log_extras_create(guc);
> if (ret)
> goto err;
>
> - ret = guc_log_create_relay_file(guc);
> + ret = guc_log_relay_file_create(guc);
> if (ret)
> - goto err;
> + goto err_extras;
>
> return 0;
> +err_extras:
> + guc_log_extras_destroy(guc);
> err:
> - guc_log_cleanup(guc);
> /* logging will remain off */
> i915.guc_log_level = -1;
> +
> return ret;
> }
>
> @@ -586,6 +530,68 @@ static void guc_flush_logs(struct intel_guc *guc)
> guc_log_capture_logs(guc);
> }
>
> +int intel_guc_log_create(struct intel_guc *guc)
> +{
> + struct i915_vma *vma;
> + unsigned long offset;
> + uint32_t size, flags;
> + int ret;
> +
If we expect this to not be called if the log already exists, for safety
we could add something like:
if (WARN_ON(guc->log.vma))
return 0;
To make sure we don't mess this up if we keep the object alive across
resets.
> + if (i915.guc_log_level > GUC_LOG_VERBOSITY_MAX)
> + i915.guc_log_level = GUC_LOG_VERBOSITY_MAX;
> +
> + /* The first page is to save log buffer state. Allocate one
> + * extra page for others in case for overlap */
> + size = (1 + GUC_LOG_DPC_PAGES + 1 +
> + GUC_LOG_ISR_PAGES + 1 +
> + GUC_LOG_CRASH_PAGES + 1) << PAGE_SHIFT;
> +
> + /* We require SSE 4.1 for fast reads from the GuC log buffer and
> + * it should be present on the chipsets supporting GuC based
> + * submisssions.
> + */
> + if (WARN_ON(!i915_has_memcpy_from_wc())) {
> + ret = -EINVAL;
> + goto err;
> + }
> +
> + vma = intel_guc_allocate_vma(guc, size);
> + if (IS_ERR(vma)) {
> + ret = PTR_ERR(vma);
> + goto err;
> + }
> +
> + guc->log.vma = vma;
> +
> + ret = guc_log_extras_create(guc);
> + if (ret < 0)
> + goto err_vma;
> +
> + /* each allocated unit is a page */
> + flags = GUC_LOG_VALID | GUC_LOG_NOTIFY_ON_HALF_FULL |
> + (GUC_LOG_DPC_PAGES << GUC_LOG_DPC_SHIFT) |
> + (GUC_LOG_ISR_PAGES << GUC_LOG_ISR_SHIFT) |
> + (GUC_LOG_CRASH_PAGES << GUC_LOG_CRASH_SHIFT);
> +
> + offset = guc_ggtt_offset(vma) >> PAGE_SHIFT; /* in pages */
> + guc->log.flags = (offset << GUC_LOG_BUF_ADDR_SHIFT) | flags;
> +
> + return 0;
> +err_vma:
> + i915_vma_unpin_and_release(&guc->log.vma);
> +err:
> + /* logging will be off */
> + i915.guc_log_level = -1;
> +
> + return ret;
> +}
> +
> +void intel_guc_log_destroy(struct intel_guc *guc)
> +{
> + guc_log_extras_destroy(guc);
> + i915_vma_unpin_and_release(&guc->log.vma);
> +}
> +
> int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> {
> struct intel_guc *guc = &dev_priv->guc;
> @@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> return ret;
> }
>
> - i915.guc_log_level = log_param.verbosity;
> + if (log_param.logging_enabled)
> + {
> + i915.guc_log_level = log_param.verbosity;
>
> - /* If log_level was set as -1 at boot time, then the relay channel file
> - * wouldn't have been created by now and interrupts also would not have
> - * been enabled.
> - */
> - if (!dev_priv->guc.log.relay_chan) {
> + /* If log_level was set as -1 at boot time, then the relay channel file
> + * wouldn't have been created by now and interrupts also would not have
> + * been enabled. Try again now, just in case.
> + */
> ret = guc_log_late_setup(guc);
> - if (!ret)
> - gen9_enable_guc_interrupts(dev_priv);
> - } else if (!log_param.logging_enabled) {
> + if (ret < 0) {
> + DRM_DEBUG_DRIVER("GuC log late setup failed %d\n", ret);
> + return ret;
> + }
> +
> + gen9_enable_guc_interrupts(dev_priv);
> + }
> + else
> + {
> /* Once logging is disabled, GuC won't generate logs & send an
> * interrupt. But there could be some data in the log buffer
> * which is yet to be captured. So request GuC to update the log
> @@ -629,9 +642,6 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
>
> /* As logging is disabled, update log level to reflect that */
> i915.guc_log_level = -1;
> - } else {
> - /* In case interrupts were disabled, enable them now */
> - gen9_enable_guc_interrupts(dev_priv);
> }
>
> return ret;
> @@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> return;
>
> mutex_lock(&dev_priv->drm.struct_mutex);
> - guc_log_cleanup(&dev_priv->guc);
> + gen9_disable_guc_interrupts(dev_priv);
> + intel_guc_log_destroy(&dev_priv->guc);
OCD nitpick: i915_guc_log_unregister is not simmetric with
i915_guc_log_register after this change, because intel_guc_log_destroy()
is destroying the vma, which was not created by guc_log_late_setup().
Thanks,
Daniele
> mutex_unlock(&dev_priv->drm.struct_mutex);
> }
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 511b96b..f34448b 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -217,10 +217,11 @@ void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
>
> /* intel_guc_log.c */
> -void intel_guc_log_create(struct intel_guc *guc);
> +int intel_guc_log_create(struct intel_guc *guc);
> +void intel_guc_log_destroy(struct intel_guc *guc);
> +int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
> void i915_guc_log_register(struct drm_i915_private *dev_priv);
> void i915_guc_log_unregister(struct drm_i915_private *dev_priv);
> -int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val);
>
> static inline u32 guc_ggtt_offset(struct i915_vma *vma)
> {
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: Add onion teardown to the GuC setup
2017-02-17 23:06 ` Daniele Ceraolo Spurio
@ 2017-02-20 10:39 ` Joonas Lahtinen
0 siblings, 0 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2017-02-20 10:39 UTC (permalink / raw)
To: Daniele Ceraolo Spurio, Oscar Mateo, intel-gfx
On pe, 2017-02-17 at 15:06 -0800, Daniele Ceraolo Spurio wrote:
>
> On 16/02/17 06:18, Oscar Mateo wrote:
> >
> > Starting with intel_guc_loader, down to intel_guc_submission
> > and finally to intel_guc_log.
> >
> > Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
<SNIP>
> > @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
> > sizeof(struct guc_mmio_reg_state) +
> > GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
> >
> > - vma = guc->ads_vma;
> > - if (!vma) {
>
> I believe the if was here to avoid re-allocating the vma if this
> function was called after a GPU reset. I agree that the check should be
> outside this function (and it already is), but we might want to still
> add here something like:
>
> if (WARN_ON(guc->ads_vma))
> return 0;
GEM_BUG_ON(guc->ads_vma); and then make sure we have sufficient
I-G-T/selftest coverage.
> > +void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> > +{
> > + struct intel_guc *guc = &dev_priv->guc;
> > +
>
> if I'm not missing anything, this function is called from
> intel_guc_fini(), which can in turn be called from the onion unwinding
> of i915_load_modeset_init() before intel_guc_init() is ever called, so
> we need to either fix that unwinding or add some kind of guard here.
Proper unwinding to the rescue!
<SNIP>
> > +int intel_guc_log_create(struct intel_guc *guc)
> > +{
> > + struct i915_vma *vma;
> > + unsigned long offset;
> > + uint32_t size, flags;
> > + int ret;
> > +
>
> If we expect this to not be called if the log already exists, for safety
> we could add something like:
>
> if (WARN_ON(guc->log.vma))
> return 0;
>
> To make sure we don't mess this up if we keep the object alive across
> resets.
GEM_BUG_ON(guc->log.vma);
> > @@ -653,6 +663,7 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> > return;
> >
> > mutex_lock(&dev_priv->drm.struct_mutex);
> > - guc_log_cleanup(&dev_priv->guc);
> > + gen9_disable_guc_interrupts(dev_priv);
> > + intel_guc_log_destroy(&dev_priv->guc);
>
> OCD nitpick: i915_guc_log_unregister is not simmetric with
> i915_guc_log_register after this change, because intel_guc_log_destroy()
> is destroying the vma, which was not created by guc_log_late_setup().
Yeah, destroy should be hoisted to calling function. And not so much
OCD, this is what we get double-free errors for all the time :)
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] drm/i915/guc: Add onion teardown to the GuC setup
2017-02-16 14:18 ` [PATCH] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
2017-02-16 14:21 ` Oscar Mateo
2017-02-17 23:06 ` Daniele Ceraolo Spurio
@ 2017-02-20 10:52 ` Joonas Lahtinen
2 siblings, 0 replies; 10+ messages in thread
From: Joonas Lahtinen @ 2017-02-20 10:52 UTC (permalink / raw)
To: Oscar Mateo, intel-gfx
On to, 2017-02-16 at 06:18 -0800, Oscar Mateo wrote:
> Starting with intel_guc_loader, down to intel_guc_submission
> and finally to intel_guc_log.
>
> Signed-off-by: Oscar Mateo <oscar.mateo@intel.com>
<SNIP>
> -static void guc_client_free(struct i915_guc_client *client)
> +static void guc_client_free(struct i915_guc_client **p_client)
> {
> + struct i915_guc_client *client;
> +
> + client = fetch_and_zero(p_client);
> + if (!client)
> + return;
> +
Might be useful to wrap the reminder of this function into
__guc_client_free, which can be called directly. But that can be added
later, when code described by Daniele appears.
> @@ -835,14 +841,11 @@ static void guc_addon_create(struct intel_guc *guc)
> sizeof(struct guc_mmio_reg_state) +
> GUC_S3_SAVE_SPACE_PAGES * PAGE_SIZE;
>
> - vma = guc->ads_vma;
> - if (!vma) {
> - vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> - if (IS_ERR(vma))
> - return;
> + vma = intel_guc_allocate_vma(guc, PAGE_ALIGN(size));
> + if (IS_ERR(vma))
> + return PTR_ERR(vma);
>
> - guc->ads_vma = vma;
> - }
> + guc->ads_vma = vma;
Only now realized the connection, may I suggest s/ads_vma/addon_vma/ as
a follow-up patch :P
> @@ -935,14 +954,33 @@ int i915_guc_submission_init(struct drm_i915_private *dev_priv)
> dev_priv->kernel_context);
> if (IS_ERR(guc->execbuf_client)) {
> DRM_ERROR("Failed to create GuC client for execbuf!\n");
> - goto err;
> + ret = PTR_ERR(guc->execbuf_client);
> + goto err_ads;
> }
>
> return 0;
\n here to make Chris happy.
> +err_ads:
> + guc_addon_destroy(guc);
> +err_log:
> + intel_guc_log_destroy(guc);
> +err_vaddr:
> + i915_gem_object_unpin_map(guc->ctx_pool->obj);
> +err_vma:
> + i915_vma_unpin_and_release(&guc->ctx_pool);
>
> -err:
> - i915_guc_submission_fini(dev_priv);
> - return -ENOMEM;
> + return ret;
> +}
<SNIP>
> -static int guc_log_create_relay_channel(struct intel_guc *guc)
> +static int guc_log_relay_channel_create(struct intel_guc *guc)
If guc_log_relay_channel is going to be a thing, then this is the right
thing to do.
> @@ -172,12 +166,21 @@ static int guc_log_create_relay_channel(struct intel_guc *guc)
> return 0;
> }
>
> -static int guc_log_create_relay_file(struct intel_guc *guc)
> +static void guc_log_relay_channel_destroy(struct intel_guc *guc)
> +{
> + relay_close(guc->log.relay_chan);
> + guc->log.relay_chan = NULL;
If the relay channel is a dynamic thing which gets recreated and
destroyed in runtime, then this is OKish, but if it's only created at
driver init and destroyed at shutdown, then don't bother assigning
NULL.
> +static int guc_log_relay_file_create(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> struct dentry *log_dir;
> int ret;
>
> + if (i915.guc_log_level < 0)
> + return 0; /* nothing to do */
The comment is not necessary, the check is rather self-explaining.
> @@ -198,7 +201,10 @@ static int guc_log_create_relay_file(struct intel_guc *guc)
> > }
>
> ret = relay_late_setup_files(guc->log.relay_chan, "guc_log", log_dir);
> - if (ret) {
> + if (ret == -EEXIST) {
> + /* Ignore "file already exists" */
Comment again is redundant. Just;
if (ret < 0 && ret != -EEXISTS)
> + }
> + else if (ret < 0) {
> DRM_ERROR("Couldn't associate relay chan with file %d\n", ret);
> return ret;
> }
<SNIP>
> -static int guc_log_create_extras(struct intel_guc *guc)
> +static int guc_log_extras_create(struct intel_guc *guc)
The naming convention we have is rather difficult, here
guc_log_create_extras would be the right name, as "guc_log" is the
structure, and "create extras" is our verb. If we had "guc_log_extras",
then "guc_log_extras" would be the structure and "create" the verb. But
if you intend to break out guc_log_extras, then it's good.
Although, the purpose of this function sounds more like init_extras.
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> void *vaddr;
> - int ret;
> + int ret = 0;
>
> lockdep_assert_held(&dev_priv->drm.struct_mutex);
>
> - /* Nothing to do */
> if (i915.guc_log_level < 0)
> - return 0;
> + return 0; /* nothing to do */
Don't be shy to nuke such comments.
>
> - if (!guc->log.buf_addr) {
> - /* Create a WC (Uncached for read) vmalloc mapping of log
> - * buffer pages, so that we can directly get the data
> - * (up-to-date) from memory.
> - */
> - vaddr = i915_gem_object_pin_map(guc->log.vma->obj, I915_MAP_WC);
> - if (IS_ERR(vaddr)) {
> - ret = PTR_ERR(vaddr);
> - DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> - return ret;
> - }
> + if (guc->log.buf_addr)
> + return 0; /* already allocated */
This check can be hoisted to calling function and if you feel like so,
add "guc_has_log_extras" helper. (Comment is redundant again).
Generally speaking, the calls should not be idempotent, so instead of
checking, add GEM_BUG_ON(guc->log.buf_addr); The more "if"s we have,
the harder it's to get good testing coverage.
<SNIP>
> + guc->log.flush_wq = alloc_ordered_workqueue("i915-guc_log",
> + WQ_HIGHPRI | WQ_FREEZABLE);
> + if (guc->log.flush_wq == NULL) {
While around, make it if (!guc->log.flush_wq)
> + DRM_ERROR("Couldn't allocate the wq for GuC logging\n");
> + ret = -ENOMEM;
> + goto err_relaychan;
> }
>
> return 0;
\n here and other spots.
> +err_relaychan:
> + guc_log_relay_channel_destroy(guc);
> +err_vaddr:
> > + i915_gem_object_unpin_map(guc->log.vma->obj);
> +
> > + return ret;
> }
<SNIP>
> @@ -609,17 +615,24 @@ int i915_guc_log_control(struct drm_i915_private *dev_priv, u64 control_val)
> return ret;
> }
>
> - i915.guc_log_level = log_param.verbosity;
> + if (log_param.logging_enabled)
Extra newline here to be removed.
A much wanted improvement. I might move the function renames to
follow-up patches when new structs get introduced. I'd also like some
clarity between "extras", "addon" and "ads" in follow-up :)
With above changes, this patch is looking good to me.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-02-20 10:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-09 10:24 [PATCH] drm/i915/guc: In the submission cleanup, do not bail out if there is no execbuf_client Oscar Mateo
2017-02-09 19:22 ` ✓ Fi.CI.BAT: success for " Patchwork
2017-02-10 12:04 ` [PATCH] " Chris Wilson
2017-02-13 15:55 ` Oscar Mateo
2017-02-14 14:20 ` Joonas Lahtinen
2017-02-16 14:18 ` [PATCH] drm/i915/guc: Add onion teardown to the GuC setup Oscar Mateo
2017-02-16 14:21 ` Oscar Mateo
2017-02-17 23:06 ` Daniele Ceraolo Spurio
2017-02-20 10:39 ` Joonas Lahtinen
2017-02-20 10:52 ` Joonas Lahtinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.