On Mon, May 26, 2014 at 7:20 PM, Tobias Klausmann
<tobias.johannes.klausmann at mni.thm.de> wrote:> v2: change patch according to Ilia Mirkins review
This doesn't really add any particularly useful information for the
commit log... 1 year from now, looking at this commit, is that really
useful information to see? I tend to put that sort of thing below the
--- that way it's in the email, but not in the commit. Also nice to
itemize the changes, but not always necessary; use your judgement. I
tend to do stuff like
v2 -> v3:
- did x
- updated y
v1 -> v2:
- changed a
- changed b
- added c
but it's not a universal format.
> ---
> src/gallium/drivers/nouveau/nvc0/nvc0_surface.c | 151
++++++++++++++++++++++++
> 1 file changed, 151 insertions(+)
>
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_surface.c
b/src/gallium/drivers/nouveau/nvc0/nvc0_surface.c
> index 6b7c30c..242924a 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_surface.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_surface.c
> @@ -345,6 +345,156 @@ nvc0_clear_render_target(struct pipe_context *pipe,
> }
>
> static void
> +nvc0_clear_buffer_cpu(struct pipe_context *pipe,
> + struct pipe_resource *res,
> + unsigned offset, unsigned size,
> + const void *data, int data_size)
> +{
> + struct nv04_resource *buf = nv04_resource(res);
> + struct pipe_transfer *pt;
> + struct pipe_box box;
> + unsigned elements, i;
> +
> + elements = size / data_size;
> +
> + u_box_1d(0, size, &box);
> +
> + uint8_t *tf_map = buf->vtbl->transfer_map(pipe, res,
tf_map is an odd name... how about just 'map'.
> + 0, PIPE_TRANSFER_WRITE, &box, &pt);
Just checking, this doesn't fit on a single line, right? Normally
second lines are wrapped to the opening parens (unless there are no
args on the first line, in which case it's an extra 3-6 spaces,
depending on who you ask).
> +
> + for (i = 0; i < elements ; ++i) {
you have an extra space before the ;
> + memcpy(&tf_map[i*data_size], data, data_size);
> + }
Unnecessary braces
> + buf->vtbl->transfer_unmap(pipe, pt);
> +}
> +
> +static void
> +nvc0_clear_buffer(struct pipe_context *pipe,
> + struct pipe_resource *res,
> + unsigned offset, unsigned size,
> + const void *data, int data_size)
> +{
> + struct nvc0_context *nvc0 = nvc0_context(pipe);
> + struct nouveau_pushbuf *push = nvc0->base.pushbuf;
> + struct nv04_resource *buf = nv04_resource(res);
> + union pipe_color_union color;
> + enum pipe_format dst_fmt;
> + unsigned width, height, elements;
> +
> + assert(res->target == PIPE_BUFFER);
> + assert(nouveau_bo_memtype(buf->bo) == 0);
> +
> + switch (data_size) {
> + case 16:
> + dst_fmt = PIPE_FORMAT_R32G32B32A32_UINT;
> + memcpy(&color.ui, data, 16);
> + break;
> + case 12:
> + //dst_fmt = PIPE_FORMAT_R32G32B32_UINT;
> + //memcpy(&color.ui, data, 12);
> + //memset(&color.ui[3], 0, 4);
> + break;
> + case 8:
> + dst_fmt = PIPE_FORMAT_R32G32_UINT;
> + memcpy(&color.ui, data, 8);
> + memset(&color.ui[2], 0, 8);
> + break;
> + case 4:
> + dst_fmt = PIPE_FORMAT_R32_UINT;
> + memcpy(&color.ui, data, 4);
> + memset(&color.ui[1], 0, 12);
> + break;
> + case 2:
> + dst_fmt = PIPE_FORMAT_R16_UINT;
> + color.ui[0] = util_cpu_to_le32(
> + util_le16_to_cpu(*(unsigned short *)data));
> + memset(&color.ui[1], 0, 12);
> + break;
> + case 1:
> + dst_fmt = PIPE_FORMAT_R8_UINT;
> + color.ui[0] = util_cpu_to_le32(*(unsigned char *)data);
> + memset(&color.ui[1], 0, 12);
> + break;
> + default:
> + assert(!"Unsupported element size");
> + return;
> + }
> +
> + assert(size % data_size == 0);
> +
> + if (data_size == 12) {
> + // TODO: Find a way to do this with the GPU!
Here and above, please use C-style comments (i.e. /* */). This is a .c file.
> + nvc0_clear_buffer_cpu(pipe,res,offset,size,data,data_size);
You still end up marking the framebuffer dirty for no reason. And you
have the second section indented. That's why I suggested
if (...) {
clear_cpu();
return;
}
...
That way no else block required -- it's all just the "normal"
flow.
BTW, add spaces between the params. i.e. f(a, b, c) not f(a,b,c).
> + }
> + else {
> +
> + elements = size / data_size;
> +
> + height = (elements + 16383) / 16384;
> +
> + width = elements / height;
> +
> + if (!PUSH_SPACE(push, 40))
> + return;
> +
> + PUSH_REFN (push, buf->bo, buf->domain | NOUVEAU_BO_WR);
> +
> + BEGIN_NVC0(push, NVC0_3D(CLEAR_COLOR(0)), 4);
> + PUSH_DATAf(push, color.f[0]);
> + PUSH_DATAf(push, color.f[1]);
> + PUSH_DATAf(push, color.f[2]);
> + PUSH_DATAf(push, color.f[3]);
> + BEGIN_NVC0(push, NVC0_3D(SCREEN_SCISSOR_HORIZ), 2);
> + PUSH_DATA (push, width << 16);
> + PUSH_DATA (push, height << 16);
> +
> + IMMED_NVC0(push, NVC0_3D(RT_CONTROL), 1);
> +
> + BEGIN_NVC0(push, NVC0_3D(RT_ADDRESS_HIGH(0)), 9);
> + PUSH_DATAh(push, buf->address + offset);
> + PUSH_DATA (push, buf->address + offset);
> +
> + PUSH_DATA (push, width * data_size);
> + PUSH_DATA (push, height);
> +
> + PUSH_DATA (push, nvc0_format_table[dst_fmt].rt);
> + PUSH_DATA (push, 1 << 12);
> + PUSH_DATA (push, 1);
> + PUSH_DATA (push, 0);
> + PUSH_DATA (push, 0);
> +
> + IMMED_NVC0(push, NVC0_3D(ZETA_ENABLE), 0);
> +
> + IMMED_NVC0(push, NVC0_3D(CLEAR_BUFFERS), 0x3c);
> +
> + if (width * height != elements) {
> + offset += width * height * data_size;
> + width = elements - width * height;
> + height = 1;
> +
> + BEGIN_NVC0(push, NVC0_3D(SCREEN_SCISSOR_HORIZ), 2);
> + PUSH_DATA (push, width << 16);
> + PUSH_DATA (push, height << 16);
> +
> + BEGIN_NVC0(push, NVC0_3D(RT_ADDRESS_HIGH(0)), 2);
> + PUSH_DATAh(push, buf->address + offset);
> + PUSH_DATA (push, buf->address + offset);
> +
> + BEGIN_NVC0(push, NVC0_3D(RT_HORIZ(0)), 2);
> + PUSH_DATA (push, width * data_size);
> + PUSH_DATA (push, height);
> +
> + IMMED_NVC0(push, NVC0_3D(CLEAR_BUFFERS), 0x3c);
> + }
> +
> + nouveau_fence_ref(nvc0->screen->base.fence.current,
&buf->fence);
> + nouveau_fence_ref(nvc0->screen->base.fence.current,
&buf->fence_wr);
> + }
> +
> + nvc0->dirty |= NVC0_NEW_FRAMEBUFFER;
> +}
> +
> +static void
> nvc0_clear_depth_stencil(struct pipe_context *pipe,
> struct pipe_surface *dst,
> unsigned clear_flags,
> @@ -1363,4 +1513,5 @@ nvc0_init_surface_functions(struct nvc0_context
*nvc0)
> pipe->flush_resource = nvc0_flush_resource;
> pipe->clear_render_target = nvc0_clear_render_target;
> pipe->clear_depth_stencil = nvc0_clear_depth_stencil;
> + pipe->clear_buffer = nvc0_clear_buffer;
> }
> --
> 1.8.4.5
>