Re: [PATCH 1/1] drm/amdgpu: add kernel doc for amdgpu_object.c

2018-05-18 Thread Samuel Li 


On 2018-05-18 02:54 PM, Alex Deucher wrote:
> On Thu, May 17, 2018 at 6:20 PM, Samuel Li  wrote:
>> Signed-off-by: Samuel Li 
> 
> Please also add a separate DOC section at the top of this file giving
> a brief overview of how amdgpu bo API works.  E.g., how you would
> allocate, free, kmap, etc.  What pinning means and why you would need
> to do it, etc.  What domains we support and what they are.  What
> shadow bos are.
OK. For domains, I can create a separate change to add to "amdgpu_drm.h", in 
which they were defined.
For pinning/shadow bo, I can add it before the respective functions.

Sam


> 
> Additional comments below for things to add or clarify to enhance the
> documentation.
> 
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 233 
>> +
>>  1 file changed, 233 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> index 6a9e46a..271dbfa 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
>> @@ -73,6 +73,15 @@ static void amdgpu_ttm_bo_destroy(struct 
>> ttm_buffer_object *tbo)
>> kfree(bo);
>>  }
>>
>> +/**
>> + * amdgpu_ttm_bo_is_amdgpu_bo - if the buffer object belongs to an 
>> _bo
>> + * @bo: buffer object to be checked
>> + *
>> + * Uses destroy function associated with the object to determine if this is
>> + * _bo.
>> + *
>> + * Returns true if the object belongs to _bo, false if not.
>> + */
> 
> Just to clarify, it checks whether a ttm bo is an amdgpu buffer object or not.
> 
>>  bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object *bo)
>>  {
>> if (bo->destroy == _ttm_bo_destroy)
>> @@ -80,6 +89,14 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object 
>> *bo)
>> return false;
>>  }
>>
>> +/**
>> + * amdgpu_ttm_placement_from_domain - set buffer's placement
>> + * @abo: _bo buffer object whose placement is to be set
>> + * @domain: requested domain
>> + *
>> + * Sets buffer's placement according to requested domain and the buffer's
>> + * flags.
>> + */
>>  void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
>>  {
>> struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
>> @@ -498,6 +515,17 @@ static int amdgpu_bo_create_shadow(struct amdgpu_device 
>> *adev,
>> return r;
>>  }
>>
>> +/**
>> + * amdgpu_bo_create - create an _bo buffer object
>> + * @adev: amdgpu device object
>> + * @bp: parameters to be used for the buffer object
>> + * @bo_ptr: pointer to the buffer object pointer
>> + *
>> + * Creates an _bo buffer object; and if requested, also creates a
>> + * shadow object.
>> + *
>> + * Returns 0 for success or a negative error code on failure.
>> + */
>>  int amdgpu_bo_create(struct amdgpu_device *adev,
>>  struct amdgpu_bo_param *bp,
>>  struct amdgpu_bo **bo_ptr)
>> @@ -527,6 +555,20 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
>> return r;
>>  }
>>
>> +/**
>> + * amdgpu_bo_backup_to_shadow - Backs up an _bo buffer object
>> + * @adev: amdgpu device object
>> + * @ring: amdgpu_ring for the engine handling the buffer operations
>> + * @bo: _bo buffer to be backed up
>> + * @resv: reservation object with embedded fence
>> + * @fence: dma_fence associated with the operation
>> + * @direct: whether to submit the job directly
>> + *
>> + * Copies an _bo buffer object to its shadow object.
>> + * Not used for now.
>> + *
>> + * Returns 0 for success or a negative error code on failure.
>> + */
>>  int amdgpu_bo_backup_to_shadow(struct amdgpu_device *adev,
>>struct amdgpu_ring *ring,
>>struct amdgpu_bo *bo,
>> @@ -559,6 +601,15 @@ int amdgpu_bo_backup_to_shadow(struct amdgpu_device 
>> *adev,
>> return r;
>>  }
>>
>> +/**
>> + * amdgpu_bo_validate - validate an _bo buffer object
>> + * @bo: pointer to the buffer object
>> + *
>> + * Sets placement according to domain; and changes placement and caching
>> + * policy of the buffer object according to the placement.
>> + *
>> + * Returns 0 for success or a negative error code on failure.
>> + */
> 
> This is used for validating shadow bos.  Also worth mentioning that it
> calls ttm_bo_validate() which will make sure the buffer is resident
> where it needs to be.
> 
>>  int amdgpu_bo_validate(struct amdgpu_bo *bo)
>>  {
>> struct ttm_operation_ctx ctx = { false, false };
>> @@ -581,6 +632,19 @@ int amdgpu_bo_validate(struct amdgpu_bo *bo)
>> return r;
>>  }
>>
>> +/**
>> + * amdgpu_bo_restore_from_shadow - restore an _bo buffer object
>> + * @adev: amdgpu device object
>> + * @ring: amdgpu_ring for the engine handling the buffer operations
>> + * @bo: _bo buffer to be restored
>> + * @resv: reservation object with embedded fence
>> + * @fence: dma_fence associated with the operation
>> + * @direct: whether to submit

Re: [PATCH 1/1] drm/amdgpu: add kernel doc for amdgpu_object.c

2018-05-18 Thread Alex Deucher 
On Thu, May 17, 2018 at 6:20 PM, Samuel Li  wrote:
> Signed-off-by: Samuel Li 

Please also add a separate DOC section at the top of this file giving
a brief overview of how amdgpu bo API works.  E.g., how you would
allocate, free, kmap, etc.  What pinning means and why you would need
to do it, etc.  What domains we support and what they are.  What
shadow bos are.

Additional comments below for things to add or clarify to enhance the
documentation.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 233 
> +
>  1 file changed, 233 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6a9e46a..271dbfa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -73,6 +73,15 @@ static void amdgpu_ttm_bo_destroy(struct ttm_buffer_object 
> *tbo)
> kfree(bo);
>  }
>
> +/**
> + * amdgpu_ttm_bo_is_amdgpu_bo - if the buffer object belongs to an _bo
> + * @bo: buffer object to be checked
> + *
> + * Uses destroy function associated with the object to determine if this is
> + * _bo.
> + *
> + * Returns true if the object belongs to _bo, false if not.
> + */

Just to clarify, it checks whether a ttm bo is an amdgpu buffer object or not.

>  bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object *bo)
>  {
> if (bo->destroy == _ttm_bo_destroy)
> @@ -80,6 +89,14 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object 
> *bo)
> return false;
>  }
>
> +/**
> + * amdgpu_ttm_placement_from_domain - set buffer's placement
> + * @abo: _bo buffer object whose placement is to be set
> + * @domain: requested domain
> + *
> + * Sets buffer's placement according to requested domain and the buffer's
> + * flags.
> + */
>  void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
>  {
> struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
> @@ -498,6 +515,17 @@ static int amdgpu_bo_create_shadow(struct amdgpu_device 
> *adev,
> return r;
>  }
>
> +/**
> + * amdgpu_bo_create - create an _bo buffer object
> + * @adev: amdgpu device object
> + * @bp: parameters to be used for the buffer object
> + * @bo_ptr: pointer to the buffer object pointer
> + *
> + * Creates an _bo buffer object; and if requested, also creates a
> + * shadow object.
> + *
> + * Returns 0 for success or a negative error code on failure.
> + */
>  int amdgpu_bo_create(struct amdgpu_device *adev,
>  struct amdgpu_bo_param *bp,
>  struct amdgpu_bo **bo_ptr)
> @@ -527,6 +555,20 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
> return r;
>  }
>
> +/**
> + * amdgpu_bo_backup_to_shadow - Backs up an _bo buffer object
> + * @adev: amdgpu device object
> + * @ring: amdgpu_ring for the engine handling the buffer operations
> + * @bo: _bo buffer to be backed up
> + * @resv: reservation object with embedded fence
> + * @fence: dma_fence associated with the operation
> + * @direct: whether to submit the job directly
> + *
> + * Copies an _bo buffer object to its shadow object.
> + * Not used for now.
> + *
> + * Returns 0 for success or a negative error code on failure.
> + */
>  int amdgpu_bo_backup_to_shadow(struct amdgpu_device *adev,
>struct amdgpu_ring *ring,
>struct amdgpu_bo *bo,
> @@ -559,6 +601,15 @@ int amdgpu_bo_backup_to_shadow(struct amdgpu_device 
> *adev,
> return r;
>  }
>
> +/**
> + * amdgpu_bo_validate - validate an _bo buffer object
> + * @bo: pointer to the buffer object
> + *
> + * Sets placement according to domain; and changes placement and caching
> + * policy of the buffer object according to the placement.
> + *
> + * Returns 0 for success or a negative error code on failure.
> + */

This is used for validating shadow bos.  Also worth mentioning that it
calls ttm_bo_validate() which will make sure the buffer is resident
where it needs to be.

>  int amdgpu_bo_validate(struct amdgpu_bo *bo)
>  {
> struct ttm_operation_ctx ctx = { false, false };
> @@ -581,6 +632,19 @@ int amdgpu_bo_validate(struct amdgpu_bo *bo)
> return r;
>  }
>
> +/**
> + * amdgpu_bo_restore_from_shadow - restore an _bo buffer object
> + * @adev: amdgpu device object
> + * @ring: amdgpu_ring for the engine handling the buffer operations
> + * @bo: _bo buffer to be restored
> + * @resv: reservation object with embedded fence
> + * @fence: dma_fence associated with the operation
> + * @direct: whether to submit the job directly
> + *
> + * Copies a buffer object's shadow content back to the object.
> + *

Mention that this is used for recovering a buffer from it's shadow in
the case of a gpu reset where vram context may be lost.


> + * Returns 0 for success or a negative error code on failure.
> + */
>  int amdgpu_bo_restore_from_shadow(struct amdgpu_device *adev,
>

[PATCH 1/1] drm/amdgpu: add kernel doc for amdgpu_object.c

2018-05-17 Thread Samuel Li 
Signed-off-by: Samuel Li 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 233 +
 1 file changed, 233 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 6a9e46a..271dbfa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -73,6 +73,15 @@ static void amdgpu_ttm_bo_destroy(struct ttm_buffer_object 
*tbo)
kfree(bo);
 }
 
+/**
+ * amdgpu_ttm_bo_is_amdgpu_bo - if the buffer object belongs to an _bo
+ * @bo: buffer object to be checked
+ *
+ * Uses destroy function associated with the object to determine if this is
+ * _bo.
+ *
+ * Returns true if the object belongs to _bo, false if not.
+ */
 bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object *bo)
 {
if (bo->destroy == _ttm_bo_destroy)
@@ -80,6 +89,14 @@ bool amdgpu_ttm_bo_is_amdgpu_bo(struct ttm_buffer_object *bo)
return false;
 }
 
+/**
+ * amdgpu_ttm_placement_from_domain - set buffer's placement
+ * @abo: _bo buffer object whose placement is to be set
+ * @domain: requested domain
+ *
+ * Sets buffer's placement according to requested domain and the buffer's
+ * flags.
+ */
 void amdgpu_ttm_placement_from_domain(struct amdgpu_bo *abo, u32 domain)
 {
struct amdgpu_device *adev = amdgpu_ttm_adev(abo->tbo.bdev);
@@ -498,6 +515,17 @@ static int amdgpu_bo_create_shadow(struct amdgpu_device 
*adev,
return r;
 }
 
+/**
+ * amdgpu_bo_create - create an _bo buffer object
+ * @adev: amdgpu device object
+ * @bp: parameters to be used for the buffer object
+ * @bo_ptr: pointer to the buffer object pointer
+ *
+ * Creates an _bo buffer object; and if requested, also creates a
+ * shadow object.
+ *
+ * Returns 0 for success or a negative error code on failure.
+ */
 int amdgpu_bo_create(struct amdgpu_device *adev,
 struct amdgpu_bo_param *bp,
 struct amdgpu_bo **bo_ptr)
@@ -527,6 +555,20 @@ int amdgpu_bo_create(struct amdgpu_device *adev,
return r;
 }
 
+/**
+ * amdgpu_bo_backup_to_shadow - Backs up an _bo buffer object
+ * @adev: amdgpu device object
+ * @ring: amdgpu_ring for the engine handling the buffer operations
+ * @bo: _bo buffer to be backed up
+ * @resv: reservation object with embedded fence
+ * @fence: dma_fence associated with the operation
+ * @direct: whether to submit the job directly
+ *
+ * Copies an _bo buffer object to its shadow object.
+ * Not used for now.
+ *
+ * Returns 0 for success or a negative error code on failure.
+ */
 int amdgpu_bo_backup_to_shadow(struct amdgpu_device *adev,
   struct amdgpu_ring *ring,
   struct amdgpu_bo *bo,
@@ -559,6 +601,15 @@ int amdgpu_bo_backup_to_shadow(struct amdgpu_device *adev,
return r;
 }
 
+/**
+ * amdgpu_bo_validate - validate an _bo buffer object
+ * @bo: pointer to the buffer object
+ *
+ * Sets placement according to domain; and changes placement and caching
+ * policy of the buffer object according to the placement.
+ *
+ * Returns 0 for success or a negative error code on failure.
+ */
 int amdgpu_bo_validate(struct amdgpu_bo *bo)
 {
struct ttm_operation_ctx ctx = { false, false };
@@ -581,6 +632,19 @@ int amdgpu_bo_validate(struct amdgpu_bo *bo)
return r;
 }
 
+/**
+ * amdgpu_bo_restore_from_shadow - restore an _bo buffer object
+ * @adev: amdgpu device object
+ * @ring: amdgpu_ring for the engine handling the buffer operations
+ * @bo: _bo buffer to be restored
+ * @resv: reservation object with embedded fence
+ * @fence: dma_fence associated with the operation
+ * @direct: whether to submit the job directly
+ *
+ * Copies a buffer object's shadow content back to the object.
+ *
+ * Returns 0 for success or a negative error code on failure.
+ */
 int amdgpu_bo_restore_from_shadow(struct amdgpu_device *adev,
  struct amdgpu_ring *ring,
  struct amdgpu_bo *bo,
@@ -613,6 +677,16 @@ int amdgpu_bo_restore_from_shadow(struct amdgpu_device 
*adev,
return r;
 }
 
+/**
+ * amdgpu_bo_kmap - map an _bo buffer object
+ * @bo: _bo buffer object to be mapped
+ * @ptr: kernel virtual address to be returned
+ *
+ * Calls ttm_bo_kmap() to set up the kernel virtual mapping; calls
+ * amdgpu_bo_kptr() to get the kernel virtual address.
+ *
+ * Returns 0 for success or a negative error code on failure.
+ */
 int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr)
 {
void *kptr;
@@ -643,6 +717,14 @@ int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr)
return 0;
 }
 
+/**
+ * amdgpu_bo_kptr - returns a kernel virtual address of the buffer object
+ * @bo: _bo buffer object
+ *
+ * Calls ttm_kmap_obj_virtual() to get the kernel virtual address
+ *
+ * Returns the virtual address of a buffer object area.
+ */
 void *amdgpu_bo_kptr(struct amdgpu_bo *bo)
 {
bool is_iomem;
@@ -650,12