[Mesa-dev] [PATCH] gallium: Formatted code in pb_buffer_fenced.c according to style guide.
Thomas Helland
thomashelland90 at gmail.com
Fri Mar 25 01:03:29 UTC 2016
Welcome to the community!
Some small comments.
We usually write patch messages in present tense.
So, "Format code in .... ".
Some more comments below.
On Mar 25, 2016 01:24, "Rovanion Luckey" <rovanion.luckey at gmail.com> wrote:
>
> This is a tiny housekeeping patch which does the following:
>
> * Replaced tabs with three spaces.
> * Formatted oneline and multiline code comments. Some doxygen
> comments weren't marked as such and some code comments
were marked
> as doxygen comments.
> * Spaces between if- and while-statements and their parenthesis.
>
> As specified on: http://mesa3d.org/devinfo.html#style
>
Nice detailed commit message!
I'm not sure, but I think you should just write:
"According to coding style standards" or something.
Links may die, and so having them in the commit message
may not give us much in the future.
> The only interesting point would be @@ -364,14 +363,9 @@ where the
> following seemingly trivial change is applied.
>
> - boolean destroyed;
> -
> - destroyed = fenced_buffer_remove_locked(fenced_mgr, fenced_buf);
> + boolean destroyed = fenced_buffer_remove_locked(fenced_mgr,
fenced_buf);
>
> It may be that I'm missing some of the finer points of C making this
> into a semantic change instead of the only syntactic change I was
> after, in which case the change should be removed. It might also be
> that it should be removed from this change set either way since it
> could be considered non-trivial.
>
I'm not sure how this works now, but I believe there was
something with older versions of MSVC that didn't allow
initializing variables like this, and that's why it is separated
in declaration and initialization.
I believe C89 was the thing here? The VMware guys will know.
I believe the requirement was lifted to a newer C standard(C99?).
Therefore this is now OK I believe.
I haven't looked to closely at the other changes.
I can do that tomorrow, if no one gets to it before me.
Don't let the minor nitpicking get you down ;-)
There's always the occasional nitpick when you first adapt
to how things are done in the project.
Thanks for your first patch!
> ---
> .../auxiliary/pipebuffer/pb_buffer_fenced.c | 226
+++++++++------------
> 1 file changed, 97 insertions(+), 129 deletions(-)
>
> diff --git a/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
b/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> index 2678268..fbbe8d1 100644
> --- a/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> +++ b/src/gallium/auxiliary/pipebuffer/pb_buffer_fenced.c
> @@ -108,14 +108,14 @@ struct fenced_manager
> */
> struct fenced_buffer
> {
> - /*
> + /**
> * Immutable members.
> */
>
> struct pb_buffer base;
> struct fenced_manager *mgr;
>
> - /*
> + /**
> * Following members are mutable and protected by
fenced_manager::mutex.
> */
>
> @@ -205,7 +205,7 @@ fenced_manager_dump_locked(struct fenced_manager
*fenced_mgr)
>
> curr = fenced_mgr->unfenced.next;
> next = curr->next;
> - while(curr != &fenced_mgr->unfenced) {
> + while (curr != &fenced_mgr->unfenced) {
> fenced_buf = LIST_ENTRY(struct fenced_buffer, curr, head);
> assert(!fenced_buf->fence);
> debug_printf("%10p %7u %8u %7s\n",
> @@ -219,7 +219,7 @@ fenced_manager_dump_locked(struct fenced_manager
*fenced_mgr)
>
> curr = fenced_mgr->fenced.next;
> next = curr->next;
> - while(curr != &fenced_mgr->fenced) {
> + while (curr != &fenced_mgr->fenced) {
> int signaled;
> fenced_buf = LIST_ENTRY(struct fenced_buffer, curr, head);
> assert(fenced_buf->buffer);
> @@ -340,7 +340,7 @@ fenced_buffer_finish_locked(struct fenced_manager
*fenced_mgr,
> assert(pipe_is_referenced(&fenced_buf->base.reference));
> assert(fenced_buf->fence);
>
> - if(fenced_buf->fence) {
> + if (fenced_buf->fence) {
> struct pipe_fence_handle *fence = NULL;
> int finished;
> boolean proceed;
> @@ -355,8 +355,7 @@ fenced_buffer_finish_locked(struct fenced_manager
*fenced_mgr,
>
> assert(pipe_is_referenced(&fenced_buf->base.reference));
>
> - /*
> - * Only proceed if the fence object didn't change in the meanwhile.
> + /* Only proceed if the fence object didn't change in the meanwhile.
> * Otherwise assume the work has been already carried out by
another
> * thread that re-aquired the lock before us.
> */
> @@ -364,14 +363,9 @@ fenced_buffer_finish_locked(struct fenced_manager
*fenced_mgr,
>
> ops->fence_reference(ops, &fence, NULL);
>
> - if(proceed && finished == 0) {
> - /*
> - * Remove from the fenced list
> - */
> -
> - boolean destroyed;
> -
> - destroyed = fenced_buffer_remove_locked(fenced_mgr, fenced_buf);
> + if (proceed && finished == 0) {
> + /* Remove from the fenced list. */
> + boolean destroyed = fenced_buffer_remove_locked(fenced_mgr,
fenced_buf);
>
> /* TODO: remove consequents buffers with the same fence? */
>
> @@ -405,36 +399,33 @@ fenced_manager_check_signalled_locked(struct
fenced_manager *fenced_mgr,
>
> curr = fenced_mgr->fenced.next;
> next = curr->next;
> - while(curr != &fenced_mgr->fenced) {
> + while (curr != &fenced_mgr->fenced) {
> fenced_buf = LIST_ENTRY(struct fenced_buffer, curr, head);
>
> - if(fenced_buf->fence != prev_fence) {
> - int signaled;
> + if (fenced_buf->fence != prev_fence) {
> + int signaled;
>
> - if (wait) {
> - signaled = ops->fence_finish(ops, fenced_buf->fence, 0);
> + if (wait) {
> + signaled = ops->fence_finish(ops, fenced_buf->fence, 0);
>
> - /*
> - * Don't return just now. Instead preemptively check if the
> - * following buffers' fences already expired, without further
waits.
> - */
> - wait = FALSE;
> - }
> - else {
> - signaled = ops->fence_signalled(ops, fenced_buf->fence, 0);
> - }
> + /* Don't return just now. Instead preemptively check if the
> + * following buffers' fences already expired, without
further waits.
> + */
> + wait = FALSE;
> + } else {
> + signaled = ops->fence_signalled(ops, fenced_buf->fence, 0);
> + }
>
> - if (signaled != 0) {
> - return ret;
> + if (signaled != 0) {
> + return ret;
> }
>
> - prev_fence = fenced_buf->fence;
> - }
> - else {
> + prev_fence = fenced_buf->fence;
> + } else {
> /* This buffer's fence object is identical to the previous
buffer's
> * fence object, so no need to check the fence again.
> */
> - assert(ops->fence_signalled(ops, fenced_buf->fence, 0) == 0);
> + assert(ops->fence_signalled(ops, fenced_buf->fence, 0) == 0);
> }
>
> fenced_buffer_remove_locked(fenced_mgr, fenced_buf);
> @@ -462,22 +453,21 @@ fenced_manager_free_gpu_storage_locked(struct
fenced_manager *fenced_mgr)
>
> curr = fenced_mgr->unfenced.next;
> next = curr->next;
> - while(curr != &fenced_mgr->unfenced) {
> + while (curr != &fenced_mgr->unfenced) {
> fenced_buf = LIST_ENTRY(struct fenced_buffer, curr, head);
>
> - /*
> - * We can only move storage if the buffer is not mapped and not
> + /* We can only move storage if the buffer is not mapped and not
> * validated.
> */
> - if(fenced_buf->buffer &&
> + if (fenced_buf->buffer &&
> !fenced_buf->mapcount &&
> !fenced_buf->vl) {
> enum pipe_error ret;
>
> ret = fenced_buffer_create_cpu_storage_locked(fenced_mgr,
fenced_buf);
> - if(ret == PIPE_OK) {
> + if (ret == PIPE_OK) {
> ret = fenced_buffer_copy_storage_to_cpu_locked(fenced_buf);
> - if(ret == PIPE_OK) {
> + if (ret == PIPE_OK) {
> fenced_buffer_destroy_gpu_storage_locked(fenced_buf);
> return TRUE;
> }
> @@ -499,7 +489,7 @@ fenced_manager_free_gpu_storage_locked(struct
fenced_manager *fenced_mgr)
> static void
> fenced_buffer_destroy_cpu_storage_locked(struct fenced_buffer
*fenced_buf)
> {
> - if(fenced_buf->data) {
> + if (fenced_buf->data) {
> align_free(fenced_buf->data);
> fenced_buf->data = NULL;
> assert(fenced_buf->mgr->cpu_total_size >= fenced_buf->size);
> @@ -516,14 +506,14 @@ fenced_buffer_create_cpu_storage_locked(struct
fenced_manager *fenced_mgr,
> struct fenced_buffer *fenced_buf)
> {
> assert(!fenced_buf->data);
> - if(fenced_buf->data)
> + if (fenced_buf->data)
> return PIPE_OK;
>
> if (fenced_mgr->cpu_total_size + fenced_buf->size >
fenced_mgr->max_cpu_total_size)
> return PIPE_ERROR_OUT_OF_MEMORY;
>
> fenced_buf->data = align_malloc(fenced_buf->size,
fenced_buf->desc.alignment);
> - if(!fenced_buf->data)
> + if (!fenced_buf->data)
> return PIPE_ERROR_OUT_OF_MEMORY;
>
> fenced_mgr->cpu_total_size += fenced_buf->size;
> @@ -538,7 +528,7 @@ fenced_buffer_create_cpu_storage_locked(struct
fenced_manager *fenced_mgr,
> static void
> fenced_buffer_destroy_gpu_storage_locked(struct fenced_buffer
*fenced_buf)
> {
> - if(fenced_buf->buffer) {
> + if (fenced_buf->buffer) {
> pb_reference(&fenced_buf->buffer, NULL);
> }
> }
> @@ -575,41 +565,37 @@ fenced_buffer_create_gpu_storage_locked(struct
fenced_manager *fenced_mgr,
> {
> assert(!fenced_buf->buffer);
>
> - /*
> - * Check for signaled buffers before trying to allocate.
> - */
> + /* Check for signaled buffers before trying to allocate. */
> fenced_manager_check_signalled_locked(fenced_mgr, FALSE);
>
> fenced_buffer_try_create_gpu_storage_locked(fenced_mgr, fenced_buf);
>
> - /*
> - * Keep trying while there is some sort of progress:
> + /* Keep trying while there is some sort of progress:
> * - fences are expiring,
> * - or buffers are being being swapped out from GPU memory into CPU
memory.
> */
> - while(!fenced_buf->buffer &&
> + while (!fenced_buf->buffer &&
> (fenced_manager_check_signalled_locked(fenced_mgr, FALSE) ||
> fenced_manager_free_gpu_storage_locked(fenced_mgr))) {
> fenced_buffer_try_create_gpu_storage_locked(fenced_mgr,
fenced_buf);
> }
>
> - if(!fenced_buf->buffer && wait) {
> - /*
> - * Same as before, but this time around, wait to free buffers if
> + if (!fenced_buf->buffer && wait) {
> + /* Same as before, but this time around, wait to free buffers if
> * necessary.
> */
> - while(!fenced_buf->buffer &&
> + while (!fenced_buf->buffer &&
> (fenced_manager_check_signalled_locked(fenced_mgr, TRUE) ||
> fenced_manager_free_gpu_storage_locked(fenced_mgr))) {
> fenced_buffer_try_create_gpu_storage_locked(fenced_mgr,
fenced_buf);
> }
> }
>
> - if(!fenced_buf->buffer) {
> - if(0)
> + if (!fenced_buf->buffer) {
> + if (0)
> fenced_manager_dump_locked(fenced_mgr);
>
> - /* give up */
> + /* Give up. */
> return PIPE_ERROR_OUT_OF_MEMORY;
> }
>
> @@ -686,18 +672,16 @@ fenced_buffer_map(struct pb_buffer *buf,
>
> assert(!(flags & PB_USAGE_GPU_READ_WRITE));
>
> - /*
> - * Serialize writes.
> - */
> - while((fenced_buf->flags & PB_USAGE_GPU_WRITE) ||
> + /* Serialize writes. */
> + while ((fenced_buf->flags & PB_USAGE_GPU_WRITE) ||
> ((fenced_buf->flags & PB_USAGE_GPU_READ) &&
> (flags & PB_USAGE_CPU_WRITE))) {
>
> - /*
> - * Don't wait for the GPU to finish accessing it, if blocking is
forbidden.
> + /* Don't wait for the GPU to finish accessing it,
> + * if blocking is forbidden.
> */
> - if((flags & PB_USAGE_DONTBLOCK) &&
> - ops->fence_signalled(ops, fenced_buf->fence, 0) != 0) {
> + if ((flags & PB_USAGE_DONTBLOCK) &&
> + ops->fence_signalled(ops, fenced_buf->fence, 0) != 0) {
> goto done;
> }
>
> @@ -705,17 +689,15 @@ fenced_buffer_map(struct pb_buffer *buf,
> break;
> }
>
> - /*
> - * Wait for the GPU to finish accessing. This will release and
re-acquire
> + /* Wait for the GPU to finish accessing. This will release and
re-acquire
> * the mutex, so all copies of mutable state must be discarded.
> */
> fenced_buffer_finish_locked(fenced_mgr, fenced_buf);
> }
>
> - if(fenced_buf->buffer) {
> + if (fenced_buf->buffer) {
> map = pb_map(fenced_buf->buffer, flags, flush_ctx);
> - }
> - else {
> + } else {
> assert(fenced_buf->data);
> map = fenced_buf->data;
> }
> @@ -725,7 +707,7 @@ fenced_buffer_map(struct pb_buffer *buf,
> fenced_buf->flags |= flags & PB_USAGE_CPU_READ_WRITE;
> }
>
> -done:
> + done:
> pipe_mutex_unlock(fenced_mgr->mutex);
>
> return map;
> @@ -741,12 +723,12 @@ fenced_buffer_unmap(struct pb_buffer *buf)
> pipe_mutex_lock(fenced_mgr->mutex);
>
> assert(fenced_buf->mapcount);
> - if(fenced_buf->mapcount) {
> + if (fenced_buf->mapcount) {
> if (fenced_buf->buffer)
> pb_unmap(fenced_buf->buffer);
> --fenced_buf->mapcount;
> - if(!fenced_buf->mapcount)
> - fenced_buf->flags &= ~PB_USAGE_CPU_READ_WRITE;
> + if (!fenced_buf->mapcount)
> + fenced_buf->flags &= ~PB_USAGE_CPU_READ_WRITE;
> }
>
> pipe_mutex_unlock(fenced_mgr->mutex);
> @@ -765,7 +747,7 @@ fenced_buffer_validate(struct pb_buffer *buf,
> pipe_mutex_lock(fenced_mgr->mutex);
>
> if (!vl) {
> - /* invalidate */
> + /* Invalidate. */
> fenced_buf->vl = NULL;
> fenced_buf->validation_flags = 0;
> ret = PIPE_OK;
> @@ -776,40 +758,37 @@ fenced_buffer_validate(struct pb_buffer *buf,
> assert(!(flags & ~PB_USAGE_GPU_READ_WRITE));
> flags &= PB_USAGE_GPU_READ_WRITE;
>
> - /* Buffer cannot be validated in two different lists */
> - if(fenced_buf->vl && fenced_buf->vl != vl) {
> + /* Buffer cannot be validated in two different lists. */
> + if (fenced_buf->vl && fenced_buf->vl != vl) {
> ret = PIPE_ERROR_RETRY;
> goto done;
> }
>
> - if(fenced_buf->vl == vl &&
> + if (fenced_buf->vl == vl &&
> (fenced_buf->validation_flags & flags) == flags) {
> - /* Nothing to do -- buffer already validated */
> + /* Nothing to do -- buffer already validated. */
> ret = PIPE_OK;
> goto done;
> }
>
> - /*
> - * Create and update GPU storage.
> - */
> - if(!fenced_buf->buffer) {
> + /* Create and update GPU storage. */
> + if (!fenced_buf->buffer) {
> assert(!fenced_buf->mapcount);
>
> ret = fenced_buffer_create_gpu_storage_locked(fenced_mgr,
fenced_buf, TRUE);
> - if(ret != PIPE_OK) {
> + if (ret != PIPE_OK) {
> goto done;
> }
>
> ret = fenced_buffer_copy_storage_to_gpu_locked(fenced_buf);
> - if(ret != PIPE_OK) {
> + if (ret != PIPE_OK) {
> fenced_buffer_destroy_gpu_storage_locked(fenced_buf);
> goto done;
> }
>
> - if(fenced_buf->mapcount) {
> + if (fenced_buf->mapcount) {
> debug_printf("warning: validating a buffer while it is still
mapped\n");
> - }
> - else {
> + } else {
> fenced_buffer_destroy_cpu_storage_locked(fenced_buf);
> }
> }
> @@ -821,7 +800,7 @@ fenced_buffer_validate(struct pb_buffer *buf,
> fenced_buf->vl = vl;
> fenced_buf->validation_flags |= flags;
>
> -done:
> + done:
> pipe_mutex_unlock(fenced_mgr->mutex);
>
> return ret;
> @@ -841,13 +820,12 @@ fenced_buffer_fence(struct pb_buffer *buf,
> assert(pipe_is_referenced(&fenced_buf->base.reference));
> assert(fenced_buf->buffer);
>
> - if(fence != fenced_buf->fence) {
> + if (fence != fenced_buf->fence) {
> assert(fenced_buf->vl);
> assert(fenced_buf->validation_flags);
>
> if (fenced_buf->fence) {
> - boolean destroyed;
> - destroyed = fenced_buffer_remove_locked(fenced_mgr, fenced_buf);
> + boolean destroyed = fenced_buffer_remove_locked(fenced_mgr,
fenced_buf);
> assert(!destroyed);
> }
> if (fence) {
> @@ -876,16 +854,15 @@ fenced_buffer_get_base_buffer(struct pb_buffer *buf,
>
> pipe_mutex_lock(fenced_mgr->mutex);
>
> - /*
> - * This should only be called when the buffer is validated. Typically
> + /* This should only be called when the buffer is validated. Typically
> * when processing relocations.
> */
> assert(fenced_buf->vl);
> assert(fenced_buf->buffer);
>
> - if(fenced_buf->buffer)
> + if (fenced_buf->buffer) {
> pb_get_base_buffer(fenced_buf->buffer, base_buf, offset);
> - else {
> + } else {
> *base_buf = buf;
> *offset = 0;
> }
> @@ -896,12 +873,12 @@ fenced_buffer_get_base_buffer(struct pb_buffer *buf,
>
> static const struct pb_vtbl
> fenced_buffer_vtbl = {
> - fenced_buffer_destroy,
> - fenced_buffer_map,
> - fenced_buffer_unmap,
> - fenced_buffer_validate,
> - fenced_buffer_fence,
> - fenced_buffer_get_base_buffer
> + fenced_buffer_destroy,
> + fenced_buffer_map,
> + fenced_buffer_unmap,
> + fenced_buffer_validate,
> + fenced_buffer_fence,
> + fenced_buffer_get_base_buffer
> };
>
>
> @@ -917,12 +894,11 @@ fenced_bufmgr_create_buffer(struct pb_manager *mgr,
> struct fenced_buffer *fenced_buf;
> enum pipe_error ret;
>
> - /*
> - * Don't stall the GPU, waste time evicting buffers, or waste memory
> + /* Don't stall the GPU, waste time evicting buffers, or waste memory
> * trying to create a buffer that will most likely never fit into the
> * graphics aperture.
> */
> - if(size > fenced_mgr->max_buffer_size) {
> + if (size > fenced_mgr->max_buffer_size) {
> goto no_buffer;
> }
>
> @@ -942,29 +918,21 @@ fenced_bufmgr_create_buffer(struct pb_manager *mgr,
>
> pipe_mutex_lock(fenced_mgr->mutex);
>
> - /*
> - * Try to create GPU storage without stalling,
> - */
> + /* Try to create GPU storage without stalling. */
> ret = fenced_buffer_create_gpu_storage_locked(fenced_mgr, fenced_buf,
FALSE);
>
> - /*
> - * Attempt to use CPU memory to avoid stalling the GPU.
> - */
> - if(ret != PIPE_OK) {
> + /* Attempt to use CPU memory to avoid stalling the GPU. */
> + if (ret != PIPE_OK) {
> ret = fenced_buffer_create_cpu_storage_locked(fenced_mgr,
fenced_buf);
> }
>
> - /*
> - * Create GPU storage, waiting for some to be available.
> - */
> - if(ret != PIPE_OK) {
> + /* Create GPU storage, waiting for some to be available. */
> + if (ret != PIPE_OK) {
> ret = fenced_buffer_create_gpu_storage_locked(fenced_mgr,
fenced_buf, TRUE);
> }
>
> - /*
> - * Give up.
> - */
> - if(ret != PIPE_OK) {
> + /* Give up. */
> + if (ret != PIPE_OK) {
> goto no_storage;
> }
>
> @@ -976,10 +944,10 @@ fenced_bufmgr_create_buffer(struct pb_manager *mgr,
>
> return &fenced_buf->base;
>
> -no_storage:
> + no_storage:
> pipe_mutex_unlock(fenced_mgr->mutex);
> FREE(fenced_buf);
> -no_buffer:
> + no_buffer:
> return NULL;
> }
>
> @@ -990,12 +958,12 @@ fenced_bufmgr_flush(struct pb_manager *mgr)
> struct fenced_manager *fenced_mgr = fenced_manager(mgr);
>
> pipe_mutex_lock(fenced_mgr->mutex);
> - while(fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
> + while (fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
> ;
> pipe_mutex_unlock(fenced_mgr->mutex);
>
> assert(fenced_mgr->provider->flush);
> - if(fenced_mgr->provider->flush)
> + if (fenced_mgr->provider->flush)
> fenced_mgr->provider->flush(fenced_mgr->provider);
> }
>
> @@ -1007,25 +975,25 @@ fenced_bufmgr_destroy(struct pb_manager *mgr)
>
> pipe_mutex_lock(fenced_mgr->mutex);
>
> - /* Wait on outstanding fences */
> + /* Wait on outstanding fences. */
> while (fenced_mgr->num_fenced) {
> pipe_mutex_unlock(fenced_mgr->mutex);
> #if defined(PIPE_OS_LINUX) || defined(PIPE_OS_BSD) ||
defined(PIPE_OS_SOLARIS)
> sched_yield();
> #endif
> pipe_mutex_lock(fenced_mgr->mutex);
> - while(fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
> + while (fenced_manager_check_signalled_locked(fenced_mgr, TRUE))
> ;
> }
>
> #ifdef DEBUG
> - /*assert(!fenced_mgr->num_unfenced);*/
> + /* assert(!fenced_mgr->num_unfenced); */
> #endif
>
> pipe_mutex_unlock(fenced_mgr->mutex);
> pipe_mutex_destroy(fenced_mgr->mutex);
>
> - if(fenced_mgr->provider)
> + if (fenced_mgr->provider)
> fenced_mgr->provider->destroy(fenced_mgr->provider);
>
> fenced_mgr->ops->destroy(fenced_mgr->ops);
> --
> 2.7.3
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160325/07a6a67e/attachment-0001.html>
More information about the mesa-dev
mailing list