Re: [PATCH] glamor: fallback if font is too large for FBO size.

[Keith Packard]

Dave Airlie  writes:

> from glTexImage2D man page:
>GL_INVALID_VALUE is generated if width is less than 0 or greater than
>GL_MAX_TEXTURE_SIZE.
>
> Granted we should bail early since we know we are going to fail.

Ah, ok. Maybe we should check for that as well then, just in case the
driver has some other crazy error states.

Sorry for not adding my

Reviewed-by: Keith Packard 

before!

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] glamor: fallback if font is too large for FBO size.

[Keith Packard]

Dave Airlie  writes:

> From: Dave Airlie 
>
> running xfontsel on haswell here, with a max texture size
> of 8kx8k, one font wants 9711 height. This fallsback to
> sw in this case.
>
> A proper solution probably involves using an array texture.

Given that we almost always need more height than width for 16-bit
fonts, a single texture solution could be to divide the texture in half
horizontally for the 16-bit font case.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] glamor: fallback if font is too large for FBO size.

[Keith Packard]

Dave Airlie  writes:

> From: Dave Airlie 
>
> running xfontsel on haswell here, with a max texture size
> of 8kx8k, one font wants 9711 height. This fallsback to
> sw in this case.
>
> A proper solution probably involves using an array texture.
>
> Signed-off-by: Dave Airlie 
> ---
>  glamor/glamor_font.c | 4 
>  1 file changed, 4 insertions(+)
>
> diff --git a/glamor/glamor_font.c b/glamor/glamor_font.c
> index 6753d50..bda1b58 100644
> --- a/glamor/glamor_font.c
> +++ b/glamor/glamor_font.c
> @@ -80,6 +80,10 @@ glamor_font_get(ScreenPtr screen, FontPtr font)
>  overall_width = glyph_width_bytes * num_cols;
>  overall_height = glyph_height * num_rows;
>  
> +if (overall_width > glamor_priv->max_fbo_size ||
> +overall_height > glamor_priv->max_fbo_size) {
> +return NULL;
> +}

I wonder why glTexImage2D didn't return GL_OUT_OF_MEMORY for this case;
we check for that. Maybe there's a different error we should be looking
for?

In any case, bailing early seems like a fine plan, and this looks like
the right solution

>  bits = malloc(overall_width * overall_height);
>  if (!bits)
>  return NULL;

I note that the case where GL_OUT_OF_MEMORY is returned fails to free
the allocated bits (oops!). Here's a patch that just moves the free
above the error return:

diff --git a/glamor/glamor_font.c b/glamor/glamor_font.c
index 6753d50..3925f2a 100644
--- a/glamor/glamor_font.c
+++ b/glamor/glamor_font.c
@@ -132,11 +132,12 @@ glamor_font_get(ScreenPtr screen, FontPtr font)
 glTexImage2D(GL_TEXTURE_2D, 0, GL_R8UI, overall_width, overall_height,
  0, GL_RED_INTEGER, GL_UNSIGNED_BYTE, bits);
 glamor_priv->suppress_gl_out_of_memory_logging = false;
-if (glGetError() == GL_OUT_OF_MEMORY)
-return NULL;
 
 free(bits);
 
+if (glGetError() == GL_OUT_OF_MEMORY)
+return NULL;
+
 glamor_font->realized = TRUE;
 
 return glamor_font;


-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH] glamor: fallback if font is too large for FBO size.

[Dave Airlie]

On 12 January 2016 at 16:15, Keith Packard  wrote:
> Dave Airlie  writes:
>
>> From: Dave Airlie 
>>
>> running xfontsel on haswell here, with a max texture size
>> of 8kx8k, one font wants 9711 height. This fallsback to
>> sw in this case.
>>
>> A proper solution probably involves using an array texture.
>>
>> Signed-off-by: Dave Airlie 
>> ---
>>  glamor/glamor_font.c | 4 
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/glamor/glamor_font.c b/glamor/glamor_font.c
>> index 6753d50..bda1b58 100644
>> --- a/glamor/glamor_font.c
>> +++ b/glamor/glamor_font.c
>> @@ -80,6 +80,10 @@ glamor_font_get(ScreenPtr screen, FontPtr font)
>>  overall_width = glyph_width_bytes * num_cols;
>>  overall_height = glyph_height * num_rows;
>>
>> +if (overall_width > glamor_priv->max_fbo_size ||
>> +overall_height > glamor_priv->max_fbo_size) {
>> +return NULL;
>> +}
>
> I wonder why glTexImage2D didn't return GL_OUT_OF_MEMORY for this case;
> we check for that. Maybe there's a different error we should be looking
> for?

from glTexImage2D man page:
   GL_INVALID_VALUE is generated if width is less than 0 or greater than
   GL_MAX_TEXTURE_SIZE.

Granted we should bail early since we know we are going to fail.

>
> In any case, bailing early seems like a fine plan, and this looks like
> the right solution
>
>>  bits = malloc(overall_width * overall_height);
>>  if (!bits)
>>  return NULL;
>
> I note that the case where GL_OUT_OF_MEMORY is returned fails to free
> the allocated bits (oops!). Here's a patch that just moves the free
> above the error return:

Reviewed-by: Dave Airlie  for your hunk to move bits.

Dave.
>
> diff --git a/glamor/glamor_font.c b/glamor/glamor_font.c
> index 6753d50..3925f2a 100644
> --- a/glamor/glamor_font.c
> +++ b/glamor/glamor_font.c
> @@ -132,11 +132,12 @@ glamor_font_get(ScreenPtr screen, FontPtr font)
>  glTexImage2D(GL_TEXTURE_2D, 0, GL_R8UI, overall_width, overall_height,
>   0, GL_RED_INTEGER, GL_UNSIGNED_BYTE, bits);
>  glamor_priv->suppress_gl_out_of_memory_logging = false;
> -if (glGetError() == GL_OUT_OF_MEMORY)
> -return NULL;
>
>  free(bits);
>
> +if (glGetError() == GL_OUT_OF_MEMORY)
> +return NULL;
> +
>  glamor_font->realized = TRUE;
>
>  return glamor_font;
>
>
> --
> -keith
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

[PATCH] glamor: fallback if font is too large for FBO size.

[Dave Airlie]

From: Dave Airlie 

running xfontsel on haswell here, with a max texture size
of 8kx8k, one font wants 9711 height. This fallsback to
sw in this case.

A proper solution probably involves using an array texture.

Signed-off-by: Dave Airlie 
---
 glamor/glamor_font.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/glamor/glamor_font.c b/glamor/glamor_font.c
index 6753d50..bda1b58 100644
--- a/glamor/glamor_font.c
+++ b/glamor/glamor_font.c
@@ -80,6 +80,10 @@ glamor_font_get(ScreenPtr screen, FontPtr font)
 overall_width = glyph_width_bytes * num_cols;
 overall_height = glyph_height * num_rows;
 
+if (overall_width > glamor_priv->max_fbo_size ||
+overall_height > glamor_priv->max_fbo_size) {
+return NULL;
+}
 bits = malloc(overall_width * overall_height);
 if (!bits)
 return NULL;
-- 
2.4.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel