Re: [PATCH v4 4/6] drm/xe/configfs: Allow to select by class only

[Raag Jadav <raag.jadav@intel.com>]

On Fri, Sep 12, 2025 at 12:30:03AM -0500, Lucas De Marchi wrote:
> On Fri, Sep 12, 2025 at 07:18:37AM +0200, Raag Jadav wrote:
> > On Thu, Sep 11, 2025 at 12:36:28PM -0700, Lucas De Marchi wrote:
> > > For a future configfs attribute, it's desirable to select by engine mask
> > > only as the instance doesn't make sense.
> > > 
> > > Rename the function lookup_engine_mask() to lookup_engine_info() and
> > > make it return the entry. This allows parse_engine() to still return an
> > > item if the caller wants to allow parsing a class-only string like
> > > "rcs", "bcs", "ccs", etc.
> > > 
> > > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Reviewed-by: Raag Jadav <raag.jadav@intel.com>
> > > ---
> > > v2:
> > > - Rename function to lookup_engine_info() and return the entry
> > >   directly instead of the index (Raag Jadav)
> > > - Add named initializer for new entry for consistency (Raag Jadav)
> > > ---
> > >  drivers/gpu/drm/xe/xe_configfs.c | 50 ++++++++++++++++++++++++++++------------
> > >  1 file changed, 35 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_configfs.c b/drivers/gpu/drm/xe/xe_configfs.c
> > > index 8db7a2af11011..45fdb6cf26b5d 100644
> > > --- a/drivers/gpu/drm/xe/xe_configfs.c
> > > +++ b/drivers/gpu/drm/xe/xe_configfs.c
> > > @@ -152,6 +152,7 @@ static void set_device_defaults(struct xe_config_device *config)
> > >  struct engine_info {
> > >  	const char *cls;
> > >  	u64 mask;
> > > +	enum xe_engine_class engine_class;
> > >  };
> > > 
> > >  /* Some helpful macros to aid on the sizing of buffer allocation when parsing */
> > > @@ -159,12 +160,12 @@ struct engine_info {
> > >  #define MAX_ENGINE_INSTANCE_CHARS 2
> > > 
> > >  static const struct engine_info engine_info[] = {
> > > -	{ .cls = "rcs", .mask = XE_HW_ENGINE_RCS_MASK },
> > > -	{ .cls = "bcs", .mask = XE_HW_ENGINE_BCS_MASK },
> > > -	{ .cls = "vcs", .mask = XE_HW_ENGINE_VCS_MASK },
> > > -	{ .cls = "vecs", .mask = XE_HW_ENGINE_VECS_MASK },
> > > -	{ .cls = "ccs", .mask = XE_HW_ENGINE_CCS_MASK },
> > > -	{ .cls = "gsccs", .mask = XE_HW_ENGINE_GSCCS_MASK },
> > > +	{ .cls = "rcs", .mask = XE_HW_ENGINE_RCS_MASK, .engine_class = XE_ENGINE_CLASS_RENDER },
> > > +	{ .cls = "bcs", .mask = XE_HW_ENGINE_BCS_MASK, .engine_class = XE_ENGINE_CLASS_COPY },
> > > +	{ .cls = "vcs", .mask = XE_HW_ENGINE_VCS_MASK, .engine_class = XE_ENGINE_CLASS_VIDEO_DECODE },
> > > +	{ .cls = "vecs", .mask = XE_HW_ENGINE_VECS_MASK, .engine_class = XE_ENGINE_CLASS_VIDEO_ENHANCE },
> > 
> > Hmm, so now checkpatch doesn't feel so good about this.
> > I usually try to shorten the variable/macro names instead of wrapping,
> > but I guess a few more lines in an already huge driver wouldn't be much
> > noticeable ;)
> 
> it's a table and wrapping the line doesn't really improve readability.
> In this case we should taked checkpatch output with a grain of salt

Ofcourse. I had the same understanding, but I've always managed to find
myself coming across random DRM rules :D

Raag