Re: [Mesa-dev] [PATCH 1/7] i965: Move texture buffer dispatch into single location
On Thu, May 7, 2015 at 8:01 AM, Pohjolainen, Topi wrote: > If Ken and Matt are happy with this series, so am I. I'm just glad if we > can land it. I don't have a preference. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/7] i965: Move texture buffer dispatch into single location
On Thu, May 07, 2015 at 05:55:48PM +0300, Francisco Jerez wrote:
> "Pohjolainen, Topi" writes:
>
> > On Thu, May 07, 2015 at 05:15:35PM +0300, Francisco Jerez wrote:
> >> From: Topi Pohjolainen
> >>
> >> All generations do the same exact dispatch and it could be
> >> therefore done in the hardware independent stage.
> >>
> >> Reviewed-by: Matt Turner
> >> Reviewed-by: Kenneth Graunke
> >> Signed-off-by: Topi Pohjolainen
> >> [ Francisco Jerez: Non-trivial rebase. ]
> >> Reviewed-by: Francisco Jerez
> >> ---
> >> src/mesa/drivers/dri/i965/brw_context.h | 3 -
> >> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 31 ++
> >> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 69
> >> +++
> >> src/mesa/drivers/dri/i965/gen8_surface_state.c| 67
> >> ++
> >> 4 files changed, 83 insertions(+), 87 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> >> b/src/mesa/drivers/dri/i965/brw_context.h
> >> index 2fcdcfa..a6282f4 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_context.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> >> @@ -1698,9 +1698,6 @@ void brw_create_constant_surface(struct brw_context
> >> *brw,
> >> uint32_t size,
> >> uint32_t *out_offset,
> >> bool dword_pitch);
> >> -void brw_update_buffer_texture_surface(struct gl_context *ctx,
> >> - unsigned unit,
> >> - uint32_t *surf_offset);
> >> void
> >> brw_update_sol_surface(struct brw_context *brw,
> >> struct gl_buffer_object *buffer_obj,
> >> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> index 160dd2f..2b8040c 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> >> @@ -274,10 +274,10 @@ gen4_emit_buffer_surface_state(struct brw_context
> >> *brw,
> >> }
> >> }
> >>
> >> -void
> >> -brw_update_buffer_texture_surface(struct gl_context *ctx,
> >> - unsigned unit,
> >> - uint32_t *surf_offset)
> >> +static void
> >> +update_buffer_texture_surface(struct gl_context *ctx,
> >> + unsigned unit,
> >> + uint32_t *surf_offset)
> >> {
> >> struct brw_context *brw = brw_context(ctx);
> >> struct gl_texture_object *tObj = ctx->Texture.Unit[unit]._Current;
> >> @@ -320,12 +320,6 @@ brw_update_texture_surface(struct gl_context *ctx,
> >> struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
> >> uint32_t *surf;
> >>
> >> - /* BRW_NEW_TEXTURE_BUFFER */
> >> - if (tObj->Target == GL_TEXTURE_BUFFER) {
> >> - brw_update_buffer_texture_surface(ctx, unit, surf_offset);
> >> - return;
> >> - }
> >> -
> >> surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> >> 6 * 4, 32, surf_offset);
> >>
> >> @@ -795,6 +789,21 @@ const struct brw_tracked_state
> >> gen6_renderbuffer_surfaces = {
> >> .emit = update_renderbuffer_surfaces,
> >> };
> >>
> >> +static void
> >> +update_texture_surface(struct gl_context *ctx,
> >> + unsigned unit,
> >> + uint32_t *surf_offset,
> >> + bool for_gather)
> >> +{
> >> + struct brw_context *brw = brw_context(ctx);
> >> + struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
> >> +
> >> + if (obj->Target == GL_TEXTURE_BUFFER) {
> >> + update_buffer_texture_surface(ctx, unit, surf_offset);
> >
> > In order to avoid extra level of indentation I used the following. I would
> > have preferred it here also.
> >
> > if (obj->Target == GL_TEXTURE_BUFFER) {
> > update_buffer_texture_surface(ctx, unit, surf_offset);
> > return;
> > }
> >
> I kept this as an indented block because it's harmless IMHO and it
> seemed a somewhat lesser evil than:
> 1/ Define all texture-specific variables (i.e. things that are not
>applicable to buffer textures, including some pointer dereferences)
>at the top level, which is what you did, but it seemed a bit dodgy.
> 2/ Mix statements and declarations. (Granted, this file is likely
>already relying on other C99 features, so it wouldn't matter in
>practice, it's just a codestyle itch)
> 3/ Declare stuff and leave it uninitialized until later.
>
> That said, the reason was largely subjective, and I don't really have a
> strong preference. As you are still the author of this commit you're
> free to format it as you wish, you can keep my R-b if you simply
> reindent this function.
If Ken and Matt are happy with this series, so am I. I'm just glad if we
can land it.
___
mesa
Re: [Mesa-dev] [PATCH 1/7] i965: Move texture buffer dispatch into single location
"Pohjolainen, Topi" writes:
> On Thu, May 07, 2015 at 05:15:35PM +0300, Francisco Jerez wrote:
>> From: Topi Pohjolainen
>>
>> All generations do the same exact dispatch and it could be
>> therefore done in the hardware independent stage.
>>
>> Reviewed-by: Matt Turner
>> Reviewed-by: Kenneth Graunke
>> Signed-off-by: Topi Pohjolainen
>> [ Francisco Jerez: Non-trivial rebase. ]
>> Reviewed-by: Francisco Jerez
>> ---
>> src/mesa/drivers/dri/i965/brw_context.h | 3 -
>> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 31 ++
>> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 69
>> +++
>> src/mesa/drivers/dri/i965/gen8_surface_state.c| 67
>> ++
>> 4 files changed, 83 insertions(+), 87 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
>> b/src/mesa/drivers/dri/i965/brw_context.h
>> index 2fcdcfa..a6282f4 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -1698,9 +1698,6 @@ void brw_create_constant_surface(struct brw_context
>> *brw,
>> uint32_t size,
>> uint32_t *out_offset,
>> bool dword_pitch);
>> -void brw_update_buffer_texture_surface(struct gl_context *ctx,
>> - unsigned unit,
>> - uint32_t *surf_offset);
>> void
>> brw_update_sol_surface(struct brw_context *brw,
>> struct gl_buffer_object *buffer_obj,
>> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> index 160dd2f..2b8040c 100644
>> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
>> @@ -274,10 +274,10 @@ gen4_emit_buffer_surface_state(struct brw_context *brw,
>> }
>> }
>>
>> -void
>> -brw_update_buffer_texture_surface(struct gl_context *ctx,
>> - unsigned unit,
>> - uint32_t *surf_offset)
>> +static void
>> +update_buffer_texture_surface(struct gl_context *ctx,
>> + unsigned unit,
>> + uint32_t *surf_offset)
>> {
>> struct brw_context *brw = brw_context(ctx);
>> struct gl_texture_object *tObj = ctx->Texture.Unit[unit]._Current;
>> @@ -320,12 +320,6 @@ brw_update_texture_surface(struct gl_context *ctx,
>> struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
>> uint32_t *surf;
>>
>> - /* BRW_NEW_TEXTURE_BUFFER */
>> - if (tObj->Target == GL_TEXTURE_BUFFER) {
>> - brw_update_buffer_texture_surface(ctx, unit, surf_offset);
>> - return;
>> - }
>> -
>> surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
>>6 * 4, 32, surf_offset);
>>
>> @@ -795,6 +789,21 @@ const struct brw_tracked_state
>> gen6_renderbuffer_surfaces = {
>> .emit = update_renderbuffer_surfaces,
>> };
>>
>> +static void
>> +update_texture_surface(struct gl_context *ctx,
>> + unsigned unit,
>> + uint32_t *surf_offset,
>> + bool for_gather)
>> +{
>> + struct brw_context *brw = brw_context(ctx);
>> + struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
>> +
>> + if (obj->Target == GL_TEXTURE_BUFFER) {
>> + update_buffer_texture_surface(ctx, unit, surf_offset);
>
> In order to avoid extra level of indentation I used the following. I would
> have preferred it here also.
>
> if (obj->Target == GL_TEXTURE_BUFFER) {
> update_buffer_texture_surface(ctx, unit, surf_offset);
> return;
> }
>
I kept this as an indented block because it's harmless IMHO and it
seemed a somewhat lesser evil than:
1/ Define all texture-specific variables (i.e. things that are not
applicable to buffer textures, including some pointer dereferences)
at the top level, which is what you did, but it seemed a bit dodgy.
2/ Mix statements and declarations. (Granted, this file is likely
already relying on other C99 features, so it wouldn't matter in
practice, it's just a codestyle itch)
3/ Declare stuff and leave it uninitialized until later.
That said, the reason was largely subjective, and I don't really have a
strong preference. As you are still the author of this commit you're
free to format it as you wish, you can keep my R-b if you simply
reindent this function.
>> + } else {
>> + brw->vtbl.update_texture_surface(ctx, unit, surf_offset, for_gather);
>> + }
>> +}
>>
>> static void
>> update_stage_texture_surfaces(struct brw_context *brw,
>> @@ -824,7 +833,7 @@ update_stage_texture_surfaces(struct brw_context *brw,
>>
>> /* _NEW_TEXTURE */
>> if (ctx->Texture.Unit[unit]._Current) {
>> -brw->vtbl.update_texture_surface(ctx, unit, surf_off
Re: [Mesa-dev] [PATCH 1/7] i965: Move texture buffer dispatch into single location
On Thu, May 07, 2015 at 05:15:35PM +0300, Francisco Jerez wrote:
> From: Topi Pohjolainen
>
> All generations do the same exact dispatch and it could be
> therefore done in the hardware independent stage.
>
> Reviewed-by: Matt Turner
> Reviewed-by: Kenneth Graunke
> Signed-off-by: Topi Pohjolainen
> [ Francisco Jerez: Non-trivial rebase. ]
> Reviewed-by: Francisco Jerez
> ---
> src/mesa/drivers/dri/i965/brw_context.h | 3 -
> src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 31 ++
> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 69
> +++
> src/mesa/drivers/dri/i965/gen8_surface_state.c| 67 ++
> 4 files changed, 83 insertions(+), 87 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 2fcdcfa..a6282f4 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1698,9 +1698,6 @@ void brw_create_constant_surface(struct brw_context
> *brw,
> uint32_t size,
> uint32_t *out_offset,
> bool dword_pitch);
> -void brw_update_buffer_texture_surface(struct gl_context *ctx,
> - unsigned unit,
> - uint32_t *surf_offset);
> void
> brw_update_sol_surface(struct brw_context *brw,
> struct gl_buffer_object *buffer_obj,
> diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> index 160dd2f..2b8040c 100644
> --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
> @@ -274,10 +274,10 @@ gen4_emit_buffer_surface_state(struct brw_context *brw,
> }
> }
>
> -void
> -brw_update_buffer_texture_surface(struct gl_context *ctx,
> - unsigned unit,
> - uint32_t *surf_offset)
> +static void
> +update_buffer_texture_surface(struct gl_context *ctx,
> + unsigned unit,
> + uint32_t *surf_offset)
> {
> struct brw_context *brw = brw_context(ctx);
> struct gl_texture_object *tObj = ctx->Texture.Unit[unit]._Current;
> @@ -320,12 +320,6 @@ brw_update_texture_surface(struct gl_context *ctx,
> struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
> uint32_t *surf;
>
> - /* BRW_NEW_TEXTURE_BUFFER */
> - if (tObj->Target == GL_TEXTURE_BUFFER) {
> - brw_update_buffer_texture_surface(ctx, unit, surf_offset);
> - return;
> - }
> -
> surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
> 6 * 4, 32, surf_offset);
>
> @@ -795,6 +789,21 @@ const struct brw_tracked_state
> gen6_renderbuffer_surfaces = {
> .emit = update_renderbuffer_surfaces,
> };
>
> +static void
> +update_texture_surface(struct gl_context *ctx,
> + unsigned unit,
> + uint32_t *surf_offset,
> + bool for_gather)
> +{
> + struct brw_context *brw = brw_context(ctx);
> + struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
> +
> + if (obj->Target == GL_TEXTURE_BUFFER) {
> + update_buffer_texture_surface(ctx, unit, surf_offset);
In order to avoid extra level of indentation I used the following. I would
have preferred it here also.
if (obj->Target == GL_TEXTURE_BUFFER) {
update_buffer_texture_surface(ctx, unit, surf_offset);
return;
}
> + } else {
> + brw->vtbl.update_texture_surface(ctx, unit, surf_offset, for_gather);
> + }
> +}
>
> static void
> update_stage_texture_surfaces(struct brw_context *brw,
> @@ -824,7 +833,7 @@ update_stage_texture_surfaces(struct brw_context *brw,
>
> /* _NEW_TEXTURE */
> if (ctx->Texture.Unit[unit]._Current) {
> -brw->vtbl.update_texture_surface(ctx, unit, surf_offset + s,
> for_gather);
> +update_texture_surface(ctx, unit, surf_offset + s, for_gather);
> }
>}
> }
> diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> index 15ab2b0..098b5c8 100644
> --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
> @@ -356,43 +356,38 @@ gen7_update_texture_surface(struct gl_context *ctx,
> struct brw_context *brw = brw_context(ctx);
> struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
>
> - if (obj->Target == GL_TEXTURE_BUFFER) {
> - brw_update_buffer_texture_surface(ctx, unit, surf_offset);
> -
> - } else {
> - struct intel_texture_object *intel_obj = intel_texture_object(obj);
> - struct intel_mipmap_tree *mt = intel_obj->mt;
> - struct gl_sampler_object *sampler = _mes
[Mesa-dev] [PATCH 1/7] i965: Move texture buffer dispatch into single location
From: Topi Pohjolainen
All generations do the same exact dispatch and it could be
therefore done in the hardware independent stage.
Reviewed-by: Matt Turner
Reviewed-by: Kenneth Graunke
Signed-off-by: Topi Pohjolainen
[ Francisco Jerez: Non-trivial rebase. ]
Reviewed-by: Francisco Jerez
---
src/mesa/drivers/dri/i965/brw_context.h | 3 -
src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 31 ++
src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 69 +++
src/mesa/drivers/dri/i965/gen8_surface_state.c| 67 ++
4 files changed, 83 insertions(+), 87 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_context.h
b/src/mesa/drivers/dri/i965/brw_context.h
index 2fcdcfa..a6282f4 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1698,9 +1698,6 @@ void brw_create_constant_surface(struct brw_context *brw,
uint32_t size,
uint32_t *out_offset,
bool dword_pitch);
-void brw_update_buffer_texture_surface(struct gl_context *ctx,
- unsigned unit,
- uint32_t *surf_offset);
void
brw_update_sol_surface(struct brw_context *brw,
struct gl_buffer_object *buffer_obj,
diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
index 160dd2f..2b8040c 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -274,10 +274,10 @@ gen4_emit_buffer_surface_state(struct brw_context *brw,
}
}
-void
-brw_update_buffer_texture_surface(struct gl_context *ctx,
- unsigned unit,
- uint32_t *surf_offset)
+static void
+update_buffer_texture_surface(struct gl_context *ctx,
+ unsigned unit,
+ uint32_t *surf_offset)
{
struct brw_context *brw = brw_context(ctx);
struct gl_texture_object *tObj = ctx->Texture.Unit[unit]._Current;
@@ -320,12 +320,6 @@ brw_update_texture_surface(struct gl_context *ctx,
struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
uint32_t *surf;
- /* BRW_NEW_TEXTURE_BUFFER */
- if (tObj->Target == GL_TEXTURE_BUFFER) {
- brw_update_buffer_texture_surface(ctx, unit, surf_offset);
- return;
- }
-
surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,
6 * 4, 32, surf_offset);
@@ -795,6 +789,21 @@ const struct brw_tracked_state gen6_renderbuffer_surfaces
= {
.emit = update_renderbuffer_surfaces,
};
+static void
+update_texture_surface(struct gl_context *ctx,
+ unsigned unit,
+ uint32_t *surf_offset,
+ bool for_gather)
+{
+ struct brw_context *brw = brw_context(ctx);
+ struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
+
+ if (obj->Target == GL_TEXTURE_BUFFER) {
+ update_buffer_texture_surface(ctx, unit, surf_offset);
+ } else {
+ brw->vtbl.update_texture_surface(ctx, unit, surf_offset, for_gather);
+ }
+}
static void
update_stage_texture_surfaces(struct brw_context *brw,
@@ -824,7 +833,7 @@ update_stage_texture_surfaces(struct brw_context *brw,
/* _NEW_TEXTURE */
if (ctx->Texture.Unit[unit]._Current) {
-brw->vtbl.update_texture_surface(ctx, unit, surf_offset + s,
for_gather);
+update_texture_surface(ctx, unit, surf_offset + s, for_gather);
}
}
}
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
index 15ab2b0..098b5c8 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
@@ -356,43 +356,38 @@ gen7_update_texture_surface(struct gl_context *ctx,
struct brw_context *brw = brw_context(ctx);
struct gl_texture_object *obj = ctx->Texture.Unit[unit]._Current;
- if (obj->Target == GL_TEXTURE_BUFFER) {
- brw_update_buffer_texture_surface(ctx, unit, surf_offset);
-
- } else {
- struct intel_texture_object *intel_obj = intel_texture_object(obj);
- struct intel_mipmap_tree *mt = intel_obj->mt;
- struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
- /* If this is a view with restricted NumLayers, then our effective depth
- * is not just the miptree depth.
- */
- const unsigned depth = (obj->Immutable && obj->Target != GL_TEXTURE_3D ?
- obj->NumLayers : mt->logical_depth0);
-
- /* Handling GL_ALPHA as a surface format override breaks 1.30+ style
- * texturing functions that return a float, as our code generation always
- * selects the .x channel (which would always be 0).
- */
