Nouveau - Jan 2014 - [PATCH] nouveau: add framebuffer validation callback

Fixes assertions when trying to attach textures to fbs with formats not
supported by the render engines.

See https://bugs.freedesktop.org/show_bug.cgi?id=73459

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---

In a perfect world I'd have separate callbacks for depth and color, but
given
the list of supported values, I don't think this matters. Also I used
st_validate_framebuffer as a template, but I don't know if there can
actually
be many attachments. Should MaxColorAttachments be set to 1? I think it's
set
to 8 right now.

And there's also an odd nouveau_validate_framebuffer thing in
nouveau_context.c, but I think that's related to actually
rendering/invalidating the fb displayed to the screen.

 src/mesa/drivers/dri/nouveau/nouveau_driver.h |  1 +
 src/mesa/drivers/dri/nouveau/nouveau_fbo.c    | 38 +++++++++++++++++++++++++++
 src/mesa/drivers/dri/nouveau/nv04_context.c   | 14 ++++++++++
 src/mesa/drivers/dri/nouveau/nv10_context.c   | 16 +++++++++++
 src/mesa/drivers/dri/nouveau/nv20_context.c   | 16 +++++++++++
 5 files changed, 85 insertions(+)

diff --git a/src/mesa/drivers/dri/nouveau/nouveau_driver.h
b/src/mesa/drivers/dri/nouveau/nouveau_driver.h
index e03b2c1..84953da 100644
--- a/src/mesa/drivers/dri/nouveau/nouveau_driver.h
+++ b/src/mesa/drivers/dri/nouveau/nouveau_driver.h
@@ -60,6 +60,7 @@ struct nouveau_driver {
 			     struct nouveau_surface *dst,
 			     unsigned mask, unsigned value,
 			     int dx, int dy, int w, int h);
+	bool (*is_rt_format_supported)(gl_format format);
 
 	nouveau_state_func *emit;
 	int num_emit;
diff --git a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
index 25543e4..fba0d52 100644
--- a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
+++ b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
@@ -268,6 +268,43 @@ nouveau_finish_render_texture(struct gl_context *ctx,
 	texture_dirty(rb->TexImage->TexObject);
 }
 
+static void
+nouveau_framebuffer_validate(struct gl_context *ctx,
+			     struct gl_framebuffer *fb)
+{
+	const struct nouveau_driver *drv = context_drv(ctx);
+	int i, count = 0;
+
+	for (i = 0; i < ctx->Const.MaxColorAttachments; i++) {
+		struct gl_renderbuffer_attachment *rba +		
&fb->Attachment[BUFFER_COLOR0 + i];
+		if (rba->Type == GL_NONE)
+			continue;
+
+		count++;
+		if (rba->Type != GL_TEXTURE)
+			continue;
+
+		if (!drv->is_rt_format_supported(
+				    rba->Renderbuffer->TexImage->TexFormat))
+			goto err;
+	}
+	if (count > 1)
+		goto err;
+
+	if (fb->Attachment[BUFFER_DEPTH].Type == GL_TEXTURE) {
+		struct gl_texture_image *ti +		
fb->Attachment[BUFFER_DEPTH].Renderbuffer->TexImage;
+		if (!drv->is_rt_format_supported(ti->TexFormat))
+			goto err;
+	}
+
+	return;
+err:
+	fb->_Status = GL_FRAMEBUFFER_UNSUPPORTED_EXT;
+	return;
+}
+
 void
 nouveau_fbo_functions_init(struct dd_function_table *functions)
 {
@@ -279,4 +316,5 @@ nouveau_fbo_functions_init(struct dd_function_table
*functions)
 	functions->FramebufferRenderbuffer = nouveau_framebuffer_renderbuffer;
 	functions->RenderTexture = nouveau_render_texture;
 	functions->FinishRenderTexture = nouveau_finish_render_texture;
+	functions->ValidateFramebuffer = nouveau_framebuffer_validate;
 }
