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

[Rodrigo Vivi]

On Wed, Dec 18, 2024 at 10:03:51PM +0100, Michal Wajdeczko wrote:
> 
> 
> On 13.12.2024 21:13, Rodrigo Vivi wrote:
> > 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
> 
> no, they are just simple wrappers around SA types, but allows strict
> type checks to avoid mismatch with ordinary SA objects
> 
> > 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....
> 
> can't be static in .c since we need full definition externally
> 
> and IMO we shouldn't define them in .h either as our "rule" is to keep
> all type definitions in _types.h, since some types, xe_guc_buf_cache,
> will be referenced directly by other _types.h files (guc_types.h)

fair enough...

Acked-by: Rodrigo Vivi <rodrigo.vivi at intel.com>

> 
> > 
> >>
> >> 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
>