[RFC 21/34] drm/xe: Convert GuC CT paths from mem_access to xe_pm_runtime
Rodrigo Vivi
rodrigo.vivi at intel.com
Wed Feb 28 16:51:31 UTC 2024
On Mon, Feb 05, 2024 at 12:23:41PM +0000, Matthew Auld wrote:
> On 26/01/2024 20:30, Rodrigo Vivi wrote:
> > For the G2H and TLB invalidation cases, the outer bounds
> > protection of the pm_runtime are not going to work. So
> > we need to have the inner side protections here.
> >
> > Cc: Matthew Auld <matthew.auld at intel.com>
> > Suggested-by: Matthew Brost <matthew.brost at intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_debugfs.c | 3 ++
> > drivers/gpu/drm/xe/xe_guc_ct.c | 79 +++++++++++++---------------
> > drivers/gpu/drm/xe/xe_guc_ct_types.h | 2 +
> > 3 files changed, 41 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_debugfs.c b/drivers/gpu/drm/xe/xe_debugfs.c
> > index 8abdf3c17e1d..b3669eac23c9 100644
> > --- a/drivers/gpu/drm/xe/xe_debugfs.c
> > +++ b/drivers/gpu/drm/xe/xe_debugfs.c
> > @@ -64,6 +64,9 @@ static int info(struct seq_file *m, void *data)
> > xe_force_wake_ref(gt_to_fw(gt), XE_FW_GT));
> > drm_printf(&p, "gt%d engine_mask 0x%llx\n", id,
> > gt->info.engine_mask);
> > + drm_printf(&p, "gt%d g2h_outstanding %d g2h_pm_refs %d\n", id,
> > + gt->uc.guc.ct.g2h_outstanding,
> > + gt->uc.guc.ct.g2h_pm_refs);
> > }
> > xe_pm_runtime_put(xe);
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > index f3d356383ced..139cfe733661 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > @@ -287,16 +287,49 @@ static int guc_ct_control_toggle(struct xe_guc_ct *ct, bool enable)
> > return ret > 0 ? -EPROTO : ret;
> > }
> > +static void guc_ct_g2h_outstanding_clear(struct xe_guc_ct *ct)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ct->g2h_pm_refs; i++)
> > + xe_pm_runtime_put(ct_to_xe(ct));
> > + ct->g2h_outstanding = 0;
> > + ct->g2h_pm_refs = 0;
> > +}
> > +
> > +static void guc_ct_g2h_outstanding_dec(struct xe_guc_ct *ct)
> > +{
> > + if (ct->g2h_pm_refs > 0)
> > + xe_pm_runtime_put(ct_to_xe(ct));
> > + ct->g2h_pm_refs--;
> > + ct->g2h_outstanding--;
> > +}
> > +
> > +static void guc_ct_g2h_outstanding_add(struct xe_guc_ct *ct, int num_g2h)
> > +{
> > + struct xe_device *xe = ct_to_xe(ct);
> > + int i;
> > +
> > + for (i = 0; i < num_g2h; i++) {
> > + if (xe_pm_runtime_get_if_in_use(xe))
> > + ct->g2h_pm_refs++;
> > + else
> > + drm_err(&xe->drm, "Failed to grab RPM ref for outstanding g2h\n");
> > + }
>
> I guess we could maybe drop the g2h_pm_refs, and rather just track single
> rpm ref for entire ct when g2h_outstanding 0 -> 1, and likewise drop the rpm
> ref when g2h_outstanding reaches zero?
I thought about that, but I was afraid of some unbalanced case when the
get_if_in_use fails.
>
> > + ct->g2h_outstanding += num_g2h;
> > +}
> > +
> > static void xe_guc_ct_set_state(struct xe_guc_ct *ct,
> > enum xe_guc_ct_state state)
> > {
> > mutex_lock(&ct->lock); /* Serialise dequeue_one_g2h() */
> > spin_lock_irq(&ct->fast_lock); /* Serialise CT fast-path */
> > +
> > xe_gt_assert(ct_to_gt(ct), ct->g2h_outstanding == 0 ||
> > state == XE_GUC_CT_STATE_STOPPED);
> > - ct->g2h_outstanding = 0;
> > + guc_ct_g2h_outstanding_clear(ct);
> > ct->state = state;
> > spin_unlock_irq(&ct->fast_lock);
> > @@ -428,7 +461,7 @@ static void __g2h_reserve_space(struct xe_guc_ct *ct, u32 g2h_len, u32 num_g2h)
> > lockdep_assert_held(&ct->fast_lock);
> > ct->ctbs.g2h.info.space -= g2h_len;
> > - ct->g2h_outstanding += num_g2h;
> > + guc_ct_g2h_outstanding_add(ct, num_g2h);
> > }
> > }
> > @@ -439,7 +472,7 @@ static void __g2h_release_space(struct xe_guc_ct *ct, u32 g2h_len)
> > ct->ctbs.g2h.info.size - ct->ctbs.g2h.info.resv_space);
> > ct->ctbs.g2h.info.space += g2h_len;
> > - --ct->g2h_outstanding;
> > + guc_ct_g2h_outstanding_dec(ct);
> > }
> > static void g2h_release_space(struct xe_guc_ct *ct, u32 g2h_len)
> > @@ -1199,14 +1232,8 @@ static void g2h_fast_path(struct xe_guc_ct *ct, u32 *msg, u32 len)
> > */
> > void xe_guc_ct_fast_path(struct xe_guc_ct *ct)
> > {
> > - struct xe_device *xe = ct_to_xe(ct);
> > - bool ongoing;
> > int len;
> > - ongoing = xe_device_mem_access_get_if_ongoing(ct_to_xe(ct));
> > - if (!ongoing && xe_pm_read_callback_task(ct_to_xe(ct)) == NULL)
> > - return;
> > -
> > spin_lock(&ct->fast_lock);
> > do {
> > len = g2h_read(ct, ct->fast_msg, true);
> > @@ -1214,9 +1241,6 @@ void xe_guc_ct_fast_path(struct xe_guc_ct *ct)
> > g2h_fast_path(ct, ct->fast_msg, len);
> > } while (len > 0);
> > spin_unlock(&ct->fast_lock);
> > -
> > - if (ongoing)
> > - xe_device_mem_access_put(xe);
> > }
> > /* Returns less than zero on error, 0 on done, 1 on more available */
> > @@ -1247,36 +1271,8 @@ static int dequeue_one_g2h(struct xe_guc_ct *ct)
> > static void g2h_worker_func(struct work_struct *w)
> > {
> > struct xe_guc_ct *ct = container_of(w, struct xe_guc_ct, g2h_worker);
> > - bool ongoing;
> > int ret;
> > - /*
> > - * Normal users must always hold mem_access.ref around CT calls. However
> > - * during the runtime pm callbacks we rely on CT to talk to the GuC, but
> > - * at this stage we can't rely on mem_access.ref and even the
> > - * callback_task will be different than current. For such cases we just
> > - * need to ensure we always process the responses from any blocking
> > - * ct_send requests or where we otherwise expect some response when
> > - * initiated from those callbacks (which will need to wait for the below
> > - * dequeue_one_g2h()). The dequeue_one_g2h() will gracefully fail if
> > - * the device has suspended to the point that the CT communication has
> > - * been disabled.
> > - *
> > - * If we are inside the runtime pm callback, we can be the only task
> > - * still issuing CT requests (since that requires having the
> > - * mem_access.ref). It seems like it might in theory be possible to
> > - * receive unsolicited events from the GuC just as we are
> > - * suspending-resuming, but those will currently anyway be lost when
> > - * eventually exiting from suspend, hence no need to wake up the device
> > - * here. If we ever need something stronger than get_if_ongoing() then
> > - * we need to be careful with blocking the pm callbacks from getting CT
> > - * responses, if the worker here is blocked on those callbacks
> > - * completing, creating a deadlock.
> > - */
> > - ongoing = xe_device_mem_access_get_if_ongoing(ct_to_xe(ct));
> > - if (!ongoing && xe_pm_read_callback_task(ct_to_xe(ct)) == NULL)
> > - return;
> > -
> > do {
> > mutex_lock(&ct->lock);
> > ret = dequeue_one_g2h(ct);
> > @@ -1290,9 +1286,6 @@ static void g2h_worker_func(struct work_struct *w)
> > kick_reset(ct);
> > }
> > } while (ret == 1);
> > -
> > - if (ongoing)
> > - xe_device_mem_access_put(ct_to_xe(ct));
> > }
> > static void guc_ctb_snapshot_capture(struct xe_device *xe, struct guc_ctb *ctb,
> > diff --git a/drivers/gpu/drm/xe/xe_guc_ct_types.h b/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > index d29144c9f20b..4220230e7be4 100644
> > --- a/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > +++ b/drivers/gpu/drm/xe/xe_guc_ct_types.h
> > @@ -108,6 +108,8 @@ struct xe_guc_ct {
> > } ctbs;
> > /** @g2h_outstanding: number of outstanding G2H */
> > u32 g2h_outstanding;
> > + /** @g2h_pm_refs: number of pm_refs for pending G2H */
> > + u32 g2h_pm_refs;
> > /** @g2h_worker: worker to process G2H messages */
> > struct work_struct g2h_worker;
> > /** @state: CT state */
More information about the Intel-xe
mailing list