[Nouveau] [PATCH v2 0/2] S3TC support for nv10-20
Viktor Novotný
noviktor at seznam.cz
Tue May 1 06:23:49 PDT 2012
Hi,
thanks for feedback, I finally got to get through your comments - I hope it's OK now.
BTW if you are going to test this, it seems to me that 784dd51198 have broken piglit's
fbo-generatemipmap-formats and s3tc-teximage tests, could you confirm?
Viktor
>>> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_surface.c b/src/mesa/drivers/dri/nouveau/nouveau_surface.c
>>> index f252114..349000a 100644
>>> --- a/src/mesa/drivers/dri/nouveau/nouveau_surface.c
>>> +++ b/src/mesa/drivers/dri/nouveau/nouveau_surface.c
>>> @@ -45,7 +49,7 @@ nouveau_surface_alloc(struct gl_context *ctx, struct nouveau_surface *s,
>>> .width = width,
>>> .height = height,
>>> .cpp = cpp,
>>> - .pitch = width * cpp,
>>> + .pitch = pitch,
>
> No need for the additional local variables:
> | .pitch = _mesa_format_row_stride(format, width),
>
Done
>>> };
>>>
>>> if (layout == TILED) {
>>> @@ -64,8 +68,12 @@ nouveau_surface_alloc(struct gl_context *ctx, struct nouveau_surface *s,
>>> s->pitch = align(s->pitch, 64);
>>> }
>>>
>>> - ret = nouveau_bo_new(context_dev(ctx), flags, 0, s->pitch * height,
>>> - &config, &s->bo);
>>> + if (_mesa_is_format_compressed(format))
>>> + size = s->pitch * nouveau_format_get_nblocksy(format, height);
>>> + else
>>> + size = s->pitch * height;
>>> +
>>> + ret = nouveau_bo_new(context_dev(ctx), flags, 0, size, &config, &s->bo);
>
> No need for the conditional here:
>
> | ret = nouveau_bo_new(context_dev(ctx), flags, 0,
> | s->pitch * get_format_blocksy(format, height),
> | &config, &s->bo);"
>
Done
>>> assert(!ret);
>>> }
>>>
>>> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_texture.c b/src/mesa/drivers/dri/nouveau/nouveau_texture.c
>>> index 643b890..52f0259 100644
>>> --- a/src/mesa/drivers/dri/nouveau/nouveau_texture.c
>>> +++ b/src/mesa/drivers/dri/nouveau/nouveau_texture.c
>>> @@ -291,7 +301,24 @@ nouveau_choose_tex_format(struct gl_context *ctx, GLint internalFormat,
>>> case GL_INTENSITY8:
>>> return MESA_FORMAT_I8;
>>>
>>> + case GL_RGB_S3TC:
>>> + case GL_RGB4_S3TC:
>>> + case GL_COMPRESSED_RGB_S3TC_DXT1_EXT:
>>> + return MESA_FORMAT_RGB_DXT1;
>>> +
>>> + case GL_COMPRESSED_RGBA_S3TC_DXT1_EXT:
>>> + return MESA_FORMAT_RGBA_DXT1;
>>> +
>>> + case GL_RGBA_S3TC:
>>> + case GL_RGBA4_S3TC:
>>> + case GL_COMPRESSED_RGBA_S3TC_DXT3_EXT:
>>> + return MESA_FORMAT_RGBA_DXT3;
>>> +
>>> + case GL_COMPRESSED_RGBA_S3TC_DXT5_EXT:
>>> + return MESA_FORMAT_RGBA_DXT5;
>>> +
>>> default:
>>> + nouveau_error("unexpected internalFormat 0x%x\n", internalFormat);
>
> We already have an assert, what's the point?
>
Well, the idea was to inform the user which was the offending format, but anybody
who might be interested can get that in another way - removed.
>>> assert(0);
>>> }
>>> }
>>> @@ -358,7 +385,7 @@ relayout_texture(struct gl_context *ctx, struct gl_texture_object *t)
>>> struct nouveau_surface *ss = to_nouveau_texture(t)->surfaces;
>>> struct nouveau_surface *s = &to_nouveau_teximage(base)->surface;
>>> int i, ret, last = get_last_level(t);
>
> You can set the layout here because it's always going to be the same for
> all mipmap levels:
>
> | enum nouveau_surface_layout layout =
> | (_mesa_is_format_compressed(s->format) ? LINEAR : SWIZZLED);
>
Done
>>> @@ -368,7 +395,16 @@ relayout_texture(struct gl_context *ctx, struct gl_texture_object *t)
>>>
>>> /* Relayout the mipmap tree. */
>>> for (i = t->BaseLevel; i <= last; i++) {
>>> - size = width * height * s->cpp;
>>> +
>>> + if (_mesa_is_format_compressed(s->format)) {
>>> + layout = LINEAR;
>>> + pitch = _mesa_format_row_stride(s->format, width);
>>> + size = nouveau_format_get_nblocksy(s->format, height) * pitch;
>>> + } else {
>>> + layout = SWIZZLED;
>>> + pitch = width * s->cpp;
>>> + size = height * pitch;
>>> + }
>
> No need for the conditional here:
>
> | pitch = _mesa_format_row_stride(s->format, width);
> | size = get_format_blocksy(s->format, height) * pitch;
>
Done
>>>
>>> /* Images larger than 16B have to be aligned. */
>>> if (size > 16)
>>> @@ -376,12 +412,12 @@ relayout_texture(struct gl_context *ctx, struct gl_texture_object *t)
>>>
>>> ss[i] = (struct nouveau_surface) {
>>> .offset = offset,
>>> - .layout = SWIZZLED,
>>> + .layout = layout,
>>> .format = s->format,
>>> .width = width,
>>> .height = height,
>>> .cpp = s->cpp,
>>> - .pitch = width * s->cpp,
>>> + .pitch = pitch,
>>> };
>>>
>>> offset += size;
>>> @@ -472,9 +510,16 @@ nouveau_teximage(struct gl_context *ctx, GLint dims,
>>> ti->TexFormat, width, height);
>>> nti->base.RowStride = s->pitch / s->cpp;
>>>
>>> - pixels = _mesa_validate_pbo_teximage(ctx, dims, width, height, depth,
>>> - format, type, pixels, packing,
>>> - "glTexImage");
>>> + if (compressed) {
>>> + pixels = _mesa_validate_pbo_compressed_teximage(ctx,
>>> + imageSize,
>>> + pixels, packing, "glCompressedTexImage");
>>> + } else {
>>> + pixels = _mesa_validate_pbo_teximage(ctx,
>>> + dims, width, height, depth, format, type,
>>> + pixels, packing, "glTexImage");
>>> + }
>>> +
>
> The rest of the code doesn't use braces in if blocks with only one
> statement inside.
>
Fixed
>>> @@ -551,21 +608,30 @@ nouveau_texsubimage(struct gl_context *ctx, GLint dims,
>>> struct gl_texture_image *ti,
>>> GLint xoffset, GLint yoffset, GLint zoffset,
>>> GLint width, GLint height, GLint depth,
>>> + GLsizei imageSize,
>>> GLenum format, GLenum type, const void *pixels,
>>> - const struct gl_pixelstore_attrib *packing)
>>> + const struct gl_pixelstore_attrib *packing,
>>> + GLboolean compressed)
>>> {
>>> struct nouveau_surface *s = &to_nouveau_teximage(ti)->surface;
>>> struct nouveau_teximage *nti = to_nouveau_teximage(ti);
>>> int ret;
>>>
>>> - pixels = _mesa_validate_pbo_teximage(ctx, dims, width, height, depth,
>>> - format, type, pixels, packing,
>>> - "glTexSubImage");
>>> + if (compressed) {
>>> + pixels = _mesa_validate_pbo_compressed_teximage(ctx,
>>> + imageSize,
>>> + pixels, packing, "glCompressedTexSubImage");
>>> + } else {
>>> + pixels = _mesa_validate_pbo_teximage(ctx,
>>> + dims, width, height, depth, format, type,
>>> + pixels, packing, "glTexSubImage");
>>> + }
>>> +
>
> Same comment as in nouveau_teximage().
>
ditto
>>> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_util.h b/src/mesa/drivers/dri/nouveau/nouveau_util.h
>>> index d4cc5c4..af2b175 100644
>>> --- a/src/mesa/drivers/dri/nouveau/nouveau_util.h
>>> +++ b/src/mesa/drivers/dri/nouveau/nouveau_util.h
>>> @@ -207,4 +207,23 @@ get_texgen_coeff(struct gl_texgen *c)
>>> return NULL;
>>> }
>>>
>>> +static inline unsigned
>>> +nouveau_format_get_nblocksx(gl_format format,
>>> + unsigned x)
>>> +{
>>> + GLuint blockwidth;
>>> + GLuint blockheight;
>>> + _mesa_get_format_block_size(format, &blockwidth, &blockheight);
>>> + return (x + blockwidth - 1) / blockwidth;
>>> +}
>>> +
>>> +static inline unsigned
>>> +nouveau_format_get_nblocksy(gl_format format,
>>> + unsigned y)
>>> +{
>>> + GLuint blockwidth;
>>> + GLuint blockheight;
>>> + _mesa_get_format_block_size(format, &blockwidth, &blockheight);
>>> + return (y + blockheight - 1) / blockheight;
>>> +}
>
> For consistency with the other nouveau_util.h functions, use
> "get_format_blocksx/y()" or something similar.
>
Done
>>> #endif
>>> diff --git a/src/mesa/drivers/dri/nouveau/nv04_surface.c b/src/mesa/drivers/dri/nouveau/nv04_surface.c
>>> index b2b260d..bc3cace 100644
>>> --- a/src/mesa/drivers/dri/nouveau/nv04_surface.c
>>> +++ b/src/mesa/drivers/dri/nouveau/nv04_surface.c
>>> @@ -291,7 +291,7 @@ nv04_surface_copy_m2mf(struct gl_context *ctx,
>>> while (h) {
>>> int count = (h > 2047) ? 2047 : h;
>>>
>>> - if (nouveau_pushbuf_space(push, 16, 4, 0) ||
>>> + if (nouveau_pushbuf_space(push, 18, 4, 0) ||
>>> nouveau_pushbuf_refn (push, refs, 2))
>>> return;
>>>
>>> @@ -307,6 +307,10 @@ nv04_surface_copy_m2mf(struct gl_context *ctx,
>>> PUSH_DATA (push, count);
>>> PUSH_DATA (push, 0x0101);
>>> PUSH_DATA (push, 0);
>>> + BEGIN_NV04(push, NV04_GRAPH(M2MF, NOP), 1);
>>> + PUSH_DATA (push, 0);
>>> + BEGIN_NV04(push, NV03_M2MF(OFFSET_OUT), 1);
>>> + PUSH_DATA (push, 0);
>
> What's this for?
>
I was getting lock-ups and tried to use this to prevent them, since it's
needed on nv30, that probably isn't the case on nv10-20, the problem was
elsewhere - removed.
>>>
>>> src_offset += src->pitch * count;
>>> dst_offset += dst->pitch * count;
>>> @@ -400,6 +404,17 @@ nv04_surface_copy(struct gl_context *ctx,
>>> int dx, int dy, int sx, int sy,
>>> int w, int h)
>>> {
>>> + bool compressed = _mesa_is_format_compressed(src->format);
>>> +
>>> + if (compressed) {
>
> No need for the local variable:
> | if (_mesa_is_format_compressed(src->format)) {
>
Done
>>> + sx = nouveau_format_get_nblocksx(src->format, sx);
>>> + sy = nouveau_format_get_nblocksy(src->format, sy);
>
>>> + dx = nouveau_format_get_nblocksx(dst->format, sx);
>>> + dy = nouveau_format_get_nblocksy(dst->format, sy);
>
> This looks wrong.
>
D'oh, fixed
>>> + w = nouveau_format_get_nblocksx(src->format, w);
>>> + h = nouveau_format_get_nblocksy(src->format, h);
>>> + }
>>> +
>>> /* Linear texture copy. */
>>> if ((src->layout == LINEAR && dst->layout == LINEAR) ||
>>> dst->width <= 2 || dst->height <= 1) {
Viktor Novotný (2):
dri/nouveau: Add general support for compressed formats.
dri/nouveau: nv10, nv20: Add support for S3TC
src/mesa/drivers/dri/nouveau/nouveau_surface.c | 9 +-
src/mesa/drivers/dri/nouveau/nouveau_texture.c | 133 ++++++++++++++++++------
src/mesa/drivers/dri/nouveau/nouveau_util.h | 20 ++++
src/mesa/drivers/dri/nouveau/nv04_surface.c | 10 ++
src/mesa/drivers/dri/nouveau/nv10_context.c | 4 +
src/mesa/drivers/dri/nouveau/nv10_state_tex.c | 10 ++
src/mesa/drivers/dri/nouveau/nv20_context.c | 4 +
src/mesa/drivers/dri/nouveau/nv20_state_tex.c | 10 ++
8 files changed, 166 insertions(+), 34 deletions(-)
--
1.7.8.5
More information about the Nouveau
mailing list