Re: [Mesa-dev] [PATCH 14/15] intel: Refactor intel_screen_make_configs
On 07/23/2012 01:35 PM, Ian Romanick wrote: On 07/21/2012 05:36 PM, Chad Versace wrote: Transform the code from clever, obfuscated, and imperative to straight-forward and table-driven. Does the output of glxinfo change? It's somewhat advantageous for the IDs associated with a specific set of values to remain the same. We have a lot of bug reports, for example, that list specific IDs. It makes it a lot easier to reproduce the bugs that way. Ok. In the final patch, I'll be sure to verify that the config order remains unchanged. I'm rewriting this patch based on Eric's feedback, so I'll CC you on v2. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 14/15] intel: Refactor intel_screen_make_configs
On 07/25/2012 11:11 AM, Eric Anholt wrote: Chad Versace chad.vers...@linux.intel.com writes: Transform the code from clever, obfuscated, and imperative to straight-forward and table-driven. I don't like this change. It increases the amount of code, and to change any single option, you need to change more code. If we were to change back_buffer_modes (and I think we need to, since we potentially pageflip instead of blitting, so we don't adhere to GLX_SWAP_COPY_OML), we would need to hit each struct's count instead of just the shared list. And MSAA was incoherent with single sample in those bits for reasons I don't understand. If we were to expose 16 and 24 depth on both 16 and 32 color, we would need to duplicate it across all the color setup, instead of just removing the if/else in the previous code and bumping the count. If we were to choose whether to expose non-stencil depth configs based on whether the hardware requires separate stencil or not... I don't know what we would do. If I was to go cleaning up config creation code, I would change driCreateConfigs to have sentinels terminating the arrays instead of separate counts. Agreed. After revisiting this patch, I no longer like it either. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 14/15] intel: Refactor intel_screen_make_configs
Chad Versace chad.vers...@linux.intel.com writes: Transform the code from clever, obfuscated, and imperative to straight-forward and table-driven. I don't like this change. It increases the amount of code, and to change any single option, you need to change more code. If we were to change back_buffer_modes (and I think we need to, since we potentially pageflip instead of blitting, so we don't adhere to GLX_SWAP_COPY_OML), we would need to hit each struct's count instead of just the shared list. And MSAA was incoherent with single sample in those bits for reasons I don't understand. If we were to expose 16 and 24 depth on both 16 and 32 color, we would need to duplicate it across all the color setup, instead of just removing the if/else in the previous code and bumping the count. If we were to choose whether to expose non-stencil depth configs based on whether the hardware requires separate stencil or not... I don't know what we would do. If I was to go cleaning up config creation code, I would change driCreateConfigs to have sentinels terminating the arrays instead of separate counts. pgpQT5pGnP5Nt.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 14/15] intel: Refactor intel_screen_make_configs
On 07/21/2012 05:36 PM, Chad Versace wrote:
Transform the code from clever, obfuscated, and imperative to
straight-forward and table-driven.
Does the output of glxinfo change? It's somewhat advantageous for the
IDs associated with a specific set of values to remain the same. We
have a lot of bug reports, for example, that list specific IDs. It
makes it a lot easier to reproduce the bugs that way.
CC: Ian Romanick i...@freedesktop.org
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/intel/intel_screen.c | 167 +-
1 file changed, 97 insertions(+), 70 deletions(-)
diff --git a/src/mesa/drivers/dri/intel/intel_screen.c
b/src/mesa/drivers/dri/intel/intel_screen.c
index 9bb42dd..61daea7 100644
--- a/src/mesa/drivers/dri/intel/intel_screen.c
+++ b/src/mesa/drivers/dri/intel/intel_screen.c
@@ -851,85 +851,112 @@ intel_detect_swizzling(struct intel_screen *screen)
static __DRIconfig**
intel_screen_make_configs(__DRIscreen *dri_screen)
{
- static const GLenum back_buffer_modes[] = {
- GLX_NONE, GLX_SWAP_UNDEFINED_OML, GLX_SWAP_COPY_OML
+ struct config_params {
+ GLenum color_format;
+ GLenum color_type;
+ const uint8_t depth_sizes[4];
+ const uint8_t stencil_sizes[4];
Do you really mean const here?
+ unsigned num_depth_stencil_sizes;
+ unsigned num_back_buffer_modes;
+ unsigned num_msaa_modes;
+ bool enable_accum;
};
- GLenum fb_format[3];
- GLenum fb_type[3];
- uint8_t depth_bits[4], stencil_bits[4], msaa_samples_array[1];
- int color;
- __DRIconfig **configs = NULL;
-
- msaa_samples_array[0] = 0;
-
- fb_format[0] = GL_RGB;
- fb_type[0] = GL_UNSIGNED_SHORT_5_6_5;
-
- fb_format[1] = GL_BGR;
- fb_type[1] = GL_UNSIGNED_INT_8_8_8_8_REV;
-
- fb_format[2] = GL_BGRA;
- fb_type[2] = GL_UNSIGNED_INT_8_8_8_8_REV;
+ static const GLenum back_buffer_modes[] = {
+ GLX_SWAP_UNDEFINED_OML, GLX_NONE, GLX_SWAP_COPY_OML,
+ };
- depth_bits[0] = 0;
- stencil_bits[0] = 0;
+ static const uint8_t msaa_samples[] = {0};
- /* Generate a rich set of useful configs that do not include an
-* accumulation buffer.
+ /* Starting with DRI2 protocol version 1.1 we can request a depth/stencil
+* buffer that has a different number of bits per pixel than the color
+* buffer. This isn't yet supported here.
*/
- for (color = 0; color ARRAY_SIZE(fb_format); color++) {
- __DRIconfig **new_configs;
- int depth_factor;
-
- /* Starting with DRI2 protocol version 1.1 we can request a depth/stencil
- * buffer that has a different number of bits per pixel than the color
- * buffer. This isn't yet supported here.
+ struct config_params params[] = {
It seems like this should be const instead of the pair of fields in the
structure.
+ /* Configs without accumulation buffer. */
+ {
+ .color_format = GL_RGB,
+ .color_type = GL_UNSIGNED_SHORT_5_6_5,
+ .depth_sizes = {0, 16},
+ .stencil_sizes = {0, 0},
+ .num_depth_stencil_sizes = 2,
+ .num_back_buffer_modes = 3,
+ .num_msaa_modes = 1,
+ .enable_accum = false,
+ },
+ {
+ .color_format = GL_BGR,
+ .color_type = GL_UNSIGNED_INT_8_8_8_8_REV,
+ .depth_sizes = {0, 24},
+ .stencil_sizes = {0, 8},
+ .num_depth_stencil_sizes = 2,
+ .num_back_buffer_modes = 3,
+ .num_msaa_modes = 1,
+ .enable_accum = false,
+ },
+ {
+ .color_format = GL_BGRA,
+ .color_type = GL_UNSIGNED_INT_8_8_8_8_REV,
+ .depth_sizes = {0, 24},
+ .stencil_sizes = {0, 8},
+ .num_depth_stencil_sizes = 2,
+ .num_back_buffer_modes = 3,
+ .num_msaa_modes = 1,
+ .enable_accum = false,
+ },
+
+ /* Configs with accumulation buffer.
+ *
+ * We generate the minimum possible set of configs that include an
+ * accumulation buffer.
*/
- if (fb_type[color] == GL_UNSIGNED_SHORT_5_6_5) {
- depth_bits[1] = 16;
- stencil_bits[1] = 0;
- } else {
- depth_bits[1] = 24;
- stencil_bits[1] = 8;
- }
-
- depth_factor = 2;
-
- new_configs = driCreateConfigs(fb_format[color], fb_type[color],
- depth_bits,
- stencil_bits,
- depth_factor,
- back_buffer_modes,
- ARRAY_SIZE(back_buffer_modes),
- msaa_samples_array,
- ARRAY_SIZE(msaa_samples_array),
- false);
- if (configs == NULL)
- configs = new_configs;
- else
- configs = driConcatConfigs(configs, new_configs);
- }
+ {
+ .color_format = GL_RGB,
+
[Mesa-dev] [PATCH 14/15] intel: Refactor intel_screen_make_configs
Transform the code from clever, obfuscated, and imperative to
straight-forward and table-driven.
CC: Ian Romanick i...@freedesktop.org
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/intel/intel_screen.c | 167 +-
1 file changed, 97 insertions(+), 70 deletions(-)
diff --git a/src/mesa/drivers/dri/intel/intel_screen.c
b/src/mesa/drivers/dri/intel/intel_screen.c
index 9bb42dd..61daea7 100644
--- a/src/mesa/drivers/dri/intel/intel_screen.c
+++ b/src/mesa/drivers/dri/intel/intel_screen.c
@@ -851,85 +851,112 @@ intel_detect_swizzling(struct intel_screen *screen)
static __DRIconfig**
intel_screen_make_configs(__DRIscreen *dri_screen)
{
- static const GLenum back_buffer_modes[] = {
- GLX_NONE, GLX_SWAP_UNDEFINED_OML, GLX_SWAP_COPY_OML
+ struct config_params {
+ GLenum color_format;
+ GLenum color_type;
+ const uint8_t depth_sizes[4];
+ const uint8_t stencil_sizes[4];
+ unsigned num_depth_stencil_sizes;
+ unsigned num_back_buffer_modes;
+ unsigned num_msaa_modes;
+ bool enable_accum;
};
- GLenum fb_format[3];
- GLenum fb_type[3];
- uint8_t depth_bits[4], stencil_bits[4], msaa_samples_array[1];
- int color;
- __DRIconfig **configs = NULL;
-
- msaa_samples_array[0] = 0;
-
- fb_format[0] = GL_RGB;
- fb_type[0] = GL_UNSIGNED_SHORT_5_6_5;
-
- fb_format[1] = GL_BGR;
- fb_type[1] = GL_UNSIGNED_INT_8_8_8_8_REV;
-
- fb_format[2] = GL_BGRA;
- fb_type[2] = GL_UNSIGNED_INT_8_8_8_8_REV;
+ static const GLenum back_buffer_modes[] = {
+ GLX_SWAP_UNDEFINED_OML, GLX_NONE, GLX_SWAP_COPY_OML,
+ };
- depth_bits[0] = 0;
- stencil_bits[0] = 0;
+ static const uint8_t msaa_samples[] = {0};
- /* Generate a rich set of useful configs that do not include an
-* accumulation buffer.
+ /* Starting with DRI2 protocol version 1.1 we can request a depth/stencil
+* buffer that has a different number of bits per pixel than the color
+* buffer. This isn't yet supported here.
*/
- for (color = 0; color ARRAY_SIZE(fb_format); color++) {
- __DRIconfig **new_configs;
- int depth_factor;
-
- /* Starting with DRI2 protocol version 1.1 we can request a depth/stencil
- * buffer that has a different number of bits per pixel than the color
- * buffer. This isn't yet supported here.
+ struct config_params params[] = {
+ /* Configs without accumulation buffer. */
+ {
+ .color_format = GL_RGB,
+ .color_type = GL_UNSIGNED_SHORT_5_6_5,
+ .depth_sizes = {0, 16},
+ .stencil_sizes = {0, 0},
+ .num_depth_stencil_sizes = 2,
+ .num_back_buffer_modes = 3,
+ .num_msaa_modes = 1,
+ .enable_accum = false,
+ },
+ {
+ .color_format = GL_BGR,
+ .color_type = GL_UNSIGNED_INT_8_8_8_8_REV,
+ .depth_sizes = {0, 24},
+ .stencil_sizes = {0, 8},
+ .num_depth_stencil_sizes = 2,
+ .num_back_buffer_modes = 3,
+ .num_msaa_modes = 1,
+ .enable_accum = false,
+ },
+ {
+ .color_format = GL_BGRA,
+ .color_type = GL_UNSIGNED_INT_8_8_8_8_REV,
+ .depth_sizes = {0, 24},
+ .stencil_sizes = {0, 8},
+ .num_depth_stencil_sizes = 2,
+ .num_back_buffer_modes = 3,
+ .num_msaa_modes = 1,
+ .enable_accum = false,
+ },
+
+ /* Configs with accumulation buffer.
+ *
+ * We generate the minimum possible set of configs that include an
+ * accumulation buffer.
*/
- if (fb_type[color] == GL_UNSIGNED_SHORT_5_6_5) {
- depth_bits[1] = 16;
- stencil_bits[1] = 0;
- } else {
- depth_bits[1] = 24;
- stencil_bits[1] = 8;
- }
-
- depth_factor = 2;
-
- new_configs = driCreateConfigs(fb_format[color], fb_type[color],
- depth_bits,
- stencil_bits,
- depth_factor,
- back_buffer_modes,
- ARRAY_SIZE(back_buffer_modes),
- msaa_samples_array,
- ARRAY_SIZE(msaa_samples_array),
- false);
- if (configs == NULL)
- configs = new_configs;
- else
- configs = driConcatConfigs(configs, new_configs);
- }
+ {
+ .color_format = GL_RGB,
+ .color_type = GL_UNSIGNED_SHORT_5_6_5,
+ .depth_sizes = {16},
+ .stencil_sizes = {0},
+ .num_depth_stencil_sizes = 1,
+ .num_back_buffer_modes = 1,
+ .num_msaa_modes = 1,
+ .enable_accum = true,
+ },
+ {
+ .color_format = GL_BGR,
+ .color_type = GL_UNSIGNED_INT_8_8_8_8_REV,
+ .depth_sizes = {24},
+ .stencil_sizes = {8},
+
