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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 04 May 2023, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxxx> wrote:
> On Thu, May 04, 2023 at 12:43:05PM +0300, Jani Nikula wrote:
>> On Wed, 03 May 2023, Rodrigo Vivi <rodrigo.vivi@xxxxxxxxxx> 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.
>
> It does:
>
> 'In-line member documentation comments' definition of:
> Documentation/doc-guide/kernel-doc.rst

Regardless of what that says, I can't make it work. :(

$ scripts/kernel-doc -function i915_vma_resource drivers/gpu/drm/i915/i915_vma_resource.h

only documents ``bi`` but not the sub-struct. Regardless of whether I
refer to the members as @bi.pages: or @pages: etc.

Let me know if and how you can make it work.

BR,
Jani.

>
>> 
>> 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.
>
> what I had in mind was something more like:
>
>         /**
> -        * 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 {
> +               /** @pages: The pages sg-table. */
>                 struct sg_table *pages;
> +               /** @page_sizes: Page sizes of the pages. */
>                 struct i915_page_sizes page_sizes;
> +               /**
> +                * @pages_rsgt: Refcounted sg-table when delayed object
> +                *              destruction is supported. May be NULL.
> +                */
>                 struct i915_refct_sgt *pages_rsgt;
> +               /** @readonly: Whether the vma should be bound read-only. */
>                 bool readonly:1;
> +               /** @lmem: Whether the vma points to lmem. */
>                 bool lmem:1;
>         } bi;
>
>
> But let's not block the progress of the much needed fixes. It's your call:
>
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
>
>> 
>> 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@xxxxxxxxx>
>> >> ---
>> >>  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

-- 
Jani Nikula, Intel Open Source Graphics Center



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux