[Intel-gfx] [PATCH 06/20] drm/i915: Handle log buffer flush interrupt event from GuC
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Fri Aug 12 13:17:37 UTC 2016
On 12/08/16 07:25, akash.goel at intel.com wrote:
> From: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>
> GuC ukernel sends an interrupt to Host to flush the log buffer
> and expects Host to correspondingly update the read pointer
> information in the state structure, once it has consumed the
> log buffer contents by copying them to a file or buffer.
> Even if Host couldn't copy the contents, it can still update the
> read pointer so that logging state is not disturbed on GuC side.
>
> v2:
> - Use a dedicated workqueue for handling flush interrupt. (Tvrtko)
> - Reduce the overall log buffer copying time by skipping the copy of
> crash buffer area for regular cases and copying only the state
> structure data in first page.
>
> v3:
> - Create a vmalloc mapping of log buffer. (Chris)
> - Cover the flush acknowledgment under rpm get & put.(Chris)
> - Revert the change of skipping the copy of crash dump area, as
> not really needed, will be covered by subsequent patch.
>
> v4:
> - Destroy the wq under the same condition in which it was created,
> pass dev_piv pointer instead of dev to newly added GuC function,
> add more comments & rename variable for clarity. (Tvrtko)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> Signed-off-by: Akash Goel <akash.goel at intel.com>
> ---
> drivers/gpu/drm/i915/i915_drv.c | 14 +++
> drivers/gpu/drm/i915/i915_guc_submission.c | 150 +++++++++++++++++++++++++++++
> drivers/gpu/drm/i915/i915_irq.c | 5 +-
> drivers/gpu/drm/i915/intel_guc.h | 3 +
> 4 files changed, 170 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 0fcd1c0..fc2da32 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -770,8 +770,20 @@ static int i915_workqueues_init(struct drm_i915_private *dev_priv)
> if (dev_priv->hotplug.dp_wq == NULL)
> goto out_free_wq;
>
> + if (HAS_GUC_SCHED(dev_priv)) {
This just reminded me that a previous patch had:
+ if (HAS_GUC_UCODE(dev))
+ dev_priv->pm_guc_events = GEN9_GUC_TO_HOST_INT_EVENT;
In the interrupt setup. I don't think there is a bug right now, but
there is a disagreement between the two which would be good to resolve.
This HAS_GUC_UCODE in the other patch should probably be HAS_GUC_SCHED
for correctness. I think.
> + /* Need a dedicated wq to process log buffer flush interrupts
> + * from GuC without much delay so as to avoid any loss of logs.
> + */
> + dev_priv->guc.log.wq =
> + alloc_ordered_workqueue("i915-guc_log", 0);
> + if (dev_priv->guc.log.wq == NULL)
> + goto out_free_hotplug_dp_wq;
> + }
> +
> return 0;
>
> +out_free_hotplug_dp_wq:
> + destroy_workqueue(dev_priv->hotplug.dp_wq);
> out_free_wq:
> destroy_workqueue(dev_priv->wq);
> out_err:
> @@ -782,6 +794,8 @@ out_err:
>
> static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv)
> {
> + if (HAS_GUC_SCHED(dev_priv))
> + destroy_workqueue(dev_priv->guc.log.wq);
> destroy_workqueue(dev_priv->hotplug.dp_wq);
> destroy_workqueue(dev_priv->wq);
> }
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index c7c679f..2635b67 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -172,6 +172,15 @@ static int host2guc_sample_forcewake(struct intel_guc *guc,
> return host2guc_action(guc, data, ARRAY_SIZE(data));
> }
>
> +static int host2guc_logbuffer_flush_complete(struct intel_guc *guc)
> +{
> + u32 data[1];
> +
> + data[0] = HOST2GUC_ACTION_LOG_BUFFER_FILE_FLUSH_COMPLETE;
> +
> + return host2guc_action(guc, data, 1);
> +}
> +
> /*
> * Initialise, update, or clear doorbell data shared with the GuC
> *
> @@ -840,6 +849,127 @@ err:
> return NULL;
> }
>
> +static void guc_move_to_next_buf(struct intel_guc *guc)
> +{
> + return;
> +}
> +
> +static void* guc_get_write_buffer(struct intel_guc *guc)
> +{
> + return NULL;
> +}
> +
> +static void guc_read_update_log_buffer(struct intel_guc *guc)
> +{
> + struct guc_log_buffer_state *log_buffer_state, *log_buffer_snapshot_state;
> + struct guc_log_buffer_state log_buffer_state_local;
> + void *src_data_ptr, *dst_data_ptr;
> + u32 i, buffer_size;
unsigned int i if you can be bothered.
> +
> + if (!guc->log.buf_addr)
> + return;
Can it hit this? If yes, I think better disable GuC logging when pin map
on the object fails rather than let it generate interrupts in vain.
> +
> + /* Get the pointer to shared GuC log buffer */
> + log_buffer_state = src_data_ptr = guc->log.buf_addr;
> +
> + /* Get the pointer to local buffer to store the logs */
> + dst_data_ptr = log_buffer_snapshot_state = guc_get_write_buffer(guc);
> +
> + /* Actual logs are present from the 2nd page */
> + src_data_ptr += PAGE_SIZE;
> + dst_data_ptr += PAGE_SIZE;
> +
> + for (i = 0; i < GUC_MAX_LOG_BUFFER; i++) {
> + /* Make a copy of the state structure in GuC log buffer (which
> + * is uncached mapped) on the stack to avoid reading from it
> + * multiple times.
> + */
> + memcpy(&log_buffer_state_local, log_buffer_state,
> + sizeof(struct guc_log_buffer_state));
> + buffer_size = log_buffer_state_local.size;
Needs checking (and logging) that a bad firmware or some other error
does not report a dangerous size (too big) which would then overwrite
memory pointed by dst_data_ptr memory. (And/or read from random memory.)
> +
> + if (log_buffer_snapshot_state) {
> + /* First copy the state structure in local buffer */
Maybe "destination buffer" would be clearer?
> + memcpy(log_buffer_snapshot_state, &log_buffer_state_local,
> + sizeof(struct guc_log_buffer_state));
> +
> + /* The write pointer could have been updated by the GuC
> + * firmware, after sending the flush interrupt to Host,
> + * for consistency set the write pointer value to same
> + * value of sampled_write_ptr in the snapshot buffer.
> + */
> + log_buffer_snapshot_state->write_ptr =
> + log_buffer_snapshot_state->sampled_write_ptr;
> +
> + log_buffer_snapshot_state++;
> +
> + /* Now copy the actual logs */
> + memcpy(dst_data_ptr, src_data_ptr, buffer_size);
> +
> + src_data_ptr += buffer_size;
> + dst_data_ptr += buffer_size;
> + }
> +
> + /* FIXME: invalidate/flush for log buffer needed */
Yes no maybe? :)
> +
> + /* Update the read pointer in the shared log buffer */
> + log_buffer_state->read_ptr =
> + log_buffer_state_local.sampled_write_ptr;
> +
> + /* Clear the 'flush to file' flag */
> + log_buffer_state->flush_to_file = 0;
> + log_buffer_state++;
> + }
> +
> + if (log_buffer_snapshot_state)
> + guc_move_to_next_buf(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);
> +
> + if (i915.guc_log_level < 0)
> + return;
> +
> + /* First disable the flush interrupt */
> + gen9_disable_guc_interrupts(dev_priv);
> +
> + if (guc->log.buf_addr)
> + i915_gem_object_unpin_map(guc->log.obj);
> +
> + guc->log.buf_addr = NULL;
> +}
> +
> +static int guc_create_log_extras(struct intel_guc *guc)
> +{
> + struct drm_i915_private *dev_priv = guc_to_i915(guc);
> + void *vaddr;
> + int ret;
> +
> + lockdep_assert_held(&dev_priv->drm.struct_mutex);
> +
> + /* Nothing to do */
> + if (i915.guc_log_level < 0)
> + return 0;
> +
> + if (!guc->log.buf_addr) {
> + /* Create a vmalloc mapping of log buffer pages */
> + vaddr = i915_gem_object_pin_map(guc->log.obj);
> + if (IS_ERR(vaddr)) {
> + ret = PTR_ERR(vaddr);
> + DRM_ERROR("Couldn't map log buffer pages %d\n", ret);
> + return ret;
> + }
> +
> + guc->log.buf_addr = vaddr;
> + }
> +
> + return 0;
> +}
> +
> static void guc_create_log(struct intel_guc *guc)
> {
> struct drm_i915_private *dev_priv = guc_to_i915(guc);
> @@ -866,6 +996,13 @@ static void guc_create_log(struct intel_guc *guc)
> }
>
> guc->log.obj = obj;
> +
> + if (guc_create_log_extras(guc)) {
> + gem_release_guc_obj(guc->log.obj);
> + guc->log.obj = NULL;
> + i915.guc_log_level = -1;
> + return;
> + }
> }
>
> /* each allocated unit is a page */
> @@ -1048,6 +1185,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> gem_release_guc_obj(dev_priv->guc.ads_obj);
> guc->ads_obj = NULL;
>
> + guc_log_cleanup(guc);
> gem_release_guc_obj(dev_priv->guc.log.obj);
> guc->log.obj = NULL;
>
> @@ -1111,3 +1249,15 @@ int intel_guc_resume(struct drm_device *dev)
>
> return host2guc_action(guc, data, ARRAY_SIZE(data));
> }
> +
> +void i915_guc_capture_logs(struct drm_i915_private *dev_priv)
> +{
> + guc_read_update_log_buffer(&dev_priv->guc);
> +
> + /* Generally device is expected to be active only at this
> + * time, so get/put should be really quick.
> + */
> + intel_runtime_pm_get(dev_priv);
> + host2guc_logbuffer_flush_complete(&dev_priv->guc);
> + intel_runtime_pm_put(dev_priv);
> +}
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5f1974f..d4d6f0a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1213,7 +1213,7 @@ static void gen9_guc2host_events_work(struct work_struct *work)
> }
> spin_unlock_irq(&dev_priv->irq_lock);
>
> - /* TODO: Handle the events for which GuC interrupted host */
> + i915_guc_capture_logs(dev_priv);
> }
>
> /**
> @@ -1703,7 +1703,8 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir)
> I915_READ(SOFT_SCRATCH(15)) & ~msg);
>
> /* Handle flush interrupt event in bottom half */
> - queue_work(dev_priv->wq, &dev_priv->guc.events_work);
> + queue_work(dev_priv->guc.log.wq,
> + &dev_priv->guc.events_work);
> }
> }
> }
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index be1e04d..7c0bdba 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -124,6 +124,8 @@ struct intel_guc_fw {
> struct intel_guc_log {
> uint32_t flags;
> struct drm_i915_gem_object *obj;
> + struct workqueue_struct *wq;
> + void *buf_addr;
> };
>
> struct intel_guc {
> @@ -169,5 +171,6 @@ int i915_guc_submission_enable(struct drm_i915_private *dev_priv);
> int i915_guc_wq_check_space(struct drm_i915_gem_request *rq);
> void i915_guc_submission_disable(struct drm_i915_private *dev_priv);
> void i915_guc_submission_fini(struct drm_i915_private *dev_priv);
> +void i915_guc_capture_logs(struct drm_i915_private *dev_priv);
>
> #endif
>
Regards,
Tvrtko
More information about the Intel-gfx
mailing list