| Message ID | 1470983123-22127-7-git-send-email-akash.goel@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 12, 2016 at 11:55:09AM +0530, akash.goel@intel.com wrote: > 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 > static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) > { > + if (HAS_GUC_SCHED(dev_priv)) > + destroy_workqueue(dev_priv->guc.log.wq); if (dev_priv->guc.log.wq) destroy_workqueue(dev_priv->guc.log.wq); This shouldn't be here, but in guc teardown. Likewise this is > @@ -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)) { > + /* 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 = creating guc specific wq, not drm_i915_private's. They can even be managed by guc.log? -Chris
On 8/12/2016 11:58 AM, Chris Wilson wrote: > On Fri, Aug 12, 2016 at 11:55:09AM +0530, akash.goel@intel.com wrote: >> 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 >> static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) >> { >> + if (HAS_GUC_SCHED(dev_priv)) >> + destroy_workqueue(dev_priv->guc.log.wq); > > if (dev_priv->guc.log.wq) > destroy_workqueue(dev_priv->guc.log.wq); > > This shouldn't be here, but in guc teardown. > > Likewise this is > Fine will move it to GuC teardown. >> @@ -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)) { >> + /* 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 = > > creating guc specific wq, not drm_i915_private's. They can even be > managed by guc.log? Sorry for the inconsistency here, but didn't get your question. dev_priv->guc.log.wq dev_priv->guc.events_work Best regards Akash > -Chris >
On Fri, Aug 12, 2016 at 12:14:28PM +0530, Goel, Akash wrote: > > > On 8/12/2016 11:58 AM, Chris Wilson wrote: > >On Fri, Aug 12, 2016 at 11:55:09AM +0530, akash.goel@intel.com wrote: > >>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 > >> static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) > >> { > >>+ if (HAS_GUC_SCHED(dev_priv)) > >>+ destroy_workqueue(dev_priv->guc.log.wq); > > > >if (dev_priv->guc.log.wq) > > destroy_workqueue(dev_priv->guc.log.wq); > > > >This shouldn't be here, but in guc teardown. > > > >Likewise this is > > > Fine will move it to GuC teardown. > > >>@@ -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)) { > >>+ /* 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 = > > > >creating guc specific wq, not drm_i915_private's. They can even be > >managed by guc.log? > Sorry for the inconsistency here, but didn't get your question. > dev_priv->guc.log.wq Just somewhere inside guc, I was just noting that you probably already have setup/teardown for dev_priv->guc.log itself. -Chris
On 8/12/2016 12:21 PM, Chris Wilson wrote: > On Fri, Aug 12, 2016 at 12:14:28PM +0530, Goel, Akash wrote: >> >> >> On 8/12/2016 11:58 AM, Chris Wilson wrote: >>> On Fri, Aug 12, 2016 at 11:55:09AM +0530, akash.goel@intel.com wrote: >>>> 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 >>>> static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv) >>>> { >>>> + if (HAS_GUC_SCHED(dev_priv)) >>>> + destroy_workqueue(dev_priv->guc.log.wq); >>> >>> if (dev_priv->guc.log.wq) >>> destroy_workqueue(dev_priv->guc.log.wq); >>> >>> This shouldn't be here, but in guc teardown. >>> >>> Likewise this is >>> >> Fine will move it to GuC teardown. >> >>>> @@ -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)) { >>>> + /* 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 = >>> >>> creating guc specific wq, not drm_i915_private's. They can even be >>> managed by guc.log? >> Sorry for the inconsistency here, but didn't get your question. >> dev_priv->guc.log.wq > > Just somewhere inside guc, I was just noting that you probably already > have setup/teardown for dev_priv->guc.log itself. Fine, will move the dedicated wq creation/destruction in the setup/teardown routines for guc.log. Best Regards Akash > -Chris >
On 12/08/16 07:25, akash.goel@intel.com wrote: > From: Sagar Arun Kamble <sagar.a.kamble@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@intel.com> > Signed-off-by: Akash Goel <akash.goel@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
On 8/12/2016 6:47 PM, Tvrtko Ursulin wrote: > > On 12/08/16 07:25, akash.goel@intel.com wrote: >> From: Sagar Arun Kamble <sagar.a.kamble@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@intel.com> >> Signed-off-by: Akash Goel <akash.goel@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. Sorry for inconsistency, Will use HAS_GUC_SCHED in the previous patch. As per Chris's comments will move the wq init/destroy to the GuC logging setup/teardown routines (guc_create_log_extras, guc_log_cleanup) You are fine with that ?. > >> + /* 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. Fine will do that for both i & buffer_size. But I remember earlier in one of the patch, you suggested to use u32 as a type for some variables. Please could you share the guideline. Should u32, u64 be used we are exactly sure of the range of the variable, like for variables containing the register values ? > >> + >> + 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.) > Have done the range checking for the read/write offset values, which are updated repeatedly by GuC firmware. The buffer size is updated only at init time by GuC firmware, hence less vulnerable. But nevertheless will add the checks. >> + >> + if (log_buffer_snapshot_state) { >> + /* First copy the state structure in local buffer */ > > Maybe "destination buffer" would be clearer? Sorry my bad, well spotted. > >> + 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? :) Will like to keep it, if you allow. > >> + >> + /* 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
On 12/08/16 14:45, Goel, Akash wrote: > > > On 8/12/2016 6:47 PM, Tvrtko Ursulin wrote: >> >> On 12/08/16 07:25, akash.goel@intel.com wrote: >>> From: Sagar Arun Kamble <sagar.a.kamble@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@intel.com> >>> Signed-off-by: Akash Goel <akash.goel@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. > > Sorry for inconsistency, Will use HAS_GUC_SCHED in the previous patch. > > As per Chris's comments will move the wq init/destroy to the GuC logging > setup/teardown routines (guc_create_log_extras, guc_log_cleanup) > You are fine with that ?. Yes thats OK 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. > > Fine will do that for both i & buffer_size. buffer_size can match the type of log_buffer_state_local.size or use something else if more appropriate. > But I remember earlier in one of the patch, you suggested to use u32 as > a type for some variables. > Please could you share the guideline. > Should u32, u64 be used we are exactly sure of the range of the > variable, like for variables containing the register values ? Depends what the variable is. "i" in this case is just a standard counter so not appropriate to use u32. It is not that wrong on x86, just looks a bit out of place. grep "u32 i;" has no hits in i915. :) They can/should be used for variables tied with hardware or protocols. Or in some cases where you really want to save space by using a small type. > >> >>> + >>> + 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.) >> > Have done the range checking for the read/write offset values, which are > updated repeatedly by GuC firmware. > The buffer size is updated only at init time by GuC firmware, hence less > vulnerable. > > But nevertheless will add the checks. Ok good. >>> + >>> + if (log_buffer_snapshot_state) { >>> + /* First copy the state structure in local buffer */ >> >> Maybe "destination buffer" would be clearer? > > Sorry my bad, well spotted. > >> >>> + 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? :) > > Will like to keep it, if you allow. I think you need a really good justification to get r-b on patches with FIXMEs, especially like this one. Do you maybe handle it or remove it in a following patch or something? Regards, Tvrtko
On 8/12/2016 7:37 PM, Tvrtko Ursulin wrote: > > On 12/08/16 14:45, Goel, Akash wrote: >> >> >> On 8/12/2016 6:47 PM, Tvrtko Ursulin wrote: >>> >>> On 12/08/16 07:25, akash.goel@intel.com wrote: >>>> From: Sagar Arun Kamble <sagar.a.kamble@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@intel.com> >>>> Signed-off-by: Akash Goel <akash.goel@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. >> >> Sorry for inconsistency, Will use HAS_GUC_SCHED in the previous patch. >> >> As per Chris's comments will move the wq init/destroy to the GuC logging >> setup/teardown routines (guc_create_log_extras, guc_log_cleanup) >> You are fine with that ?. > > Yes thats OK 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. >> >> Fine will do that for both i & buffer_size. > > buffer_size can match the type of log_buffer_state_local.size or use > something else if more appropriate. > >> But I remember earlier in one of the patch, you suggested to use u32 as >> a type for some variables. >> Please could you share the guideline. >> Should u32, u64 be used we are exactly sure of the range of the >> variable, like for variables containing the register values ? > > Depends what the variable is. "i" in this case is just a standard > counter so not appropriate to use u32. It is not that wrong on x86, just > looks a bit out of place. > > grep "u32 i;" has no hits in i915. :) > > They can/should be used for variables tied with hardware or protocols. > Or in some cases where you really want to save space by using a small type. > Thanks will be mindful of this. >> >>> >>>> + >>>> + 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. >>> Ideally it should not, as logging itself is disabled (interrupts also disabled) on log buffer allocation failure. >>>> + >>>> + /* 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.) >>> >> Have done the range checking for the read/write offset values, which are >> updated repeatedly by GuC firmware. >> The buffer size is updated only at init time by GuC firmware, hence less >> vulnerable. >> >> But nevertheless will add the checks. > > Ok good. > >>>> + >>>> + if (log_buffer_snapshot_state) { >>>> + /* First copy the state structure in local buffer */ >>> >>> Maybe "destination buffer" would be clearer? >> >> Sorry my bad, well spotted. >> >>> >>>> + 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? :) >> >> Will like to keep it, if you allow. > > I think you need a really good justification to get r-b on patches with > FIXMEs, especially like this one. Do you maybe handle it or remove it in > a following patch or something? > Have removed this FIXME in the later patch "[PATCH 17/20] drm/i915: Use uncached(WC) mapping for acessing the GuC log buffer" Best regards Akash > Regards, > > Tvrtko
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)) { + /* 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; + + if (!guc->log.buf_addr) + return; + + /* 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; + + if (log_buffer_snapshot_state) { + /* First copy the state structure in local buffer */ + 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 */ + + /* 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