Re: [PATCH] [RFC]drm/xe: Introduce active ccs tracking

[Matthew Brost <matthew.brost@intel.com>]

On Fri, Sep 20, 2024 at 08:54:50AM -0500, Lucas De Marchi wrote:
> On Fri, Sep 20, 2024 at 07:05:14PM GMT, Tejas Upadhyay wrote:
> > Current ccs_mode setting is allowed when no client has
> > actively opened device. Instead it should be restricted
> > if there is any active ccs engine in use.
> > 
> > Closing device fd is always a async and may lead to show
> > client present even after fd is closed from user perspective.
> > 
> > Signed-off-by: Tejas Upadhyay <tejas.upadhyay@intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_force_wake.c  | 3 +++
> > drivers/gpu/drm/xe/xe_gt_ccs_mode.c | 8 ++++----
> > drivers/gpu/drm/xe/xe_gt_types.h    | 6 ++++++
> > 3 files changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_force_wake.c b/drivers/gpu/drm/xe/xe_force_wake.c
> > index a64c14757c84..51ab8e5dd583 100644
> > --- a/drivers/gpu/drm/xe/xe_force_wake.c
> > +++ b/drivers/gpu/drm/xe/xe_force_wake.c
> > @@ -195,6 +195,9 @@ int xe_force_wake_put(struct xe_force_wake *fw,
> > 		ret |= domain_sleep_wait(gt, domain);
> > 	}
> > 	fw->awake_domains &= ~sleep;
> > +	if (!(fw->awake_domains & XE_FW_GT))
> > +		if (gt->ccs_mode && !IS_SRIOV_VF(gt_to_xe(gt)))
> > +			gt->ccs_active = 0;
> > 	spin_unlock_irqrestore(&fw->lock, flags);
> > 
> > 	return ret;
> > diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> > index 9360ac4de489..dbc49d595a6c 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> > @@ -69,6 +69,7 @@ static void __xe_gt_apply_ccs_mode(struct xe_gt *gt, u32 num_engines)
> > 	}
> > 
> > 	xe_mmio_write32(&gt->mmio, CCS_MODE, mode);
> > +	gt->ccs_active = mode;
> > 
> > 	xe_gt_dbg(gt, "CCS_MODE=%x config:%08x, num_engines:%d, num_slices:%d\n",
> > 		  mode, config, num_engines, num_slices);
> > @@ -132,10 +133,9 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
> > 		return -EINVAL;
> > 	}
> > 
> > -	/* CCS mode can only be updated when there are no drm clients */
> > -	spin_lock(&xe->clients.lock);
> > -	if (xe->clients.count) {
> > -		spin_unlock(&xe->clients.lock);
> > +	/* CCS mode can only be updated when there is no active ccs_mode */
> 
> same question as before:  what happens if userspace cached the available
> engines from a query (like it normally does) and then decide to submit
> later?  CCS being idle doesn't mean we can change it. I think what we
> really want is to track the userspace clients (i.e. ignore kernel
> clients since those don't do anything with CCS, but probably add an
> assert somewhere to guarantee it).
> 
> AFAIK it's not expected from umd point of view to have dynamic
> changes on available engines. +Joonas, +Matthew Brost
> 

Certainly we cannot be changing the engines we report as valid to the
user.

I think what Lucas is suggesting is correct unless user space queries
engine before every exec queue open, even then that could race.

Matt

> Lucas De Marchi