Re: [Intel-gfx] [PATCH 05/24] drm/i915/vma: fix struct i915_vma_bindinfo kernel-doc

[Jani Nikula <jani.nikula@intel.com>]

On Wed, 03 May 2023, Rodrigo Vivi <rodrigo.vivi@kernel.org> wrote:
> On Tue, May 02, 2023 at 06:37:22PM +0300, Jani Nikula wrote:
>> You can't document both a sub-struct type and a struct member at the
>> same time. Separate them.
>
> another way would be to kill the 'i915_vma_bindinfo' name entirely and
> document only as '@bi:' and then move the individual documentations near
> their definitions.

I don't think that will work, because AFAIK kernel-doc does not descend
into struct members recursively.

You can either declare and document the sub-structs as separate types
(the patch at hand), or you can document sub-struct members directly
from the embedding struct as shown below. I don't think the latter is
very nice.

BR,
Jani.


index 2053c037dbfb..ee767cc4de43 100644
--- a/drivers/gpu/drm/i915/i915_vma_resource.h
+++ b/drivers/gpu/drm/i915/i915_vma_resource.h
@@ -48,6 +48,12 @@ struct i915_page_sizes {
  * @rb: Rb node for the vm's pending unbind interval tree.
  * @__subtree_last: Interval tree private member.
  * @wakeref: wakeref.
+ * @bi.pages: The pages sg-table.
+ * @bi.page_sizes: Page sizes of the pages.
+ * @bi.pages_rsgt: Refcounted sg-table when delayed object destruction
+ * is supported. May be NULL.
+ * @bi.readonly: Whether the vma should be bound read-only.
+ * @bi.lmem: Whether the vma points to lmem.
  * @vm: non-refcounted pointer to the vm. This is for internal use only and
  * this member is cleared after vm_resource unbind.
  * @mr: The memory region of the object pointed to by the vma.
@@ -89,17 +95,11 @@ struct i915_vma_resource {
        intel_wakeref_t wakeref;
 
        /**
-        * struct i915_vma_bindinfo - Information needed for async bind
-        * only but that can be dropped after the bind has taken place.
-        * Consider making this a separate argument to the bind_vma
-        * op, coalescing with other arguments like vm, stash, cache_level
-        * and flags
-        * @pages: The pages sg-table.
-        * @page_sizes: Page sizes of the pages.
-        * @pages_rsgt: Refcounted sg-table when delayed object destruction
-        * is supported. May be NULL.
-        * @readonly: Whether the vma should be bound read-only.
-        * @lmem: Whether the vma points to lmem.
+        * @bi: Information needed for async bind only but that can be dropped
+        * after the bind has taken place.
+        *
+        * Consider making this a separate argument to the bind_vma op,
+        * coalescing with other arguments like vm, stash, cache_level and flags
         */
        struct i915_vma_bindinfo {
                struct sg_table *pages;


>
>> 
>> drivers/gpu/drm/i915/i915_vma_resource.h:91: warning: Incorrect use of kernel-doc format:          * struct i915_vma_bindinfo - Information needed for async bind
>> drivers/gpu/drm/i915/i915_vma_resource.h:129: warning: Function parameter or member 'bi' not described in 'i915_vma_resource'
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_vma_resource.h | 45 ++++++++++++++----------
>>  1 file changed, 27 insertions(+), 18 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_vma_resource.h b/drivers/gpu/drm/i915/i915_vma_resource.h
>> index 2053c037dbfb..ca2b0f7f59bc 100644
>> --- a/drivers/gpu/drm/i915/i915_vma_resource.h
>> +++ b/drivers/gpu/drm/i915/i915_vma_resource.h
>> @@ -33,6 +33,27 @@ struct i915_page_sizes {
>>  	unsigned int sg;
>>  };
>>  
>> +/**
>> + * struct i915_vma_bindinfo - Information needed for async bind
>> + * only but that can be dropped after the bind has taken place.
>> + * Consider making this a separate argument to the bind_vma
>> + * op, coalescing with other arguments like vm, stash, cache_level
>> + * and flags
>> + * @pages: The pages sg-table.
>> + * @page_sizes: Page sizes of the pages.
>> + * @pages_rsgt: Refcounted sg-table when delayed object destruction
>> + * is supported. May be NULL.
>> + * @readonly: Whether the vma should be bound read-only.
>> + * @lmem: Whether the vma points to lmem.
>> + */
>> +struct i915_vma_bindinfo {
>> +	struct sg_table *pages;
>> +	struct i915_page_sizes page_sizes;
>> +	struct i915_refct_sgt *pages_rsgt;
>> +	bool readonly:1;
>> +	bool lmem:1;
>
> btw, I believe we should move all the individual docs near to its
> declarations. easier to forget updating the documentation when updating
> fields here.
>
>> +};
>> +
>>  /**
>>   * struct i915_vma_resource - Snapshotted unbind information.
>>   * @unbind_fence: Fence to mark unbinding complete. Note that this fence
>> @@ -89,25 +110,13 @@ struct i915_vma_resource {
>>  	intel_wakeref_t wakeref;
>>  
>>  	/**
>> -	 * struct i915_vma_bindinfo - Information needed for async bind
>> -	 * only but that can be dropped after the bind has taken place.
>> -	 * Consider making this a separate argument to the bind_vma
>> -	 * op, coalescing with other arguments like vm, stash, cache_level
>> -	 * and flags
>> -	 * @pages: The pages sg-table.
>> -	 * @page_sizes: Page sizes of the pages.
>> -	 * @pages_rsgt: Refcounted sg-table when delayed object destruction
>> -	 * is supported. May be NULL.
>> -	 * @readonly: Whether the vma should be bound read-only.
>> -	 * @lmem: Whether the vma points to lmem.
>> +	 * @bi: Information needed for async bind only but that can be dropped
>> +	 * after the bind has taken place.
>> +	 *
>> +	 * Consider making this a separate argument to the bind_vma op,
>> +	 * coalescing with other arguments like vm, stash, cache_level and flags
>>  	 */
>> -	struct i915_vma_bindinfo {
>> -		struct sg_table *pages;
>> -		struct i915_page_sizes page_sizes;
>> -		struct i915_refct_sgt *pages_rsgt;
>> -		bool readonly:1;
>> -		bool lmem:1;
>> -	} bi;
>> +	struct i915_vma_bindinfo bi;
>>  
>>  #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
>>  	struct intel_memory_region *mr;
>> -- 
>> 2.39.2
>> 

-- 
Jani Nikula, Intel Open Source Graphics Center