drm/amdgpu: add kernel DOC for ioctls in amdgpu_cs.c file

Submitted by Leo Liu on May 30, 2018, 6:42 p.m.

Details

Message ID 20180530184251.6314-1-leo.liu@amd.com
State New
Headers show
Series "drm/amdgpu: add kernel DOC for ioctls in amdgpu_cs.c file" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Leo Liu May 30, 2018, 6:42 p.m.
There are four ioctls in this files, and DOC gives details of
data structures for each of ioctls, and their functionalities.

Signed-off-by: Leo Liu <leo.liu@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 142 +++++++++++++++++++++++++
 1 file changed, 142 insertions(+)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index 12f0d18c6ee8..343ff115cff1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1217,6 +1217,49 @@  static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
 	return 0;
 }
 
+/**
+ * DOC:  amdgpu_cs_ioctl
+ *
+ * This ioctl processes user space command submission chunks,
+ * return a fence sequence number associated to a amdgpu fence.
+ *
+ * In data structure:
+ *
+ * __u32 ctx_id:
+ * Integer ID, created via DRM_AMDGPU_CTX ioctl call when user space
+ * command submission context created. It will be used as ID for later
+ * command submission context.
+ *
+ * __u32 bo_list_handle:
+ * Handle of resources list associated with this CS, created via
+ * DRM_AMDGPU_BO_LIST ioctl call before command submission, and
+ * the BOs in the list will be validated.
+ *
+ * __u32 num_chunks:
+ * Number of chunks, their types include:
+ * AMDGPU_CHUNK_ID_IB
+ * The data will be filled into IB buffer, and mappped to a HW ring.
+ *
+ * AMDGPU_CHUNK_ID_FENCE
+ * The data will be used to find user fence BO and its offset.
+ *
+ * AMDGPU_CHUNK_ID_DEPENDENCIES
+ * AMDGPU_CHUNK_ID_SYNCOBJ_IN
+ * AMDGPU_CHUNK_ID_SYNCOBJ_OUT
+ * These will be parsed as fence dependencies in given requirement,
+ * and will be remembered and to be synced later.
+ *
+ * __u32 _pad:
+ *
+ * __u64 chunks:
+ * Point to the CS chunks.
+ *
+ * amdgpu_cs_submit() function will be called to initialize a scheduler
+ * job, and associate it to a HW ring, add a new fence to the context,
+ * and then push the job to the queue for scheduler to process,
+ * it will return fence sequence number to user space.
+ *
+ */
 int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 {
 	struct amdgpu_device *adev = dev->dev_private;
@@ -1272,6 +1315,38 @@  int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
 	return r;
 }
 
+/**
+ * DOC:  amdgpu_cs_wait_ioctl
+ *
+ * This ioctl checks a user space fence associated to a amdgpu fence whether
+ * it's signaled or waits to be signaled till timeout with kernel dma fence.
+ * Once signaled, the associated CS is completed.
+ *
+ * In data structure:
+ *
+ * __u64 handle:
+ * The fence sequence number from amdgpu_cs_ioctl returned.
+ *
+ * __u64 timeout:
+ * Absolute timeout to wait.
+ *
+ * __u32 ip_type:
+ * __u32 ip_instance:
+ * __u32 ring:
+ * Map user space ring to a kernel HW ring, then use the seq(handle) to
+ * find the amdgpu fence, that will be checked and waited.
+ *
+ * __u32 ctx_id:
+ * ID for command submission context
+ *
+ * Out data:
+ *
+ * __u64 status:
+ * 0 CS completed
+ * 1 CS busy
+ *
+ */
+
 /**
  * amdgpu_cs_wait_ioctl - wait for a command submission to finish
  *
@@ -1358,6 +1433,37 @@  static struct dma_fence *amdgpu_cs_get_fence(struct amdgpu_device *adev,
 	return fence;
 }
 
+/**
+ * DOC: amdgpu_cs_fence_to_handle_ioctl
+ *
+ * This ioctl converts a user space fence into a fence object handle or fd,
+ * or file fd based on the purpose in “what”, since using handles or fd will
+ * be more efficient than ioctl call from user space to check signaled.
+ *
+ * In data structure:
+ *
+ * struct drm_amdgpu_fence fence:
+ * User space fence structured with ctx_id, ip_type, ip_instance, ring, seq_no
+ *
+ * __u32 what:
+ * Types of handle to convert include:
+ *
+ * AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ
+ * It is required to return a process-dependent handle for a sync object
+ *
+ * AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ_FD
+ * It is  required to return a process-independent fd for a sync object
+ *
+ * AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD
+ * It is required to return a process-independent fd to a sync file
+ *
+ * __u32 pad
+ *
+ * Out data:
+ * __u32 handle:
+ * Handle to be returned to user space
+ *
+ */
 int amdgpu_cs_fence_to_handle_ioctl(struct drm_device *dev, void *data,
 				    struct drm_file *filp)
 {
@@ -1524,6 +1630,42 @@  static int amdgpu_cs_wait_any_fence(struct amdgpu_device *adev,
 	return r;
 }
 
+/**
+ * DOC:  amdgpu_cs_wait_fences_ioctl
+ *
+ * This ioctl checks either all fences or any fence from multiple fences
+ * to be signaled or waits to be signaled till timeout. So it's used to
+ * check and wait multiple CS to be completed.
+ *
+ * In data structure:
+ *
+ * __u64 fences
+ * Point to the multiple fences
+ *
+ * __u32 fence_count
+ * number of fences
+ *
+ * __u32 wait_all
+ * ways to wait either wait_all or wait_any
+ *
+ * __u64 timeout_ns
+ * Absolute timeout to wait
+ *
+ * The function will extract user space fences based on pointer and counts,
+ * then mapping them amdgpu fences and check if they are signaled or wait
+ * to timeout.
+ *
+ * Out data:
+ *
+ *__u32 status
+ * 0 CS completed
+ * 1 CS busy
+ *
+ *__u32 first_signaled;
+ * First signaled fence index
+ *
+ */
+
 /**
  * amdgpu_cs_wait_fences_ioctl - wait for multiple command submissions to finish
  *

Comments

On Wed, May 30, 2018 at 2:42 PM, Leo Liu <leo.liu@amd.com> wrote:
> There are four ioctls in this files, and DOC gives details of
> data structures for each of ioctls, and their functionalities.
>
> Signed-off-by: Leo Liu <leo.liu@amd.com>

A few comments below about readability.  With those fixed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 142 +++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 12f0d18c6ee8..343ff115cff1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>         return 0;
>  }
>
> +/**
> + * DOC:  amdgpu_cs_ioctl
> + *
> + * This ioctl processes user space command submission chunks,

Maybe give an overview of why we use the ioctl.  Something like:
The CS (Command Submission) ioctl is used to submit command buffers to
the kernel for scheduling on hw queues.  The chunk interface gives us
the flexibility to add new features to the ioctl by simply adding a
new chunk type.

> + * return a fence sequence number associated to a amdgpu fence.
> + *
> + * In data structure:
> + *
> + * __u32 ctx_id:
> + * Integer ID, created via DRM_AMDGPU_CTX ioctl call when user space
> + * command submission context created. It will be used as ID for later
> + * command submission context.

Required for all command submissions.  The kernel uses this to track
dependencies and guilt for command submissions that lead to an engine
lock up.

> + *
> + * __u32 bo_list_handle:
> + * Handle of resources list associated with this CS, created via
> + * DRM_AMDGPU_BO_LIST ioctl call before command submission, and
> + * the BOs in the list will be validated.

to make sure they are resident when the command buffer executes.

> + *
> + * __u32 num_chunks:
> + * Number of chunks, their types include:
> + * AMDGPU_CHUNK_ID_IB
> + * The data will be filled into IB buffer, and mappped to a HW ring.
> + *

Expand this a bit.  E.g.,
An IB (Indirect Buffer) is a stand alone command buffer that can be
scheduled fetching for execution on a hw queue.


> + * AMDGPU_CHUNK_ID_FENCE
> + * The data will be used to find user fence BO and its offset.
> + *

Expand this a bit.  E.g., add something like:
Allows user space to specify their own fences within the command stream.

> + * AMDGPU_CHUNK_ID_DEPENDENCIES
> + * AMDGPU_CHUNK_ID_SYNCOBJ_IN
> + * AMDGPU_CHUNK_ID_SYNCOBJ_OUT
> + * These will be parsed as fence dependencies in given requirement,
> + * and will be remembered and to be synced later.
> + *
> + * __u32 _pad:
> + *
> + * __u64 chunks:
> + * Point to the CS chunks.

Pointer

> + *
> + * amdgpu_cs_submit() function will be called to initialize a scheduler
> + * job, and associate it to a HW ring, add a new fence to the context,

associate it with a

> + * and then push the job to the queue for scheduler to process,
> + * it will return fence sequence number to user space.
> + *
> + */
>  int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>  {
>         struct amdgpu_device *adev = dev->dev_private;
> @@ -1272,6 +1315,38 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>         return r;
>  }
>
> +/**
> + * DOC:  amdgpu_cs_wait_ioctl
> + *
> + * This ioctl checks a user space fence associated to a amdgpu fence whether
> + * it's signaled or waits to be signaled till timeout with kernel dma fence.

This reads sort of oddly.  how about:
The CS (Command Submission) wait ioctl waits for the specified fence
on the specified hw queue to signal or for the timeout, whichever
comes first.

> + * Once signaled, the associated CS is completed.
> + *
> + * In data structure:
> + *
> + * __u64 handle:
> + * The fence sequence number from amdgpu_cs_ioctl returned.
> + *
> + * __u64 timeout:
> + * Absolute timeout to wait.
> + *
> + * __u32 ip_type:
> + * __u32 ip_instance:
> + * __u32 ring:
> + * Map user space ring to a kernel HW ring, then use the seq(handle) to
> + * find the amdgpu fence, that will be checked and waited.
> + *
> + * __u32 ctx_id:
> + * ID for command submission context
> + *
> + * Out data:
> + *
> + * __u64 status:
> + * 0 CS completed
> + * 1 CS busy
> + *
> + */
> +
>  /**
>   * amdgpu_cs_wait_ioctl - wait for a command submission to finish
>   *
> @@ -1358,6 +1433,37 @@ static struct dma_fence *amdgpu_cs_get_fence(struct amdgpu_device *adev,
>         return fence;
>  }
>
> +/**
> + * DOC: amdgpu_cs_fence_to_handle_ioctl
> + *
> + * This ioctl converts a user space fence into a fence object handle or fd,
> + * or file fd based on the purpose in “what”, since using handles or fd will
> + * be more efficient than ioctl call from user space to check signaled.


This reads sort of oddly.  how about:
The CS (Command Submission) fence to handle ioctl converts a CS fence
into a drm sync object as specified in the what parameter.


> + *
> + * In data structure:
> + *
> + * struct drm_amdgpu_fence fence:
> + * User space fence structured with ctx_id, ip_type, ip_instance, ring, seq_no
> + *
> + * __u32 what:
> + * Types of handle to convert include:
> + *
> + * AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ
> + * It is required to return a process-dependent handle for a sync object
> + *
> + * AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ_FD
> + * It is  required to return a process-independent fd for a sync object
> + *
> + * AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD
> + * It is required to return a process-independent fd to a sync file
> + *
> + * __u32 pad
> + *
> + * Out data:
> + * __u32 handle:
> + * Handle to be returned to user space
> + *
> + */
>  int amdgpu_cs_fence_to_handle_ioctl(struct drm_device *dev, void *data,
>                                     struct drm_file *filp)
>  {
> @@ -1524,6 +1630,42 @@ static int amdgpu_cs_wait_any_fence(struct amdgpu_device *adev,
>         return r;
>  }
>
> +/**
> + * DOC:  amdgpu_cs_wait_fences_ioctl
> + *
> + * This ioctl checks either all fences or any fence from multiple fences
> + * to be signaled or waits to be signaled till timeout. So it's used to
> + * check and wait multiple CS to be completed.

This reads sort of oddly.  how about:
The CS (Command Submission) wait fences ioctl is an extended version
of the CS wait ioctl that allows the user to specify multiple fences
and wait for all or any of them to signal or for the timeout,
whichever comes first.


> + *
> + * In data structure:
> + *
> + * __u64 fences
> + * Point to the multiple fences

Pointer

> + *
> + * __u32 fence_count
> + * number of fences
> + *
> + * __u32 wait_all
> + * ways to wait either wait_all or wait_any
> + *
> + * __u64 timeout_ns
> + * Absolute timeout to wait
> + *
> + * The function will extract user space fences based on pointer and counts,
> + * then mapping them amdgpu fences and check if they are signaled or wait
> + * to timeout.


This reads sort of oddly.  how about:
This function extracts fences based on pointer and count specified and
waits for them to signal or timeout.


> + *
> + * Out data:
> + *
> + *__u32 status
> + * 0 CS completed
> + * 1 CS busy
> + *
> + *__u32 first_signaled;
> + * First signaled fence index
> + *
> + */
> +
>  /**
>   * amdgpu_cs_wait_fences_ioctl - wait for multiple command submissions to finish
>   *
> --
> 2.17.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 05/31/2018 11:43 AM, Alex Deucher wrote:
> On Wed, May 30, 2018 at 2:42 PM, Leo Liu <leo.liu@amd.com> wrote:
>> There are four ioctls in this files, and DOC gives details of
>> data structures for each of ioctls, and their functionalities.
>>
>> Signed-off-by: Leo Liu <leo.liu@amd.com>
> A few comments below about readability.  With those fixed:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thanks for the reviews, I will update it accordingly.

Leo


>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 142 +++++++++++++++++++++++++
>>   1 file changed, 142 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 12f0d18c6ee8..343ff115cff1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>          return 0;
>>   }
>>
>> +/**
>> + * DOC:  amdgpu_cs_ioctl
>> + *
>> + * This ioctl processes user space command submission chunks,
> Maybe give an overview of why we use the ioctl.  Something like:
> The CS (Command Submission) ioctl is used to submit command buffers to
> the kernel for scheduling on hw queues.  The chunk interface gives us
> the flexibility to add new features to the ioctl by simply adding a
> new chunk type.
>
>> + * return a fence sequence number associated to a amdgpu fence.
>> + *
>> + * In data structure:
>> + *
>> + * __u32 ctx_id:
>> + * Integer ID, created via DRM_AMDGPU_CTX ioctl call when user space
>> + * command submission context created. It will be used as ID for later
>> + * command submission context.
> Required for all command submissions.  The kernel uses this to track
> dependencies and guilt for command submissions that lead to an engine
> lock up.
>
>> + *
>> + * __u32 bo_list_handle:
>> + * Handle of resources list associated with this CS, created via
>> + * DRM_AMDGPU_BO_LIST ioctl call before command submission, and
>> + * the BOs in the list will be validated.
> to make sure they are resident when the command buffer executes.
>
>> + *
>> + * __u32 num_chunks:
>> + * Number of chunks, their types include:
>> + * AMDGPU_CHUNK_ID_IB
>> + * The data will be filled into IB buffer, and mappped to a HW ring.
>> + *
> Expand this a bit.  E.g.,
> An IB (Indirect Buffer) is a stand alone command buffer that can be
> scheduled fetching for execution on a hw queue.
>
>
>> + * AMDGPU_CHUNK_ID_FENCE
>> + * The data will be used to find user fence BO and its offset.
>> + *
> Expand this a bit.  E.g., add something like:
> Allows user space to specify their own fences within the command stream.
>
>> + * AMDGPU_CHUNK_ID_DEPENDENCIES
>> + * AMDGPU_CHUNK_ID_SYNCOBJ_IN
>> + * AMDGPU_CHUNK_ID_SYNCOBJ_OUT
>> + * These will be parsed as fence dependencies in given requirement,
>> + * and will be remembered and to be synced later.
>> + *
>> + * __u32 _pad:
>> + *
>> + * __u64 chunks:
>> + * Point to the CS chunks.
> Pointer
>
>> + *
>> + * amdgpu_cs_submit() function will be called to initialize a scheduler
>> + * job, and associate it to a HW ring, add a new fence to the context,
> associate it with a
>
>> + * and then push the job to the queue for scheduler to process,
>> + * it will return fence sequence number to user space.
>> + *
>> + */
>>   int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>   {
>>          struct amdgpu_device *adev = dev->dev_private;
>> @@ -1272,6 +1315,38 @@ int amdgpu_cs_ioctl(struct drm_device *dev, void *data, struct drm_file *filp)
>>          return r;
>>   }
>>
>> +/**
>> + * DOC:  amdgpu_cs_wait_ioctl
>> + *
>> + * This ioctl checks a user space fence associated to a amdgpu fence whether
>> + * it's signaled or waits to be signaled till timeout with kernel dma fence.
> This reads sort of oddly.  how about:
> The CS (Command Submission) wait ioctl waits for the specified fence
> on the specified hw queue to signal or for the timeout, whichever
> comes first.
>
>> + * Once signaled, the associated CS is completed.
>> + *
>> + * In data structure:
>> + *
>> + * __u64 handle:
>> + * The fence sequence number from amdgpu_cs_ioctl returned.
>> + *
>> + * __u64 timeout:
>> + * Absolute timeout to wait.
>> + *
>> + * __u32 ip_type:
>> + * __u32 ip_instance:
>> + * __u32 ring:
>> + * Map user space ring to a kernel HW ring, then use the seq(handle) to
>> + * find the amdgpu fence, that will be checked and waited.
>> + *
>> + * __u32 ctx_id:
>> + * ID for command submission context
>> + *
>> + * Out data:
>> + *
>> + * __u64 status:
>> + * 0 CS completed
>> + * 1 CS busy
>> + *
>> + */
>> +
>>   /**
>>    * amdgpu_cs_wait_ioctl - wait for a command submission to finish
>>    *
>> @@ -1358,6 +1433,37 @@ static struct dma_fence *amdgpu_cs_get_fence(struct amdgpu_device *adev,
>>          return fence;
>>   }
>>
>> +/**
>> + * DOC: amdgpu_cs_fence_to_handle_ioctl
>> + *
>> + * This ioctl converts a user space fence into a fence object handle or fd,
>> + * or file fd based on the purpose in “what”, since using handles or fd will
>> + * be more efficient than ioctl call from user space to check signaled.
>
> This reads sort of oddly.  how about:
> The CS (Command Submission) fence to handle ioctl converts a CS fence
> into a drm sync object as specified in the what parameter.
>
>
>> + *
>> + * In data structure:
>> + *
>> + * struct drm_amdgpu_fence fence:
>> + * User space fence structured with ctx_id, ip_type, ip_instance, ring, seq_no
>> + *
>> + * __u32 what:
>> + * Types of handle to convert include:
>> + *
>> + * AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ
>> + * It is required to return a process-dependent handle for a sync object
>> + *
>> + * AMDGPU_FENCE_TO_HANDLE_GET_SYNCOBJ_FD
>> + * It is  required to return a process-independent fd for a sync object
>> + *
>> + * AMDGPU_FENCE_TO_HANDLE_GET_SYNC_FILE_FD
>> + * It is required to return a process-independent fd to a sync file
>> + *
>> + * __u32 pad
>> + *
>> + * Out data:
>> + * __u32 handle:
>> + * Handle to be returned to user space
>> + *
>> + */
>>   int amdgpu_cs_fence_to_handle_ioctl(struct drm_device *dev, void *data,
>>                                      struct drm_file *filp)
>>   {
>> @@ -1524,6 +1630,42 @@ static int amdgpu_cs_wait_any_fence(struct amdgpu_device *adev,
>>          return r;
>>   }
>>
>> +/**
>> + * DOC:  amdgpu_cs_wait_fences_ioctl
>> + *
>> + * This ioctl checks either all fences or any fence from multiple fences
>> + * to be signaled or waits to be signaled till timeout. So it's used to
>> + * check and wait multiple CS to be completed.
> This reads sort of oddly.  how about:
> The CS (Command Submission) wait fences ioctl is an extended version
> of the CS wait ioctl that allows the user to specify multiple fences
> and wait for all or any of them to signal or for the timeout,
> whichever comes first.
>
>
>> + *
>> + * In data structure:
>> + *
>> + * __u64 fences
>> + * Point to the multiple fences
> Pointer
>
>> + *
>> + * __u32 fence_count
>> + * number of fences
>> + *
>> + * __u32 wait_all
>> + * ways to wait either wait_all or wait_any
>> + *
>> + * __u64 timeout_ns
>> + * Absolute timeout to wait
>> + *
>> + * The function will extract user space fences based on pointer and counts,
>> + * then mapping them amdgpu fences and check if they are signaled or wait
>> + * to timeout.
>
> This reads sort of oddly.  how about:
> This function extracts fences based on pointer and count specified and
> waits for them to signal or timeout.
>
>
>> + *
>> + * Out data:
>> + *
>> + *__u32 status
>> + * 0 CS completed
>> + * 1 CS busy
>> + *
>> + *__u32 first_signaled;
>> + * First signaled fence index
>> + *
>> + */
>> +
>>   /**
>>    * amdgpu_cs_wait_fences_ioctl - wait for multiple command submissions to finish
>>    *
>> --
>> 2.17.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2018-05-30 08:42 PM, Leo Liu wrote:
> There are four ioctls in this files, and DOC gives details of
> data structures for each of ioctls, and their functionalities.
> 
> Signed-off-by: Leo Liu <leo.liu@amd.com>

This isn't enough to actually make this part of the generated
documentation. It needs to be hooked up to a *.rst file for that.

I'm adding an amdgpu.rst file in
https://patchwork.freedesktop.org/series/44035/ , where you could hook
it up accordingly.

Please check that generating the documentation (e.g. with make htmldocs)
doesn't produce any warnings about amdgpu_cs.c, and that the result
looks good in Documentation/output/gpu/amdgpu.html . The documentation
format itself is documented in Documentation/output/doc-guide/index.html .


> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 12f0d18c6ee8..343ff115cff1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  	return 0;
>  }
>  
> +/**
> + * DOC:  amdgpu_cs_ioctl

DOC comments shouldn't be used for functions, see
Documentation/output/doc-guide/kernel-doc.html#function-documentation


> @@ -1524,6 +1630,42 @@ static int amdgpu_cs_wait_any_fence(struct amdgpu_device *adev,
>  	return r;
>  }
>  
> +/**
> + * DOC:  amdgpu_cs_wait_fences_ioctl
> + *
> + * This ioctl checks either all fences or any fence from multiple fences
> + * to be signaled or waits to be signaled till timeout. So it's used to
> + * check and wait multiple CS to be completed.
> + *
> + * In data structure:
> + *
> + * __u64 fences
> + * Point to the multiple fences
> + *
> + * __u32 fence_count
> + * number of fences
> + *
> + * __u32 wait_all
> + * ways to wait either wait_all or wait_any
> + *
> + * __u64 timeout_ns
> + * Absolute timeout to wait
> + *
> + * The function will extract user space fences based on pointer and counts,
> + * then mapping them amdgpu fences and check if they are signaled or wait
> + * to timeout.
> + *
> + * Out data:
> + *
> + *__u32 status
> + * 0 CS completed
> + * 1 CS busy
> + *
> + *__u32 first_signaled;
> + * First signaled fence index
> + *
> + */
> +
>  /**
>   * amdgpu_cs_wait_fences_ioctl - wait for multiple command submissions to finish
>   *

Any reason for not adding the above to the existing function
documentation comment here?
On 05/31/2018 12:30 PM, Michel Dänzer wrote:
> On 2018-05-30 08:42 PM, Leo Liu wrote:
>> There are four ioctls in this files, and DOC gives details of
>> data structures for each of ioctls, and their functionalities.
>>
>> Signed-off-by: Leo Liu <leo.liu@amd.com>
> This isn't enough to actually make this part of the generated
> documentation. It needs to be hooked up to a *.rst file for that.
>
> I'm adding an amdgpu.rst file in
> https://patchwork.freedesktop.org/series/44035/ , where you could hook
> it up accordingly.
>
> Please check that generating the documentation (e.g. with make htmldocs)
> doesn't produce any warnings about amdgpu_cs.c, and that the result
> looks good in Documentation/output/gpu/amdgpu.html . The documentation
> format itself is documented in Documentation/output/doc-guide/index.html .
>
I will take a look this.

>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index 12f0d18c6ee8..343ff115cff1 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>>   	return 0;
>>   }
>>   
>> +/**
>> + * DOC:  amdgpu_cs_ioctl
> DOC comments shouldn't be used for functions, see
> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>
This doc is not for the functions, it's like something in commit message 
about the details of in/out data, and what is the functionality for this 
ioctl.


>> @@ -1524,6 +1630,42 @@ static int amdgpu_cs_wait_any_fence(struct amdgpu_device *adev,
>>   	return r;
>>   }
>>   
>> +/**
>> + * DOC:  amdgpu_cs_wait_fences_ioctl
>> + *
>> + * This ioctl checks either all fences or any fence from multiple fences
>> + * to be signaled or waits to be signaled till timeout. So it's used to
>> + * check and wait multiple CS to be completed.
>> + *
>> + * In data structure:
>> + *
>> + * __u64 fences
>> + * Point to the multiple fences
>> + *
>> + * __u32 fence_count
>> + * number of fences
>> + *
>> + * __u32 wait_all
>> + * ways to wait either wait_all or wait_any
>> + *
>> + * __u64 timeout_ns
>> + * Absolute timeout to wait
>> + *
>> + * The function will extract user space fences based on pointer and counts,
>> + * then mapping them amdgpu fences and check if they are signaled or wait
>> + * to timeout.
>> + *
>> + * Out data:
>> + *
>> + *__u32 status
>> + * 0 CS completed
>> + * 1 CS busy
>> + *
>> + *__u32 first_signaled;
>> + * First signaled fence index
>> + *
>> + */
>> +
>>   /**
>>    * amdgpu_cs_wait_fences_ioctl - wait for multiple command submissions to finish
>>    *
> Any reason for not adding the above to the existing function
> documentation comment here?
Yes. It's used by close source Vulkan, where it might be lack of BO 
fence dependencies implementations like we do in Mesa.

Leo

>
>
On 05/31/2018 12:39 PM, Leo Liu wrote:
>
>
> On 05/31/2018 12:30 PM, Michel Dänzer wrote:
>> On 2018-05-30 08:42 PM, Leo Liu wrote:
>>> There are four ioctls in this files, and DOC gives details of
>>> data structures for each of ioctls, and their functionalities.
>>>
>>> Signed-off-by: Leo Liu <leo.liu@amd.com>
>> This isn't enough to actually make this part of the generated
>> documentation. It needs to be hooked up to a *.rst file for that.
>>
>> I'm adding an amdgpu.rst file in
>> https://patchwork.freedesktop.org/series/44035/ , where you could hook
>> it up accordingly.
>>
>> Please check that generating the documentation (e.g. with make htmldocs)
>> doesn't produce any warnings about amdgpu_cs.c, and that the result
>> looks good in Documentation/output/gpu/amdgpu.html . The documentation
>> format itself is documented in 
>> Documentation/output/doc-guide/index.html .
>>
> I will take a look this.
>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 12f0d18c6ee8..343ff115cff1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct 
>>> amdgpu_cs_parser *p,
>>>       return 0;
>>>   }
>>>   +/**
>>> + * DOC:  amdgpu_cs_ioctl
>> DOC comments shouldn't be used for functions, see
>> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>>
> This doc is not for the functions, it's like something in commit 
> message about the details of in/out data, and what is the 
> functionality for this ioctl.
>
>
>>> @@ -1524,6 +1630,42 @@ static int amdgpu_cs_wait_any_fence(struct 
>>> amdgpu_device *adev,
>>>       return r;
>>>   }
>>>   +/**
>>> + * DOC:  amdgpu_cs_wait_fences_ioctl
>>> + *
>>> + * This ioctl checks either all fences or any fence from multiple 
>>> fences
>>> + * to be signaled or waits to be signaled till timeout. So it's 
>>> used to
>>> + * check and wait multiple CS to be completed.
>>> + *
>>> + * In data structure:
>>> + *
>>> + * __u64 fences
>>> + * Point to the multiple fences
>>> + *
>>> + * __u32 fence_count
>>> + * number of fences
>>> + *
>>> + * __u32 wait_all
>>> + * ways to wait either wait_all or wait_any
>>> + *
>>> + * __u64 timeout_ns
>>> + * Absolute timeout to wait
>>> + *
>>> + * The function will extract user space fences based on pointer and 
>>> counts,
>>> + * then mapping them amdgpu fences and check if they are signaled 
>>> or wait
>>> + * to timeout.
>>> + *
>>> + * Out data:
>>> + *
>>> + *__u32 status
>>> + * 0 CS completed
>>> + * 1 CS busy
>>> + *
>>> + *__u32 first_signaled;
>>> + * First signaled fence index
>>> + *
>>> + */
>>> +
>>>   /**
>>>    * amdgpu_cs_wait_fences_ioctl - wait for multiple command 
>>> submissions to finish
>>>    *
>> Any reason for not adding the above to the existing function
>> documentation comment here?
The existing is the function document, I am adding kernel doc for this 
ioctl.

Leo

> Yes. It's used by close source Vulkan, where it might be lack of BO 
> fence dependencies implementations like we do in Mesa.
>
> Leo
>
>>
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On 2018-05-31 06:39 PM, Leo Liu wrote:
> On 05/31/2018 12:30 PM, Michel Dänzer wrote:
>> On 2018-05-30 08:42 PM, Leo Liu wrote:
> 
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> index 12f0d18c6ee8..343ff115cff1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct
>>> amdgpu_cs_parser *p,
>>>       return 0;
>>>   }
>>>   +/**
>>> + * DOC:  amdgpu_cs_ioctl
>> DOC comments shouldn't be used for functions, see
>> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>>
> This doc is not for the functions, it's like something in commit message
> about the details of in/out data, and what is the functionality for this
> ioctl.

Data structures should be documented in comments directly above their
definitions, per
Documentation/output/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
. What an ioctl does should be described in the function documentation
comment above its implementation, as described above.
On 05/31/2018 12:47 PM, Michel Dänzer wrote:
> On 2018-05-31 06:39 PM, Leo Liu wrote:
>> On 05/31/2018 12:30 PM, Michel Dänzer wrote:
>>> On 2018-05-30 08:42 PM, Leo Liu wrote:
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 12f0d18c6ee8..343ff115cff1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct
>>>> amdgpu_cs_parser *p,
>>>>        return 0;
>>>>    }
>>>>    +/**
>>>> + * DOC:  amdgpu_cs_ioctl
>>> DOC comments shouldn't be used for functions, see
>>> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>>>
>> This doc is not for the functions, it's like something in commit message
>> about the details of in/out data, and what is the functionality for this
>> ioctl.
> Data structures should be documented in comments directly above their
> definitions, per
> Documentation/output/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
> . What an ioctl does should be described in the function documentation
> comment above its implementation, as described above.
Then what do you think the kernel doc for ioctl should do?

Leo

>
>
On 2018-05-31 06:49 PM, Leo Liu wrote:
> On 05/31/2018 12:47 PM, Michel Dänzer wrote:
>> On 2018-05-31 06:39 PM, Leo Liu wrote:
>>> On 05/31/2018 12:30 PM, Michel Dänzer wrote:
>>>> On 2018-05-30 08:42 PM, Leo Liu wrote:
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> index 12f0d18c6ee8..343ff115cff1 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct
>>>>> amdgpu_cs_parser *p,
>>>>>        return 0;
>>>>>    }
>>>>>    +/**
>>>>> + * DOC:  amdgpu_cs_ioctl
>>>> DOC comments shouldn't be used for functions, see
>>>> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>>>>
>>> This doc is not for the functions, it's like something in commit message
>>> about the details of in/out data, and what is the functionality for this
>>> ioctl.
>> Data structures should be documented in comments directly above their
>> definitions, per
>> Documentation/output/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
>>
>> . What an ioctl does should be described in the function documentation
>> comment above its implementation, as described above.
> Then what do you think the kernel doc for ioctl should do?

The data structure should be documented in a comment above its
definition, and what the ioctl does should be documented in a comment
above the function implementing it. The source files need to be hooked
up to the amdgpu.rst file.

Does that answer your question?
On 05/31/2018 01:04 PM, Michel Dänzer wrote:
> On 2018-05-31 06:49 PM, Leo Liu wrote:
>> On 05/31/2018 12:47 PM, Michel Dänzer wrote:
>>> On 2018-05-31 06:39 PM, Leo Liu wrote:
>>>> On 05/31/2018 12:30 PM, Michel Dänzer wrote:
>>>>> On 2018-05-30 08:42 PM, Leo Liu wrote:
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> index 12f0d18c6ee8..343ff115cff1 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct
>>>>>> amdgpu_cs_parser *p,
>>>>>>         return 0;
>>>>>>     }
>>>>>>     +/**
>>>>>> + * DOC:  amdgpu_cs_ioctl
>>>>> DOC comments shouldn't be used for functions, see
>>>>> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>>>>>
>>>> This doc is not for the functions, it's like something in commit message
>>>> about the details of in/out data, and what is the functionality for this
>>>> ioctl.
>>> Data structures should be documented in comments directly above their
>>> definitions, per
>>> Documentation/output/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
>>>
>>> . What an ioctl does should be described in the function documentation
>>> comment above its implementation, as described above.
>> Then what do you think the kernel doc for ioctl should do?
> The data structure should be documented in a comment above its
> definition, and what the ioctl does should be documented in a comment
> above the function implementing it. The source files need to be hooked
> up to the amdgpu.rst file.
>
> Does that answer your question?
Sort of. It tells me what should not be in the kernel DOC. I will put 
those details about data and functionalities where they should be.
And focus on what the ioctl is used for for the kernel DOC section.

Thanks Michel, the information is helpful.

Leo

>
>
On 2018-05-31 08:02 PM, Leo Liu wrote:
> On 05/31/2018 01:04 PM, Michel Dänzer wrote:
>> On 2018-05-31 06:49 PM, Leo Liu wrote:
>>> On 05/31/2018 12:47 PM, Michel Dänzer wrote:
>>>> On 2018-05-31 06:39 PM, Leo Liu wrote:
>>>>> On 05/31/2018 12:30 PM, Michel Dänzer wrote:
>>>>>> On 2018-05-30 08:42 PM, Leo Liu wrote:
>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>>> index 12f0d18c6ee8..343ff115cff1 100644
>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>>>>> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct
>>>>>>> amdgpu_cs_parser *p,
>>>>>>>         return 0;
>>>>>>>     }
>>>>>>>     +/**
>>>>>>> + * DOC:  amdgpu_cs_ioctl
>>>>>> DOC comments shouldn't be used for functions, see
>>>>>> Documentation/output/doc-guide/kernel-doc.html#function-documentation
>>>>>>
>>>>> This doc is not for the functions, it's like something in commit
>>>>> message
>>>>> about the details of in/out data, and what is the functionality for
>>>>> this
>>>>> ioctl.
>>>> Data structures should be documented in comments directly above their
>>>> definitions, per
>>>> Documentation/output/doc-guide/kernel-doc.html#structure-union-and-enumeration-documentation
>>>>
>>>>
>>>> . What an ioctl does should be described in the function documentation
>>>> comment above its implementation, as described above.
>>> Then what do you think the kernel doc for ioctl should do?
>> The data structure should be documented in a comment above its
>> definition, and what the ioctl does should be documented in a comment
>> above the function implementing it. The source files need to be hooked
>> up to the amdgpu.rst file.
>>
>> Does that answer your question?
> Sort of. It tells me what should not be in the kernel DOC. I will put
> those details about data and functionalities where they should be.
> And focus on what the ioctl is used for for the kernel DOC section.

I'm afraid I don't see DOC comments being useful for documenting ioctls.
They're for general/overview kind of documentation. For ioctls, the data
structure and function documentation comments should be sufficient. You
can reference a data structure in a function documentation comment like
this:

/**
 * amdgpu_cs_ioctl - ...
 * @...: ...
 *
 * ...
 *
 * The userspace parameters for this ioctl are passed in via
 * &drm_amdgpu_cs.
 *
 * Returns:
 * ...
 */


See e.g. the documentation about i915_gem_get/set_tiling_ioctl in
drivers/gpu/drm/i915/i915_gem_tiling.c.
On 2018-05-30 08:42 PM, Leo Liu wrote:
> There are four ioctls in this files, and DOC gives details of
> data structures for each of ioctls, and their functionalities.
> 
> Signed-off-by: Leo Liu <leo.liu@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 142 +++++++++++++++++++++++++
>  1 file changed, 142 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index 12f0d18c6ee8..343ff115cff1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1217,6 +1217,49 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,
>  	return 0;
>  }
>  
> +/**
> + * DOC:  amdgpu_cs_ioctl
> + *
> + * This ioctl processes user space command submission chunks,
> + * return a fence sequence number associated to a amdgpu fence.
> + *
> + * In data structure:
> + *
> + * __u32 ctx_id:
> + * Integer ID, created via DRM_AMDGPU_CTX ioctl call when user space
> + * command submission context created. It will be used as ID for later
> + * command submission context.
> + *
> + * __u32 bo_list_handle:
> + * Handle of resources list associated with this CS, created via
> + * DRM_AMDGPU_BO_LIST ioctl call before command submission, and
> + * the BOs in the list will be validated.
> + *
> + * __u32 num_chunks:
> + * Number of chunks, their types include:
> + * AMDGPU_CHUNK_ID_IB
> + * The data will be filled into IB buffer, and mappped to a HW ring.
> + *
> + * AMDGPU_CHUNK_ID_FENCE
> + * The data will be used to find user fence BO and its offset.
> + *
> + * AMDGPU_CHUNK_ID_DEPENDENCIES
> + * AMDGPU_CHUNK_ID_SYNCOBJ_IN
> + * AMDGPU_CHUNK_ID_SYNCOBJ_OUT
> + * These will be parsed as fence dependencies in given requirement,
> + * and will be remembered and to be synced later.
> + *
> + * __u32 _pad:
> + *
> + * __u64 chunks:
> + * Point to the CS chunks.

BTW, this kind of formatting isn't preserved in the generated
documentation, so it probably won't look as intended.
On 2018-05-31 12:30 PM, Michel Dänzer wrote:
> On 2018-05-30 08:42 PM, Leo Liu wrote:
>> There are four ioctls in this files, and DOC gives details of
>> data structures for each of ioctls, and their functionalities.
>>
>> Signed-off-by: Leo Liu <leo.liu@amd.com>
> 
> This isn't enough to actually make this part of the generated
> documentation. It needs to be hooked up to a *.rst file for that.
> 
> I'm adding an amdgpu.rst file in
> https://patchwork.freedesktop.org/series/44035/ , where you could hook
> it up accordingly.
Michel, have you submitted this one? So we can hook up the files gradually.
I do not see the patches in this list.

Sam

> 
> Please check that generating the documentation (e.g. with make htmldocs)
> doesn't produce any warnings about amdgpu_cs.c, and that the result
> looks good in Documentation/output/gpu/amdgpu.html . The documentation
> format itself is documented in Documentation/output/doc-guide/index.html .
> 
> 


[...]
I can see them now.

Sam

> -----Original Message-----

> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf

> Of Samuel Li

> Sent: Friday, June 01, 2018 5:10 PM

> To: Michel Dänzer <michel@daenzer.net>; Liu, Leo <Leo.Liu@amd.com>

> Cc: amd-gfx@lists.freedesktop.org

> Subject: Re: [PATCH] drm/amdgpu: add kernel DOC for ioctls in amdgpu_cs.c

> file

> 

> 

> 

> On 2018-05-31 12:30 PM, Michel Dänzer wrote:

> > On 2018-05-30 08:42 PM, Leo Liu wrote:

> >> There are four ioctls in this files, and DOC gives details of data

> >> structures for each of ioctls, and their functionalities.

> >>

> >> Signed-off-by: Leo Liu <leo.liu@amd.com>

> >

> > This isn't enough to actually make this part of the generated

> > documentation. It needs to be hooked up to a *.rst file for that.

> >