[Mesa-dev,02/14] i965: Allocate binding table space for shader images.

Submitted by Francisco Jerez on Feb. 6, 2015, 5:23 p.m.

Details

Message ID 1423243408-24744-2-git-send-email-currojerez@riseup.net
State New
Headers show

Not browsing as part of any series.

Commit Message

Francisco Jerez Feb. 6, 2015, 5:23 p.m.
Reviewed-by: Paul Berry <stereotype441@gmail.com>
---
 src/mesa/drivers/dri/i965/brw_context.h  | 5 +++++
 src/mesa/drivers/dri/i965/brw_shader.cpp | 7 +++++++
 2 files changed, 12 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
index bebb0be..e28c65d 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -341,6 +341,7 @@  struct brw_stage_prog_data {
       uint32_t gather_texture_start;
       uint32_t ubo_start;
       uint32_t abo_start;
+      uint32_t image_start;
       uint32_t shader_time_start;
       /** @} */
    } binding_table;
@@ -621,6 +622,9 @@  struct brw_vs_prog_data {
 /** Max number of atomic counter buffer objects in a shader */
 #define BRW_MAX_ABO 16
 
+/** Max number of image units in a shader */
+#define BRW_MAX_IMAGES 16
+
 /**
  * Max number of binding table entries used for stream output.
  *
@@ -653,6 +657,7 @@  struct brw_vs_prog_data {
                             BRW_MAX_TEX_UNIT * 2 + /* normal, gather */ \
                             12 + /* ubo */                              \
                             BRW_MAX_ABO +                               \
+                            BRW_MAX_IMAGES +                            \
                             2 /* shader time, pull constants */)
 
 #define SURF_INDEX_GEN6_SOL_BINDING(t) (t)
diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
index 67d8de4..5c8f3d5 100644
--- a/src/mesa/drivers/dri/i965/brw_shader.cpp
+++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
@@ -1116,6 +1116,13 @@  backend_visitor::assign_common_binding_table_offsets(uint32_t next_binding_table
       stage_prog_data->binding_table.abo_start = 0xd0d0d0d0;
    }
 
+   if (shader && shader->base.NumImages) {
+      stage_prog_data->binding_table.image_start = next_binding_table_offset;
+      next_binding_table_offset += shader->base.NumImages;
+   } else {
+      stage_prog_data->binding_table.image_start = 0xd0d0d0d0;
+   }
+
    /* This may or may not be used depending on how the compile goes. */
    stage_prog_data->binding_table.pull_constants_start = next_binding_table_offset;
    next_binding_table_offset++;

Comments

On Friday, February 06, 2015 07:23:16 PM Francisco Jerez wrote:
> Reviewed-by: Paul Berry <stereotype441@gmail.com>
> ---
>  src/mesa/drivers/dri/i965/brw_context.h  | 5 +++++
>  src/mesa/drivers/dri/i965/brw_shader.cpp | 7 +++++++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> index bebb0be..e28c65d 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -341,6 +341,7 @@ struct brw_stage_prog_data {
>        uint32_t gather_texture_start;
>        uint32_t ubo_start;
>        uint32_t abo_start;
> +      uint32_t image_start;
>        uint32_t shader_time_start;
>        /** @} */
>     } binding_table;
> @@ -621,6 +622,9 @@ struct brw_vs_prog_data {
>  /** Max number of atomic counter buffer objects in a shader */
>  #define BRW_MAX_ABO 16
>  
> +/** Max number of image units in a shader */
> +#define BRW_MAX_IMAGES 16
> +

Out of curiosity, why 16?  The Windows driver and Nvidia both seem to
use 8, while AMD offers 32:

http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_MAX_IMAGE_UNITS

We have 32 texture units on Haswell these days (though honestly, it's
pretty arbitrary...we could have more if we wanted...)

That's not an objection - I'm just curious.  As is,
Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

>  /**
>   * Max number of binding table entries used for stream output.
>   *
> @@ -653,6 +657,7 @@ struct brw_vs_prog_data {
>                              BRW_MAX_TEX_UNIT * 2 + /* normal, gather */ \
>                              12 + /* ubo */                              \
>                              BRW_MAX_ABO +                               \
> +                            BRW_MAX_IMAGES +                            \
>                              2 /* shader time, pull constants */)
>  
>  #define SURF_INDEX_GEN6_SOL_BINDING(t) (t)
> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
> index 67d8de4..5c8f3d5 100644
> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
> @@ -1116,6 +1116,13 @@ backend_visitor::assign_common_binding_table_offsets(uint32_t next_binding_table
>        stage_prog_data->binding_table.abo_start = 0xd0d0d0d0;
>     }
>  
> +   if (shader && shader->base.NumImages) {
> +      stage_prog_data->binding_table.image_start = next_binding_table_offset;
> +      next_binding_table_offset += shader->base.NumImages;
> +   } else {
> +      stage_prog_data->binding_table.image_start = 0xd0d0d0d0;
> +   }
> +
>     /* This may or may not be used depending on how the compile goes. */
>     stage_prog_data->binding_table.pull_constants_start = next_binding_table_offset;
>     next_binding_table_offset++;
>
Kenneth Graunke <kenneth@whitecape.org> writes:

