Nouveau - Apr 2012 - [PATCH] nouveau/vieux: only advertise supported texture formats

Fixes an assertion seen by users.

Signed-off-by: Lucas Stach <dev at lynxeye.de>
Tested-by: JohnDoe_71Rus on irc
---
 src/mesa/drivers/dri/nouveau/nouveau_context.c |    9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
index 4845767..46c0d70 100644
--- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
+++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
@@ -117,6 +117,15 @@ nouveau_context_init(struct gl_context *ctx, struct
nouveau_screen *screen,
 	nouveau_span_functions_init(ctx);
 	_mesa_allow_light_in_model(ctx, GL_FALSE);
 
+	/* only advertise supported texture formats */
+	memset(&ctx->TextureFormatSupported, 0,
+	       sizeof(ctx->TextureFormatSupported));
+	ctx->TextureFormatSupported[MESA_FORMAT_XRGB8888] = true;
+	ctx->TextureFormatSupported[MESA_FORMAT_ARGB8888] = true;
+	ctx->TextureFormatSupported[MESA_FORMAT_RGB565] = true;
+	ctx->TextureFormatSupported[MESA_FORMAT_Z16] = true;
+	ctx->TextureFormatSupported[MESA_FORMAT_Z24_S8] = true;
+
 	/* Allocate a hardware channel. */
 	ret = nouveau_object_new(&context_dev(ctx)->object, 0xbeef0000,
 				 NOUVEAU_FIFO_CHANNEL_CLASS,
-- 
1.7.10

On 30.04.2012 19:16, Lucas Stach wrote:
> Fixes an assertion seen by users.
>
> Signed-off-by: Lucas Stach <dev at lynxeye.de>
> Tested-by: JohnDoe_71Rus on irc
> ---
>  src/mesa/drivers/dri/nouveau/nouveau_context.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> index 4845767..46c0d70 100644
> --- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
> +++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> @@ -117,6 +117,15 @@ nouveau_context_init(struct gl_context *ctx, struct
nouveau_screen *screen,
>  	nouveau_span_functions_init(ctx);
>  	_mesa_allow_light_in_model(ctx, GL_FALSE);
>  
> +	/* only advertise supported texture formats */
> +	memset(&ctx->TextureFormatSupported, 0,
> +	       sizeof(ctx->TextureFormatSupported));
> +	ctx->TextureFormatSupported[MESA_FORMAT_XRGB8888] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_ARGB8888] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_RGB565] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_Z16] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_Z24_S8] = true;
> +
But it also seems to support ARGB1555, L8, A8 and I8, see
nouveau_choose_tex_format ...
>  	/* Allocate a hardware channel. */
>  	ret = nouveau_object_new(&context_dev(ctx)->object, 0xbeef0000,
>  				 NOUVEAU_FIFO_CHANNEL_CLASS,

Lucas Stach <dev at lynxeye.de> writes:
> Fixes an assertion seen by users.
>
> Signed-off-by: Lucas Stach <dev at lynxeye.de>
> Tested-by: JohnDoe_71Rus on irc
> ---
>  src/mesa/drivers/dri/nouveau/nouveau_context.c |    9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> index 4845767..46c0d70 100644
> --- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
> +++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> @@ -117,6 +117,15 @@ nouveau_context_init(struct gl_context *ctx, struct
nouveau_screen *screen,
>  	nouveau_span_functions_init(ctx);
>  	_mesa_allow_light_in_model(ctx, GL_FALSE);
>  
> +	/* only advertise supported texture formats */
> +	memset(&ctx->TextureFormatSupported, 0,
> +	       sizeof(ctx->TextureFormatSupported));
> +	ctx->TextureFormatSupported[MESA_FORMAT_XRGB8888] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_ARGB8888] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_RGB565] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_Z16] = true;
> +	ctx->TextureFormatSupported[MESA_FORMAT_Z24_S8] = true;
> +
Hi Lucas,

It's not obvious to me how this works.  AFAIK nouveau doesn't make use
of this array.  And even if it did, the texture formats you're listing
aren't the supported ones.
>  	/* Allocate a hardware channel. */
>  	ret = nouveau_object_new(&context_dev(ctx)->object, 0xbeef0000,
>  				 NOUVEAU_FIFO_CHANNEL_CLASS,
-------------- 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/20120430/3fa8530f/attachment.pgp>

And I managed to hit "reply privately" two times in a row. -.-

Am Montag, den 30.04.2012, 20:22 +0200 schrieb Lucas
Stach:
> Am Montag, den 30.04.2012, 20:08 +0200 schrieb Francisco Jerez:
> > Lucas Stach <dev at lynxeye.de> writes:
> > 
> > > Fixes an assertion seen by users.
> > >
> > > Signed-off-by: Lucas Stach <dev at lynxeye.de>
> > > Tested-by: JohnDoe_71Rus on irc
> > > ---
> > >  src/mesa/drivers/dri/nouveau/nouveau_context.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> > > index 4845767..46c0d70 100644
> > > --- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
> > > +++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> > > @@ -117,6 +117,15 @@ nouveau_context_init(struct gl_context *ctx,
struct nouveau_screen *screen,
> > >  	nouveau_span_functions_init(ctx);
> > >  	_mesa_allow_light_in_model(ctx, GL_FALSE);
> > >  
> > > +	/* only advertise supported texture formats */
> > > +	memset(&ctx->TextureFormatSupported, 0,
> > > +	       sizeof(ctx->TextureFormatSupported));
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_XRGB8888] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_ARGB8888] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_RGB565] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_Z16] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_Z24_S8] = true;
> > > +
> > Hi Lucas,
> > 
> > It's not obvious to me how this works.  AFAIK nouveau doesn't
make use
> > of this array.  And even if it did, the texture formats you're
listing
> > aren't the supported ones.
> 
> Hello Francisco,
> 
> The core mesa code decides what texture formats to advertise based on
> this array. And obviously someone thought it would be a good idea to
> init this array to "everything supported" if the driver
doesn't
> overrides this.
> 
> I opted to only advertise renderable texture formats, for reasons
> explained in my mail a minute ago.
> 
> -- Lucas
> > 
> > >  	/* Allocate a hardware channel. */
> > >  	ret = nouveau_object_new(&context_dev(ctx)->object,
0xbeef0000,
> > >  				 NOUVEAU_FIFO_CHANNEL_CLASS,
>

And I managed to hit "reply privately" two times in a row. -.-

Am Montag, den 30.04.2012, 20:19 +0200 schrieb Lucas
Stach:
> Am Montag, den 30.04.2012, 19:54 +0200 schrieb Christoph Bumiller:
> > On 30.04.2012 19:16, Lucas Stach wrote:
> > > Fixes an assertion seen by users.
> > >
> > > Signed-off-by: Lucas Stach <dev at lynxeye.de>
> > > Tested-by: JohnDoe_71Rus on irc
> > > ---
> > >  src/mesa/drivers/dri/nouveau/nouveau_context.c |    9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > >
> > > diff --git a/src/mesa/drivers/dri/nouveau/nouveau_context.c
b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> > > index 4845767..46c0d70 100644
> > > --- a/src/mesa/drivers/dri/nouveau/nouveau_context.c
> > > +++ b/src/mesa/drivers/dri/nouveau/nouveau_context.c
> > > @@ -117,6 +117,15 @@ nouveau_context_init(struct gl_context *ctx,
struct nouveau_screen *screen,
> > >  	nouveau_span_functions_init(ctx);
> > >  	_mesa_allow_light_in_model(ctx, GL_FALSE);
> > >  
> > > +	/* only advertise supported texture formats */
> > > +	memset(&ctx->TextureFormatSupported, 0,
> > > +	       sizeof(ctx->TextureFormatSupported));
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_XRGB8888] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_ARGB8888] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_RGB565] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_Z16] = true;
> > > +	ctx->TextureFormatSupported[MESA_FORMAT_Z24_S8] = true;
> > > +
> > 
> > But it also seems to support ARGB1555, L8, A8 and I8, see
> > nouveau_choose_tex_format ...
> 
> Right, but I have no idea if we are able to render to those formats,
> which seems unlikely. Sadly I see no way to specify a texture format
> only for sampling in TextureFormatSupported, that's why I opted for the
> safe option here. We may have to revisit this at the latest when
> considering the S3TC work.
> 
> In any case this patch fixes an actual problem, which breaks every
> application that doesn't by luck choose one of the safe formats, so I
> would really like to see this patch pushed, even if it's only a stop
> gap.
> 
> -- Lucas
> > 
> > >  	/* Allocate a hardware channel. */
> > >  	ret = nouveau_object_new(&context_dev(ctx)->object,
0xbeef0000,
> > >  				 NOUVEAU_FIFO_CHANNEL_CLASS,
> > 
>

Lucas Stach <dev at lynxeye.de> writes:
> Am Montag, den 30.04.2012, 20:08 +0200 schrieb Francisco Jerez:
>> Lucas Stach <dev at lynxeye.de> writes:
>>[...]
>> > +	/* only advertise supported texture formats */
>> > +	memset(&ctx->TextureFormatSupported, 0,
>> > +	       sizeof(ctx->TextureFormatSupported));
>> > +	ctx->TextureFormatSupported[MESA_FORMAT_XRGB8888] = true;
>> > +	ctx->TextureFormatSupported[MESA_FORMAT_ARGB8888] = true;
>> > +	ctx->TextureFormatSupported[MESA_FORMAT_RGB565] = true;
>> > +	ctx->TextureFormatSupported[MESA_FORMAT_Z16] = true;
>> > +	ctx->TextureFormatSupported[MESA_FORMAT_Z24_S8] = true;
>> > +
>> Hi Lucas,
>> 
>> It's not obvious to me how this works.  AFAIK nouveau doesn't
make use
>> of this array.  And even if it did, the texture formats you're
listing
>> aren't the supported ones.
>
> Hello Francisco,
>
> The core mesa code decides what texture formats to advertise based on
> this array.
Are you sure?  TextureFormatSupported doesn't seem to have any effect
except in drivers that use mesa's default for the ChooseTextureFormat
hook.
> And obviously someone thought it would be a good idea to
> init this array to "everything supported" if the driver
doesn't
> overrides this.
>
> I opted to only advertise renderable texture formats, for reasons
> explained in my mail a minute ago.
-------------- 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/20120430/b149c516/attachment.pgp>