diff --git a/src/mesa/drivers/dri/nouveau/nv04_context.c
b/src/mesa/drivers/dri/nouveau/nv04_context.c
index c198c03..665cadd 100644
--- a/src/mesa/drivers/dri/nouveau/nv04_context.c
+++ b/src/mesa/drivers/dri/nouveau/nv04_context.c
@@ -199,11 +199,25 @@ fail:
 	return NULL;
 }
 
+static bool
+nv04_is_rt_format_supported(gl_format format)
+{
+	switch (format) {
+	case MESA_FORMAT_XRGB8888:
+	case MESA_FORMAT_ARGB8888:
+	case MESA_FORMAT_RGB565:
+		return true;
+	default:
+		return false;
+	}
+}
+
 const struct nouveau_driver nv04_driver = {
 	.context_create = nv04_context_create,
 	.context_destroy = nv04_context_destroy,
 	.surface_copy = nv04_surface_copy,
 	.surface_fill = nv04_surface_fill,
+	.is_rt_format_supported = nv04_is_rt_format_supported,
 	.emit = (nouveau_state_func[]) {
 		nv04_defer_control,
 		nouveau_emit_nothing,
diff --git a/src/mesa/drivers/dri/nouveau/nv10_context.c
b/src/mesa/drivers/dri/nouveau/nv10_context.c
index 1918f12..9c5cfcb 100644
--- a/src/mesa/drivers/dri/nouveau/nv10_context.c
+++ b/src/mesa/drivers/dri/nouveau/nv10_context.c
@@ -492,11 +492,27 @@ fail:
 	return NULL;
 }
 
+static bool
+nv10_is_rt_format_supported(gl_format format)
+{
+	switch (format) {
+	case MESA_FORMAT_XRGB8888:
+	case MESA_FORMAT_ARGB8888:
+	case MESA_FORMAT_RGB565:
+	case MESA_FORMAT_Z16:
+	case MESA_FORMAT_Z24_S8:
+		return true;
+	default:
+		return false;
+	}
+}
+
 const struct nouveau_driver nv10_driver = {
 	.context_create = nv10_context_create,
 	.context_destroy = nv10_context_destroy,
 	.surface_copy = nv04_surface_copy,
 	.surface_fill = nv04_surface_fill,
+       .is_rt_format_supported = nv10_is_rt_format_supported,
 	.emit = (nouveau_state_func[]) {
 		nv10_emit_alpha_func,
 		nv10_emit_blend_color,
diff --git a/src/mesa/drivers/dri/nouveau/nv20_context.c
b/src/mesa/drivers/dri/nouveau/nv20_context.c
index 1d77132..e233025 100644
--- a/src/mesa/drivers/dri/nouveau/nv20_context.c
+++ b/src/mesa/drivers/dri/nouveau/nv20_context.c
@@ -500,11 +500,27 @@ fail:
 	return NULL;
 }
 
+static bool
+nv20_is_rt_format_supported(gl_format format)
+{
+	switch (format) {
+	case MESA_FORMAT_XRGB8888:
+	case MESA_FORMAT_ARGB8888:
+	case MESA_FORMAT_RGB565:
+	case MESA_FORMAT_Z16:
+	case MESA_FORMAT_Z24_S8:
+		return true;
+	default:
+		return false;
+	}
+}
+
 const struct nouveau_driver nv20_driver = {
 	.context_create = nv20_context_create,
 	.context_destroy = nv20_context_destroy,
 	.surface_copy = nv04_surface_copy,
 	.surface_fill = nv04_surface_fill,
+	.is_rt_format_supported = nv20_is_rt_format_supported,
 	.emit = (nouveau_state_func[]) {
 		nv10_emit_alpha_func,
 		nv10_emit_blend_color,
-- 
1.8.3.2

Ilia Mirkin <imirkin at alum.mit.edu> writes:
> Fixes assertions when trying to attach textures to fbs with formats not
> supported by the render engines.
>
> See https://bugs.freedesktop.org/show_bug.cgi?id=73459
>
> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
> ---
Hi Ilia,
>
> In a perfect world I'd have separate callbacks for depth and color, but
given
> the list of supported values, I don't think this matters. Also I used
> st_validate_framebuffer as a template, but I don't know if there can
actually
> be many attachments. Should MaxColorAttachments be set to 1? I think
it's set
> to 8 right now.
>
Yes, we should probably set that to one, and that will make the loop in
your nouveau_framebuffer_validate() code unnecessary.
> And there's also an odd nouveau_validate_framebuffer thing in
> nouveau_context.c, but I think that's related to actually
> rendering/invalidating the fb displayed to the screen.
>
That's called before any rendering happens to check if we need to renew
any of the render buffers, e.g. in case the window was resized or
SwapBuffers() was called.  It's rather unfortunate that the name of your
new function is just a permutation of the old one, how about
'nouveau_check_framebuffer_completeness'?
>  src/mesa/drivers/dri/nouveau/nouveau_driver.h |  1 +
>  src/mesa/drivers/dri/nouveau/nouveau_fbo.c    | 38
+++++++++++++++++++++++++++
>  src/mesa/drivers/dri/nouveau/nv04_context.c   | 14 ++++++++++
>  src/mesa/drivers/dri/nouveau/nv10_context.c   | 16 +++++++++++
>  src/mesa/drivers/dri/nouveau/nv20_context.c   | 16 +++++++++++
>  5 files changed, 85 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_driver.h
b/src/mesa/drivers/dri/nouveau/nouveau_driver.h
> index e03b2c1..84953da 100644
> --- a/src/mesa/drivers/dri/nouveau/nouveau_driver.h
> +++ b/src/mesa/drivers/dri/nouveau/nouveau_driver.h
> @@ -60,6 +60,7 @@ struct nouveau_driver {
>  			     struct nouveau_surface *dst,
>  			     unsigned mask, unsigned value,
>  			     int dx, int dy, int w, int h);
> +	bool (*is_rt_format_supported)(gl_format format);
>  
>  	nouveau_state_func *emit;
>  	int num_emit;
> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
> index 25543e4..fba0d52 100644
> --- a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
> +++ b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
> @@ -268,6 +268,43 @@ nouveau_finish_render_texture(struct gl_context *ctx,
>  	texture_dirty(rb->TexImage->TexObject);
>  }
>  
> +static void
> +nouveau_framebuffer_validate(struct gl_context *ctx,
> +			     struct gl_framebuffer *fb)
> +{
> +	const struct nouveau_driver *drv = context_drv(ctx);
> +	int i, count = 0;
> +
> +	for (i = 0; i < ctx->Const.MaxColorAttachments; i++) {
> +		struct gl_renderbuffer_attachment *rba > +		
&fb->Attachment[BUFFER_COLOR0 + i];
> +		if (rba->Type == GL_NONE)
> +			continue;
> +
> +		count++;
> +		if (rba->Type != GL_TEXTURE)
> +			continue;
> +
> +		if (!drv->is_rt_format_supported(
> +				    rba->Renderbuffer->TexImage->TexFormat))
> +			goto err;
> +	}
> +	if (count > 1)
> +		goto err;
> +
> +	if (fb->Attachment[BUFFER_DEPTH].Type == GL_TEXTURE) {
> +		struct gl_texture_image *ti > +		
fb->Attachment[BUFFER_DEPTH].Renderbuffer->TexImage;
> +		if (!drv->is_rt_format_supported(ti->TexFormat))
> +			goto err;
> +	}
> +
> +	return;
> +err:
> +	fb->_Status = GL_FRAMEBUFFER_UNSUPPORTED_EXT;
> +	return;
> +}
> +
>  void
>  nouveau_fbo_functions_init(struct dd_function_table *functions)
>  {
> @@ -279,4 +316,5 @@ nouveau_fbo_functions_init(struct dd_function_table
*functions)
>  	functions->FramebufferRenderbuffer = nouveau_framebuffer_renderbuffer;
>  	functions->RenderTexture = nouveau_render_texture;
>  	functions->FinishRenderTexture = nouveau_finish_render_texture;
> +	functions->ValidateFramebuffer = nouveau_framebuffer_validate;
>  }
> diff --git a/src/mesa/drivers/dri/nouveau/nv04_context.c
b/src/mesa/drivers/dri/nouveau/nv04_context.c
> index c198c03..665cadd 100644
> --- a/src/mesa/drivers/dri/nouveau/nv04_context.c
> +++ b/src/mesa/drivers/dri/nouveau/nv04_context.c
> @@ -199,11 +199,25 @@ fail:
>  	return NULL;
>  }
>  
> +static bool
> +nv04_is_rt_format_supported(gl_format format)
> +{
> +	switch (format) {
> +	case MESA_FORMAT_XRGB8888:
> +	case MESA_FORMAT_ARGB8888:
> +	case MESA_FORMAT_RGB565:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
You're missing the depth/stencil formats here.  nv04 is kind of annoying
because the depth and color buffers have to be of the same bpp, so if
the color buffer is 32bpp we should only accept MESA_FORMAT_Z24_S8, if
it's 16bpp MESA_FORMAT_Z16.  You probably want to implement that logic
in the top-level ValidateFramebuffer() hook instead of using a driver
call-back mechanism, because (aside from the bpp limitation) we're going
to expose the same set of formats on all generations.

Thanks.
>  const struct nouveau_driver nv04_driver = {
>  	.context_create = nv04_context_create,
>  	.context_destroy = nv04_context_destroy,
>  	.surface_copy = nv04_surface_copy,
>  	.surface_fill = nv04_surface_fill,
> +	.is_rt_format_supported = nv04_is_rt_format_supported,
>  	.emit = (nouveau_state_func[]) {
>  		nv04_defer_control,
>  		nouveau_emit_nothing,
> diff --git a/src/mesa/drivers/dri/nouveau/nv10_context.c
b/src/mesa/drivers/dri/nouveau/nv10_context.c
> index 1918f12..9c5cfcb 100644
> --- a/src/mesa/drivers/dri/nouveau/nv10_context.c
> +++ b/src/mesa/drivers/dri/nouveau/nv10_context.c
> @@ -492,11 +492,27 @@ fail:
>  	return NULL;
>  }
>  
> +static bool
> +nv10_is_rt_format_supported(gl_format format)
> +{
> +	switch (format) {
> +	case MESA_FORMAT_XRGB8888:
> +	case MESA_FORMAT_ARGB8888:
> +	case MESA_FORMAT_RGB565:
> +	case MESA_FORMAT_Z16:
> +	case MESA_FORMAT_Z24_S8:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  const struct nouveau_driver nv10_driver = {
>  	.context_create = nv10_context_create,
>  	.context_destroy = nv10_context_destroy,
>  	.surface_copy = nv04_surface_copy,
>  	.surface_fill = nv04_surface_fill,
> +       .is_rt_format_supported = nv10_is_rt_format_supported,
>  	.emit = (nouveau_state_func[]) {
>  		nv10_emit_alpha_func,
>  		nv10_emit_blend_color,
> diff --git a/src/mesa/drivers/dri/nouveau/nv20_context.c
b/src/mesa/drivers/dri/nouveau/nv20_context.c
> index 1d77132..e233025 100644
> --- a/src/mesa/drivers/dri/nouveau/nv20_context.c
> +++ b/src/mesa/drivers/dri/nouveau/nv20_context.c
> @@ -500,11 +500,27 @@ fail:
>  	return NULL;
>  }
>  
> +static bool
> +nv20_is_rt_format_supported(gl_format format)
> +{
> +	switch (format) {
> +	case MESA_FORMAT_XRGB8888:
> +	case MESA_FORMAT_ARGB8888:
> +	case MESA_FORMAT_RGB565:
> +	case MESA_FORMAT_Z16:
> +	case MESA_FORMAT_Z24_S8:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  const struct nouveau_driver nv20_driver = {
>  	.context_create = nv20_context_create,
>  	.context_destroy = nv20_context_destroy,
>  	.surface_copy = nv04_surface_copy,
>  	.surface_fill = nv04_surface_fill,
> +	.is_rt_format_supported = nv20_is_rt_format_supported,
>  	.emit = (nouveau_state_func[]) {
>  		nv10_emit_alpha_func,
>  		nv10_emit_blend_color,
> -- 
> 1.8.3.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 229 bytes
Desc: not available
URL:
<http://lists.freedesktop.org/archives/nouveau/attachments/20140114/9f7a6395/attachment.pgp>

Fixes assertions when trying to attach textures to fbs with formats not
supported by the render engines.

See https://bugs.freedesktop.org/show_bug.cgi?id=73459

Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
---

Francisco, thanks for the review. Is this more like what you had in mind?
Interesting that nv10/nv20 support different-bitness color/depth -- that
requirement came back for nv30/nv40.

 src/mesa/drivers/dri/nouveau/nouveau_context.c |  1 +
 src/mesa/drivers/dri/nouveau/nouveau_fbo.c     | 52 ++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
index 181c9d0..ec474d4 100644
--- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
+++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
@@ -187,6 +187,7 @@ nouveau_context_init(struct gl_context *ctx, struct
nouveau_screen *screen,
 	ctx->Extensions.EXT_framebuffer_blit = true;
 	ctx->Extensions.EXT_texture_filter_anisotropic = true;
 	ctx->Extensions.NV_texture_env_combine4 = true;
+	ctx->Const.MaxColorAttachments = 1;
 
 	return GL_TRUE;
 }
diff --git a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
index 25543e4..427eb00 100644
--- a/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
+++ b/src/mesa/drivers/dri/nouveau/nouveau_fbo.c
@@ -268,6 +268,57 @@ nouveau_finish_render_texture(struct gl_context *ctx,
 	texture_dirty(rb->TexImage->TexObject);
 }
 
+static int
+validate_format_bpp(gl_format format)
+{
+	switch (format) {
+	case MESA_FORMAT_XRGB8888:
+	case MESA_FORMAT_ARGB8888:
+	case MESA_FORMAT_Z24_S8:
+		return 32;
+	case MESA_FORMAT_RGB565:
+	case MESA_FORMAT_Z16:
+		return 16;
+	default:
+		return 0;
+	}
+}
+
+static void
+nouveau_check_framebuffer_complete(struct gl_context *ctx,
+				   struct gl_framebuffer *fb)
+{
+	const struct nouveau_driver *drv = context_drv(ctx);
+	struct gl_renderbuffer_attachment *color +	
&fb->Attachment[BUFFER_COLOR0];
+	struct gl_renderbuffer_attachment *depth +	
&fb->Attachment[BUFFER_DEPTH];
+	int color_bpp = 0, zeta_bpp;
+
+	if (color->Type == GL_TEXTURE) {
+		color_bpp = validate_format_bpp(
+				color->Renderbuffer->TexImage->TexFormat);
+		if (!color_bpp)
+			goto err;
+	}
+
+	if (depth->Type == GL_TEXTURE) {
+		zeta_bpp = validate_format_bpp(
+				depth->Renderbuffer->TexImage->TexFormat);
+		if (!zeta_bpp)
+			goto err;
+		/* NV04/NV05 requires same bpp-ness for color/zeta */
+		if (context_chipset(ctx) < 0x10 &&
+		    color_bpp && color_bpp != zeta_bpp)
+			goto err;
+	}
+
+	return;
+err:
+	fb->_Status = GL_FRAMEBUFFER_UNSUPPORTED_EXT;
+	return;
+}
+
 void
 nouveau_fbo_functions_init(struct dd_function_table *functions)
 {
@@ -279,4 +330,5 @@ nouveau_fbo_functions_init(struct dd_function_table
*functions)
 	functions->FramebufferRenderbuffer = nouveau_framebuffer_renderbuffer;
 	functions->RenderTexture = nouveau_render_texture;
 	functions->FinishRenderTexture = nouveau_finish_render_texture;
+	functions->ValidateFramebuffer = nouveau_check_framebuffer_complete;
 }
-- 
1.8.3.2