Quoting Sagar Arun Kamble (2018-01-24 10:52:28)
>
>
> On 1/24/2018 3:48 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2018-01-24 04:09:09)
> >> @@ -197,7 +212,7 @@ static void guc_move_to_next_buf(struct intel_guc *guc)
> >>
> >> static void *guc_get_write_buffer(struct intel_guc *guc)
> >> {
> >> - if (!guc->log.runtime.relay_chan)
> >> + if (!guc_log_has_relay(guc))
> >> return NULL;
> >>
> >> /* Just get the base address of a new sub buffer and copy data into it
> >> @@ -265,6 +280,8 @@ static void guc_read_update_log_buffer(struct intel_guc *guc)
> >> /* Get the pointer to shared GuC log buffer */
> >> log_buf_state = src_data = guc->log.runtime.buf_addr;
> >>
> >> + mutex_lock(&guc->log.runtime.relay_lock);
> >> +
> >> /* Get the pointer to local buffer to store the logs */
> >> log_buf_snapshot_state = dst_data = guc_get_write_buffer(guc);
> > Hmm. The locking here tells me that we are being careful in case the
> > relay_chan disappears, but we don't handle the NULL pointer here.
> >
> There is check for log_bug_snapshot_state below in for loop. But yes, we
> should return from here.
> Will update.
> >> @@ -643,6 +724,8 @@ void i915_guc_log_unregister(struct drm_i915_private *dev_priv)
> >> gen9_disable_guc_interrupts(dev_priv);
> >> intel_runtime_pm_put(dev_priv);
> >>
> >> - guc_log_runtime_destroy(&dev_priv->guc);
> >> + guc_log_runtime_destroy(guc);
> >> mutex_unlock(&dev_priv->drm.struct_mutex);
> >> +
> >> + intel_guc_log_relay_destroy(guc);
> >> }
> > This looks all reasonably well described by the addition of the
> > relay_lock and the interactions look fine. The only mistake I could see,
> > in the story told by this patch, was the runtime checking.
> Could you please elaborate more on this.
The previous comment :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
