Re: [Mesa-dev] [PATCH V4 1/5] i965 Gen4/5: Compute interpolation status for every varying in one place.
I like that. It also provides a sensible type to pass to the new `is
there any flat shading?` helper.
On Wed, Jul 31, 2013 at 9:52 AM, Paul Berry wrote:
> On 23 July 2013 01:16, Chris Forbes wrote:
>>
>> The program keys are updated accordingly, but the values are not used
>> yet.
>>
>> [V1-2]: Signed-off-by: Olivier Galibert
>>
>> V3: Updated for vue_map changes, intel -> brw merge, etc. (Chris Forbes)
>> V4: Compute interpolation map as a new state atom rather than tacking it
>> on the front of the clip setup
>>
>> Signed-off-by: Chris Forbes
>> ---
>> src/mesa/drivers/dri/i965/Makefile.sources| 1 +
>> src/mesa/drivers/dri/i965/brw_clip.c | 8 ++-
>> src/mesa/drivers/dri/i965/brw_clip.h | 1 +
>> src/mesa/drivers/dri/i965/brw_context.h | 7 ++
>> src/mesa/drivers/dri/i965/brw_interpolation_map.c | 85
>> +++
>> src/mesa/drivers/dri/i965/brw_sf.c| 7 +-
>> src/mesa/drivers/dri/i965/brw_sf.h| 1 +
>> src/mesa/drivers/dri/i965/brw_state.h | 1 +
>> src/mesa/drivers/dri/i965/brw_state_upload.c | 3 +
>> 9 files changed, 112 insertions(+), 2 deletions(-)
>> create mode 100644 src/mesa/drivers/dri/i965/brw_interpolation_map.c
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
>> b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 1f401fe..ac8487b 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -62,6 +62,7 @@ i965_FILES = \
>> brw_gs.c \
>> brw_gs_emit.c \
>> brw_gs_state.c \
>> + brw_interpolation_map.c \
>> brw_lower_texture_gradients.cpp \
>> brw_misc_state.c \
>> brw_program.c \
>> diff --git a/src/mesa/drivers/dri/i965/brw_clip.c
>> b/src/mesa/drivers/dri/i965/brw_clip.c
>> index 2ebf3f6..60d85e8 100644
>> --- a/src/mesa/drivers/dri/i965/brw_clip.c
>> +++ b/src/mesa/drivers/dri/i965/brw_clip.c
>> @@ -141,6 +141,10 @@ brw_upload_clip_prog(struct brw_context *brw)
>>
>> /* Populate the key:
>> */
>> +
>> + /* BRW_NEW_INTERPOLATION_MAP */
>> + memcpy(key.interpolation_mode, brw->interpolation_mode,
>> BRW_VARYING_SLOT_COUNT);
>
>
> Random idea, which you may feel free to disregard: What if we make
> interpolation_mode a struct, e.g.:
>
> struct interpolation_mode_map
> {
> unsigned char mode[BRW_VARYING_SLOT_COUNT];
> }
>
> That way, this memcpy can just change to:
>
> key.interpolation_mode = brw->interpolation_mode;
>
> And the compiler takes care of making sure the types match and figuring out
> the right number of bytes to copy.
>
> The disadvantage, of course, is that everywhere you use
> key.interpolation_mode[...] in the rest of the series you'll have to replace
> that with key.interpolation_mode.mode[...], which looks pretty silly :)
>
> Either way, this patch is:
>
> Reviewed-by: Paul Berry
>
>>
>> +
>> /* BRW_NEW_REDUCED_PRIMITIVE */
>> key.primitive = brw->reduced_primitive;
>> /* BRW_NEW_VUE_MAP_GEOM_OUT */
>> @@ -256,7 +260,9 @@ const struct brw_tracked_state brw_clip_prog = {
>> _NEW_TRANSFORM |
>> _NEW_POLYGON |
>> _NEW_BUFFERS),
>> - .brw = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
>> + .brw = (BRW_NEW_REDUCED_PRIMITIVE |
>> +BRW_NEW_VUE_MAP_GEOM_OUT |
>> +BRW_NEW_INTERPOLATION_MAP)
>> },
>> .emit = brw_upload_clip_prog
>> };
>> diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
>> b/src/mesa/drivers/dri/i965/brw_clip.h
>> index 02259d4..fcbe2a0 100644
>> --- a/src/mesa/drivers/dri/i965/brw_clip.h
>> +++ b/src/mesa/drivers/dri/i965/brw_clip.h
>> @@ -43,6 +43,7 @@
>> */
>> struct brw_clip_prog_key {
>> GLbitfield64 attrs;
>> + unsigned char interpolation_mode[BRW_VARYING_SLOT_COUNT];
>> GLuint primitive:4;
>> GLuint nr_userclip:4;
>> GLuint do_flat_shading:1;
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h
>> index 86f9f71..29e522c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -154,6 +154,7 @@ enum brw_state_id {
>> BRW_STATE_STATS_WM,
>> BRW_STATE_UNIFORM_BUFFER,
>> BRW_STATE_META_IN_PROGRESS,
>> + BRW_STATE_INTERPOLATION_MAP,
>> };
>>
>> #define BRW_NEW_URB_FENCE (1 << BRW_STATE_URB_FENCE)
>> @@ -186,6 +187,7 @@ enum brw_state_id {
>> #define BRW_NEW_STATS_WM (1 << BRW_STATE_STATS_WM)
>> #define BRW_NEW_UNIFORM_BUFFER (1 << BRW_STATE_UNIFORM_BUFFER)
>> #define BRW_NEW_META_IN_PROGRESS(1 << BRW_STATE_META_IN_PROGRESS)
>> +#define BRW_NEW_INTERPOLATION_MAP (1 <<
>> BRW_STATE_INTERPOLATION_MAP)
>>
>> struct brw_state_flags {
>> /** State update flags signalled by mesa internals */
>> @@ -1203,6 +1205,11 @@ struct brw_context
>> uint32_t render_target_format[MESA_FORMAT_COUNT];
>
Re: [Mesa-dev] [PATCH V4 1/5] i965 Gen4/5: Compute interpolation status for every varying in one place.
On 23 July 2013 01:16, Chris Forbes wrote:
> The program keys are updated accordingly, but the values are not used
> yet.
>
> [V1-2]: Signed-off-by: Olivier Galibert
>
> V3: Updated for vue_map changes, intel -> brw merge, etc. (Chris Forbes)
> V4: Compute interpolation map as a new state atom rather than tacking it
> on the front of the clip setup
>
> Signed-off-by: Chris Forbes
> ---
> src/mesa/drivers/dri/i965/Makefile.sources| 1 +
> src/mesa/drivers/dri/i965/brw_clip.c | 8 ++-
> src/mesa/drivers/dri/i965/brw_clip.h | 1 +
> src/mesa/drivers/dri/i965/brw_context.h | 7 ++
> src/mesa/drivers/dri/i965/brw_interpolation_map.c | 85
> +++
> src/mesa/drivers/dri/i965/brw_sf.c| 7 +-
> src/mesa/drivers/dri/i965/brw_sf.h| 1 +
> src/mesa/drivers/dri/i965/brw_state.h | 1 +
> src/mesa/drivers/dri/i965/brw_state_upload.c | 3 +
> 9 files changed, 112 insertions(+), 2 deletions(-)
> create mode 100644 src/mesa/drivers/dri/i965/brw_interpolation_map.c
>
> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources
> b/src/mesa/drivers/dri/i965/Makefile.sources
> index 1f401fe..ac8487b 100644
> --- a/src/mesa/drivers/dri/i965/Makefile.sources
> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
> @@ -62,6 +62,7 @@ i965_FILES = \
> brw_gs.c \
> brw_gs_emit.c \
> brw_gs_state.c \
> + brw_interpolation_map.c \
> brw_lower_texture_gradients.cpp \
> brw_misc_state.c \
> brw_program.c \
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.c
> b/src/mesa/drivers/dri/i965/brw_clip.c
> index 2ebf3f6..60d85e8 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.c
> +++ b/src/mesa/drivers/dri/i965/brw_clip.c
> @@ -141,6 +141,10 @@ brw_upload_clip_prog(struct brw_context *brw)
>
> /* Populate the key:
> */
> +
> + /* BRW_NEW_INTERPOLATION_MAP */
> + memcpy(key.interpolation_mode, brw->interpolation_mode,
> BRW_VARYING_SLOT_COUNT);
>
Random idea, which you may feel free to disregard: What if we make
interpolation_mode a struct, e.g.:
struct interpolation_mode_map
{
unsigned char mode[BRW_VARYING_SLOT_COUNT];
}
That way, this memcpy can just change to:
key.interpolation_mode = brw->interpolation_mode;
And the compiler takes care of making sure the types match and figuring out
the right number of bytes to copy.
The disadvantage, of course, is that everywhere you use
key.interpolation_mode[...] in the rest of the series you'll have to
replace that with key.interpolation_mode.mode[...], which looks pretty
silly :)
Either way, this patch is:
Reviewed-by: Paul Berry
> +
> /* BRW_NEW_REDUCED_PRIMITIVE */
> key.primitive = brw->reduced_primitive;
> /* BRW_NEW_VUE_MAP_GEOM_OUT */
> @@ -256,7 +260,9 @@ const struct brw_tracked_state brw_clip_prog = {
> _NEW_TRANSFORM |
> _NEW_POLYGON |
> _NEW_BUFFERS),
> - .brw = (BRW_NEW_REDUCED_PRIMITIVE | BRW_NEW_VUE_MAP_GEOM_OUT)
> + .brw = (BRW_NEW_REDUCED_PRIMITIVE |
> +BRW_NEW_VUE_MAP_GEOM_OUT |
> +BRW_NEW_INTERPOLATION_MAP)
> },
> .emit = brw_upload_clip_prog
> };
> diff --git a/src/mesa/drivers/dri/i965/brw_clip.h
> b/src/mesa/drivers/dri/i965/brw_clip.h
> index 02259d4..fcbe2a0 100644
> --- a/src/mesa/drivers/dri/i965/brw_clip.h
> +++ b/src/mesa/drivers/dri/i965/brw_clip.h
> @@ -43,6 +43,7 @@
> */
> struct brw_clip_prog_key {
> GLbitfield64 attrs;
> + unsigned char interpolation_mode[BRW_VARYING_SLOT_COUNT];
> GLuint primitive:4;
> GLuint nr_userclip:4;
> GLuint do_flat_shading:1;
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 86f9f71..29e522c 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -154,6 +154,7 @@ enum brw_state_id {
> BRW_STATE_STATS_WM,
> BRW_STATE_UNIFORM_BUFFER,
> BRW_STATE_META_IN_PROGRESS,
> + BRW_STATE_INTERPOLATION_MAP,
> };
>
> #define BRW_NEW_URB_FENCE (1 << BRW_STATE_URB_FENCE)
> @@ -186,6 +187,7 @@ enum brw_state_id {
> #define BRW_NEW_STATS_WM (1 << BRW_STATE_STATS_WM)
> #define BRW_NEW_UNIFORM_BUFFER (1 << BRW_STATE_UNIFORM_BUFFER)
> #define BRW_NEW_META_IN_PROGRESS(1 << BRW_STATE_META_IN_PROGRESS)
> +#define BRW_NEW_INTERPOLATION_MAP (1 << BRW_STATE_INTERPOLATION_MAP)
>
> struct brw_state_flags {
> /** State update flags signalled by mesa internals */
> @@ -1203,6 +1205,11 @@ struct brw_context
> uint32_t render_target_format[MESA_FORMAT_COUNT];
> bool format_supported_as_render_target[MESA_FORMAT_COUNT];
>
> + /* Interpolation modes, one byte per vue slot.
> +* Used Gen4/5 by the clip|sf|wm stages. Ignored on Gen6+.
> +*/
> + unsigned char interpolation_mode[BRW_VARYING_SLOT_COUNT];
> +
> /* PrimitiveRestart */
> struct