> On Friday, February 06, 2015 07:23:16 PM Francisco Jerez wrote:
>> Reviewed-by: Paul Berry <stereotype441@gmail.com>
>> ---
>>  src/mesa/drivers/dri/i965/brw_context.h  | 5 +++++
>>  src/mesa/drivers/dri/i965/brw_shader.cpp | 7 +++++++
>>  2 files changed, 12 insertions(+)
>> 
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
>> index bebb0be..e28c65d 100644
>> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> @@ -341,6 +341,7 @@ struct brw_stage_prog_data {
>>        uint32_t gather_texture_start;
>>        uint32_t ubo_start;
>>        uint32_t abo_start;
>> +      uint32_t image_start;
>>        uint32_t shader_time_start;
>>        /** @} */
>>     } binding_table;
>> @@ -621,6 +622,9 @@ struct brw_vs_prog_data {
>>  /** Max number of atomic counter buffer objects in a shader */
>>  #define BRW_MAX_ABO 16
>>  
>> +/** Max number of image units in a shader */
>> +#define BRW_MAX_IMAGES 16
>> +
>
> Out of curiosity, why 16?  The Windows driver and Nvidia both seem to
> use 8, while AMD offers 32:
>
> http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_MAX_IMAGE_UNITS
>
> We have 32 texture units on Haswell these days (though honestly, it's
> pretty arbitrary...we could have more if we wanted...)
>
> That's not an objection - I'm just curious.  As is,
> Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
>
It's completely arbitrary too, IIRC I set it to 16 because back when I
wrote this patch we were still exposing 16 texture units, so I just used
the same value.  We could definitely expose 32 image units too, should I
just increase the value to 32 locally?

>>  /**
>>   * Max number of binding table entries used for stream output.
>>   *
>> @@ -653,6 +657,7 @@ struct brw_vs_prog_data {
>>                              BRW_MAX_TEX_UNIT * 2 + /* normal, gather */ \
>>                              12 + /* ubo */                              \
>>                              BRW_MAX_ABO +                               \
>> +                            BRW_MAX_IMAGES +                            \
>>                              2 /* shader time, pull constants */)
>>  
>>  #define SURF_INDEX_GEN6_SOL_BINDING(t) (t)
>> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> index 67d8de4..5c8f3d5 100644
>> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp
>> @@ -1116,6 +1116,13 @@ backend_visitor::assign_common_binding_table_offsets(uint32_t next_binding_table
>>        stage_prog_data->binding_table.abo_start = 0xd0d0d0d0;
>>     }
>>  
>> +   if (shader && shader->base.NumImages) {
>> +      stage_prog_data->binding_table.image_start = next_binding_table_offset;
>> +      next_binding_table_offset += shader->base.NumImages;
>> +   } else {
>> +      stage_prog_data->binding_table.image_start = 0xd0d0d0d0;
>> +   }
>> +
>>     /* This may or may not be used depending on how the compile goes. */
>>     stage_prog_data->binding_table.pull_constants_start = next_binding_table_offset;
>>     next_binding_table_offset++;
>>
On Saturday, February 07, 2015 03:03:44 AM Francisco Jerez wrote:
> Kenneth Graunke <kenneth@whitecape.org> writes:
> 
> > On Friday, February 06, 2015 07:23:16 PM Francisco Jerez wrote:
> >> Reviewed-by: Paul Berry <stereotype441@gmail.com>
> >> ---
> >>  src/mesa/drivers/dri/i965/brw_context.h  | 5 +++++
> >>  src/mesa/drivers/dri/i965/brw_shader.cpp | 7 +++++++
> >>  2 files changed, 12 insertions(+)
> >> 
> >> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
> >> index bebb0be..e28c65d 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_context.h
> >> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> >> @@ -341,6 +341,7 @@ struct brw_stage_prog_data {
> >>        uint32_t gather_texture_start;
> >>        uint32_t ubo_start;
> >>        uint32_t abo_start;
> >> +      uint32_t image_start;
> >>        uint32_t shader_time_start;
> >>        /** @} */
> >>     } binding_table;
> >> @@ -621,6 +622,9 @@ struct brw_vs_prog_data {
> >>  /** Max number of atomic counter buffer objects in a shader */
> >>  #define BRW_MAX_ABO 16
> >>  
> >> +/** Max number of image units in a shader */
> >> +#define BRW_MAX_IMAGES 16
> >> +
> >
> > Out of curiosity, why 16?  The Windows driver and Nvidia both seem to
> > use 8, while AMD offers 32:
> >
> > http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_MAX_IMAGE_UNITS
> >
> > We have 32 texture units on Haswell these days (though honestly, it's
> > pretty arbitrary...we could have more if we wanted...)
> >
> > That's not an objection - I'm just curious.  As is,
> > Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
> >
> It's completely arbitrary too, IIRC I set it to 16 because back when I
> wrote this patch we were still exposing 16 texture units, so I just used
> the same value.  We could definitely expose 32 image units too, should I
> just increase the value to 32 locally?

That seems like a good idea to me, unless you can think of some reason
not to.  I'll leave it up to you :)
Kenneth Graunke <kenneth@whitecape.org> writes:

> On Saturday, February 07, 2015 03:03:44 AM Francisco Jerez wrote:
>> Kenneth Graunke <kenneth@whitecape.org> writes:
>> 
>> > On Friday, February 06, 2015 07:23:16 PM Francisco Jerez wrote:
>> >> Reviewed-by: Paul Berry <stereotype441@gmail.com>
>> >> ---
>> >>  src/mesa/drivers/dri/i965/brw_context.h  | 5 +++++
>> >>  src/mesa/drivers/dri/i965/brw_shader.cpp | 7 +++++++
>> >>  2 files changed, 12 insertions(+)
>> >> 
>> >> diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h
>> >> index bebb0be..e28c65d 100644
>> >> --- a/src/mesa/drivers/dri/i965/brw_context.h
>> >> +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> >> @@ -341,6 +341,7 @@ struct brw_stage_prog_data {
>> >>        uint32_t gather_texture_start;
>> >>        uint32_t ubo_start;
>> >>        uint32_t abo_start;
>> >> +      uint32_t image_start;
>> >>        uint32_t shader_time_start;
>> >>        /** @} */
>> >>     } binding_table;
>> >> @@ -621,6 +622,9 @@ struct brw_vs_prog_data {
>> >>  /** Max number of atomic counter buffer objects in a shader */
>> >>  #define BRW_MAX_ABO 16
>> >>  
>> >> +/** Max number of image units in a shader */
>> >> +#define BRW_MAX_IMAGES 16
>> >> +
>> >
>> > Out of curiosity, why 16?  The Windows driver and Nvidia both seem to
>> > use 8, while AMD offers 32:
>> >
>> > http://delphigl.de/glcapsviewer/gl_stats_caps_single.php?listreportsbycap=GL_MAX_IMAGE_UNITS
>> >
>> > We have 32 texture units on Haswell these days (though honestly, it's
>> > pretty arbitrary...we could have more if we wanted...)
>> >
>> > That's not an objection - I'm just curious.  As is,
>> > Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>
>> >
>> It's completely arbitrary too, IIRC I set it to 16 because back when I
>> wrote this patch we were still exposing 16 texture units, so I just used
>> the same value.  We could definitely expose 32 image units too, should I
>> just increase the value to 32 locally?
>
> That seems like a good idea to me, unless you can think of some reason
> not to.  I'll leave it up to you :)

Cool, I've increased BRW_MAX_IMAGES to 32 locally.  The core mesa patch
I just sent will also be required so the GL context and shader objects
have enough state entries to keep track of all these image units. :)