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

["Lahtinen, Joonas" <joonas.lahtinen@intel.com>]

(Adding Andi, Swapping to my @linux.intel.com address)

On Fri, 2024-09-20 at 08:54 -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>

<SNIP>

> > +++ 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?

How this was handled in downstream for PVC was that different clients
did have a different view of the hardware, and there is an indefinite
wait loop at batchbuffer submission time until the hardware is in the
right mode. It was based on first come first serve (until yielding).

It was specifically decided that for upstream we want a sysfs or
modparam to explicitly choose the mode for all clients for i915 and xe.

> 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).

Right, the in-kernel clients indeed need to be excluded. I thought that
was already solved problem in Andi's series for i915?

> AFAIK it's not expected from umd point of view to have dynamic
> changes on available engines. +Joonas, +Matthew Brost

Correct. While this can be partially hacked around, it leads to
multiple ugly corner cases and some workloads even simply not working
when they execute with less EUs than expected (when CCS_MODE = 2, but
client expects CCS_MODE = 1 so it only gets 50% of EUs).

So we should stick to sysfs or modparam. Sysfs has the benefit that
compute workloads can change the mode before starting, modparam will of
course be simpler.

Regards, Joonas

> 
> Lucas De Marchi