Re: [PATCH 1/1] drm/xe/eustall: Do not support EU stall on SRIOV VF

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 22 Apr 2025 12:01:27 -0700, Harish Chegondi wrote:
>
> On Mon, Apr 21, 2025 at 05:59:43PM -0700, Dixit, Ashutosh wrote:
> > On Sun, 20 Apr 2025 22:59:01 -0700, Harish Chegondi wrote:
> > >
> > > EU stall sampling is not supported on SRIOV VF. Do not
> > > initialize or open EU stall stream on SRIOV VF.
> > >
> > > Signed-off-by: Harish Chegondi <harish.chegondi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_eu_stall.c | 3 +++
> > >  drivers/gpu/drm/xe/xe_eu_stall.h | 3 ++-
> > >  2 files changed, 5 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_eu_stall.c b/drivers/gpu/drm/xe/xe_eu_stall.c
> > > index f2bb9168967c..64788277ea52 100644
> > > --- a/drivers/gpu/drm/xe/xe_eu_stall.c
> > > +++ b/drivers/gpu/drm/xe/xe_eu_stall.c
> > > @@ -208,6 +208,9 @@ int xe_eu_stall_init(struct xe_gt *gt)
> > >	struct xe_device *xe = gt_to_xe(gt);
> > >	int ret;
> > >
> > > +	if (!xe_eu_stall_supported_on_platform(xe))
> > > +		return 0;
> >
> > This check is not strictly needed. If the check is not there, just some
> > unnecessary stuff will get initialized.
> >
> > But if you want to add this check here, you will also need to a
> > 'if (!gt->eu_stall)' check in xe_eu_stall_fini(). If you test this patch,
> > maybe you will already see oops when unloading the driver?
> Hi Ashutosh,
> This check would prevent calling devm_add_action_or_reset() for those
> platforms that do not support EU stall sampling and therefore
> xe_eu_stall_fini() will not be registered for callback during module
> unload. So, I think it is not required to add the additional check in
> xe_eu_stall_fini().

Sorry, yes you are right. I forgot xe_eu_stall_fini() is not called
directly now.

In that case, maybe we should still change the
xe_eu_stall_supported_on_platform() check in xe_eu_stall_stream_open() to
'if (!gt->eu_stall)' check? It's a nit but still maybe better do it I
think.

> >
> > So either remove this check or add the additional check in
> > xe_eu_stall_fini().
> >
> > > +
> > >	gt->eu_stall = kzalloc(sizeof(*gt->eu_stall), GFP_KERNEL);
> > >	if (!gt->eu_stall) {
> > >		ret = -ENOMEM;
> > > diff --git a/drivers/gpu/drm/xe/xe_eu_stall.h b/drivers/gpu/drm/xe/xe_eu_stall.h
> > > index ed9d0f233566..d1c76e503799 100644
> > > --- a/drivers/gpu/drm/xe/xe_eu_stall.h
> > > +++ b/drivers/gpu/drm/xe/xe_eu_stall.h
> > > @@ -7,6 +7,7 @@
> > >  #define __XE_EU_STALL_H__
> > >
> > >  #include "xe_gt_types.h"
> > > +#include "xe_sriov.h"
> > >
> > >  size_t xe_eu_stall_get_per_xecore_buf_size(void);
> > >  size_t xe_eu_stall_data_record_size(struct xe_device *xe);
> > > @@ -19,6 +20,6 @@ int xe_eu_stall_stream_open(struct drm_device *dev,
> > >
> > >  static inline bool xe_eu_stall_supported_on_platform(struct xe_device *xe)
> > >  {
> > > -	return xe->info.platform == XE_PVC || GRAPHICS_VER(xe) >= 20;
> > > +	return !IS_SRIOV_VF(xe) && (xe->info.platform == XE_PVC || GRAPHICS_VER(xe) >= 20);
> > >  }
> > >  #endif
> > > --
> > > 2.48.1
> > >