[PATCH i-g-t 1/2] lib/xe/xe_util: introduce helper for enabling ccs mode

[Kamil Konieczny]

Hi Manszewski,,
On 2025-01-17 at 14:18:53 +0100, Manszewski, Christoph wrote:
> On 17.01.2025 14:07, Manszewski, Christoph wrote:
> > Hi Dominik,
> > 
> > On 16.12.2024 14:11, Dominik Grzegorzek wrote:
> > > From: Andrzej Hajda <andrzej.hajda at intel.com>
> > > 
> > > Multiple tests will require enabled ccs mode. Add special helper to
> > > avoid code duplication.
> > > 
> > > Signed-off-by: Andrzej Hajda <andrzej.hajda at intel.com>
> > > ---
> > >   lib/xe/xe_util.c | 31 +++++++++++++++++++++++++++++++
> > >   lib/xe/xe_util.h |  1 +
> > >   2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/lib/xe/xe_util.c b/lib/xe/xe_util.c
> > > index 9482819c2..f7ae19f08 100644
> > > --- a/lib/xe/xe_util.c
> > > +++ b/lib/xe/xe_util.c
> > > @@ -302,3 +302,34 @@ int xe_gt_count_engines_by_class(int fd, int
> > > gt, int class)
> > >       return n;
> > >   }
> > > +
> > > +/**
> > > + * xe_sysfs_enable_ccs_mode:
> > > + * @fd: pointer to xe drm fd, can reopened multiple times
> > > + *
> > > + * Enables ccs_mode on all GTs, it must succeed for at least one GT.
> > > + * Since function requires the driver to be closed during ccs_mode
> > > change
> > > + * @fd will be closed then re-opened.
> > > + */
> > > +void xe_sysfs_enable_ccs_mode(int *fd)
> > > +{
> > > +    int gt, gt_fd, num_slices, ccs_mode, num_gt, enabled_count = 0;
> > > +
> > > +    num_gt = xe_number_gt(*fd);
> > > +
> > > +    for (gt = 0; gt < num_gt; gt++) {
> > > +        gt_fd = xe_sysfs_gt_open(*fd, gt);
> > > +        drm_close_driver(*fd);
> > 
> > Why not outside the loop?

imho this is a valid question, why not collect all gt_fd
and use them later, so drm_close_driver/open_driver will
be called only once? Second concern is that this lib function
assumes a caller have only one open drm fd, this also needs
to be documented.

> 
> Nevermind
> 
> > 
> > Thanks,
> > Christoph
> > 
> > > +
> > > +        if (!igt_debug_on(igt_sysfs_scanf(gt_fd, "num_cslices",
> > > "%u", &num_slices) <= 0) &&
> > > +            !igt_debug_on(igt_sysfs_printf(gt_fd, "ccs_mode", "%u",
> > > num_slices) <= 0) &&
> > > +            !igt_debug_on(igt_sysfs_scanf(gt_fd, "ccs_mode", "%u",
> > > &ccs_mode) <= 0) &&
> > > +            !igt_debug_on(num_slices != ccs_mode))
> > > +            enabled_count++;
> > > +        close(gt_fd);
> > > +        *fd = drm_open_driver(DRIVER_XE);
> > > +    }
> > > +
> > > +    if (!enabled_count)
> > > +        igt_require_f(0, "Cannot enable ccs mode for any GT\n");
> 
> Unless I am missing something (again):
> igt_require_f(enabled_count, "Cannot enable ccs mode for any GT\n")?

And this one is also tricky, it was ok in a test but in library imho
it should be avoided. Why not returning bitfield with bits sets for
each gt with ccs enabled? So in case of no css just return zero and
igt_require will be called in a test, not in a lib.

Also it should have a counterpart with _disable()

Regards,
Kamil

> 
> Reviewed-by: Chritoph Manszewski <christoph.manszewski at intel.com>
> 
> > > +}
> > > diff --git a/lib/xe/xe_util.h b/lib/xe/xe_util.h
> > > index b9fbfc5cd..a82c44a2a 100644
> > > --- a/lib/xe/xe_util.h
> > > +++ b/lib/xe/xe_util.h
> > > @@ -53,5 +53,6 @@ int xe_gt_fill_engines_by_class(int fd, int gt,
> > > int class,
> > >                   struct drm_xe_engine_class_instance eci[static
> > > XE_MAX_ENGINE_INSTANCE]);
> > >   int xe_gt_count_engines_by_class(int fd, int gt, int class);
> > > +void xe_sysfs_enable_ccs_mode(int *fd);
> > >   #endif /* XE_UTIL_H */