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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

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)

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