Re: [PATCH 10/13] drm/xe/guc: Introduce the GuC Buffer Cache

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Fri, Dec 13, 2024 at 10:38:05AM -0800, Matthew Brost wrote:
> On Thu, Dec 12, 2024 at 10:48:34PM +0100, Michal Wajdeczko wrote:
> > 
> > 
> > On 12.12.2024 04:30, Matthew Brost wrote:
> > > On Thu, Dec 12, 2024 at 02:01:38AM +0100, Michal Wajdeczko wrote:
> > 
> > ...
> > 
> > >> diff --git a/drivers/gpu/drm/xe/xe_guc_buf_types.h b/drivers/gpu/drm/xe/xe_guc_buf_types.h
> > >> new file mode 100644
> > >> index 000000000000..9e123d71c064
> > >> --- /dev/null
> > >> +++ b/drivers/gpu/drm/xe/xe_guc_buf_types.h
> > >> @@ -0,0 +1,28 @@
> > >> +/* SPDX-License-Identifier: MIT */
> > >> +/*
> > >> + * Copyright © 2024 Intel Corporation
> > >> + */
> > >> +
> > >> +#ifndef _XE_GUC_BUF_TYPES_H_
> > >> +#define _XE_GUC_BUF_TYPES_H_
> > >> +
> > >> +struct drm_suballoc;
> > >> +struct xe_sa_manager;
> > >> +
> > >> +/**
> > >> + * struct xe_guc_buf_cache - GuC Data Buffer Cache.
> > >> + */
> > >> +struct xe_guc_buf_cache {
> > >> +	/* private: internal sub-allocation manager */
> > > 
> > > I think this generate kerenl doc complaints.
> > > 
> > 
> > or maybe not, see [1] which says:
> > 
> > "Inside a struct or union description, you can use the private: and
> > public: comment tags. Structure fields that are inside a private: area
> > are not listed in the generated output documentation."
> > 
> > and CI.hooks is also fine with it
> > 
> > [1] https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#members
> > 
> 
> Ah, ok. However AFIAK we don't use this style anywhere in Xe so maybe
> double check with the maintainers on there preference.

As long as it is in-line member doc comment we should be okay.

But do you intend to expand these 2 structs later? It is kind of strange
that they are exported in a _types.h with only 'private' members. In
the current way it doesn't look like we need this _types.h at all and
only static struct in .h or .c itself....

> 
> Matt 
> 
> > > i.e. Should be:
> > > 
> > > /* @sam: private - internal sub-allocation manager */
> > > 
> > >> +	struct xe_sa_manager *sam;
> > >> +};
> > >> +
> > >> +/**
> > >> + * struct xe_guc_buf - GuC Data Buffer Reference.
> > >> + */
> > >> +struct xe_guc_buf {
> > >> +	/* private: internal sub-allocation reference */
> > > 
> > > Same here.
> > > 
> > > Matt
> > > 
> > >> +	struct drm_suballoc *sa;
> > >> +};
> > >> +
> > >> +#endif