Re: [PATCH v4 2/5] dma-buf/sync_file: refactor fence storage in struct sync_file

[Sumit Semwal]

Hi Greg,


On 12 July 2016 at 23:38, Gustavo Padovan <gustavo@xxxxxxxxxxx> wrote:
> From: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
>
> Create sync_file->fence to abstract the type of fence we are using for
> each sync_file. If only one fence is present we use a normal struct fence
> but if there is more fences to be added to the sync_file a fence_array
> is created.
>
> This change cleans up sync_file a bit. We don't need to have sync_file_cb
> array anymore. Instead, as we always have  one fence, only one fence
> callback is registered per sync_file.
>
Since this is a simple change in sync_debug,c, may I request for your
Ack so I could take it along with the other dma-buf patches?

> v4: fixes checkpatch warnings
>
> v4: Comments from Chris Wilson
>         - use sizeof(*fence) to reallocate array
>         - fix typo in comments
>         - protect num_fences sum against overflows
>         - use array->base instead of casting the to struct fence
>
> v3: Comments from Chris Wilson and Christian KÃnig
>         - struct sync_file lost status member in favor of fence_is_signaled()
>         - drop use of fence_array_teardown()
>         - use sizeof(*fence) to allocate only an array on fence pointers
>
> v2: Comments from Chris Wilson and Christian KÃnig
>         - Not using fence_ops anymore
>         - fence_is_array() was created to differentiate fence from fence_array
>         - fence_array_teardown() is now exported and used under fence_is_array()
>         - struct sync_file lost num_fences member
>
> Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Cc: Christian KÃnig <christian.koenig@xxxxxxx>
> Signed-off-by: Gustavo Padovan <gustavo.padovan@xxxxxxxxxxxxxxx>
> Reviewed-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> Acked-by: Christian KÃnig <christian.koenig@xxxxxxx>
> ---
>  drivers/dma-buf/sync_file.c          | 169 +++++++++++++++++++++++------------
>  drivers/staging/android/sync_debug.c |  12 ++-
>  include/linux/sync_file.h            |  17 ++--
>  3 files changed, 124 insertions(+), 74 deletions(-)
>
<snip>

> diff --git a/drivers/staging/android/sync_debug.c b/drivers/staging/android/sync_debug.c
> index 5f57499..e07958c 100644
> --- a/drivers/staging/android/sync_debug.c
> +++ b/drivers/staging/android/sync_debug.c
> @@ -159,10 +159,16 @@ static void sync_print_sync_file(struct seq_file *s,
>         int i;
>
>         seq_printf(s, "[%p] %s: %s\n", sync_file, sync_file->name,
> -                  sync_status_str(atomic_read(&sync_file->status)));
> +                  sync_status_str(!fence_is_signaled(sync_file->fence)));
>
> -       for (i = 0; i < sync_file->num_fences; ++i)
> -               sync_print_fence(s, sync_file->cbs[i].fence, true);
> +       if (fence_is_array(sync_file->fence)) {
> +               struct fence_array *array = to_fence_array(sync_file->fence);
> +
> +               for (i = 0; i < array->num_fences; ++i)
> +                       sync_print_fence(s, array->fences[i], true);
> +       } else {
> +               sync_print_fence(s, sync_file->fence, true);
> +       }
>  }
>
>  static int sync_debugfs_show(struct seq_file *s, void *unused)
> diff --git a/include/linux/sync_file.h b/include/linux/sync_file.h
> index c6ffe8b..2efc5ec 100644
> --- a/include/linux/sync_file.h
> +++ b/include/linux/sync_file.h
> @@ -19,12 +19,7 @@
>  #include <linux/list.h>
>  #include <linux/spinlock.h>
>  #include <linux/fence.h>
> -
> -struct sync_file_cb {
> -       struct fence_cb cb;
> -       struct fence *fence;
> -       struct sync_file *sync_file;
> -};
> +#include <linux/fence-array.h>
>
>  /**
>   * struct sync_file - sync file to export to the userspace
> @@ -32,10 +27,9 @@ struct sync_file_cb {
>   * @kref:              reference count on fence.
>   * @name:              name of sync_file.  Useful for debugging
>   * @sync_file_list:    membership in global file list
> - * @num_fences:                number of sync_pts in the fence
>   * @wq:                        wait queue for fence signaling
> - * @status:            0: signaled, >0:active, <0: error
> - * @cbs:               sync_pts callback information
> + * @fence:             fence with the fences in the sync_file
> + * @cb:                        fence callback information
>   */
>  struct sync_file {
>         struct file             *file;
> @@ -44,12 +38,11 @@ struct sync_file {
>  #ifdef CONFIG_DEBUG_FS
>         struct list_head        sync_file_list;
>  #endif
> -       int num_fences;
>
>         wait_queue_head_t       wq;
> -       atomic_t                status;
>
> -       struct sync_file_cb     cbs[];
> +       struct fence            *fence;
> +       struct fence_cb cb;
>  };
>
>  struct sync_file *sync_file_create(struct fence *fence);
> --
> 2.5.5
>

BR,
~Sumit.

-- 
Thanks and regards,

Sumit Semwal
Linaro Mobile Group - Kernel Team Lead
Linaro.org â Open source software for ARM SoCs