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