Re: [PATCH 5/9] drm/xe/vf: Don't initialize OA if VF

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

On Wed, 19 Jun 2024 15:40:43 -0700, Michal Wajdeczko wrote:
>
> On 20.06.2024 00:10, Dixit, Ashutosh wrote:
> > On Wed, 19 Jun 2024 14:56:32 -0700, Dixit, Ashutosh wrote:
> >>
> >> On Wed, 19 Jun 2024 14:45:53 -0700, Michal Wajdeczko wrote:
> >>>
> >>> We don't support Observation Architecture on the VF device.
> >>>
> >>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> Cc: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>> ---
> >>>  drivers/gpu/drm/xe/xe_oa.c | 4 ++++
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/drivers/gpu/drm/xe/xe_oa.c b/drivers/gpu/drm/xe/xe_oa.c
> >>> index 34206e0b6a08..f54a97954452 100644
> >>> --- a/drivers/gpu/drm/xe/xe_oa.c
> >>> +++ b/drivers/gpu/drm/xe/xe_oa.c
> >>> @@ -35,6 +35,7 @@
> >>>  #include "xe_perf.h"
> >>>  #include "xe_pm.h"
> >>>  #include "xe_sched_job.h"
> >>> +#include "xe_sriov.h"
> >>>
> >>>  #define DEFAULT_POLL_FREQUENCY_HZ 200
> >>>  #define DEFAULT_POLL_PERIOD_NS (NSEC_PER_SEC / DEFAULT_POLL_FREQUENCY_HZ)
> >>> @@ -2385,6 +2386,9 @@ int xe_oa_init(struct xe_device *xe)
> >>>	if (XE_WARN_ON(!xe_device_uc_enabled(xe)) || XE_WARN_ON(GRAPHICS_VER(xe) < 12))
> >>>		return 0;
> >>>
> >>> +	if (IS_SRIOV_VF(xe))
> >>> +		return 0;
> >>> +
> >>
> >> Do we want to add XE_WARN_ON's
> >
> > Though maybe not, if we don't want to see the WARN_ON when loading the VF driver...
>
> yep, we don't want WARN on every VF boot
>
> and TBH IMO above XE_WARN_ON are all wrong, as:
>
> - why WARN if someone used legal param xe.force_execlist ?
>   we can rely on existing drm_dbg() when someone will attempt to use oa
>
> - why WARN if Xe driver by design does not support gen < 12 ?
>   if you want to be paranoid just add xe_assert() to document that

I agree, best to get rid of these WARN_ON's (xe_assert will also cause a
crash which we don't want). The ioctl's have drm_dbg's which should be
sufficient as you say. I just sent a patch to do this:

https://patchwork.freedesktop.org/series/135081/

Could you please take a look at this. Thanks.

>
> >
> >> or combine this if statement with the previous one?
> >>
> >> In any case:
> >>
> >> Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> thanks
>
> >>
> >>
> >>>	oa->xe = xe;
> >>>	oa->oa_formats = oa_formats;
> >>>
> >>> --
> >>> 2.43.0
> >>>