Re: [PATCH 05/10] drm/radeon: document radeon_fence.c

["Christian König" <deathsimple@vodafone.de>]

Some minor comments spread over the whole file, see below:

On 29.06.2012 16:28, alexdeucher@gmail.com wrote:
> From: Alex Deucher <alexander.deucher@amd.com>
>
> Adds documentation to most of the functions in
> radeon_fence.c
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>   drivers/gpu/drm/radeon/radeon_fence.c |  373 +++++++++++++++++++++++++++++++++
>   1 files changed, 373 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c b/drivers/gpu/drm/radeon/radeon_fence.c
> index 67f6fa9..604352e 100644
> --- a/drivers/gpu/drm/radeon/radeon_fence.c
> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
> @@ -40,6 +40,22 @@
>   #include "radeon.h"
>   #include "radeon_trace.h"
>   
> +/**
> + * radeon_fence_write - write a fence value
> + *
> + * @rdev: radeon_device pointer
> + * @seq: sequence number to write
> + * @ring: ring index the fence is associated with
> + *
> + * Writes a fence value to memory or a scratch register (all asics).
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
We should not repeat the description what a fence is for every function, 
instead a general comment over the whole file should be better. Also I 
would only mention the "all asics" stuff just once in the header of the 
file.

> + */
>   static void radeon_fence_write(struct radeon_device *rdev, u32 seq, int ring)
>   {
>   	if (rdev->wb.enabled) {
> @@ -49,6 +65,22 @@ static void radeon_fence_write(struct radeon_device *rdev, u32 seq, int ring)
>   	}
>   }
>   
> +/**
> + * radeon_fence_read - read a fence value
> + *
> + * @rdev: radeon_device pointer
> + * @ring: ring index the fence is associated with
> + *
> + * Reads a fence value from memory or a scratch register (all asics).
> + * Returns the value of the fence read from memory or register.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   static u32 radeon_fence_read(struct radeon_device *rdev, int ring)
>   {
>   	u32 seq = 0;
> @@ -61,6 +93,23 @@ static u32 radeon_fence_read(struct radeon_device *rdev, int ring)
>   	return seq;
>   }
>   
> +/**
> + * radeon_fence_emit - emit a fence on the requested ring
> + *
> + * @rdev: radeon_device pointer
> + * @fence: radeon fence object
> + * @ring: ring index the fence is associated with
> + *
> + * Emits a fence command on the requested ring (all asics).
> + * Returns 0 on success, -ENOMEM on failure.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_emit(struct radeon_device *rdev,
>   		      struct radeon_fence **fence,
>   		      int ring)
> @@ -79,6 +128,22 @@ int radeon_fence_emit(struct radeon_device *rdev,
>   	return 0;
>   }
>   
> +/**
> + * radeon_fence_process - process a fence
> + *
> + * @rdev: radeon_device pointer
> + * @ring: ring index the fence is associated with
> + *
> + * Checks the current fence value and wakes the fence queue
> + * if the sequence number has increased (all asics).
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   void radeon_fence_process(struct radeon_device *rdev, int ring)
>   {
>   	uint64_t seq, last_seq;
> @@ -139,6 +204,20 @@ void radeon_fence_process(struct radeon_device *rdev, int ring)
>   	}
>   }
>   
> +/**
> + * radeon_fence_destroy - destroy a fence
> + *
> + * @kref: fence kref
> + *
> + * Frees the fence object (all asics).
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   static void radeon_fence_destroy(struct kref *kref)
>   {
>   	struct radeon_fence *fence;
> @@ -147,6 +226,27 @@ static void radeon_fence_destroy(struct kref *kref)
>   	kfree(fence);
>   }
>   
> +/**
> + * radeon_fence_seq_signaled - check if a fence sequeuce number has signaled
> + *
> + * @rdev: radeon device pointer
> + * @seq: sequence number
> + * @ring: ring index the fence is associated with
> + *
> + * Check if the last singled fence sequnce number is >= the requested
> + * sequence number (all asics).
> + * Returns true if the fence has signaled (current fence value
> + * is >= requested value) or false if it has not (current fence
> + * value is < the requested value.  Helper function for
> + * radeon_fence_signaled().
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   static bool radeon_fence_seq_signaled(struct radeon_device *rdev,
>   				      u64 seq, unsigned ring)
>   {
> @@ -161,6 +261,21 @@ static bool radeon_fence_seq_signaled(struct radeon_device *rdev,
>   	return false;
>   }
>   
> +/**
> + * radeon_fence_signaled - check if a fence has signaled
> + *
> + * @fence: radeon fence object
> + *
> + * Check if the requested fence has signaled (all asics).
> + * Returns true if the fence has signaled or false if it has not.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   bool radeon_fence_signaled(struct radeon_fence *fence)
>   {
>   	if (!fence) {
> @@ -176,6 +291,27 @@ bool radeon_fence_signaled(struct radeon_fence *fence)
>   	return false;
>   }
>   
> +/**
> + * radeon_fence_wait_seq - wait for a specific sequence number
> + *
> + * @rdev: radeon device pointer
> + * @target_seq: sequence number we want to wait for
> + * @ring: ring index the fence is associated with
> + * @intr: use interrupt or not
> + * @lock_ring: whether the ring should be locked or not
> + *
> + * Wait for the requested sequence number to be written (all asics).
> + * If @intr is true, use an interrupt to wake up.  Helper function
> + * for radeon_fence_wait(), et al.
The parameter description is incorrect. @intr tells the function to use 
an interruptible or not interruptible sleep, not if we use or not use 
the interrupt.

> + * Returns 0 if the sequence number has passed, error for all other cases.
We should better document possible EDEADLK error, and how to handle it.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */

>   static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 target_seq,
>   				 unsigned ring, bool intr, bool lock_ring)
>   {
> @@ -271,6 +407,23 @@ static int radeon_fence_wait_seq(struct radeon_device *rdev, u64 target_seq,
>   	return 0;
>   }
>   
> +/**
> + * radeon_fence_wait - wait for a fence to signal
> + *
> + * @fence: radeon fence object
> + * @intr: use interrupt or not
> + *
> + * Wait for the requested fence to signal (all asics).
> + * If @intr is true, use an interrupt to wake up.
Same as above, misleading description of the parameter.
> + * Returns 0 if the fence has passed, error for all other cases.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_wait(struct radeon_fence *fence, bool intr)
>   {
>   	int r;
> @@ -301,6 +454,26 @@ static bool radeon_fence_any_seq_signaled(struct radeon_device *rdev, u64 *seq)
>   	return false;
>   }
>   
> +/**
> + * radeon_fence_wait_any_seq - wait for a sequence number on any ring
> + *
> + * @rdev: radeon device pointer
> + * @target_seq: sequence number(s) we want to wait for
> + * @intr: use interrupt or not
> + *
> + * Wait for the requested sequence number(s) to be written by any ring
> + * (all asics).  Sequnce number array is indexed by ring id.
> + * If @intr is true, use an interrupt to wake up.  Helper function
> + * for radeon_fence_wait_any(), et al.
> + * Returns 0 if the sequence number has passed, error for all other cases.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   static int radeon_fence_wait_any_seq(struct radeon_device *rdev,
>   				     u64 *target_seq, bool intr)
>   {
> @@ -410,6 +583,25 @@ static int radeon_fence_wait_any_seq(struct radeon_device *rdev,
>   	return 0;
>   }
>   
> +/**
> + * radeon_fence_wait_any - wait for a fence to signal on any ring
> + *
> + * @rdev: radeon device pointer
> + * @fences: radeon fence object(s)
> + * @intr: use interrupt or not
> + *
> + * Wait for any requested fence to signal (all asics).  Fence
> + * array is indexed by ring id. If @intr is true, use an interrupt
> + * to wake up.  Used by the suballocator.
> + * Returns 0 if any fence has passed, error for all other cases.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_wait_any(struct radeon_device *rdev,
>   			  struct radeon_fence **fences,
>   			  bool intr)
> @@ -440,6 +632,22 @@ int radeon_fence_wait_any(struct radeon_device *rdev,
>   	return 0;
>   }
>   
> +/**
> + * radeon_fence_wait_next_locked - wait for the next fence to signal
> + *
> + * @rdev: radeon device pointer
> + * @ring: ring index the fence is associated with
> + *
> + * Wait for the next fence on the requested ring to signal (all asics).
> + * Returns 0 if the next fence has passed, error for all other cases.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_wait_next_locked(struct radeon_device *rdev, int ring)
>   {
>   	uint64_t seq;
> @@ -457,6 +665,22 @@ int radeon_fence_wait_next_locked(struct radeon_device *rdev, int ring)
>   	return radeon_fence_wait_seq(rdev, seq, ring, false, false);
>   }
>   
> +/**
> + * radeon_fence_wait_empty_locked - wait for all fences to signal
> + *
> + * @rdev: radeon device pointer
> + * @ring: ring index the fence is associated with
> + *
> + * Wait for all fences on the requested ring to signal (all asics).
> + * Returns 0 if the fences have passed, error for all other cases.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_wait_empty_locked(struct radeon_device *rdev, int ring)
>   {
>   	/* We are not protected by ring lock when reading current seq
> @@ -468,12 +692,41 @@ int radeon_fence_wait_empty_locked(struct radeon_device *rdev, int ring)
>   				     ring, false, false);
>   }
>   
> +/**
> + * radeon_fence_ref - take a ref on a fence
> + *
> + * @fence: radeon fence object
> + *
> + * Take a reference on a fence (all asics).
> + * Returns the fence.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   struct radeon_fence *radeon_fence_ref(struct radeon_fence *fence)
>   {
>   	kref_get(&fence->kref);
>   	return fence;
>   }
>   
> +/**
> + * radeon_fence_unref - remove a ref on a fence
> + *
> + * @fence: radeon fence object
> + *
> + * Remove a reference on a fence (all asics).
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   void radeon_fence_unref(struct radeon_fence **fence)
>   {
>   	struct radeon_fence *tmp = *fence;
> @@ -484,6 +737,22 @@ void radeon_fence_unref(struct radeon_fence **fence)
>   	}
>   }
>   
> +/**
> + * radeon_fence_count_emitted - get the count of emitted fences
> + *
> + * @rdev: radeon device pointer
> + * @ring: ring index the fence is associated with
> + *
> + * Get the number of fences emitted on the requested ring (all asics).
> + * Returns the number of emitted fences on the ring.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   unsigned radeon_fence_count_emitted(struct radeon_device *rdev, int ring)
>   {
>   	uint64_t emitted;
> @@ -501,6 +770,24 @@ unsigned radeon_fence_count_emitted(struct radeon_device *rdev, int ring)
>   	return (unsigned)emitted;
>   }
>   
> +/**
> + * radeon_fence_need_sync - do we need a semaphore
> + *
> + * @fence: radeon fence object
> + * @dst_ring: which ring to check against
> + *
> + * Check if the fence needs to be synced against another ring
> + * (all asics).  If so, we need to emit a semaphore.
> + * Returns true if we need to sync with another ring, false if
> + * not.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   bool radeon_fence_need_sync(struct radeon_fence *fence, int dst_ring)
>   {
>   	struct radeon_fence_driver *fdrv;
> @@ -522,6 +809,22 @@ bool radeon_fence_need_sync(struct radeon_fence *fence, int dst_ring)
>   	return true;
>   }
>   
> +/**
> + * radeon_fence_note_sync - record the sync point
> + *
> + * @fence: radeon fence object
> + * @dst_ring: which ring to check against
> + *
> + * Note the sequence number at which point the fence will
> + * be synced with the requested ring (all asics).
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   void radeon_fence_note_sync(struct radeon_fence *fence, int dst_ring)
>   {
>   	struct radeon_fence_driver *dst, *src;
> @@ -546,6 +849,25 @@ void radeon_fence_note_sync(struct radeon_fence *fence, int dst_ring)
>   	}
>   }
>   
> +/**
> + * radeon_fence_driver_start_ring - make the fence driver
> + * ready for use on the requested ring.
> + *
> + * @rdev: radeon device pointer
> + * @ring: ring index to start the fence driver on
> + *
> + * Make the fence driver ready for processing (all asics).
> + * Not all asics have all rings, so each asic will only
> + * start the fence driver on the rings it has.
> + * Returns 0 for success, errors for failure.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring)
>   {
>   	uint64_t index;
> @@ -574,6 +896,23 @@ int radeon_fence_driver_start_ring(struct radeon_device *rdev, int ring)
>   	return 0;
>   }
>   
> +/**
> + * radeon_fence_driver_init_ring - init the fence driver
> + * for the requested ring.
> + *
> + * @rdev: radeon device pointer
> + * @ring: ring index to start the fence driver on
> + *
> + * Init the fence driver for the requested ring (all asics).
> + * Helper function for radeon_fence_driver_init().
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   static void radeon_fence_driver_init_ring(struct radeon_device *rdev, int ring)
>   {
>   	int i;
> @@ -588,6 +927,25 @@ static void radeon_fence_driver_init_ring(struct radeon_device *rdev, int ring)
>   	rdev->fence_drv[ring].initialized = false;
>   }
>   
> +/**
> + * radeon_fence_driver_init - init the fence driver
> + * for all possible rings.
> + *
> + * @rdev: radeon device pointer
> + *
> + * Init the fence driver for all possible rings (all asics).
> + * Not all asics have all rings, so each asic will only
> + * start the fence driver on the rings it has using
> + * radeon_fence_driver_start_ring().
> + * Returns 0 for success.
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   int radeon_fence_driver_init(struct radeon_device *rdev)
>   {
>   	int ring;
> @@ -602,6 +960,21 @@ int radeon_fence_driver_init(struct radeon_device *rdev)
>   	return 0;
>   }
>   
> +/**
> + * radeon_fence_driver_fini - tear down the fence driver
> + * for all possible rings.
> + *
> + * @rdev: radeon device pointer
> + *
> + * Tear down the fence driver for all possible rings (all asics).
> + * Fences mark an event in the GPUs pipeline and are used
> + * for GPU/CPU synchronization.  When the fence is written,
> + * it is expected that all buffers associated with that fence
> + * are no longer in use by the associated ring on the GPU and
> + * that the the relevant GPU caches have been flushed.  Whether
> + * we use a scratch register or memory location depends on the asic
> + * and whether writeback is enabled.
> + */
>   void radeon_fence_driver_fini(struct radeon_device *rdev)
>   {
>   	int ring;