Re: [Intel-gfx] [PATCH 05/24] drm/i915/vma: fix struct i915_vma_bindinfo kernel-doc
On Thu, 04 May 2023, Rodrigo Vivi wrote: > But let's not block the progress of the much needed fixes. It's your call: > > Reviewed-by: Rodrigo Vivi Thanks, pushed to drm-intel-gt-next as-is. BR, Jani. -- Jani Nikula, Intel Open Source Graphics Center
Re: [Intel-gfx] [PATCH 05/24] drm/i915/vma: fix struct i915_vma_bindinfo kernel-doc
On Thu, 04 May 2023, Rodrigo Vivi wrote:
> On Thu, May 04, 2023 at 12:43:05PM +0300, Jani Nikula wrote:
>> On Wed, 03 May 2023, Rodrigo Vivi 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
>
>>
>> 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
Re: [Intel-gfx] [PATCH 05/24] drm/i915/vma: fix struct i915_vma_bindinfo kernel-doc
On Thu, May 04, 2023 at 12:43:05PM +0300, Jani Nikula wrote:
> On Wed, 03 May 2023, Rodrigo Vivi 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
>
> 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
>
> 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
Re: [Intel-gfx] [PATCH 05/24] drm/i915/vma: fix struct i915_vma_bindinfo kernel-doc
On Wed, 03 May 2023, Rodrigo Vivi 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
>> ---
>> 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.
>> -
Re: [Intel-gfx] [PATCH 05/24] drm/i915/vma: fix struct i915_vma_bindinfo kernel-doc
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.
>
> 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
> ---
> 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
>
