Re: [Mesa-dev] [PATCH] configure.ac: describe what all the LIBDRM_*REQUIRED macros mean
LGTM Reviewed-by: Plamena Manolova On Thu, Feb 2, 2017 at 2:32 AM, Emil Velikov wrote: > From: Emil Velikov > > They are versions of the respective libdrm package. They are _not_ the > version of libdrm[.so] required for driver X. > > Doing the latter will lead to combinatoric explosion and in all fairness > things will likely be broken most of the time. > > To make things even more confusing the kernel UAPI is provided by libdrm > itself. > > Cc: Vinson Lee > Cc: Kenneth Graunke > Signed-off-by: Emil Velikov > --- > Ken, you/Chad have things spot on. Yet semes like other people struggle > deeply with these. > --- > configure.ac | 7 +++ > 1 file changed, 7 insertions(+) > > diff --git a/configure.ac b/configure.ac > index 92339b4a3f..e8e154b412 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -67,6 +67,13 @@ OPENCL_VERSION=1 > AC_SUBST([OPENCL_VERSION]) > > dnl Versions for external dependencies > +dnl > +dnl The LIBDRM instances reference either the generic libdrm or the > specific > +dnl specific libdrm-$hw. > +dnl They are _not_ "the min. libdrm required for dri/gallium/other driver > $foo." > +dnl > +dnl Note that the "basic" ABI for $hw -> $hw_drm.h is provided by libdrm > itself. > +dnl > LIBDRM_REQUIRED=2.4.75 > LIBDRM_RADEON_REQUIRED=2.4.56 > LIBDRM_AMDGPU_REQUIRED=2.4.63 > -- > 2.11.0 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] android: fix droid_create_image_from_prime_fd_yuv for YV12
LGTM :)
Reviewed-by: Plamena Manolova
On Thu, Feb 2, 2017 at 12:27 PM, Tapani Pälli
wrote:
> Earlier changes introduced is_ycrcb flag which checks the component
> order of u and v components. Condition for setting the flag was
> incorrect, with ycrcb we are supposed to have cr before cb.
>
> This patch (together with a fix in our gralloc) fixes corrupted
> rendering from 'test-opengl-gl2_yuvtex' native test and corrupted
> gallery thumbnail in application switcher on Android-IA.
>
> Fixes: 51727b1cf57e8c4630767eb9ead207b102ffa489
> Signed-off-by: Tapani Pälli
> ---
> src/egl/drivers/dri2/platform_android.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/egl/drivers/dri2/platform_android.c
> b/src/egl/drivers/dri2/platform_android.c
> index 36bd119..511b696 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -629,7 +629,7 @@ droid_create_image_from_prime_fd_yuv(_EGLDisplay
> *disp, _EGLContext *ctx,
> * so they can be interpreted as offsets. */
> offsets[0] = (size_t)ycbcr.y;
> /* We assume here that all the planes are located in one DMA-buf. */
> - is_ycrcb = (size_t)ycbcr.cb < (size_t)ycbcr.cr;
> + is_ycrcb = (size_t)ycbcr.cr < (size_t)ycbcr.cb;
> if (is_ycrcb) {
>offsets[1] = (size_t)ycbcr.cr;
>offsets[2] = (size_t)ycbcr.cb;
> --
> 2.9.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/disk_cache: fix d7b3707c612
Reviewed-by: Plamena Manolova On Sat, Feb 11, 2017 at 1:32 PM, Timothy Arceri wrote: > I forgot to error check stat() and also I wasn't using the subdir in > is_two_character_sub_directory(). > > Cc: Emil Velikov > --- > src/util/disk_cache.c | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/src/util/disk_cache.c b/src/util/disk_cache.c > index 63954b2..84559ab 100644 > --- a/src/util/disk_cache.c > +++ b/src/util/disk_cache.c > @@ -509,10 +509,10 @@ is_regular_non_tmp_file(struct dirent *entry, const > char *path) >return false; > > struct stat sb; > - stat(filename, &sb); > + int res = stat(filename, &sb); > free(filename); > > - if (!S_ISREG(sb.st_mode)) > + if (res == -1 || !S_ISREG(sb.st_mode)) >return false; > > size_t len = strlen (entry->d_name); > @@ -556,10 +556,10 @@ is_two_character_sub_directory(struct dirent > *entry, const char *path) >return false; > > struct stat sb; > - stat(path, &sb); > + int res = stat(subdir, &sb); > free(subdir); > > - if (!S_ISDIR(sb.st_mode)) > + if (res == -1 || !S_ISDIR(sb.st_mode)) >return false; > > if (strlen(entry->d_name) != 2) > -- > 2.9.3 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: Fix performance query id check
Looks good to me :)
Reviewed-by: Plamena Manolova
On Fri, Feb 24, 2017 at 6:46 PM, Robert Bragg wrote:
> In queryid_valid() index is unsigned so checking if it is less
> than zero is useless. On queryid_to_index() is comment
> saying 0 is reserved to be invalid thus rule it out.
>
> This is a v2 of a fix for an issue identified by Juha-Pekka (thanks)
> and the commit message is gratuitously stolen.
>
> Cc: Juha-Pekka Heikkila
> Signed-off-by: Robert Bragg
> ---
> src/mesa/main/performance_query.c | 8 ++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/main/performance_query.c
> b/src/mesa/main/performance_query.c
> index aa103516a5..56f6a7da8b 100644
> --- a/src/mesa/main/performance_query.c
> +++ b/src/mesa/main/performance_query.c
> @@ -90,8 +90,12 @@ index_to_queryid(unsigned index)
> static inline bool
> queryid_valid(const struct gl_context *ctx, unsigned numQueries, GLuint
> queryid)
> {
> - GLuint index = queryid_to_index(queryid);
> - return index >= 0 && index < numQueries;
> + /* The GL_INTEL_performance_query spec says:
> +*
> +* "Performance counter ids values start with 1. Performance counter
> id 0
> +* is reserved as an invalid counter."
> +*/
> + return queryid != 0 && queryid_to_index(queryid) < numQueries;
> }
>
> static inline GLuint
> --
> 2.11.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] mesa: Add GL and GLSL plumbing for ARB_post_depth_coverage for i965 (gen9+).
Thank you for reviewing guys! I'll go ahead and make that change.
Pam
On Wed, Nov 30, 2016 at 3:33 PM, Ilia Mirkin wrote:
> On Nov 30, 2016 7:17 AM, "Lionel Landwerlin" <
> lionel.g.landwer...@intel.com> wrote:
> >
> > On 22/11/16 21:53, Plamena Manolova wrote:
> >>
> >> This extension allows the fragment shader to control whether values in
> >> gl_SampleMaskIn[] reflect the coverage after application of the early
> >> depth and stencil tests.
> >>
> >> Signed-off-by: Plamena Manolova
> >> ---
> >> src/compiler/glsl/ast.h | 5 +
> >> src/compiler/glsl/ast_to_hir.cpp | 5 +
> >> src/compiler/glsl/ast_type.cpp | 8 +++-
> >> src/compiler/glsl/glsl_parser.yy | 11 +++
> >> src/compiler/glsl/glsl_parser_extras.cpp | 4
> >> src/compiler/glsl/glsl_parser_extras.h | 4
> >> src/compiler/glsl/linker.cpp | 4
> >> src/compiler/shader_info.h | 1 +
> >> src/mesa/main/extensions_table.h | 1 +
> >> src/mesa/main/mtypes.h | 2 ++
> >> src/mesa/main/shaderapi.c| 1 +
> >> 11 files changed, 45 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
> >> index 55f9a6c..ad19493 100644
> >> --- a/src/compiler/glsl/ast.h
> >> +++ b/src/compiler/glsl/ast.h
> >> @@ -606,6 +606,11 @@ struct ast_type_qualifier {
> >>/** \{ */
> >>unsigned blend_support:1; /**< Are there any blend_support_
> qualifiers */
> >>/** \} */
> >> +
> >> + /**
> >> + * Flag set if GL_ARB_post_depth_coverage layout qualifier is
> used.
> >> + */
> >> + unsigned post_depth_coverage:1;
> >> }
> >> /** \brief Set of flags, accessed by name. */
> >> q;
> >> diff --git a/src/compiler/glsl/ast_to_hir.cpp
> b/src/compiler/glsl/ast_to_hir.cpp
> >> index 9b8678c..c31da86 100644
> >> --- a/src/compiler/glsl/ast_to_hir.cpp
> >> +++ b/src/compiler/glsl/ast_to_hir.cpp
> >> @@ -3632,6 +3632,11 @@ apply_layout_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
> >> _mesa_glsl_error(loc, state, "early_fragment_tests layout
> qualifier only "
> >> "valid in fragment shader input layout
> declaration.");
> >> }
> >> +
> >> + if (qual->flags.q.post_depth_coverage) {
> >> + _mesa_glsl_error(loc, state, "post_depth_coverage layout
> qualifier only "
> >> + "valid in fragment shader input layout
> declaration.");
> >> + }
> >> }
> >> static void
> >> diff --git a/src/compiler/glsl/ast_type.cpp
> b/src/compiler/glsl/ast_type.cpp
> >> index 2856f18..1905721 100644
> >> --- a/src/compiler/glsl/ast_type.cpp
> >> +++ b/src/compiler/glsl/ast_type.cpp
> >> @@ -489,6 +489,7 @@ ast_type_qualifier::merge_in_qualifier(YYLTYPE
> *loc,
> >> break;
> >> case MESA_SHADER_FRAGMENT:
> >> valid_in_mask.flags.q.early_fragment_tests = 1;
> >> + valid_in_mask.flags.q.post_depth_coverage = 1;
> >> break;
> >> case MESA_SHADER_COMPUTE:
> >> create_cs_ast |=
> >> @@ -540,6 +541,10 @@ ast_type_qualifier::merge_in_qualifier(YYLTYPE
> *loc,
> >> state->fs_early_fragment_tests = true;
> >> }
> >> + if (q.flags.q.post_depth_coverage) {
> >> + state->fs_post_depth_coverage = true;
> >> + }
> >> +
> >> if (this->flags.q.vertex_spacing) {
> >> if (q.flags.q.vertex_spacing &&
> >> this->vertex_spacing != q.vertex_spacing) {
> >> @@ -671,7 +676,8 @@ ast_type_qualifier::validate_flags(YYLTYPE *loc,
> >> bad.flags.q.point_mode ? " point_mode" : "",
> >> bad.flags.q.vertices ? " vertices" : "",
> >> bad.flags.q.subroutine ? " subroutine" : "",
> >> -bad.flags.q.subroutine_def ? " subroutine_def" :
> "");
> >> +bad.flags.q.subroutine_def ? " subroutine_def" :
> "",
> >> +bad.flags.q.post_depth_coverage ? "
> post_depth_coverage" : "");
> >> return false;
> >> }
> >> diff --git a/src/compiler/glsl/glsl_parser.yy
> b/src/compiler/glsl/glsl_parser.yy
> >> index a48dc68..a53f476 100644
> >> --- a/src/compiler/glsl/glsl_parser.yy
> >> +++ b/src/compiler/glsl/glsl_parser.yy
> >> @@ -1373,6 +1373,17 @@ layout_qualifier_id:
> >> $$.flags.q.early_fragment_tests = 1;
> >>}
> >
> >
> > I wonder if the following check should include state->
> ARB_post_depth_coverage_enable as condition before matching the layout
> qualifier.
> > Maybe Ilia can confirm?
>
> Sounds right. A shader without the enable but wit the layout qualifier
> should fail to compile. There should be a test in piglit to that effect
> with glslparsertest.
>
> >
> > Thanks!
> >
> >
> >> +
> >> + if (!$$.flags.i &&
> >> + match_layout_qualifier($1, "post_depth_coverage", state)
> == 0) {
>
Re: [Mesa-dev] [PATCH 1/2] mesa: Add GL and GLSL plumbing for ARB_post_depth_coverage for i965 (gen9+).
On Wed, Nov 30, 2016 at 4:36 PM, Ilia Mirkin wrote: > On Wed, Nov 30, 2016 at 9:19 AM, Lionel Landwerlin > wrote: > > Also forgot that (like Ilia suggested for NV_image_formats) you can > update > > docs/relnotes/ to list the new feature. > > That would be appropriate for the patch that actually exposes the > feature. This one just adds the core plumbing, which is generally not > announced in release notes. > > -ilia > I'll update the subsequent patch to include the change to the release notes. Pam ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Thank you for the review. You're absolutely right, I'll go ahead fix this.
On Thu, Feb 11, 2016 at 6:39 PM, Ilia Mirkin wrote:
> On Thu, Feb 11, 2016 at 11:31 AM, Plamena Manolova
> wrote:
> > This patch moves the calculation of current uniforms to
> > link_uniforms, which makes use of UniformRemapTable which
> > stores all the reserved uniform locations.
> >
> > Location assignment for implicit uniforms now tries to use
> > any gaps left in the table after the location assignment
> > for explicit uniforms. This gives us more space to store more
> > uniforms.
> >
> > Patch is based on earlier patch with following changes/additions:
> >
> >1: Move the counting of explicit locations to
> > check_explicit_uniform_locations and then pass
> > the number to link_assign_uniform_locations.
> >2: Count the number of empty slots in UniformRemapTable
> > and store them in a list_head.
> >3: Try to find an empty slot for implicit locations from
> > the list, if that fails resize UniformRemapTable.
> >
> > Fixes following CTS tests:
> >ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max
> >
> ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max-array
> >
> > Signed-off-by: Tapani Pälli
> > Signed-off-by: Plamena Manolova
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696
> > ---
> > src/compiler/glsl/link_uniforms.cpp | 79
> -
> > src/compiler/glsl/linker.cpp| 78
> +---
> > src/compiler/glsl/linker.h | 17 +++-
> > src/mesa/main/mtypes.h | 8
> > 4 files changed, 147 insertions(+), 35 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_uniforms.cpp
> b/src/compiler/glsl/link_uniforms.cpp
> > index 7072c16..8192c21 100644
> > --- a/src/compiler/glsl/link_uniforms.cpp
> > +++ b/src/compiler/glsl/link_uniforms.cpp
> > @@ -1038,9 +1038,36 @@ assign_hidden_uniform_slot_id(const char *name,
> unsigned hidden_id,
> > uniform_size->map->put(hidden_uniform_start + hidden_id, name);
> > }
> >
> > +/**
> > + * Search through the list of empty blocks to find one that fits the
> current
> > + * uniform.
> > + */
> > +static int
> > +find_empty_block(struct gl_shader_program *prog,
> > + struct gl_uniform_storage *uniform)
> > +{
> > + const unsigned entries = MAX2(1, uniform->array_elements);
> > +
> > + list_for_each_entry_safe(struct empty_uniform_block, block,
> > +&prog->EmptyUniformLocations, link) {
> > + /* Found a block with enough slots to fit the uniform */
> > + if (block->slots == entries) {
>
> I haven't fully absorbed this patch, but if you have a bunch of holes
> of 2 slots and then you try to allocate something needing 1 slot, then
> you'll never decide you have a free block right? I think this needs to
> be
>
> block->slots >= entries
>
> and then
>
> if (block->slots > entries) {
> block->slots -= entries;
> block->start += entries;
> } else {
> list_del(&block->link);
> ralloc_free(block);
> }
>
> Or am I misunderstanding something? This sort of greedy approach
> doesn't guarantee optimal packing, you could do something much fancier
> involving dynamic programming, but that hardly seems necessary.
>
> > + unsigned start = block->start;
> > + list_del(&block->link);
> > + ralloc_free(block);
> > +
> > + return start;
> > + }
> > + }
> > +
> > + return -1;
> > +}
> > +
> > void
> > link_assign_uniform_locations(struct gl_shader_program *prog,
> > - unsigned int boolean_true)
> > + unsigned int boolean_true,
> > + unsigned int num_explicit_uniform_locs,
> > + unsigned int max_uniform_locs)
> > {
> > ralloc_free(prog->UniformStorage);
> > prog->UniformStorage = NULL;
> > @@ -1131,6 +1158,9 @@ link_assign_uniform_locations(struct
> gl_shader_program *prog,
> >
> > parcel_out_uniform_storage parcel(prog, prog->UniformHash, uniforms,
> data);
> >
> > + unsigned total_entries = num_explicit_uniform_locs;
> > + unsigned empty_locs = prog->NumUniformRemapTable -
> num_explicit_uniform_locs;
> > +
> > for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> >if (prog->_LinkedShaders[i] == NULL)
> > continue;
> > @@ -1194,21 +1224,43 @@ link_assign_uniform_locations(struct
> gl_shader_program *prog,
> >/* how many new entries for this uniform? */
> >const unsigned entries = MAX2(1, uniforms[i].array_elements);
> >
> > - /* resize remap table to fit new entries */
> > - prog->UniformRemapTable =
> > - reralloc(prog,
> > - prog->UniformRemapTable,
> > - gl_uniform_storage *,
> > - prog->NumUniformRemapTable + entries);
> > + /* Find UniformRemapTable for empty blocks where
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Thanks, this if-statement looks tidier the way you suggested. I'll update
the patch.
On Thu, Feb 11, 2016 at 7:13 PM, Ilia Mirkin wrote:
> On Thu, Feb 11, 2016 at 11:31 AM, Plamena Manolova
> wrote:
> > + struct empty_uniform_block *current_block = NULL;
> > + unsigned prev_location = -1;
> > +
> > + for (unsigned i = 0; i < prog->NumUniformRemapTable; i++) {
> > + /* We found empty space in UniformRemapTable. */
> > + if (prog->UniformRemapTable[i] == NULL) {
> > + /* We've found the beginning of new continous block of empty
> slots */
> > + if ((i == 0) || !current_block || (i != prev_location + 1)) {
>
> You could delete prev_location entirely and transform this into
>
> if (!current_block || current_block->start + current_block->slots != i)
>
> > +current_block = ralloc(NULL, struct empty_uniform_block);
> > +current_block->start = i;
> > +current_block->slots = 1;
>
> And depending on how clever you want to get, you could change the
> above to rzalloc, not touch slots here, and drop the continue. Then
> the slots++ will magically increment it to 1. Up to you if you find
> this more or less readable though, I don't have a strong preference
> either way.
>
> > +list_addtail(¤t_block->link,
> &prog->EmptyUniformLocations);
> > +prev_location = i;
> > +continue;
> > + }
> > +
> > + /* The current block continues, so we simply increment its
> slots */
> > + current_block->slots++;
> > + prev_location = i;
> > + }
> > }
> -
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Thanks for reviewing. I was actually hesitant about which list
implementation to use since the changes affect src/mesa/main/mtypes.h too.
Thank you for clearing this up.
On Thu, Feb 11, 2016 at 8:02 PM, Matt Turner wrote:
> On Thu, Feb 11, 2016 at 8:31 AM, Plamena Manolova
> wrote:
> > This patch moves the calculation of current uniforms to
> > link_uniforms, which makes use of UniformRemapTable which
> > stores all the reserved uniform locations.
> >
> > Location assignment for implicit uniforms now tries to use
> > any gaps left in the table after the location assignment
> > for explicit uniforms. This gives us more space to store more
> > uniforms.
> >
> > Patch is based on earlier patch with following changes/additions:
> >
> >1: Move the counting of explicit locations to
> > check_explicit_uniform_locations and then pass
> > the number to link_assign_uniform_locations.
> >2: Count the number of empty slots in UniformRemapTable
> > and store them in a list_head.
> >3: Try to find an empty slot for implicit locations from
> > the list, if that fails resize UniformRemapTable.
> >
> > Fixes following CTS tests:
> >ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max
> >
> ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max-array
> >
> > Signed-off-by: Tapani Pälli
> > Signed-off-by: Plamena Manolova
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696
> > ---
> > src/compiler/glsl/link_uniforms.cpp | 79
> -
> > src/compiler/glsl/linker.cpp| 78
> +---
> > src/compiler/glsl/linker.h | 17 +++-
> > src/mesa/main/mtypes.h | 8
> > 4 files changed, 147 insertions(+), 35 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_uniforms.cpp
> b/src/compiler/glsl/link_uniforms.cpp
> > index 7072c16..8192c21 100644
> > --- a/src/compiler/glsl/link_uniforms.cpp
> > +++ b/src/compiler/glsl/link_uniforms.cpp
> > @@ -1038,9 +1038,36 @@ assign_hidden_uniform_slot_id(const char *name,
> unsigned hidden_id,
> > uniform_size->map->put(hidden_uniform_start + hidden_id, name);
> > }
> >
> > +/**
> > + * Search through the list of empty blocks to find one that fits the
> current
> > + * uniform.
> > + */
> > +static int
> > +find_empty_block(struct gl_shader_program *prog,
> > + struct gl_uniform_storage *uniform)
> > +{
> > + const unsigned entries = MAX2(1, uniform->array_elements);
> > +
> > + list_for_each_entry_safe(struct empty_uniform_block, block,
> > +&prog->EmptyUniformLocations, link) {
>
> Please use exec_list in glsl code. That means embedding a "struct
> exec_node link" in empty_uniform_block instead of a head_link and
> iterating with foreach_list_typed.
> -
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
>
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
On Mon, Feb 15, 2016 at 10:50 PM, Ilia Mirkin wrote:
> In a few places your indentation is off -- please look at the 'git
> diff' output, it should be pretty obvious. You used 2 spaces instead
> of 3 (in just a handful of places).
>
Thanks for spotting this, it must've slipped by me.
>
> On Fri, Feb 12, 2016 at 7:38 AM, Plamena Manolova
> wrote:
> > This patch moves the calculation of current uniforms to
> > link_uniforms, which makes use of UniformRemapTable which
> > stores all the reserved uniform locations.
> >
> > Location assignment for implicit uniforms now tries to use
> > any gaps left in the table after the location assignment
> > for explicit uniforms. This gives us more space to store more
> > uniforms.
> >
> > Patch is based on earlier patch with following changes/additions:
> >
> >1: Move the counting of explicit locations to
> > check_explicit_uniform_locations and then pass
> > the number to link_assign_uniform_locations.
> >2: Count the number of empty slots in UniformRemapTable
> > and store them in a list_head.
> >3: Try to find an empty slot for implicit locations from
> > the list, if that fails resize UniformRemapTable.
> >
> > Fixes following CTS tests:
> >ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max
> >
> ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max-array
> >
> > Signed-off-by: Tapani Pälli
> > Signed-off-by: Plamena Manolova
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696
> > ---
> > src/compiler/glsl/link_uniforms.cpp | 85
> -
> > src/compiler/glsl/linker.cpp| 73 ---
> > src/compiler/glsl/linker.h | 17 +++-
> > src/mesa/main/mtypes.h | 8
> > 4 files changed, 148 insertions(+), 35 deletions(-)
> >
> > diff --git a/src/compiler/glsl/link_uniforms.cpp
> b/src/compiler/glsl/link_uniforms.cpp
> > index 7072c16..aa07de3 100644
> > --- a/src/compiler/glsl/link_uniforms.cpp
> > +++ b/src/compiler/glsl/link_uniforms.cpp
> > @@ -1038,9 +1038,43 @@ assign_hidden_uniform_slot_id(const char *name,
> unsigned hidden_id,
> > uniform_size->map->put(hidden_uniform_start + hidden_id, name);
> > }
> >
> > +/**
> > + * Search through the list of empty blocks to find one that fits the
> current
> > + * uniform.
> > + */
> > +static int
> > +find_empty_block(struct gl_shader_program *prog,
> > + struct gl_uniform_storage *uniform)
> > +{
> > + const unsigned entries = MAX2(1, uniform->array_elements);
> > +
> > +foreach_list_typed(struct empty_uniform_block, block, link,
> > + &prog->EmptyUniformLocations) {
> > + /* Found a block with enough slots to fit the uniform */
> > + if (block->slots == entries) {
> > + unsigned start = block->start;
> > + exec_node_remove(&block->link);
> > + ralloc_free(block);
> > +
> > + return start;
> > + /* Found a block with more slots than needed. It can still be
> used. */
> > + } else if (block->slots > entries) {
> > + unsigned start = block->start;
> > + block->start += entries;
> > + block->slots -= entries;
> > +
> > + return start;
> > + }
> > + }
> > +
> > + return -1;
> > +}
> > +
> > void
> > link_assign_uniform_locations(struct gl_shader_program *prog,
> > - unsigned int boolean_true)
> > + unsigned int boolean_true,
> > + unsigned int num_explicit_uniform_locs,
> > + unsigned int max_uniform_locs)
> > {
> > ralloc_free(prog->UniformStorage);
> > prog->UniformStorage = NULL;
> > @@ -1131,6 +1165,9 @@ link_assign_uniform_locations(struct
> gl_shader_program *prog,
> >
> > parcel_out_uniform_storage parcel(prog, prog->UniformHash, uniforms,
> data);
> >
> > + unsigned total_entries = num_explicit_uniform_locs;
> > + unsigned empty_locs = prog->NumUniformRemapTable -
> num_explicit_uniform_locs;
> > +
> > for (unsigned i = 0; i < MESA_SHADER_STAGES; i++) {
> >if (prog->_LinkedShaders[i] == NULL)
> > continue;
> > @@ -1194,21 +1231,43 @@ link_assign_uniform_locations(struct
> gl_shader_program *prog,
> >/* how many new entries for this uniform? */
> >const unsigned entries = MAX2(1, uniforms[i].array_elements);
> >
> > - /* resize remap table to fit new entries */
> > - prog->UniformRemapTable =
> > - reralloc(prog,
> > - prog->UniformRemapTable,
> > - gl_uniform_storage *,
> > - prog->NumUniformRemapTable + entries);
> > + /* Find UniformRemapTable for empty blocks where we can fit this
> uniform. */
> > + int chosen_location = -1;
> > +
> > + if (empty_locs)
> > + chosen_location = find_empty_block(prog, &uniforms[i]);
> > +
> > + if (chosen
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
On Wed, Feb 17, 2016 at 1:22 PM, Manolova, Plamena <
plamena.manol...@intel.com> wrote:
>
>
> On Mon, Feb 15, 2016 at 10:50 PM, Ilia Mirkin
> wrote:
>
>> In a few places your indentation is off -- please look at the 'git
>> diff' output, it should be pretty obvious. You used 2 spaces instead
>> of 3 (in just a handful of places).
>
>
Thanks for spotting this, it must've slipped by me.
>
>> On Fri, Feb 12, 2016 at 7:38 AM, Plamena Manolova
>> wrote:
>> > This patch moves the calculation of current uniforms to
>> > link_uniforms, which makes use of UniformRemapTable which
>> > stores all the reserved uniform locations.
>> >
>> > Location assignment for implicit uniforms now tries to use
>> > any gaps left in the table after the location assignment
>> > for explicit uniforms. This gives us more space to store more
>> > uniforms.
>> >
>> > Patch is based on earlier patch with following changes/additions:
>> >
>> >1: Move the counting of explicit locations to
>> > check_explicit_uniform_locations and then pass
>> > the number to link_assign_uniform_locations.
>> >2: Count the number of empty slots in UniformRemapTable
>> > and store them in a list_head.
>> >3: Try to find an empty slot for implicit locations from
>> > the list, if that fails resize UniformRemapTable.
>> >
>> > Fixes following CTS tests:
>> >ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max
>> >
>> ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max-array
>> >
>> > Signed-off-by: Tapani Pälli
>> > Signed-off-by: Plamena Manolova
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696
>> > ---
>> > src/compiler/glsl/link_uniforms.cpp | 85
>> -
>> > src/compiler/glsl/linker.cpp| 73
>> ---
>> > src/compiler/glsl/linker.h | 17 +++-
>> > src/mesa/main/mtypes.h | 8
>> > 4 files changed, 148 insertions(+), 35 deletions(-)
>> >
>> > diff --git a/src/compiler/glsl/link_uniforms.cpp
>> b/src/compiler/glsl/link_uniforms.cpp
>> > index 7072c16..aa07de3 100644
>> > --- a/src/compiler/glsl/link_uniforms.cpp
>> > +++ b/src/compiler/glsl/link_uniforms.cpp
>> > @@ -1038,9 +1038,43 @@ assign_hidden_uniform_slot_id(const char *name,
>> unsigned hidden_id,
>> > uniform_size->map->put(hidden_uniform_start + hidden_id, name);
>> > }
>> >
>> > +/**
>> > + * Search through the list of empty blocks to find one that fits the
>> current
>> > + * uniform.
>> > + */
>> > +static int
>> > +find_empty_block(struct gl_shader_program *prog,
>> > + struct gl_uniform_storage *uniform)
>> > +{
>> > + const unsigned entries = MAX2(1, uniform->array_elements);
>> > +
>> > +foreach_list_typed(struct empty_uniform_block, block, link,
>> > + &prog->EmptyUniformLocations) {
>> > + /* Found a block with enough slots to fit the uniform */
>> > + if (block->slots == entries) {
>> > + unsigned start = block->start;
>> > + exec_node_remove(&block->link);
>> > + ralloc_free(block);
>> > +
>> > + return start;
>> > + /* Found a block with more slots than needed. It can still be
>> used. */
>> > + } else if (block->slots > entries) {
>> > + unsigned start = block->start;
>> > + block->start += entries;
>> > + block->slots -= entries;
>> > +
>> > + return start;
>> > + }
>> > + }
>> > +
>> > + return -1;
>> > +}
>> > +
>> > void
>> > link_assign_uniform_locations(struct gl_shader_program *prog,
>> > - unsigned int boolean_true)
>> > + unsigned int boolean_true,
>> > + unsigned int num_explicit_uniform_locs,
>> > + unsigned int max_uniform_locs)
>> > {
>> > ralloc_free(prog->UniformStorage);
>> > prog->UniformStorage = NULL;
>> > @@ -1131,6 +1165,9 @@ link_assign_uniform_locations(struct
>> gl_shader_program *prog,
>> >
>&
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
You're right, changing the if statement to a > b would be less confusing, I'll go ahead and do that. On Wed, Feb 17, 2016 at 2:35 PM, Ilia Mirkin wrote: > On Wed, Feb 17, 2016 at 6:29 AM, Manolova, Plamena > wrote: > >>> > + if (entries_total > 0) > >>> > + entries_total -= 1; > >>> > >>> This seems weird... why are you doing that? > > > > > > According to the spec: > > https://www.opengl.org/registry/specs/ARB/explicit_uniform_location.txt: > > "The explicitly defined locations > > and the generated locations must be in the range of 0 to > > MAX_UNIFORM_LOCATIONS minus one.", which means that the uniforms must fit > > between array slots 0 - 98303, however if we use total_entries without > > subtracting 1 we will be comparing to the range 1 - 98304 which will > trigger > > an error. > > But that's just a question of changing a >= into a > right? As it is, > you're saying that entries_total == 0 and entries_total == 1 are the > same thing (by subtracting 1). This is very odd. > > -ilia > - > Intel Corporation (UK) Limited > Registered No. 1134945 (England) > Registered Office: Pipers Way, Swindon SN3 1RJ > VAT No: 860 2173 47 > > This e-mail and any attachments may contain confidential material for > the sole use of the intended recipient(s). Any review or distribution > by others is strictly prohibited. If you are not the intended > recipient, please contact the sender and delete all copies. > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] compiler/glsl: Fix uniform location counting.
Thank you for reviewing. The change has been run through both Piglit and CTS. I'll ask Tapani to take another look just in case. On Wed, Feb 17, 2016 at 4:20 PM, Ilia Mirkin wrote: > On Wed, Feb 17, 2016 at 8:49 AM, Plamena Manolova > wrote: > > This patch moves the calculation of current uniforms to > > link_uniforms, which makes use of UniformRemapTable which > > stores all the reserved uniform locations. > > > > Location assignment for implicit uniforms now tries to use > > any gaps left in the table after the location assignment > > for explicit uniforms. This gives us more space to store more > > uniforms. > > > > Patch is based on earlier patch with following changes/additions: > > > >1: Move the counting of explicit locations to > > check_explicit_uniform_locations and then pass > > the number to link_assign_uniform_locations. > >2: Count the number of empty slots in UniformRemapTable > > and store them in a list_head. > >3: Try to find an empty slot for implicit locations from > > the list, if that fails resize UniformRemapTable. > > > > Fixes following CTS tests: > >ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max > > > ES31-CTS.explicit_uniform_location.uniform-loc-mix-with-implicit-max-array > > > > Signed-off-by: Tapani Pälli > > Signed-off-by: Plamena Manolova > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93696 > > --- > > src/compiler/glsl/link_uniforms.cpp | 80 > - > > src/compiler/glsl/linker.cpp| 70 > +--- > > src/compiler/glsl/linker.h | 17 +++- > > src/mesa/main/mtypes.h | 8 > > 4 files changed, 140 insertions(+), 35 deletions(-) > > > > diff --git a/src/compiler/glsl/linker.cpp b/src/compiler/glsl/linker.cpp > > index bad1c17..5326bfd 100644 > > --- a/src/compiler/glsl/linker.cpp > > +++ b/src/compiler/glsl/linker.cpp > > @@ -4129,6 +4148,7 @@ link_shaders(struct gl_context *ctx, struct > gl_shader_program *prog) > > > > tfeedback_decl *tfeedback_decls = NULL; > > unsigned num_tfeedback_decls = prog->TransformFeedback.NumVarying; > > + unsigned int num_explicit_uniform_locs = 0; > > This initializer seems unnecessary, since you initialize it > unconditionally further down. Otherwise this change is > > Reviewed-by: Ilia Mirkin > > I assume you've also run this through piglit (if not, please do). It > may be good for Tapani to have another look at this since he > originally tried to implement this and is likely more aware of the > various pitfalls. > > Cheers, > > -ilia > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Ignore glPointSize when GL_POINT_SIZE_ARRAY_OES is enabled
Hi Kenneth,
You're absolutely right, just setting ctx->VertexProgram.PointSizeEnabled
would be much simpler and cleaner, I'll go ahead and update the patch.
Thank you for reviewing!
On Tue, Mar 1, 2016 at 10:07 PM, Kenneth Graunke
wrote:
> On Tuesday, March 1, 2016 6:39:49 PM PST Plamena Manolova wrote:
> > When a user defines a point size array and enables it, the point
> > size value set via glPointSize should be ignored. To achieve this,
> > we can simply omit point size when creating a batch inside
> > upload_sf_state for brw, gen6, gen7 and gen8.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42187
> > Signed-off-by: Plamena Manolova
>
> Wow! Great work tracking this down!
>
> So...not filling out these fields is equivalent to just setting
> "use point size state" to false. We can still set the actual
> point size (the big CLAMP expression) if we want.
>
> I think a simpler fix would be to make core Mesa set the
> ctx->VertexProgram.PointSizeEnabled boolean whenever
> GL_POINT_SIZE_ARRAY_OES is enabled/disabled. Then, no driver changes
> should be necessary.
>
> vao->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled means "please read the
> point size array". It makes ffvertex_prog.c copy the input point size
> to the output varying. But ctx->VertexProgram.PointSizeEnabled essentially
> means "use the value of the program's output varying", so I think it
> makes sense for drivers to check that, and core Mesa to set it.
>
> Does that seem reasonable?
>
> Thanks for fixing this longstanding bug!
>
> > ---
> > src/mesa/drivers/dri/i965/brw_sf_state.c | 19 +++
> > src/mesa/drivers/dri/i965/gen6_sf_state.c | 23 +--
> > src/mesa/drivers/dri/i965/gen7_sf_state.c | 17 ++---
> > src/mesa/drivers/dri/i965/gen8_sf_state.c | 17 ++---
> > 4 files changed, 44 insertions(+), 32 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_sf_state.c b/src/mesa/drivers/
> dri/i965/brw_sf_state.c
> > index b126f82..18504b1 100644
> > --- a/src/mesa/drivers/dri/i965/brw_sf_state.c
> > +++ b/src/mesa/drivers/dri/i965/brw_sf_state.c
> > @@ -259,15 +259,18 @@ static void upload_sf_unit( struct brw_context
> *brw )
> >
> > /* _NEW_POINT */
> > sf->sf7.sprite_point = ctx->Point.PointSprite;
> > - sf->sf7.point_size = CLAMP(rintf(CLAMP(ctx->Point.Size,
> > - ctx->Point.MinSize,
> > - ctx->Point.MaxSize)), 1.0f,
> 255.0f) *
> > -(1<<3);
> > - /* _NEW_PROGRAM | _NEW_POINT */
> > - sf->sf7.use_point_size_state = !(ctx->VertexProgram.PointSizeEnabled
> ||
> > - ctx->Point._Attenuated);
> > - sf->sf7.aa_line_distance_mode = 0;
> >
> > + if (!ctx->Array.VAO->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled) {
> > + sf->sf7.point_size = CLAMP(rintf(CLAMP(ctx->Point.Size,
> > + ctx->Point.MinSize,
> > + ctx->Point.MaxSize)),
> > + 1.0f, 255.0f) * (1<<3);
> > + /* _NEW_PROGRAM | _NEW_POINT */
> > + sf->sf7.use_point_size_state =
> !(ctx->VertexProgram.PointSizeEnabled
> ||
> > + ctx->Point._Attenuated);
> > + }
> > +
> > + sf->sf7.aa_line_distance_mode = 0;
> > /* might be BRW_NEW_PRIMITIVE if we have to adjust pv for polygons:
> > * _NEW_LIGHT
> > */
> > diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> b/src/mesa/drivers/
> dri/i965/gen6_sf_state.c
> > index 2634e6b..d506a2b 100644
> > --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> > @@ -406,16 +406,19 @@ upload_sf_state(struct brw_context *brw)
> > if (multisampled_fbo && ctx->Multisample.Enabled)
> >dw3 |= GEN6_SF_MSRAST_ON_PATTERN;
> >
> > - /* _NEW_PROGRAM | _NEW_POINT */
> > - if (!(ctx->VertexProgram.PointSizeEnabled ||
> > - ctx->Point._Attenuated))
> > - dw4 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> > -
> > - /* Clamp to ARB_point_parameters user limits */
> > - point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize, ctx-
> >Point.MaxSize);
> > -
> > - /* Clamp to the hardware limits and convert to fixed point */
> > - dw4 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> > + if (!ctx->Array.VAO->VertexAttrib[VERT_ATTRIB_POINT_SIZE].Enabled) {
> > + /* _NEW_PROGRAM | _NEW_POINT */
> > + if (!(ctx->VertexProgram.PointSizeEnabled ||
> > + ctx->Point._Attenuated))
> > + dw4 |= GEN6_SF_USE_STATE_POINT_WIDTH;
> > +
> > + /* Clamp to ARB_point_parameters user limits */
> > + point_size = CLAMP(ctx->Point.Size, ctx->Point.MinSize,
> > + ctx->Point.MaxSize);
> > +
> > + /* Clamp to the hardware limits and convert to fixed point */
> > + dw4 |= U_FIXED(CLAMP(point_size, 0.125f, 255.875f), 3);
> > + }
> >
> > /*
> >
Re: [Mesa-dev] [PATCH 3/3] anv: Port over CACHE_MODE_1 optimization fix enables from brw.
Looks good to me :)
This series is: Reviewed-by: Plamena Manolova
On Wed, May 24, 2017 at 8:56 AM, Kenneth Graunke
wrote:
> Ben and I haven't observed these to help anything, but they enable
> hardware optimizations for particular cases. It's probably best to
> enable them ahead of time, before we run into such a case.
> ---
> src/intel/vulkan/genX_state.c | 13 +
> 1 file changed, 13 insertions(+)
>
> diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
> index bf1217bbcdc..00c4105a825 100644
> --- a/src/intel/vulkan/genX_state.c
> +++ b/src/intel/vulkan/genX_state.c
> @@ -52,6 +52,19 @@ genX(init_device_state)(struct anv_device *device)
>ps.PipelineSelection = _3D;
> }
>
> +#if GEN_GEN >= 9
> + uint32_t cache_mode_1;
> + anv_pack_struct(&cache_mode_1, GENX(CACHE_MODE_1),
> + .PartialResolveDisableInVC = true,
> + .PartialResolveDisableInVCMask = true,
> + .FloatBlendOptimizationEnable = true,
> + .FloatBlendOptimizationEnableMask = true);
> + anv_batch_emit(&batch, GENX(MI_LOAD_REGISTER_IMM), lri) {
> + lri.RegisterOffset = GENX(CACHE_MODE_1_num);
> + lri.DataDWord = cache_mode_1;
> + }
> +#endif
> +
> anv_batch_emit(&batch, GENX(3DSTATE_AA_LINE_PARAMETERS), aa);
>
> anv_batch_emit(&batch, GENX(3DSTATE_DRAWING_RECTANGLE), rect) {
> --
> 2.12.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] i965/miptree: Use the new simple alloc_tiled for CCS buffers
Looks good to me :) The series is:
Reviewed-by: Plamena Manolova
On Wed, Jun 14, 2017 at 2:19 AM, Jason Ekstrand
wrote:
> ---
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 9 ++---
> 1 file changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index f24b0b5..7991f81 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1684,13 +1684,8 @@ intel_miptree_alloc_non_msrt_mcs(struct
> brw_context *brw,
> const uint32_t alloc_flags =
>is_lossless_compressed ? 0 : BO_ALLOC_FOR_RENDER;
>
> - /* ISL has stricter set of alignment rules then the drm allocator.
> -* Therefore one can pass the ISL dimensions in terms of bytes instead
> of
> -* trying to recalculate based on different format block sizes.
> -*/
> - buf->bo = brw_bo_alloc_tiled_2d(brw->bufmgr, "ccs-miptree",
> - buf->pitch, buf->size / buf->pitch,
> - 1, I915_TILING_Y, &buf->pitch,
> alloc_flags);
> + buf->bo = brw_bo_alloc_tiled(brw->bufmgr, "ccs-miptree", buf->size,
> +I915_TILING_Y, buf->pitch, alloc_flags);
> if (!buf->bo) {
>free(buf);
>free(aux_state);
> --
> 2.5.0.400.gff86faf
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix ARB_indirect_parameters logic.
Thank you for reviewing Ken!
On Thu, Oct 26, 2017 at 5:14 AM, Kenneth Graunke
wrote:
> On Wednesday, October 25, 2017 9:54:46 AM PDT Plamena Manolova wrote:
> > This patch modifies the ARB_indirect_parameters logic in
> > brw_draw_prims, so that our implementation isn't affected if
> > another application attempts to use predicates. Previously we
> > were using a predicate with a DELTAS_EQUAL comparison operation
> > and relying on the MI_PREDICATE_DATA register being 0.
>
> It's unfortunately a bit nastier of an explanation. How about:
>
> Our code to initialize MI_PREDICATE_DATA to 0 was incorrect, so we
> were accidentally using whatever value was written there. Because
> the kernel does not initialize the MI_PREDICATE_DATA register on
> hardware context creation, we might inherit the value from whatever
> context was last running on the GPU (likely another process).
>
> The Haswell command parser also does not currently allow us to write
> the MI_PREDICATE_DATA register. Rather than fixing this and requiring
> an updated kernel, we switch to a different approach which uses two
> predicates (one using SRC_EQUAL and the other DELTAS_EQUAL) that makes
> no assumption about the states of any of the predicate registers.
>
>
This sounds more accurate, thanks!
> This
> > assumtion is incorrect when another program is using predicates
> > which store data in MI_PREDICATE_DATA. This patch introduces a
> > different approach which uses 2 predicates (one using SRC_EQUAL
> > and the other DELTAS_EQUAL) that makes no assumption about the
> > states of any of the predicate registers.
> >
> > Fixes: piglit.spec.arb_indirect_parameters.tf-count-arrays
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103085
> >
> > Signed-off-by: Plamena Manolova
> > CC: Kenneth Graunke
> > ---
> > src/mesa/drivers/dri/i965/brw_draw.c | 75
> +---
> > 1 file changed, 44 insertions(+), 31 deletions(-)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 80d4891f6f..81465c79fb 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -866,7 +866,6 @@ brw_draw_prims(struct gl_context *ctx,
> > struct brw_context *brw = brw_context(ctx);
> > const struct gl_vertex_array **arrays = ctx->Array._DrawArrays;
> > int predicate_state = brw->predicate.state;
> > - int combine_op = MI_PREDICATE_COMBINEOP_SET;
> > struct brw_transform_feedback_object *xfb_obj =
> >(struct brw_transform_feedback_object *) gl_xfb_obj;
> >
> > @@ -910,49 +909,63 @@ brw_draw_prims(struct gl_context *ctx,
> > * to it.
> > */
> >
> > -if (brw->draw.draw_params_count_bo &&
> > -predicate_state == BRW_PREDICATE_STATE_USE_BIT) {
> > - /* We need to empty the MI_PREDICATE_DATA register since it might
> > - * already be set.
> > - */
> > -
> > - BEGIN_BATCH(4);
> > - OUT_BATCH(MI_PREDICATE_DATA);
> > - OUT_BATCH(0u);
> > - OUT_BATCH(MI_PREDICATE_DATA + 4);
> > - OUT_BATCH(0u);
> > - ADVANCE_BATCH();
> > -
> > - /* We need to combine the results of both predicates.*/
> > - combine_op = MI_PREDICATE_COMBINEOP_AND;
> > - }
> > -
> > for (i = 0; i < nr_prims; i++) {
> >/* Implementation of ARB_indirect_parameters via predicates */
> >if (brw->draw.draw_params_count_bo) {
> > - struct brw_bo *draw_id_bo = NULL;
> > - uint32_t draw_id_offset;
> > -
> > - intel_upload_data(brw, &prims[i].draw_id, 4, 4, &draw_id_bo,
> > - &draw_id_offset);
> > -
> > brw_emit_pipe_control_flush(brw, PIPE_CONTROL_FLUSH_ENABLE);
> >
> > + /*
>
> Extra line here, should be /* Upload the ... (also below).
>
> > + * Upload the current draw count from the draw parameters
> buffer to
> > + * MI_PREDICATE_SRC0
> > + */
> > brw_load_register_mem(brw, MI_PREDICATE_SRC0,
> > brw->draw.draw_params_count_bo,
> > brw->draw.draw_params_count_offset);
> > - brw_load_register_mem(brw, MI_PREDICATE_SRC1, draw_id_bo,
> > - draw_id_offset);
> > -
> > + /*
> > + * Upload the id of the current primitive to MI_PREDICATE_SRC1.
> > + */
> > + brw_load_register_imm64(brw, MI_PREDICATE_SRC1,
> prims[i].draw_id);
> > +
> > + /*
> > + * This calculates MI_PREDICATE_SRC0 - MI_PREDICATE_SRC1 and
> stores it
> > + * in MI_PREDICATE_DATA without modifying the Predicate State
> Bit since
> > + * we don't need the comparision result (it's
> > + * MI_PREDICATE_SRC0 == MI_PREDICATE_SRC0).
>
> MI_PREDICATE_SRC0 == MI_PREDICATE_SRC1, right?
>
>
Yeah, this is just a typo.
> + */
> > BEGIN_BATCH(1);
> > OUT_BATCH(GEN7_MI_PREDICATE |
Re: [Mesa-dev] [PATCH] i965: Fix ARB_indirect_parameters logic.
Thank you for helping me out with this Ken! On Tue, Oct 31, 2017 at 3:50 AM, Kenneth Graunke wrote: > On Monday, October 30, 2017 2:14:24 PM PDT Plamena Manolova wrote: > > This patch modifies the ARB_indirect_parameters logic in > > brw_draw_prims, so that our implementation isn't affected if > > another application attempts to use predicates. Previously we > > were using a predicate with a DELTAS_EQUAL comparison operation > > and relying on the MI_PREDICATE_DATA register being 0. Our code > > to initialize MI_PREDICATE_DATA to 0 was incorrect, so we were > > accidentally using whatever value was written there. Because the > > kernel does not initialize the MI_PREDICATE_DATA register on > > hardware context creation, we might inherit the value from whatever > > context was last running on the GPU (likely another process). > > The Haswell command parser also does not currently allow us to write > > the MI_PREDICATE_DATA register. Rather than fixing this and requiring > > an updated kernel, we switch to a different approach which uses a > > SRCS_EQUAL predicate that makes no assumptions about the states of any > > of the predicate registers. > > > > Fixes: piglit.spec.arb_indirect_parameters.tf-count-arrays > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103085 > > > > Signed-off-by: Plamena Manolova > > Reviewed-by: Kenneth Graunke > > and pushed. Thanks Pam! > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: add gitignore
Looks good to me :) Reviewed-by: Plamena Manolova On Fri, Apr 21, 2017 at 9:20 AM, Elie Tournier wrote: > Since commit ce562f9e3fa, two new files are generated. > We don't want to track them. > > Signed-off-by: Elie Tournier > --- > src/egl/.gitignore | 2 ++ > 1 file changed, 2 insertions(+) > create mode 100644 src/egl/.gitignore > > diff --git a/src/egl/.gitignore b/src/egl/.gitignore > new file mode 100644 > index 00..32331e9f3f > --- /dev/null > +++ b/src/egl/.gitignore > @@ -0,0 +1,2 @@ > +g_egldispatchstubs.c > +g_egldispatchstubs.h > -- > 2.11.0 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] intel/fs: Take into account amount of data read in spilling cost heuristic.
Hi Curro,
This series looks good to me :)
Reviewed-by: Plamena Manolova
On Fri, Apr 21, 2017 at 11:49 AM, Francisco Jerez
wrote:
> Until now the spilling cost calculation was neglecting the amount of
> data read from the register during the spilling cost calculation.
> This caused it to make suboptimal decisions in some cases leading to
> higher memory bandwidth usage than necessary.
>
> Improves Unigine Heaven performance by ~4% on BDW, reversing an
> unintended FPS regression from my previous commit
> 147e71242ce539ff28e282f009c332818c35f5ac with n=12 and statistical
> significance 5%. In addition SynMark2 OglCSDof performance is
> improved by an additional ~5% on SKL, and a Kerbal Space Program
> apitrace around the Moho planet I can provide on request improves by
> ~20%.
>
> Cc:
> ---
> src/intel/compiler/brw_fs_reg_allocate.cpp | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/intel/compiler/brw_fs_reg_allocate.cpp
> b/src/intel/compiler/brw_fs_reg_allocate.cpp
> index 2d4d46e..ec8e116 100644
> --- a/src/intel/compiler/brw_fs_reg_allocate.cpp
> +++ b/src/intel/compiler/brw_fs_reg_allocate.cpp
> @@ -822,7 +822,7 @@ fs_visitor::choose_spill_reg(struct ra_graph *g)
> foreach_block_and_inst(block, fs_inst, inst, cfg) {
>for (unsigned int i = 0; i < inst->sources; i++) {
> if (inst->src[i].file == VGRF)
> -spill_costs[inst->src[i].nr] += block_scale;
> +spill_costs[inst->src[i].nr] += regs_read(inst, i) *
> block_scale;
>}
>
>if (inst->dst.file == VGRF)
> --
> 2.10.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Always set AALINEDISTANCE_TRUE on Sandybridge.
Looks good to me :)
This series is: Reviewed-by: Plamena Manolova
On Wed, Apr 26, 2017 at 3:15 PM, Kenneth Graunke
wrote:
> We set this unconditionally on every other platform. Zero (Manhattan)
> isn't even listed as an option in the Sandybridge docs - only "true".
>
> Cc: rafael.antogno...@intel.com
> ---
> src/mesa/drivers/dri/i965/gen6_sf_state.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> index dd547790c9a..0f118b6678b 100644
> --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c
> +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c
> @@ -286,7 +286,7 @@ upload_sf_state(struct brw_context *brw)
>
> dw1 = GEN6_SF_SWIZZLE_ENABLE | num_outputs <<
> GEN6_SF_NUM_OUTPUTS_SHIFT;
> dw2 = GEN6_SF_STATISTICS_ENABLE;
> - dw3 = GEN6_SF_SCISSOR_ENABLE;
> + dw3 = GEN6_SF_SCISSOR_ENABLE | GEN6_SF_LINE_AA_MODE_TRUE;
> dw4 = 0;
>
> if (brw->sf.viewport_transform_enable)
> @@ -365,7 +365,6 @@ upload_sf_state(struct brw_context *brw)
> }
> if (ctx->Line.SmoothFlag) {
>dw3 |= GEN6_SF_LINE_AA_ENABLE;
> - dw3 |= GEN6_SF_LINE_AA_MODE_TRUE;
>dw3 |= GEN6_SF_LINE_END_CAP_WIDTH_1_0;
> }
> /* _NEW_MULTISAMPLE */
> --
> 2.12.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] mesa/main: Move NULL pointer check.
Thank you Rafael :)
On Thu, Jun 15, 2017 at 8:57 PM, Rafael Antognolli <
rafael.antogno...@intel.com> wrote:
> Indeed the other two commits are related to the no_error path, but I
> think at least this one should be applied. So it is:
>
> Reviewed-by: Rafael Antognolli
>
> On Wed, Jun 14, 2017 at 07:33:12PM +0300, Plamena Manolova wrote:
> > In blit_framebuffer we're already doing a NULL
> > pointer check for readFb and drawFb so it makes
> > sense to do it before we actually use the pointers.
> >
> > CID: 1412569
> > Signed-off-by: Plamena Manolova
> > ---
> > src/mesa/main/blit.c | 12 ++--
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/mesa/main/blit.c b/src/mesa/main/blit.c
> > index bb866b1..8bb3ba3 100644
> > --- a/src/mesa/main/blit.c
> > +++ b/src/mesa/main/blit.c
> > @@ -349,12 +349,6 @@ blit_framebuffer(struct gl_context *ctx,
> > {
> > FLUSH_VERTICES(ctx, 0);
> >
> > - /* Update completeness status of readFb and drawFb. */
> > - _mesa_update_framebuffer(ctx, readFb, drawFb);
> > -
> > - /* Make sure drawFb has an initialized bounding box. */
> > - _mesa_update_draw_buffer_bounds(ctx, drawFb);
> > -
> > if (!readFb || !drawFb) {
> >/* This will normally never happen but someday we may want to
> > * support MakeCurrent() with no drawables.
> > @@ -362,6 +356,12 @@ blit_framebuffer(struct gl_context *ctx,
> >return;
> > }
> >
> > + /* Update completeness status of readFb and drawFb. */
> > + _mesa_update_framebuffer(ctx, readFb, drawFb);
> > +
> > + /* Make sure drawFb has an initialized bounding box. */
> > + _mesa_update_draw_buffer_bounds(ctx, drawFb);
> > +
> > if (!no_error) {
> >const GLbitfield legalMaskBits = (GL_COLOR_BUFFER_BIT |
> > GL_DEPTH_BUFFER_BIT |
> > --
> > 2.9.3
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv: use devinfo for number of thread/eu
Reviewed-by: Plamena Manolova
On Wed, Jun 28, 2017 at 3:24 PM, Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:
> It turns out Gen9LP has fewer threads per EU (6 vs 7).
>
> Signed-off-by: Lionel Landwerlin
> ---
> src/intel/vulkan/anv_device.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
> index 504a757571f..3f761226484 100644
> --- a/src/intel/vulkan/anv_device.c
> +++ b/src/intel/vulkan/anv_device.c
> @@ -359,8 +359,9 @@ anv_physical_device_init(struct anv_physical_device
> *device,
>
> if (device->info.is_cherryview &&
> device->subslice_total > 0 && device->eu_total > 0) {
> - /* Logical CS threads = EUs per subslice * 7 threads per EU */
> - uint32_t max_cs_threads = device->eu_total / device->subslice_total
> * 7;
> + /* Logical CS threads = EUs per subslice * num threads per EU */
> + uint32_t max_cs_threads =
> + device->eu_total / device->subslice_total *
> device->info.num_thread_per_eu;
>
>/* Fuse configurations may give more threads than expected, never
> less. */
>if (max_cs_threads > device->info.max_cs_threads)
> --
> 2.13.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa/main: Advertise ARB_polygon_offset_clamp.
Yes, it doesn't seem to be part of the registry yet, but it's one of the requirements for GL 4.6 apparently. On Wed, Jun 28, 2017 at 4:53 PM, Ilia Mirkin wrote: > Google seems to have no mention of such an extension. > https://khronos.org/registry/OpenGL/index_gl.php doesn't either. > > On Wed, Jun 28, 2017 at 9:50 AM, Plamena Manolova > wrote: > > ARB_polygon_offset_clamp is just the ARB variation > > of EXT_polygon_offset_clamp and they operate in an > > identical manner, so there's no reason for us not > > to advertise it. > > > > Signed-off-by: Plamena Manolova > > --- > > src/mesa/main/extensions_table.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/src/mesa/main/extensions_table.h > b/src/mesa/main/extensions_table.h > > index 757b7bf..8286c76 100644 > > --- a/src/mesa/main/extensions_table.h > > +++ b/src/mesa/main/extensions_table.h > > @@ -94,6 +94,7 @@ EXT(ARB_pipeline_statistics_query , > ARB_pipeline_statistics_query > > EXT(ARB_pixel_buffer_object , EXT_pixel_buffer_object > , GLL, GLC, x , x , 2004) > > EXT(ARB_point_parameters, EXT_point_parameters > , GLL, x , x , x , 1997) > > EXT(ARB_point_sprite, ARB_point_sprite > , GLL, GLC, x , x , 2003) > > +EXT(ARB_polygon_offset_clamp, EXT_polygon_offset_clamp > , GLL, GLC, ES1, ES2, 2017) > > EXT(ARB_post_depth_coverage , ARB_post_depth_coverage > , x , GLC, x , x, 2015) > > EXT(ARB_program_interface_query , dummy_true >, GLL, GLC, x , x , 2012) > > EXT(ARB_provoking_vertex, EXT_provoking_vertex > , GLL, GLC, x , x , 2009) > > -- > > 2.9.3 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Implement ARB_indirect_parameters
Hi Ken,
Thank you so much for reviewing and helping me out with this! I'll make the
changes you suggested
to both patches and resubmit.
Thanks,
Pam
On Tue, Sep 26, 2017 at 3:32 AM, Kenneth Graunke
wrote:
> On Tuesday, August 29, 2017 9:24:01 AM PDT Plamena Manolova wrote:
> > We can implement ARB_indirect_parameters for i965 by
> > taking advantage of the conditional rendering mechanism.
> > This works by issuing maxdrawcount draw calls and using
> > conditional rendering to predicate each of them with
> > "drawcount > gl_DrawID"
> >
> > Signed-off-by: Plamena Manolova
> > ---
> > src/mesa/drivers/dri/i965/brw_context.h | 3 +
> > src/mesa/drivers/dri/i965/brw_draw.c | 97
>
> > src/mesa/drivers/dri/i965/brw_draw.h | 11
> > src/mesa/drivers/dri/i965/intel_extensions.c | 2 +
> > 4 files changed, 113 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> > index 2274fe5..4639d5b 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.h
> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
> > @@ -831,6 +831,9 @@ struct brw_context
> >int gl_drawid;
> >struct brw_bo *draw_id_bo;
> >uint32_t draw_id_offset;
> > +
> > + struct brw_bo *draw_params_count_bo;
> > + uint32_t draw_params_count_offset;
> > } draw;
> >
> > struct {
> > diff --git a/src/mesa/drivers/dri/i965/brw_draw.c
> b/src/mesa/drivers/dri/i965/brw_draw.c
> > index 7597bae..473958c 100644
> > --- a/src/mesa/drivers/dri/i965/brw_draw.c
> > +++ b/src/mesa/drivers/dri/i965/brw_draw.c
> > @@ -820,6 +820,11 @@ brw_end_draw_prims(struct gl_context *ctx,
> > brw_program_cache_check_size(brw);
> > brw_postdraw_reconcile_align_wa_slices(brw);
> > brw_postdraw_set_buffers_need_resolve(brw);
> > +
> > + if (brw->draw.draw_params_count_bo) {
> > + brw_bo_unreference(brw->draw.draw_params_count_bo);
> > + brw->draw.draw_params_count_bo = NULL;
> > + }
> > }
> >
> > void
> > @@ -837,6 +842,8 @@ brw_draw_prims(struct gl_context *ctx,
> > struct brw_context *brw = brw_context(ctx);
> > const struct gl_vertex_array **arrays = ctx->Array._DrawArrays;
> > int i;
> > + int predicate_state = brw->predicate.state;
> > + int combine_op = MI_PREDICATE_COMBINEOP_SET;
> > struct brw_transform_feedback_object *xfb_obj =
> >(struct brw_transform_feedback_object *) gl_xfb_obj;
> >
> > @@ -890,12 +897,101 @@ brw_draw_prims(struct gl_context *ctx,
> > * to it.
> > */
> >
> > + if (brw->draw.draw_params_count_bo &&
> > + predicate_state == BRW_PREDICATE_STATE_USE_BIT) {
> > + /* We do this to empty the MI_PREDICATE_DATA register */
> > + BEGIN_BATCH(4);
> > + OUT_BATCH(MI_PREDICATE_DATA);
> > + OUT_BATCH(0u);
> > + OUT_BATCH(MI_PREDICATE_DATA + 4);
> > + OUT_BATCH(0u);
> > + ADVANCE_BATCH();
> > +
> > + combine_op = MI_PREDICATE_COMBINEOP_AND;
> > + }
> > +
> > for (i = 0; i < nr_prims; i++) {
> > + if (brw->draw.draw_params_count_bo) {
> > + struct brw_bo *draw_id_bo = brw_bo_alloc(brw->bufmgr,
> "draw_id", 4, 4);
> > +
> > + brw_bo_reference(draw_id_bo);
>
> brw_bo_alloc starts you with a reference count of 1, so you don't want
> to call brw_bo_reference here - that would increase the refcount to 2,
> and your brw_bo_unreference() call below would only drop it to 1, which
> would leak the BO.
>
> > + brw_bo_subdata(draw_id_bo, 0, 4, &prims[i].draw_id);
>
> Buffer objects are always page-aligned sizes, so we're actually
> allocating a full 4 kilobytes for this 32-bit integer. That's kind
> of a bummer. To avoid this, we have a streaming upload buffer which we
> use to append random data we want to use in the batch. I'd use that
> instead:
>
>struct brw_bo *draw_id_bo;
>uint32_t draw_id_offset;
>
>intel_upload_data(brw, &prims[i].draw_id, 4, 4,
> &draw_id_bo, &draw_id_offset);
>
> (I'm assuming prims[i].draw_id is 4-byte aligned...it should be unless
> people have explicitly marked the _mesa_prim struct PACKED...)
>
> > +
> > + brw_emit_pipe_control_flush(brw, PIPE_CONTROL_FLUSH_ENABLE);
> > +
> > + brw_load_register_mem(brw, MI_PREDICATE_SRC0,
> > + brw->draw.draw_params_count_bo,
> > + brw->draw.draw_params_count_offset);
> > + brw_load_register_mem(brw, MI_PREDICATE_SRC1, draw_id_bo, 0);
>
> then make sure to use the offset:
>
> brw_load_register_mem(brw, MI_PREDICATE_SRC1,
>draw_id_bo, draw_id_offset);
>
> Note that intel_upload_* is only meant to be used for a single batch...
> if you need it to persist across multiple batches, you probably want to
> allocate your own BO like you originally did here.
>
> > +
> > + BEGIN_BATCH(1);
> > + OUT_BATCH(GEN7_MI_PREDICATE |
> > + M
Re: [Mesa-dev] [PATCH 0/4] i965: ARB_indirect_parameters
Thank you Ken! On Tue, Oct 3, 2017 at 2:26 AM, Kenneth Graunke wrote: > On Monday, October 2, 2017 1:58:23 PM PDT Plamena Manolova wrote: > > A series of patches introducing ARB_indirect_parameters > > for i965. We can implement ARB_indirect_parameters for i965 > > by taking advantage of the conditional rendering mechanism. > > This works by issuing maxdrawcount draw calls and using > > conditional rendering to predicate each of them with > > "drawcount > gl_DrawID". The first three patches are part > > of a necessary refactor of brw_try_draw_prims while the last > > one actually introduces the functionality. > > > > Plamena Manolova (4): > > i965: Introduce brw_prepare_drawing. > > i965: Indroduce brw_finish_drawing. > > i965: Refactor brw_try_draw_prims. > > i965: Implement ARB_indirect_parameters. > > > > src/mesa/drivers/dri/i965/brw_context.h | 8 + > > src/mesa/drivers/dri/i965/brw_draw.c | 384 > ++- > > src/mesa/drivers/dri/i965/brw_draw.h | 10 + > > src/mesa/drivers/dri/i965/intel_extensions.c | 4 +- > > 4 files changed, 273 insertions(+), 133 deletions(-) > > Looks great, Pam! Thank you! > > Series is: > Reviewed-by: Kenneth Graunke > > and pushed: > > To ssh://git.freedesktop.org/git/mesa/mesa >765e1fa3724..598d613dc31 master -> master > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir: add min/max optimisation
Looks good to me :)
Reviewed-by: Plamena Manolova >
On Wed, Jan 18, 2017 at 11:23 AM, Elie Tournier
wrote:
> Add the following optimisations:
>
> min(x, -x) = -abs(x)
> min(x, -abs(x)) = -abs(x)
> min(x, abs(x)) = x
> max(x, -abs(x)) = x
> max(x, abs(x)) = abs(x)
> max(x, -x) = abs(x)
>
> shader-db:
>
> total instructions in shared programs: 13067779 -> 13067775 (-0.00%)
> instructions in affected programs: 249 -> 245 (-1.61%)
> helped: 4
> HURT: 0
>
> total cycles in shared programs: 252054838 -> 252054806 (-0.00%)
> cycles in affected programs: 504 -> 472 (-6.35%)
> helped: 2
> HURT: 0
>
> Signed-off-by: Elie Tournier
> ---
> src/compiler/nir/nir_opt_algebraic.py | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/src/compiler/nir/nir_opt_algebraic.py
> b/src/compiler/nir/nir_opt_algebraic.py
> index a557f7bf37..0dfa53fbf4 100644
> --- a/src/compiler/nir/nir_opt_algebraic.py
> +++ b/src/compiler/nir/nir_opt_algebraic.py
> @@ -171,6 +171,18 @@ optimizations = [
> (('imax', a, a), a),
> (('umin', a, a), a),
> (('umax', a, a), a),
> + (('fmin', a, ('fneg', a)), ('fneg', ('fabs', a))),
> + (('imin', a, ('ineg', a)), ('ineg', ('iabs', a))),
> + (('fmin', a, ('fneg', ('fabs', a))), ('fneg', ('fabs', a))),
> + (('imin', a, ('ineg', ('iabs', a))), ('ineg', ('iabs', a))),
> + (('fmin', a, ('fabs', a)), a),
> + (('imin', a, ('iabs', a)), a),
> + (('fmax', a, ('fneg', ('fabs', a))), a),
> + (('imax', a, ('ineg', ('iabs', a))), a),
> + (('fmax', a, ('fabs', a)), ('fabs', a)),
> + (('imax', a, ('iabs', a)), ('iabs', a)),
> + (('fmax', a, ('fneg', a)), ('fabs', a)),
> + (('imax', a, ('ineg', a)), ('iabs', a)),
> (('~fmin', ('fmax', a, 0.0), 1.0), ('fsat', a),
> '!options->lower_fsat'),
> (('~fmax', ('fmin', a, 1.0), 0.0), ('fsat', a),
> '!options->lower_fsat'),
> (('fsat', a), ('fmin', ('fmax', a, 0.0), 1.0), 'options->lower_fsat'),
> --
> 2.11.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: call DrawBuffer(s) driver hook in update_framebuffer for windows-system FB
This looks good to me :)
Reviewed-by: Plamena Manolova
On Fri, Jan 20, 2017 at 9:38 AM, Boyan Ding wrote:
> When draw buffers are changed on a bound framebuffer, DrawBuffer(s) hook
> should be called. However, it is missing in update_framebuffer with
> window-system framebuffer, in which FB's draw buffer state should match
> context state, potentially resulting in a change.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99116
> Signed-off-by: Boyan Ding
> ---
> src/mesa/main/framebuffer.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/src/mesa/main/framebuffer.c b/src/mesa/main/framebuffer.c
> index c06130dc8d..55a6d5c004 100644
> --- a/src/mesa/main/framebuffer.c
> +++ b/src/mesa/main/framebuffer.c
> @@ -670,6 +670,16 @@ update_framebuffer(struct gl_context *ctx, struct
> gl_framebuffer *fb)
>if (fb->ColorDrawBuffer[0] != ctx->Color.DrawBuffer[0]) {
> _mesa_drawbuffers(ctx, fb, ctx->Const.MaxDrawBuffers,
> ctx->Color.DrawBuffer, NULL);
> +
> + /* Call device driver function if fb is the bound draw buffer. */
> + if (fb == ctx->DrawBuffer) {
> +if (ctx->Driver.DrawBuffers) {
> + ctx->Driver.DrawBuffers(ctx, ctx->Const.MaxDrawBuffers,
> + ctx->Color.DrawBuffer);
> +} else if (ctx->Driver.DrawBuffer) {
> + ctx->Driver.DrawBuffer(ctx, ctx->Color.DrawBuffer[0]);
> +}
> + }
>}
> }
> else {
> --
> 2.11.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: Enable EXT_compressed_ETC1_RGB8_sub_texture
Thank you for reviewing guys! AFAIK this extension is a driver-side feature
and can be enabled for all drivers that support ETC1. I'll go ahead and
update my patch.
On Fri, Jan 20, 2017 at 6:25 PM, Jason Ekstrand
wrote:
> On Fri, Jan 20, 2017 at 10:16 AM, Ilia Mirkin
> wrote:
>
>> What level of support would a driver need to provide? Can this just be
>> enabled for all drivers? [This seems like largely a driver-side
>> feature rather than hardware-based.]
>>
>
> My understanding is that we should just expose this extension on all
> hardware that supports ETC1. Obviously, if it doesn't support ETC1, you
> don't get this extension. :-)
>
>
>> On Fri, Jan 20, 2017 at 1:12 PM, Plamena Manolova
>> wrote:
>> > Since we already have the functionality in place and games
>> > like Game of Thrones seem to depend on this extension, I
>> > think it makes sense to enable it by making it part of
>> > the extension string even though its still a draft:
>> >
>> > https://www.khronos.org/registry/gles/extensions/EXT/EXT_
>> compressed_ETC1_RGB8_sub_texture.txt
>> >
>> > Note: OES_compressed_ETC1_RGB8_sub_texture seems to be listed
>> > in gl2ext.h, but there's no documentation for it in the KHR
>> > registry
>> >
>> > Signed-off-by: Plamena Manolova
>> > ---
>> > src/mesa/drivers/dri/i965/intel_extensions.c | 1 +
>> > src/mesa/main/extensions_table.h | 1 +
>> > src/mesa/main/mtypes.h | 1 +
>> > src/mesa/main/teximage.c | 5 +
>> > 4 files changed, 8 insertions(+)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/intel_extensions.c
>> b/src/mesa/drivers/dri/i965/intel_extensions.c
>> > index b674b2f..bdf2fa5 100644
>> > --- a/src/mesa/drivers/dri/i965/intel_extensions.c
>> > +++ b/src/mesa/drivers/dri/i965/intel_extensions.c
>> > @@ -93,6 +93,7 @@ intelInitExtensions(struct gl_context *ctx)
>> > ctx->Extensions.EXT_blend_equation_separate = true;
>> > ctx->Extensions.EXT_blend_func_separate = true;
>> > ctx->Extensions.EXT_blend_minmax = true;
>> > + ctx->Extensions.EXT_compressed_ETC1_RGB8_sub_texture = true;
>> > ctx->Extensions.EXT_draw_buffers2 = true;
>> > ctx->Extensions.EXT_framebuffer_sRGB = true;
>> > ctx->Extensions.EXT_gpu_program_parameters = true;
>> > diff --git a/src/mesa/main/extensions_table.h
>> b/src/mesa/main/extensions_table.h
>> > index 2de3c59..8b52a97 100644
>> > --- a/src/mesa/main/extensions_table.h
>> > +++ b/src/mesa/main/extensions_table.h
>> > @@ -198,6 +198,7 @@ EXT(EXT_buffer_storage ,
>> ARB_buffer_storage
>> > EXT(EXT_clip_cull_distance , ARB_cull_distance
>> , x , x , x , 30, 2016)
>> > EXT(EXT_color_buffer_float , dummy_true
>>, x , x , x , 30, 2013)
>> > EXT(EXT_compiled_vertex_array , dummy_true
>>, GLL, x , x , x , 1996)
>> > +EXT(EXT_compressed_ETC1_RGB8_sub_texture,
>> EXT_compressed_ETC1_RGB8_sub_texture , x , x , ES1, ES2, 2014)
>> > EXT(EXT_copy_image , OES_copy_image
>>, x , x , x , 30, 2014)
>> > EXT(EXT_copy_texture, dummy_true
>>, GLL, x , x , x , 1995)
>> > EXT(EXT_depth_bounds_test , EXT_depth_bounds_test
>> , GLL, GLC, x , x , 2002)
>> > diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>> > index f04ec51..719e248 100644
>> > --- a/src/mesa/main/mtypes.h
>> > +++ b/src/mesa/main/mtypes.h
>> > @@ -3948,6 +3948,7 @@ struct gl_extensions
>> > GLboolean EXT_timer_query;
>> > GLboolean EXT_vertex_array_bgra;
>> > GLboolean EXT_window_rectangles;
>> > + GLboolean EXT_compressed_ETC1_RGB8_sub_texture;
>> > GLboolean OES_copy_image;
>> > GLboolean OES_primitive_bounding_box;
>> > GLboolean OES_sample_variables;
>> > diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>> > index bc3b76a..14fe4db 100644
>> > --- a/src/mesa/main/teximage.c
>> > +++ b/src/mesa/main/teximage.c
>> > @@ -1323,6 +1323,11 @@ compressedteximage_only_format(const struct
>> gl_context *ctx, GLenum format)
>> > {
>> > switch (format) {
>> > case GL_ETC1_RGB8_OES:
>> > + if (ctx->Extensions.EXT_compressed_ETC1_RGB8_sub_texture)
>> > + return false;
>> > + else
>> > + return true;
>> > + break;
>> > case GL_PALETTE4_RGB8_OES:
>> > case GL_PALETTE4_RGBA8_OES:
>> > case GL_PALETTE4_R5_G6_B5_OES:
>> > --
>> > 2.4.3
>> >
>> > ___
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop
Re: [Mesa-dev] [PATCH 1/4] mesa: Trivial clean-ups in uniform_query.cpp
Looks good to me :) Reviewed-by: Plamena Manolova <
plamena.manol...@intel.com>
On Wed, Jan 25, 2017 at 1:30 AM, Ian Romanick wrote:
> From: Ian Romanick
>
> This is C++, so we can mix code and declarations. Doing so allows
> constification.
>
> Signed-off-by: Ian Romanick
> ---
> src/mesa/main/uniform_query.cpp | 12
> 1 file changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/main/uniform_query.cpp
> b/src/mesa/main/uniform_query.cpp
> index d5a2d0f..c2429c1 100644
> --- a/src/mesa/main/uniform_query.cpp
> +++ b/src/mesa/main/uniform_query.cpp
> @@ -992,10 +992,6 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct
> gl_shader_program *shProg,
> const GLvoid *values, enum glsl_base_type basicType)
> {
> unsigned offset;
> - unsigned vectors;
> - unsigned components;
> - unsigned elements;
> - int size_mul;
> struct gl_uniform_storage *const uni =
>validate_uniform_parameters(ctx, shProg, location, count,
>&offset, "glUniformMatrix");
> @@ -1009,11 +1005,11 @@ _mesa_uniform_matrix(struct gl_context *ctx,
> struct gl_shader_program *shProg,
> }
>
> assert(basicType == GLSL_TYPE_FLOAT || basicType == GLSL_TYPE_DOUBLE);
> - size_mul = basicType == GLSL_TYPE_DOUBLE ? 2 : 1;
> + const unsigned size_mul = basicType == GLSL_TYPE_DOUBLE ? 2 : 1;
>
> assert(!uni->type->is_sampler());
> - vectors = uni->type->matrix_columns;
> - components = uni->type->vector_elements;
> + const unsigned vectors = uni->type->matrix_columns;
> + const unsigned components = uni->type->vector_elements;
>
> /* Verify that the types are compatible. This is greatly simplified
> for
> * matrices because they can only have a float base type.
> @@ -1084,7 +1080,7 @@ _mesa_uniform_matrix(struct gl_context *ctx, struct
> gl_shader_program *shProg,
>
> /* Store the data in the "actual type" backing storage for the uniform.
> */
> - elements = components * vectors;
> + const unsigned elements = components * vectors;
>
> if (!transpose) {
>memcpy(&uni->storage[size_mul * elements * offset], values,
> --
> 2.7.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] mesa: Add GL/GLSL plumbing for ARB_fragment_shader_interlock.
Thank you for reviewing Timothy!
On Mon, Apr 17, 2017 at 7:10 PM, Timothy Arceri
wrote:
>
>
> On 18/04/17 11:25, Plamena Manolova wrote:
>
>> This extension provides new GLSL built-in functions
>> beginInvocationInterlockARB() and endInvocationInterlockARB()
>> that delimit a critical section of fragment shader code. For
>> pairs of shader invocations with "overlapping" coverage in a
>> given pixel, the OpenGL implementation will guarantee that the
>> critical section of the fragment shader will be executed for
>> only one fragment at a time.
>>
>> Signed-off-by: Plamena Manolova
>> ---
>> src/compiler/glsl/ast.h | 10 +++
>> src/compiler/glsl/ast_to_hir.cpp | 21 ++
>> src/compiler/glsl/ast_type.cpp | 90 -
>> src/compiler/glsl/builtin_functions.cpp | 79 ++
>> src/compiler/glsl/glsl_parser.yy | 109
>> +++
>> src/compiler/glsl/glsl_parser_extras.cpp | 13
>> src/compiler/glsl/glsl_parser_extras.h | 7 ++
>> src/compiler/glsl/glsl_to_nir.cpp| 12
>> src/compiler/glsl/ir.h | 2 +
>> src/compiler/glsl/linker.cpp | 8 +++
>> src/compiler/nir/nir_intrinsics.h| 2 +
>> src/compiler/shader_info.h | 5 ++
>> src/mesa/main/extensions_table.h | 1 +
>> src/mesa/main/mtypes.h | 5 ++
>> 14 files changed, 363 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/compiler/glsl/ast.h b/src/compiler/glsl/ast.h
>> index 455cb81..f2f7e9e 100644
>> --- a/src/compiler/glsl/ast.h
>> +++ b/src/compiler/glsl/ast.h
>> @@ -617,6 +617,16 @@ struct ast_type_qualifier {
>>* Flag set if GL_ARB_post_depth_coverage layout qualifier is
>> used.
>>*/
>> unsigned post_depth_coverage:1;
>> +
>> + /**
>> + * Flags for the layout qualifers added by
>> ARB_fragment_shader_interlock
>> + */
>> +
>> + unsigned pixel_interlock_ordered:1;
>> + unsigned pixel_interlock_unordered:1;
>> + unsigned sample_interlock_ordered:1;
>> + unsigned sample_interlock_unordered:1;
>>
>
> Samuel spent a bunch of time freeing up 4 bits of this flag to be able to
> implement ARB_bindless_texture. This change will use up all those free bits.
>
> These only apply to the default in right? I wonder if it's possible to
> split those qualifiers off from the layout qualifiers applied to varyings?
>
I think it should be possible to split them out, I can have a go at that.
+
>> /**
>>* Flag set if GL_INTEL_conservartive_rasterization layout
>> qualifier
>>* is used.
>> diff --git a/src/compiler/glsl/ast_to_hir.cpp
>> b/src/compiler/glsl/ast_to_hir.cpp
>> index 9ea37f4..71c52ad 100644
>> --- a/src/compiler/glsl/ast_to_hir.cpp
>> +++ b/src/compiler/glsl/ast_to_hir.cpp
>> @@ -3717,6 +3717,27 @@ apply_layout_qualifier_to_variable(const struct
>> ast_type_qualifier *qual,
>>_mesa_glsl_error(loc, state, "post_depth_coverage layout qualifier
>> only "
>> "valid in fragment shader input layout
>> declaration.");
>> }
>> +
>> + if (qual->flags.q.pixel_interlock_ordered) {
>> + _mesa_glsl_error(loc, state, "pixel_interlock_ordered layout
>> qualifier only "
>> + "valid in fragment shader input layout
>> declaration.");
>> + }
>> +
>> + if (qual->flags.q.pixel_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "pixel_interlock_unordered layout
>> qualifier only "
>> + "valid in fragment shader input layout
>> declaration.");
>> + }
>> +
>> +
>> + if (qual->flags.q.pixel_interlock_ordered) {
>> + _mesa_glsl_error(loc, state, "sample_interlock_ordered layout
>> qualifier only "
>> + "valid in fragment shader input layout
>> declaration.");
>> + }
>> +
>> + if (qual->flags.q.pixel_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "sample_interlock_unordered layout
>> qualifier only "
>> + "valid in fragment shader input layout
>> declaration.");
>> + }
>>
> Here and below we duplicated the validation done in glsl_parser.yy. Does
> the parser validation not catch everything?
>
> Also there seems to be no link time validation to check that "only one
> interlock mode can be used at any time" when we are compiling a program
> that contains multiple fragment shaders.
> Do you have any piglit tests to go with this series?
>
Yes there's a test here: https://patchwork.freedesktop.org/patch/151133
I'll look into whether we actually need validation in both glsl_parser.yy
and ast_type.cpp
>
> }
>>
>> static void
>> diff --git a/src/compiler/glsl/ast_type.cpp
>> b/src/compiler/glsl/ast_type.cpp
>> index d302fc4..0e74253 100644
>> --- a/src/compiler/glsl/ast_type.cpp
>> +++ b/src/compiler/glsl/ast_type.cpp
>> @@ -580,6 +580,10 @@ ast_type_qualifier::validate_
Re: [Mesa-dev] [PATCH 1/2] mesa: Add GL/GLSL plumbing for ARB_fragment_shader_interlock.
Thank you for reviewing Nicolai!
On Tue, Apr 18, 2017 at 4:17 AM, Nicolai Hähnle wrote:
> On 18.04.2017 03:25, Plamena Manolova wrote:
>
>> This extension provides new GLSL built-in functions
>> beginInvocationInterlockARB() and endInvocationInterlockARB()
>> that delimit a critical section of fragment shader code. For
>> pairs of shader invocations with "overlapping" coverage in a
>> given pixel, the OpenGL implementation will guarantee that the
>> critical section of the fragment shader will be executed for
>> only one fragment at a time.
>>
>> Signed-off-by: Plamena Manolova
>> ---
>> src/compiler/glsl/ast.h | 10 +++
>> src/compiler/glsl/ast_to_hir.cpp | 21 ++
>> src/compiler/glsl/ast_type.cpp | 90 -
>> src/compiler/glsl/builtin_functions.cpp | 79 ++
>> src/compiler/glsl/glsl_parser.yy | 109
>> +++
>> src/compiler/glsl/glsl_parser_extras.cpp | 13
>> src/compiler/glsl/glsl_parser_extras.h | 7 ++
>> src/compiler/glsl/glsl_to_nir.cpp| 12
>> src/compiler/glsl/ir.h | 2 +
>> src/compiler/glsl/linker.cpp | 8 +++
>> src/compiler/nir/nir_intrinsics.h| 2 +
>> src/compiler/shader_info.h | 5 ++
>> src/mesa/main/extensions_table.h | 1 +
>> src/mesa/main/mtypes.h | 5 ++
>> 14 files changed, 363 insertions(+), 1 deletion(-)
>>
>> [snip]
>
> @@ -651,6 +655,86 @@ ast_type_qualifier::merge_into_in_qualifier(YYLTYPE
>> *loc,
>>r = false;
>> }
>>
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_unordered ||
>> + state->in_qualifier->flags.q.sample_interlock_ordered ||
>> + state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (!state->ctx->Multisample.Enabled) {
>> +state->fs_pixel_interlock_ordered = true;
>> +state->in_qualifier->flags.q.pixel_interlock_ordered =
>> false;
>> + } else {
>> +_mesa_glsl_error(loc, state,
>> + "pixel_interlock_ordered can only be used
>> when "
>> + "multisampling is disabled.");
>> +r = false;
>> + }
>> + }
>> + }
>> +
>> + if (state->in_qualifier->flags.q.pixel_interlock_unordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered ||
>> + state->in_qualifier->flags.q.sample_interlock_ordered ||
>> + state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (!state->ctx->Multisample.Enabled) {
>> +state->fs_pixel_interlock_unordered = true;
>> +state->in_qualifier->flags.q.pixel_interlock_unordered =
>> false;
>> + } else {
>> +_mesa_glsl_error(loc, state,
>> + "pixel_interlock_unordered can only be used
>> when "
>> + "multisampling is disabled.");
>> +r = false;
>> + }
>> + }
>> + }
>> +
>> + if (state->in_qualifier->flags.q.sample_interlock_ordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered ||
>> + state->in_qualifier->flags.q.pixel_interlock_unordered ||
>> + state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (state->ctx->Multisample.Enabled) {
>> +state->fs_sample_interlock_ordered = true;
>> +state->in_qualifier->flags.q.sample_interlock_ordered =
>> false;
>> + } else {
>> +_mesa_glsl_error(loc, state,
>> + "sample_interlock_ordered can only be used
>> when "
>> + "multisampling is enabled.");
>> +r = false;
>> + }
>> + }
>> + }
>> +
>> + if (state->in_qualifier->flags.q.sample_interlock_unordered) {
>> + if (state->in_qualifier->flags.q.pixel_interlock_ordered ||
>> + state->in_qualifier->flags.q.pixel_interlock_unordered ||
>> + state->in_qualifier->flags.q.sample_interlock_ordered) {
>> + _mesa_glsl_error(loc, state, "only one interlock mode can be
>> used at "
>> + "any time.");
>> + r = false;
>> + } else {
>> + if (state->ctx->Multisample.Enabled) {
>> +state->fs_sample_interlock_unordered = true;
>
Re: [Mesa-dev] [PATCH 2/2] i965: Add ARB_fragment_shader_interlock support.
Hi Francisco,
Thank you for reviewing!
On Wed, Apr 19, 2017 at 4:18 PM, Francisco Jerez
wrote:
> Hi Pam, looks good overall, a couple of comments below,
>
> Plamena Manolova writes:
>
> > Adds suppport for ARB_fragment_shader_interlock. We achieve
> > the interlock and fragment ordering by issuing a memory fence
> > via sendc.
> >
> > Signed-off-by: Plamena Manolova
> > ---
> > docs/features.txt| 2 +-
> > docs/relnotes/17.1.0.html| 1 +
> > src/intel/compiler/brw_eu.h | 4 +++
> > src/intel/compiler/brw_eu_defines.h | 2 ++
> > src/intel/compiler/brw_eu_emit.c | 47
>
> > src/intel/compiler/brw_fs_generator.cpp | 4 +++
> > src/intel/compiler/brw_fs_nir.cpp| 15 +
> > src/intel/compiler/brw_shader.cpp| 4 +++
> > src/mesa/drivers/dri/i965/intel_extensions.c | 5 +++
> > 9 files changed, 83 insertions(+), 1 deletion(-)
> >
> > diff --git a/docs/features.txt b/docs/features.txt
> > index 5f63632..a6237c0 100644
> > --- a/docs/features.txt
> > +++ b/docs/features.txt
> > @@ -281,7 +281,7 @@ Khronos, ARB, and OES extensions that are not part
> of any OpenGL or OpenGL ES ve
> >GL_ARB_cl_event not started
> >GL_ARB_compute_variable_group_sizeDONE (nvc0,
> radeonsi)
> >GL_ARB_ES3_2_compatibilityDONE
> (i965/gen8+)
> > - GL_ARB_fragment_shader_interlock not started
> > + GL_ARB_fragment_shader_interlock DONE (i965)
> >GL_ARB_gl_spirv not started
> >GL_ARB_gpu_shader_int64 DONE
> (i965/gen8+, nvc0, radeonsi, softpipe, llvmpipe)
> >GL_ARB_indirect_parametersDONE (nvc0,
> radeonsi)
> > diff --git a/docs/relnotes/17.1.0.html b/docs/relnotes/17.1.0.html
> > index e7cfe38..1b2393f 100644
> > --- a/docs/relnotes/17.1.0.html
> > +++ b/docs/relnotes/17.1.0.html
> > @@ -45,6 +45,7 @@ Note: some of the new features are only available with
> certain drivers.
> >
> >
> > OpenGL 4.2 on i965/ivb
> > +GL_ARB_fragment_shader_interlock on i965
> > GL_ARB_gpu_shader_fp64 on i965/ivybridge
> > GL_ARB_gpu_shader_int64 on i965/gen8+, nvc0, radeonsi, softpipe,
> llvmpipe
> > GL_ARB_shader_ballot on nvc0, radeonsi
> > diff --git a/src/intel/compiler/brw_eu.h b/src/intel/compiler/brw_eu.h
> > index f422595..117cfae 100644
> > --- a/src/intel/compiler/brw_eu.h
> > +++ b/src/intel/compiler/brw_eu.h
> > @@ -480,6 +480,10 @@ brw_memory_fence(struct brw_codegen *p,
> > struct brw_reg dst);
> >
> > void
> > +brw_interlock(struct brw_codegen *p,
> > + struct brw_reg dst);
> > +
> > +void
> > brw_pixel_interpolator_query(struct brw_codegen *p,
> > struct brw_reg dest,
> > struct brw_reg mrf,
> > diff --git a/src/intel/compiler/brw_eu_defines.h
> b/src/intel/compiler/brw_eu_defines.h
> > index 13a70f6..9eb5210 100644
> > --- a/src/intel/compiler/brw_eu_defines.h
> > +++ b/src/intel/compiler/brw_eu_defines.h
> > @@ -444,6 +444,8 @@ enum opcode {
> > */
> > SHADER_OPCODE_BROADCAST,
> >
> > + SHADER_OPCODE_INTERLOCK,
> > +
> > VEC4_OPCODE_MOV_BYTES,
> > VEC4_OPCODE_PACK_BYTES,
> > VEC4_OPCODE_UNPACK_UNIFORM,
> > diff --git a/src/intel/compiler/brw_eu_emit.c
> b/src/intel/compiler/brw_eu_emit.c
> > index 231d6fd..52adf22 100644
> > --- a/src/intel/compiler/brw_eu_emit.c
> > +++ b/src/intel/compiler/brw_eu_emit.c
> > @@ -3403,6 +3403,53 @@ brw_memory_fence(struct brw_codegen *p,
> > }
> >
> > void
> > +brw_interlock(struct brw_codegen *p,
> > + struct brw_reg dst)
> > +{
> > + const struct gen_device_info *devinfo = p->devinfo;
> > + const bool commit_enable = devinfo->gen == 7 && !devinfo->is_haswell;
> > + struct brw_inst *insn;
> > +
> > + brw_push_insn_state(p);
> > + brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> > + brw_set_default_exec_size(p, BRW_EXECUTE_1);
> > + dst = vec1(dst);
> > +
> > + /* Set dst as destination for dependency tracking, the MEMORY_FENCE
> > +* message doesn't write anything back.
> > +*/
> > + /* BRW_OPCODE_SENDC is what the interlock actually depends on */
> > + insn = next_insn(p, BRW_OPCODE_SENDC);
> > + dst = retype(dst, BRW_REGISTER_TYPE_UW);
> > + brw_set_dest(p, insn, dst);
> > + brw_set_src0(p, insn, dst);
> > + /* Issuing a memory fence ensures the ordering of fragments */
> > + brw_set_memory_fence_message(p, insn, GEN7_SFID_DATAPORT_DATA_CACHE,
> > +commit_enable);
> > +
> > + if (devinfo->gen == 7 && !devinfo->is_haswell) {
> > + /* IVB does typed surface access through the render cache, so we
> need to
> > + * flush it too. Use a different register so both flushes can be
> > +
Re: [Mesa-dev] [PATCH] mesa/glthread: correctly comparae thread handles
Looks good to me :)
Reviewed-by: Plamena Manolova
On Thu, Apr 20, 2017 at 8:24 AM, Emil Velikov
wrote:
> From: Emil Velikov
>
> As mentioned in the manual - comparing pthread_t handles via the C
> comparison operator is incorrect and pthread_equal() should be used
> instead.
>
> Cc: Timothy Arceri
> Cc: Eric Anholt
> Fixes: d8d81fbc316 ("mesa: Add infrastructure for a worker thread to
> process GL commands.")
> Signed-off-by: Emil Velikov
> ---
> src/mesa/main/glthread.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/mesa/main/glthread.c b/src/mesa/main/glthread.c
> index c4d3f4a4349..455b829cd8d 100644
> --- a/src/mesa/main/glthread.c
> +++ b/src/mesa/main/glthread.c
> @@ -265,7 +265,7 @@ _mesa_glthread_finish(struct gl_context *ctx)
> * dri interface entrypoints), in which case we don't need to actually
> * synchronize against ourself.
> */
> - if (pthread_self() == glthread->thread)
> + if (pthread_equal(pthread_self(), glthread->thread))
>return;
>
> pthread_mutex_lock(&glthread->mutex);
> --
> 2.12.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 2/2] i965: Implement ARB_compute_variable_group_size.
Hi Karol,
Thank you for reviewing! I'll go ahead and push the changes you need from
nir_lower_system_values.c to master.
Thank you,
Pam
On Thu, Jun 28, 2018 at 5:50 AM, Karol Herbst wrote:
> Hi,
>
> if the changes inside "src/compiler/nir/nir_lower_system_values.c" are
> extracted into a seperate patch, this patch with the equal changes
> would be
>
> Reviewed-by: Karol Herbst
>
> I would need that for a nir to codegen pass for Nouveau and maybe it
> will help other drivers implementing this extension as well. I don't
> think it would hurt to extract those, right?
>
> Thanks!
>
> On Thu, Jun 7, 2018 at 5:34 PM, Plamena Manolova
> wrote:
> > This patch adds the implementation of ARB_compute_variable_group_size
> > for i965. We do this by storing the group size in a buffer surface,
> > similarly to the work group number.
> >
> > v2: Fix some indentation inconsistencies (Jordan, Ilia)
> > Do DIV_ROUND_UP correctly in brw_nir_lower_cs_intrinsics.c (Jordan)
> > Use alphabetical order in features.txt (Matt)
> > Set the extension constants properly in brw_context.c
> >
> > Signed-off-by: Plamena Manolova
> > ---
> > docs/features.txt| 2 +-
> > docs/relnotes/18.2.0.html| 1 +
> > src/compiler/nir/nir_lower_system_values.c | 13
> > src/intel/compiler/brw_compiler.h| 2 +
> > src/intel/compiler/brw_fs.cpp| 45
> > src/intel/compiler/brw_fs_nir.cpp| 20 ++
> > src/intel/compiler/brw_nir_lower_cs_intrinsics.c | 88
> +---
> > src/mesa/drivers/dri/i965/brw_compute.c | 25 ++-
> > src/mesa/drivers/dri/i965/brw_context.c | 6 ++
> > src/mesa/drivers/dri/i965/brw_context.h | 1 +
> > src/mesa/drivers/dri/i965/brw_cs.c | 4 ++
> > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 27 +++-
> > src/mesa/drivers/dri/i965/intel_extensions.c | 1 +
> > 13 files changed, 193 insertions(+), 42 deletions(-)
> >
> > diff --git a/docs/features.txt b/docs/features.txt
> > index ed4050cf98..81b6663288 100644
> > --- a/docs/features.txt
> > +++ b/docs/features.txt
> > @@ -298,7 +298,7 @@ Khronos, ARB, and OES extensions that are not part
> of any OpenGL or OpenGL ES ve
> >
> >GL_ARB_bindless_texture DONE (nvc0,
> radeonsi)
> >GL_ARB_cl_event not started
> > - GL_ARB_compute_variable_group_sizeDONE (nvc0,
> radeonsi)
> > + GL_ARB_compute_variable_group_sizeDONE (i965,
> nvc0, radeonsi)
> >GL_ARB_ES3_2_compatibilityDONE
> (i965/gen8+)
> >GL_ARB_fragment_shader_interlock DONE (i965)
> >GL_ARB_gpu_shader_int64 DONE
> (i965/gen8+, nvc0, radeonsi, softpipe, llvmpipe)
> > diff --git a/docs/relnotes/18.2.0.html b/docs/relnotes/18.2.0.html
> > index 0db37b620d..7475a56633 100644
> > --- a/docs/relnotes/18.2.0.html
> > +++ b/docs/relnotes/18.2.0.html
> > @@ -52,6 +52,7 @@ Note: some of the new features are only available with
> certain drivers.
> >
> >
> > GL_ARB_fragment_shader_interlock on i965
> > +GL_ARB_compute_variable_group_size on i965
> >
> >
> > Bug fixes
> > diff --git a/src/compiler/nir/nir_lower_system_values.c
> b/src/compiler/nir/nir_lower_system_values.c
> > index 487da04262..7ab005b000 100644
> > --- a/src/compiler/nir/nir_lower_system_values.c
> > +++ b/src/compiler/nir/nir_lower_system_values.c
> > @@ -57,6 +57,14 @@ convert_block(nir_block *block, nir_builder *b)
> >*gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"
> >*/
> >
> > + /*
> > + * If the local work group size is variable we can't lower the
> global
> > + * invocation id here.
> > + */
> > + if (b->shader->info.cs.local_size_variable) {
> > +break;
> > + }
> > +
> > nir_const_value local_size;
> > memset(&local_size, 0, sizeof(local_size));
> > local_size.u32[0] = b->shader->info.cs.local_size[0];
> > @@ -102,6 +110,11 @@ convert_block(nir_block *block, nir_builder *b)
> >}
> >
> >case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
> > + /* If the local work group size is variable we can't lower it
> here */
> > + if (b->shader->info.cs.local_size_variable) {
> > +break;
> > + }
> > +
> > nir_const_value local_size;
> > memset(&local_size, 0, sizeof(local_size));
> > local_size.u32[0] = b->shader->info.cs.local_size[0];
> > diff --git a/src/intel/compiler/brw_compiler.h b/src/intel/compiler/brw_
> compiler.h
> > index 8b4e6fe2e2..f54952c28f 100644
> > --- a/src/intel/compiler/brw_compiler.h
> > +++ b/src/intel/compiler/brw_compiler.h
> > @@ -759,6 +759,7 @@ struct brw_cs_prog_data {
> > unsigned threads;
> > bool uses_
Re: [Mesa-dev] [PATCH] Add NV_fragment_shader_interlock support.
Hi Kevin,
This looks good to me :)
Reviewed-by: Plamena Manolova
On Wed, Aug 15, 2018 at 2:29 PM, wrote:
> From: Kevin Rogovin
>
> The main purpose for having NV_fragment_shader_interlock
> extension is because that extension is also for GLES31 while
> the ARB extension is for GL only.
> ---
> src/compiler/glsl/builtin_functions.cpp | 18 ++
> src/compiler/glsl/glsl_parser.yy | 6 --
> src/compiler/glsl/glsl_parser_extras.cpp | 1 +
> src/compiler/glsl/glsl_parser_extras.h | 2 ++
> src/mesa/main/extensions_table.h | 1 +
> 5 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/src/compiler/glsl/builtin_functions.cpp
> b/src/compiler/glsl/builtin_functions.cpp
> index 7119903795..e7b78c0158 100644
> --- a/src/compiler/glsl/builtin_functions.cpp
> +++ b/src/compiler/glsl/builtin_functions.cpp
> @@ -519,6 +519,12 @@ supports_arb_fragment_shader_interlock(const
> _mesa_glsl_parse_state *state)
> return state->ARB_fragment_shader_interlock_enable;
> }
>
> +static bool
> +supports_nv_fragment_shader_interlock(const _mesa_glsl_parse_state
> *state)
> +{
> + return state->NV_fragment_shader_interlock_enable;
> +}
> +
> static bool
> shader_clock(const _mesa_glsl_parse_state *state)
> {
> @@ -3331,6 +3337,18 @@ builtin_builder::create_builtins()
> supports_arb_fragment_shader_interlock),
> NULL);
>
> + add_function("beginInvocationInterlockNV",
> +_invocation_interlock(
> + "__intrinsic_begin_invocation_interlock",
> + supports_nv_fragment_shader_interlock),
> +NULL);
> +
> + add_function("endInvocationInterlockNV",
> +_invocation_interlock(
> + "__intrinsic_end_invocation_interlock",
> + supports_nv_fragment_shader_interlock),
> +NULL);
> +
> add_function("anyInvocationARB",
> _vote("__intrinsic_vote_any", vote),
> NULL);
> diff --git a/src/compiler/glsl/glsl_parser.yy
> b/src/compiler/glsl/glsl_parser.yy
> index cb7376995d..bc2571b684 100644
> --- a/src/compiler/glsl/glsl_parser.yy
> +++ b/src/compiler/glsl/glsl_parser.yy
> @@ -1450,10 +1450,12 @@ layout_qualifier_id:
>"only valid in fragment shader input layout
> declaration.");
>} else if (pixel_interlock_ordered + pixel_interlock_unordered +
> sample_interlock_ordered + sample_interlock_unordered >
> 0 &&
> - !state->ARB_fragment_shader_interlock_enable) {
> + !state->ARB_fragment_shader_interlock_enable &&
> + !state->NV_fragment_shader_interlock_enable) {
> _mesa_glsl_error(& @1, state,
>"interlock layout qualifier present, but the "
> - "GL_ARB_fragment_shader_interlock extension is
> not "
> + "GL_ARB_fragment_shader_interlock or "
> + "GL_NV_fragment_shader_interlock extension is
> not "
>"enabled.");
>} else {
> $$.flags.q.pixel_interlock_ordered = pixel_interlock_ordered;
> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
> b/src/compiler/glsl/glsl_parser_extras.cpp
> index 6d92f24ea2..393942b62c 100644
> --- a/src/compiler/glsl/glsl_parser_extras.cpp
> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
> @@ -724,6 +724,7 @@ static const _mesa_glsl_extension
> _mesa_glsl_supported_extensions[] = {
> EXT_AEP(EXT_texture_cube_map_array),
> EXT(INTEL_conservative_rasterization),
> EXT(MESA_shader_integer_functions),
> + EXT(NV_fragment_shader_interlock),
> EXT(NV_image_formats),
> };
>
> diff --git a/src/compiler/glsl/glsl_parser_extras.h
> b/src/compiler/glsl/glsl_parser_extras.h
> index 59a173418b..3b17b54f0a 100644
> --- a/src/compiler/glsl/glsl_parser_extras.h
> +++ b/src/compiler/glsl/glsl_parser_extras.h
> @@ -810,6 +810,8 @@ struct _mesa_glsl_parse_state {
> bool INTEL_conservative_rasterization_warn;
> bool MESA_shader_integer_functions_enable;
> bool MESA_shader_integer_functions_warn;
> + bool NV_fragment_shader_interlock_enable;
> + bool NV_fragment_shader_interlock_warn;
> bool NV_image_formats_enable;
> bool NV_image_formats_warn;
> /*@}*/
> diff --git a/src/mesa/main/extensions_table.h
> b/src/mesa/main/extensions_table.h
> index af5ce118da..746e821886 100644
> --- a/src/mesa/main/extensions_table.h
> +++ b/src/mesa/main/extensions_table.h
> @@ -346,6 +346,7 @@ EXT(NV_draw_buffers ,
> dummy_true
> EXT(NV_fbo_color_attachments, dummy_true
>, x , x , x , ES2, 2010)
> EXT(NV_fill_rectangle , NV_fill_rectangle
> , GLL, GLC, x , x , 2015)
> EXT(NV_fog_distance , NV_fog_distance
> , GLL, x , x , x , 2001)
> +EXT(NV_fragment_shader_i
Re: [Mesa-dev] [PATCH] Add NV_fragment_shader_interlock support.
Sure thing!
-Pam
On Mon, Aug 20, 2018 at 3:25 PM, Kevin Rogovin
wrote:
> Hi Plamena,
>
> Thankyou for the fast review. Can you push it or have someone push it? (I
> do not have push rights)
>
> -Keviv
>
> On Mon, Aug 20, 2018 at 2:50 PM, Manolova, Plamena <
> plamena.manol...@intel.com> wrote:
>
>> Hi Kevin,
>> This looks good to me :)
>>
>> Reviewed-by: Plamena Manolova
>>
>> On Wed, Aug 15, 2018 at 2:29 PM, wrote:
>>
>>> From: Kevin Rogovin
>>>
>>> The main purpose for having NV_fragment_shader_interlock
>>> extension is because that extension is also for GLES31 while
>>> the ARB extension is for GL only.
>>> ---
>>> src/compiler/glsl/builtin_functions.cpp | 18 ++
>>> src/compiler/glsl/glsl_parser.yy | 6 --
>>> src/compiler/glsl/glsl_parser_extras.cpp | 1 +
>>> src/compiler/glsl/glsl_parser_extras.h | 2 ++
>>> src/mesa/main/extensions_table.h | 1 +
>>> 5 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/src/compiler/glsl/builtin_functions.cpp
>>> b/src/compiler/glsl/builtin_functions.cpp
>>> index 7119903795..e7b78c0158 100644
>>> --- a/src/compiler/glsl/builtin_functions.cpp
>>> +++ b/src/compiler/glsl/builtin_functions.cpp
>>> @@ -519,6 +519,12 @@ supports_arb_fragment_shader_interlock(const
>>> _mesa_glsl_parse_state *state)
>>> return state->ARB_fragment_shader_interlock_enable;
>>> }
>>>
>>> +static bool
>>> +supports_nv_fragment_shader_interlock(const _mesa_glsl_parse_state
>>> *state)
>>> +{
>>> + return state->NV_fragment_shader_interlock_enable;
>>> +}
>>> +
>>> static bool
>>> shader_clock(const _mesa_glsl_parse_state *state)
>>> {
>>> @@ -3331,6 +3337,18 @@ builtin_builder::create_builtins()
>>> supports_arb_fragment_shader_interlock),
>>> NULL);
>>>
>>> + add_function("beginInvocationInterlockNV",
>>> +_invocation_interlock(
>>> + "__intrinsic_begin_invocation_interlock",
>>> + supports_nv_fragment_shader_interlock),
>>> +NULL);
>>> +
>>> + add_function("endInvocationInterlockNV",
>>> +_invocation_interlock(
>>> + "__intrinsic_end_invocation_interlock",
>>> + supports_nv_fragment_shader_interlock),
>>> +NULL);
>>> +
>>> add_function("anyInvocationARB",
>>> _vote("__intrinsic_vote_any", vote),
>>> NULL);
>>> diff --git a/src/compiler/glsl/glsl_parser.yy
>>> b/src/compiler/glsl/glsl_parser.yy
>>> index cb7376995d..bc2571b684 100644
>>> --- a/src/compiler/glsl/glsl_parser.yy
>>> +++ b/src/compiler/glsl/glsl_parser.yy
>>> @@ -1450,10 +1450,12 @@ layout_qualifier_id:
>>>"only valid in fragment shader input layout
>>> declaration.");
>>>} else if (pixel_interlock_ordered + pixel_interlock_unordered +
>>> sample_interlock_ordered + sample_interlock_unordered
>>> > 0 &&
>>> - !state->ARB_fragment_shader_interlock_enable) {
>>> + !state->ARB_fragment_shader_interlock_enable &&
>>> + !state->NV_fragment_shader_interlock_enable) {
>>> _mesa_glsl_error(& @1, state,
>>>"interlock layout qualifier present, but the "
>>> - "GL_ARB_fragment_shader_interlock extension
>>> is not "
>>> + "GL_ARB_fragment_shader_interlock or "
>>> + "GL_NV_fragment_shader_interlock extension
>>> is not "
>>>"enabled.");
>>>} else {
>>> $$.flags.q.pixel_interlock_ordered = pixel_interlock_ordered;
>>> diff --git a/src/compiler/glsl/glsl_parser_extras.cpp
>>> b/src/compiler/glsl/glsl_parser_extras.cpp
>>> index 6d92f24ea2..393942b62c 100644
>>> --- a/src/compiler/glsl/glsl_parser_extras.cpp
>>> +++ b/src/compiler/glsl/glsl_parser_extras.cpp
>>> @@ -724,6 +724,7 @@ static const _mesa_glsl_extension
>>> _mesa_gls
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi Kevin, Could you also mark the extension as done in docs/features.txt? Otherwise the series looks good to me and with that tiny change it's Reviewed-by: Plamena Manolova < plamena.manol...@intel.com> Thanks, Pam On Mon, Aug 27, 2018 at 9:56 AM wrote: > From: Kevin Rogovin > > INTEL_fragment_shader_ordering provides the ability for shaders > to issue a call to gaurnantee memory write operation ordering > of overlapping pixels or samples. In contrast to > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering > instead of defining a critical region (which must be in main() and > under no flow control) provides a single function that acts like > a memory barrier that can be called under any control flow. > > Kevin Rogovin (2): > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. > i965: Add INTEL_fragment_shader_ordering support. > > docs/relnotes/18.3.0.html| 1 + > src/compiler/glsl/builtin_functions.cpp | 17 + > src/compiler/glsl/glsl_parser_extras.cpp | 1 + > src/compiler/glsl/glsl_parser_extras.h | 2 ++ > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ > src/compiler/glsl/ir.h | 1 + > src/compiler/nir/nir_intrinsics.py | 1 + > src/intel/compiler/brw_fs_nir.cpp| 1 + > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + > src/mesa/main/extensions_table.h | 1 + > src/mesa/main/mtypes.h | 1 + > 11 files changed, 33 insertions(+) > > -- > 2.17.1 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Actually ignore my comment, I forgot that docs/features.txt is for non-vendor specific features only. I'll go ahead and push your patches. Sorry for the confusion. Thanks, Pam On Tue, Aug 28, 2018 at 1:04 PM Manolova, Plamena < plamena.manol...@intel.com> wrote: > Hi Kevin, > Could you also mark the extension as done in docs/features.txt? Otherwise > the series looks good to me > and with that tiny change it's Reviewed-by: Plamena Manolova < > plamena.manol...@intel.com> > > Thanks, > Pam > > On Mon, Aug 27, 2018 at 9:56 AM wrote: > >> From: Kevin Rogovin >> >> INTEL_fragment_shader_ordering provides the ability for shaders >> to issue a call to gaurnantee memory write operation ordering >> of overlapping pixels or samples. In contrast to >> ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering >> instead of defining a critical region (which must be in main() and >> under no flow control) provides a single function that acts like >> a memory barrier that can be called under any control flow. >> >> Kevin Rogovin (2): >> mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. >> i965: Add INTEL_fragment_shader_ordering support. >> >> docs/relnotes/18.3.0.html| 1 + >> src/compiler/glsl/builtin_functions.cpp | 17 + >> src/compiler/glsl/glsl_parser_extras.cpp | 1 + >> src/compiler/glsl/glsl_parser_extras.h | 2 ++ >> src/compiler/glsl/glsl_to_nir.cpp| 6 ++ >> src/compiler/glsl/ir.h | 1 + >> src/compiler/nir/nir_intrinsics.py | 1 + >> src/intel/compiler/brw_fs_nir.cpp| 1 + >> src/mesa/drivers/dri/i965/intel_extensions.c | 1 + >> src/mesa/main/extensions_table.h | 1 + >> src/mesa/main/mtypes.h | 1 + >> 11 files changed, 33 insertions(+) >> >> -- >> 2.17.1 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/2] Implement INTEL_fragment_shader_ordering
Hi Mark, AFAIK there is no piglit test for this specific extension. However underneath the hood it reuses the functionality of ARB_fragment_shader_interlock, which has a test. I believe the only major difference between the two extensions is that unlike beginInvocationInterlockARB, beginFragmentShaderOrderingINTEL can be called from functions other than main(). If necessary it would be pretty straightforward to reuse most of the code for the ARB_fragment_shader_interlock test to create one for INTEL_fragment_shader_ordering. Thank you, Pam On Tue, Aug 28, 2018 at 6:41 PM Mark Janes wrote: > Is there a piglit test that verifies that this feature works properly? > > writes: > > > From: Kevin Rogovin > > > > INTEL_fragment_shader_ordering provides the ability for shaders > > to issue a call to gaurnantee memory write operation ordering > > of overlapping pixels or samples. In contrast to > > ARB_fragment_shader_interlock, INTEL_fragment_shader_ordering > > instead of defining a critical region (which must be in main() and > > under no flow control) provides a single function that acts like > > a memory barrier that can be called under any control flow. > > > > Kevin Rogovin (2): > > mesa: Add GL/GLSL plumbing for INTEL_fragment_shader_ordering. > > i965: Add INTEL_fragment_shader_ordering support. > > > > docs/relnotes/18.3.0.html| 1 + > > src/compiler/glsl/builtin_functions.cpp | 17 + > > src/compiler/glsl/glsl_parser_extras.cpp | 1 + > > src/compiler/glsl/glsl_parser_extras.h | 2 ++ > > src/compiler/glsl/glsl_to_nir.cpp| 6 ++ > > src/compiler/glsl/ir.h | 1 + > > src/compiler/nir/nir_intrinsics.py | 1 + > > src/intel/compiler/brw_fs_nir.cpp| 1 + > > src/mesa/drivers/dri/i965/intel_extensions.c | 1 + > > src/mesa/main/extensions_table.h | 1 + > > src/mesa/main/mtypes.h | 1 + > > 11 files changed, 33 insertions(+) > > > > -- > > 2.17.1 > > > > ___ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 15/15] mesa: Expose EXT_shader_framebuffer_fetch(_non_coherent) on desktop and embedded GL.
The series is: Reviewed-by: Plamena Manolova On Wed, Feb 14, 2018 at 9:18 PM, Francisco Jerez wrote: > --- > docs/relnotes/18.1.0.html| 2 ++ > src/mesa/main/extensions_table.h | 3 ++- > 2 files changed, 4 insertions(+), 1 deletion(-) > > diff --git a/docs/relnotes/18.1.0.html b/docs/relnotes/18.1.0.html > index b8a0cd0d02c..f5564e9b8fe 100644 > --- a/docs/relnotes/18.1.0.html > +++ b/docs/relnotes/18.1.0.html > @@ -46,6 +46,8 @@ Note: some of the new features are only available with > certain drivers. > > GL_EXT_semaphore on radeonsi > GL_EXT_semaphore_fd on radeonsi > +GL_EXT_shader_framebuffer_fetch on i965 on desktop GL (GLES was > already supported) > +GL_EXT_shader_framebuffer_fetch_non_coherent on i965 > > > Bug fixes > diff --git a/src/mesa/main/extensions_table.h > b/src/mesa/main/extensions_table.h > index 7de1a6cd5dc..5d51bc1e4ec 100644 > --- a/src/mesa/main/extensions_table.h > +++ b/src/mesa/main/extensions_table.h > @@ -251,7 +251,8 @@ EXT(EXT_semaphore , > EXT_semaphore > EXT(EXT_semaphore_fd, EXT_semaphore_fd >, GLL, GLC, x , ES2, 2017) > EXT(EXT_separate_shader_objects , dummy_true > , x , x , x , ES2, 2013) > EXT(EXT_separate_specular_color , dummy_true > , GLL, x , x , x , 1997) > -EXT(EXT_shader_framebuffer_fetch, > EXT_shader_framebuffer_fetch , x , x , x , ES2, 2013) > +EXT(EXT_shader_framebuffer_fetch, > EXT_shader_framebuffer_fetch , GLL, GLC, x , ES2, 2013) > +EXT(EXT_shader_framebuffer_fetch_non_coherent, > EXT_shader_framebuffer_fetch_non_coherent, GLL, GLC, x, ES2, 2018) > EXT(EXT_shader_integer_mix , EXT_shader_integer_mix >, GLL, GLC, x , 30, 2013) > EXT(EXT_shader_io_blocks, dummy_true >, x , x , x , 31, 2014) > EXT(EXT_shader_samples_identical, > EXT_shader_samples_identical , GLL, GLC, x , 31, 2015) > -- > 2.16.1 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 4/4] gallium/util: remove unused header from u_queue.c
The whole series looks good to me. Reviewed-by: Plamena Manolova On Mon, Mar 6, 2017 at 2:58 AM, Timothy Arceri wrote: > --- > src/gallium/auxiliary/util/u_queue.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/src/gallium/auxiliary/util/u_queue.c > b/src/gallium/auxiliary/util/u_queue.c > index 9565c53..bb63a6b 100644 > --- a/src/gallium/auxiliary/util/u_queue.c > +++ b/src/gallium/auxiliary/util/u_queue.c > @@ -20,21 +20,20 @@ > * USE OR OTHER DEALINGS IN THE SOFTWARE. > * > * The above copyright notice and this permission notice (including the > * next paragraph) shall be included in all copies or substantial portions > * of the Software. > */ > > #include "u_queue.h" > #include "u_memory.h" > #include "u_string.h" > -#include "os/os_time.h" > > static void util_queue_killall_and_wait(struct util_queue *queue); > > /*** > * > * Wait for all queues to assert idle when exit() is called. > * > * Otherwise, C++ static variable destructors can be called while threads > * are using the static variables. > */ > > -- > 2.9.3 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv: Add a helper for working with VK_WHOLE_SIZE for buffers
Reviewed-by: Plamena Manolova
On Sat, Mar 4, 2017 at 8:47 PM, Jason Ekstrand wrote:
> ---
> src/intel/vulkan/anv_blorp.c | 16 +++-
> src/intel/vulkan/anv_descriptor_set.c | 7 +++
> src/intel/vulkan/anv_image.c | 4 ++--
> src/intel/vulkan/anv_private.h| 12
> 4 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/src/intel/vulkan/anv_blorp.c b/src/intel/vulkan/anv_blorp.c
> index d79c5e0..05790d2 100644
> --- a/src/intel/vulkan/anv_blorp.c
> +++ b/src/intel/vulkan/anv_blorp.c
> @@ -722,11 +722,17 @@ void anv_CmdFillBuffer(
> struct blorp_batch batch;
> blorp_batch_init(&cmd_buffer->device->blorp, &batch, cmd_buffer, 0);
>
> - if (fillSize == VK_WHOLE_SIZE) {
> - fillSize = dst_buffer->size - dstOffset;
> - /* Make sure fillSize is a multiple of 4 */
> - fillSize &= ~3ull;
> - }
> + fillSize = anv_buffer_get_range(dst_buffer, dstOffset, fillSize);
> +
> + /* From the Vulkan spec:
> +*
> +*"size is the number of bytes to fill, and must be either a
> multiple
> +*of 4, or VK_WHOLE_SIZE to fill the range from offset to the end
> of
> +*the buffer. If VK_WHOLE_SIZE is used and the remaining size of
> the
> +*buffer is not a multiple of 4, then the nearest smaller multiple
> is
> +*used."
> +*/
> + fillSize &= ~3ull;
>
> /* First, we compute the biggest format that can be used with the
> * given offsets and size.
> diff --git a/src/intel/vulkan/anv_descriptor_set.c b/src/intel/vulkan/anv_
> descriptor_set.c
> index 1e8991b..2a37d7d 100644
> --- a/src/intel/vulkan/anv_descriptor_set.c
> +++ b/src/intel/vulkan/anv_descriptor_set.c
> @@ -672,10 +672,9 @@ anv_descriptor_set_write_buffer(struct
> anv_descriptor_set *set,
> /* For buffers with dynamic offsets, we use the full possible range in
> the
> * surface state and do the actual range-checking in the shader.
> */
> - if (bind_layout->dynamic_offset_index >= 0 || range == VK_WHOLE_SIZE)
> - bview->range = buffer->size - offset;
> - else
> - bview->range = range;
> + if (bind_layout->dynamic_offset_index >= 0)
> + range = VK_WHOLE_SIZE;
> + bview->range = anv_buffer_get_range(buffer, offset, range);
>
> /* If we're writing descriptors through a push command, we need to
> allocate
> * the surface state from the command buffer. Otherwise it will be
> diff --git a/src/intel/vulkan/anv_image.c b/src/intel/vulkan/anv_image.c
> index 59f730c..f54c8ea 100644
> --- a/src/intel/vulkan/anv_image.c
> +++ b/src/intel/vulkan/anv_image.c
> @@ -837,8 +837,8 @@ anv_CreateBufferView(VkDevice _device,
> const uint32_t format_bs = isl_format_get_layout(view->format)->bpb /
> 8;
> view->bo = buffer->bo;
> view->offset = buffer->offset + pCreateInfo->offset;
> - view->range = pCreateInfo->range == VK_WHOLE_SIZE ?
> - buffer->size - pCreateInfo->offset : pCreateInfo->range;
> + view->range = anv_buffer_get_range(buffer, pCreateInfo->offset,
> + pCreateInfo->range);
> view->range = align_down_npot_u32(view->range, format_bs);
>
> if (buffer->usage & VK_BUFFER_USAGE_UNIFORM_TEXEL_BUFFER_BIT) {
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index c73196a..cf9874e 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1090,6 +1090,18 @@ struct anv_buffer {
> VkDeviceSize offset;
> };
>
> +static inline uint64_t
> +anv_buffer_get_range(struct anv_buffer *buffer, uint64_t offset, uint64_t
> range)
> +{
> + assert(offset <= buffer->size);
> + if (range == VK_WHOLE_SIZE) {
> + return buffer->size - offset;
> + } else {
> + assert(range <= buffer->size);
> + return range;
> + }
> +}
> +
> enum anv_cmd_dirty_bits {
> ANV_CMD_DIRTY_DYNAMIC_VIEWPORT = 1 << 0, /*
> VK_DYNAMIC_STATE_VIEWPORT */
> ANV_CMD_DIRTY_DYNAMIC_SCISSOR = 1 << 1, /*
> VK_DYNAMIC_STATE_SCISSOR */
> --
> 2.5.0.400.gff86faf
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] anv: change BLOCK_POOL_MEMFD_SIZE to exactly 2GB
Looks good to me :)
Reviewed-by: Plamena Manolova
On Tue, Mar 7, 2017 at 11:17 AM, Tapani Pälli
wrote:
> This is what comment above definition says and change fixes issue with
> 32bit build where BLOCK_POOL_MEMFD_SIZE is used as ftruncate parameter
> and constant currently gets converted from 4294967296 to 0.
>
> Signed-off-by: Tapani Pälli
> ---
> src/intel/vulkan/anv_private.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_
> private.h
> index c73196a..86b20c4 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -386,7 +386,7 @@ struct anv_block_pool {
> };
>
> /* Block pools are backed by a fixed-size 2GB memfd */
> -#define BLOCK_POOL_MEMFD_SIZE (1ull << 32)
> +#define BLOCK_POOL_MEMFD_SIZE (1ul << 31)
>
> /* The center of the block pool is also the middle of the memfd. This may
> * change in the future if we decide differently for some reason.
> --
> 2.9.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nir: Rewrite nir_type_conversion_op
Looks good to me :)
Reviewed-by: Plamena Manolova
On Tue, Mar 14, 2017 at 4:34 AM, Jason Ekstrand
wrote:
> The original version was very convoluted and tried way too hard to not
> just have the nested switch statement that it needs. Let's just write
> the obvious code and then we know it's correct. This fixes a bunch of
> missing cases particularly with int64.
>
> ---
> This patch has to be inserted before the earlier patch I wrote to
> glsl_to_nir which makes it use this function. The function was
> sufficiently
> broken that making glsl_to_nir use it regresses a bunch of stuff.
>
> src/compiler/nir/nir.c | 155 +-
> ---
> 1 file changed, 92 insertions(+), 63 deletions(-)
>
> diff --git a/src/compiler/nir/nir.c b/src/compiler/nir/nir.c
> index a9fac96..37fd9cb 100644
> --- a/src/compiler/nir/nir.c
> +++ b/src/compiler/nir/nir.c
> @@ -1967,87 +1967,116 @@ nir_type_conversion_op(nir_alu_type src,
> nir_alu_type dst)
> unsigned src_bitsize = nir_alu_type_get_type_size(src);
> unsigned dst_bitsize = nir_alu_type_get_type_size(dst);
>
> - if (src_base_type == dst_base_type) {
> - if (src_bitsize == dst_bitsize)
> - return (src_base_type == nir_type_float) ? nir_op_fmov :
> nir_op_imov;
> -
> - assert(src_bitsize == 64 || dst_bitsize == 64);
> - if (src_base_type == nir_type_float)
> - /* TODO: implement support for float16 */
> - return (src_bitsize == 64) ? nir_op_d2f : nir_op_f2d;
> - else if (src_base_type == nir_type_uint)
> - return (src_bitsize == 64) ? nir_op_imov : nir_op_u2u64;
> - else if (src_base_type == nir_type_int)
> - return (src_bitsize == 64) ? nir_op_imov : nir_op_i2i64;
> - unreachable("Invalid conversion");
> - }
> -
> - /* Different base type but same bit_size */
> if (src_bitsize == dst_bitsize) {
> - /* TODO: This does not include specific conversions between
> - * signed or unsigned integer types of bit size different than 32
> yet.
> - */
> - assert(src_bitsize == 32);
>switch (src_base_type) {
> - case nir_type_uint:
> - return (dst_base_type == nir_type_float) ? nir_op_u2f :
> nir_op_imov;
>case nir_type_int:
> - return (dst_base_type == nir_type_float) ? nir_op_i2f :
> nir_op_imov;
> - case nir_type_bool:
> - return (dst_base_type == nir_type_float) ? nir_op_b2f :
> nir_op_b2i;
> + case nir_type_uint:
> + if (dst_base_type == nir_type_uint || dst_base_type ==
> nir_type_int)
> +return nir_op_imov;
> + break;
>case nir_type_float:
> - switch (dst_base_type) {
> - case nir_type_uint:
> -return nir_op_f2u;
> - case nir_type_bool:
> -return nir_op_f2b;
> - default:
> -return nir_op_f2i;
> - };
> + if (dst_base_type == nir_type_float)
> +return nir_op_fmov;
> + break;
> + case nir_type_bool:
> + if (dst_base_type == nir_type_bool)
> +return nir_op_imov;
> + break;
>default:
> unreachable("Invalid conversion");
> - };
> + }
> }
>
> - /* Different bit_size and different base type */
> - /* TODO: Implement integer support for types with bit_size != 32 */
> switch (src_base_type) {
> - case nir_type_uint:
> - if (dst == nir_type_float64)
> - return nir_op_u2d;
> - else if (dst == nir_type_int64)
> - return nir_op_u2i64;
> - break;
> case nir_type_int:
> - if (dst == nir_type_float64)
> - return nir_op_i2d;
> - else if (dst == nir_type_uint64)
> - return nir_op_i2i64;
> - break;
> - case nir_type_bool:
> - assert(dst == nir_type_float64);
> - return nir_op_u2d;
> - case nir_type_float:
> - assert(src_bitsize == 32 || src_bitsize == 64);
> - if (src_bitsize != 64) {
> - assert(dst == nir_type_float64);
> - return nir_op_f2d;
> + switch (dst_base_type) {
> + case nir_type_int:
> + assert(src_bitsize != dst_bitsize);
> + return (dst_bitsize == 32) ? nir_op_i2i32 : nir_op_i2i64;
> + case nir_type_uint:
> + assert(src_bitsize != dst_bitsize);
> + return (dst_bitsize == 32) ? nir_op_i2u32 : nir_op_i2u64;
> + case nir_type_float:
> + switch (src_bitsize) {
> + case 32:
> +return (dst_bitsize == 32) ? nir_op_i2f : nir_op_i2d;
> + case 64:
> +return (dst_bitsize == 32) ? nir_op_i642f : nir_op_i642d;
> + default:
> +unreachable("Invalid conversion");
> + }
> + case nir_type_bool:
> + return (src_bitsize == 32) ? nir_op_i2b : nir_op_i642b;
> + default:
> + unreachable("Invalid conversion");
>}
> - assert(dst_bitsize == 32);
> +
> + case nir_type_uint:
>switch (dst_base_type) {
> + case nir_type_i
Re: [Mesa-dev] [PATCH] android: intel/compiler: fix include paths
Looks good to me. Reviewed-by: Plamena Manolova On Tue, Mar 14, 2017 at 2:00 AM, Mauro Rossi wrote: > Fixes the following building error: > > out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libmesa_ > intel_compiler_intermediates/compiler/brw_nir_trig_workarounds.c:1:10: > fatal error: 'brw_nir.h' file not found > ^ > 1 error generated. > > Fixes: 700bebb "i965: Move the back-end compiler to src/intel/compiler" > --- > src/intel/Android.compiler.mk | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/intel/Android.compiler.mk b/src/intel/Android.compiler.mk > index 8bbfb1c..48a19a0 100644 > --- a/src/intel/Android.compiler.mk > +++ b/src/intel/Android.compiler.mk > @@ -43,7 +43,8 @@ LOCAL_C_INCLUDES := \ > $(MESA_TOP)/src/gallium/include \ > $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_glsl,,)/glsl > \ > $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir > \ > - $(MESA_TOP)/src/compiler/nir > + $(MESA_TOP)/src/compiler/nir \ > + $(MESA_TOP)/src/intel/compiler > > LOCAL_SHARED_LIBRARIES := \ > libdrm_intel > -- > 2.10.2 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] android: intel/compiler: fix include paths
Sorry Mauro seems like an identical patch has been approved and pushed a couple of hours ago. Thank you, Plamena On Tue, Mar 14, 2017 at 11:42 AM, Manolova, Plamena < plamena.manol...@intel.com> wrote: > Looks good to me. > Reviewed-by: Plamena Manolova > > On Tue, Mar 14, 2017 at 2:00 AM, Mauro Rossi > wrote: > >> Fixes the following building error: >> >> out/target/product/x86_64/obj_x86/STATIC_LIBRARIES/libmesa_i >> ntel_compiler_intermediates/compiler/brw_nir_trig_workarounds.c:1:10: >> fatal error: 'brw_nir.h' file not found >> ^ >> 1 error generated. >> >> Fixes: 700bebb "i965: Move the back-end compiler to src/intel/compiler" >> --- >> src/intel/Android.compiler.mk | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/src/intel/Android.compiler.mk b/src/intel/Android.compiler.m >> k >> index 8bbfb1c..48a19a0 100644 >> --- a/src/intel/Android.compiler.mk >> +++ b/src/intel/Android.compiler.mk >> @@ -43,7 +43,8 @@ LOCAL_C_INCLUDES := \ >> $(MESA_TOP)/src/gallium/include \ >> $(call generated-sources-dir-for,STAT >> IC_LIBRARIES,libmesa_glsl,,)/glsl \ >> $(call generated-sources-dir-for,STATIC_LIBRARIES,libmesa_nir,,)/nir >> \ >> - $(MESA_TOP)/src/compiler/nir >> + $(MESA_TOP)/src/compiler/nir \ >> + $(MESA_TOP)/src/intel/compiler >> >> LOCAL_SHARED_LIBRARIES := \ >> libdrm_intel >> -- >> 2.10.2 >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH mesa] docs/specs: update Khronos registries URLs
lgtm. Reviewed-by: Plamena Manolova On Thu, Mar 16, 2017 at 12:02 PM, Eric Engestrom wrote: > The registries were migrated to git and are now hosted on GitHub. > The old svn is now read-only, and will not be updated anymore. > > Signed-off-by: Eric Engestrom > --- > docs/specs/enums.txt | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/docs/specs/enums.txt b/docs/specs/enums.txt > index b69f1e02c9..222adbc72e 100644 > --- a/docs/specs/enums.txt > +++ b/docs/specs/enums.txt > @@ -1,10 +1,10 @@ > The definitive source for enum values and reserved ranges are the XML > files in > the Khronos registry: > > -https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/ > public/api/egl.xml > -https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/ > public/api/gl.xml > -https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/ > public/api/glx.xml > -https://cvs.khronos.org/svn/repos/ogl/trunk/doc/registry/ > public/api/wgl.xml > +https://github.com/KhronosGroup/EGL-Registry/blob/master/api/egl.xml > +https://github.com/KhronosGroup/OpenGL-Registry/ > blob/master/xml/gl.xml > +https://github.com/KhronosGroup/OpenGL-Registry/ > blob/master/xml/glx.xml > +https://github.com/KhronosGroup/OpenGL-Registry/ > blob/master/xml/wgl.xml > > GL blocks allocated to Mesa: > 0x8750-0x875F > -- > Cheers, > Eric > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] android: add marshal_generated c and h files to generated sources
Reviewed-by: Plamena Manolova
On Thu, Mar 16, 2017 at 11:24 AM, Tapani Pälli
wrote:
> Fixes: efd63e2 ("mesa: Connect the generated GL command marshalling code
> to the build.")
> Signed-off-by: Tapani Pälli
> ---
> src/mesa/Android.gen.mk | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/src/mesa/Android.gen.mk b/src/mesa/Android.gen.mk
> index a1e58a6..42d4ba1 100644
> --- a/src/mesa/Android.gen.mk
> +++ b/src/mesa/Android.gen.mk
> @@ -38,7 +38,9 @@ sources := \
> main/format_unpack.c \
> main/format_info.h \
> main/remap_helper.h \
> - main/get_hash.h
> + main/get_hash.h \
> + main/marshal_generated.c \
> + main/marshal_generated.h
>
> LOCAL_SRC_FILES := $(filter-out $(sources), $(LOCAL_SRC_FILES))
>
> --
> 2.9.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] util/build-id: check dlpi_name before strstr call
Reviewed-by: Plamena Manolova
On Thu, Mar 16, 2017 at 8:10 AM, Tapani Pälli
wrote:
> According to dl_iterate_phdr man page first object visited is the
> main program where dlpi_name is an empty string. This fixes segfault
> on Android when using build-id as identifier.
>
> Signed-off-by: Tapani Pälli
> ---
> src/util/build_id.c | 6 ++
> 1 file changed, 6 insertions(+)
>
> diff --git a/src/util/build_id.c b/src/util/build_id.c
> index c53e71d..797ea22 100644
> --- a/src/util/build_id.c
> +++ b/src/util/build_id.c
> @@ -55,6 +55,12 @@ build_id_find_nhdr_callback(struct dl_phdr_info *info,
> size_t size, void *data_)
> {
> struct callback_data *data = data_;
>
> + /* The first object visited by callback is the main program.
> +* For the main program, the dlpi_name field will be an empty string.
> +*/
> + if (info->dlpi_name == NULL)
> + return 0;
> +
> char *ptr = strstr(info->dlpi_name, data->filename);
> if (ptr == NULL || ptr[strlen(data->filename)] != '\0')
>return 0;
> --
> 2.9.3
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965: Implement ARB_indirect_parameters
Thank you for taking a look Jason! Sorry my bad, I'll make sure to add some comments to v2 at least. Hopefully it'll make editing this portion of the i965 drawing logic a bit easier. On Fri, Sep 8, 2017 at 4:48 AM, Jason Ekstrand wrote: > Hey Pam! > > A quick skim through this looks fairly good. I'll be honest, I expected > to have some quick comments on the v1 but I'm not seeing anything > immediately. That said, I've got to think about it fairly hard to convince > myself that it's actually correct. The i965 drawing logic isn't really my > area. > > On Thu, Aug 31, 2017 at 11:26 AM, Ilia Mirkin > wrote: > >> On Thu, Aug 31, 2017 at 1:45 PM, Bas Nieuwenhuizen >> wrote: >> > >> > >> > On Wed, Aug 30, 2017, at 14:59, Ilia Mirkin wrote: >> >> On Wed, Aug 30, 2017 at 3:17 AM, Bas Nieuwenhuizen >> >> wrote: >> >> > So as a random drive-by review, I think the risk in this >> implementation >> >> > is that apps just set maxdrawcount to some high value . If I'm >> reading >> >> > the spec correctly there is no real bound on the value except for the >> >> > max-representable value for the integer. Also AFAIK the AMD and >> NVidia >> >> > implementation don't really get slower if you specify maxdrawcount >> very >> >> >> >> Can't speak for AMD, but this is actually extremely similar (in >> >> principle) to how this is handled on NVIDIA. It's a little cleaner >> >> since there's a nice macro, but since macros can't have a variable >> >> number of arguments, you have to feed it maxdrawcount args. Admittedly >> >> this is the nouveau impl, but I'm almost certain that the blob does it >> >> in a similar way. >> > >> > I thought, you had a macro that looped, but seems that was a >> > misunderstanding on my side. If nvidia does it that way, then I suppose >> > there is little risk of games expecting something more efficient. Never >> > mind my comments then. >> >> There indeed is a macro that loops. However that macro takes a number >> of parameters determined at the time of cmd stream generation. Also >> the indirect draw arguments have to be added into the cmd stream (via >> IB references). Here's how nouveau does it: >> >> https://cgit.freedesktop.org/mesa/mesa/tree/src/gallium/driv >> ers/nouveau/nvc0/nvc0_vbo.c#n798 >> >> I don't remember if I looked precisely at what NVIDIA does, but I >> definitely can't think of another way. Even if you use the command >> which takes the argument length as a separate word, that won't be >> right since it's count * N arguments. macros can't read arbitrary >> things either, it has to come out of the cmdstream. >> > > I'll agree that this solution is suboptimal. However, it's really the > best we can do without getting crazy. A "real" implementation would > require a self-modifying command buffer which is a sketchy idea in the best > of circumstances. :/ > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] nir: Handle variables dependent on the local work group size.
Hi Jason,
Thank you so much for reviewing! In my initial series for
ARB_compute_variable_group_size
(https://patchwork.freedesktop.org/patch/228130) from which this is
extracted, I moved
lowering these variables to brw_nir_lower_cs_intrinsics and did what you're
suggesting i.e.
I used the load_local_group_size intrinsic. My reasoning was that this
might give other drivers
some flexibility on how they can handle this, but if it makes more sense to
keep this in
nir_lower_system_values I'm happy to rework my patch.
Thank you,
Pam
On Mon, Nov 12, 2018 at 5:07 AM Jason Ekstrand wrote:
> On Sun, Nov 11, 2018 at 8:46 PM Karol Herbst wrote:
>
>> an)
>> On Mon, Nov 12, 2018 at 12:37 AM Jason Ekstrand
>> wrote:
>> >
>> > On November 11, 2018 16:36:16 Karol Herbst wrote:
>> >
>> > > On Sun, Nov 11, 2018 at 10:48 PM Jason Ekstrand
>> wrote:
>> > >>
>> > >> On Sun, Nov 11, 2018 at 3:35 PM Plamena Manolova
>> > >> wrote:
>> > >>>
>> > >>>
>> > >>> Lowering shader variables which depend on the local work group
>> > >>> size being available in nir_lower_system_values is only possible
>> > >>> if the local work group size isn't variable, otherwise this should
>> > >>> be handled by the native driver (if it supports
>> > >>> ARB_compute_variable_group_size).
>> > >>>
>> > >>>
>> > >>> Signed-off-by: Plamena Manolova
>> > >>> ---
>> > >>> src/compiler/nir/nir_lower_system_values.c | 22
>> ++
>> > >>> 1 file changed, 22 insertions(+)
>> > >>>
>> > >>>
>> > >>> diff --git a/src/compiler/nir/nir_lower_system_values.c
>> > >>> b/src/compiler/nir/nir_lower_system_values.c
>> > >>> index bde7eb1180..6fdf9f9c51 100644
>> > >>> --- a/src/compiler/nir/nir_lower_system_values.c
>> > >>> +++ b/src/compiler/nir/nir_lower_system_values.c
>> > >>> @@ -77,6 +77,15 @@ convert_block(nir_block *block, nir_builder *b)
>> > >>>*"The value of gl_GlobalInvocationID is equal to
>> > >>>*gl_WorkGroupID * gl_WorkGroupSize + gl_LocalInvocationID"
>> > >>>*/
>> > >>> +
>> > >>> + /*
>> > >>> + * If the local work group size is variable we can't
>> lower the global
>> > >>> + * invocation id here.
>> > >>
>> > >>
>> > >> Why not?
>> > >>
>> > >>>
>> > >>>
>> > >>> + */
>> > >>> + if (b->shader->info.cs.local_size_variable) {
>> > >>
>> > >>
>> > >> I didn't realize we'd added a bit for this. At one point in time,
>> Karol
>> > >> had patches which had it keyed off of the size being zero. Having a
>> > >> separate bit is probably fine, it just surpised me.
>> > >
>> > >
>> > > yeah.. I guess I choose that, because I had nothing better. But I
>> > > guess having the size being 0 is good enough as long as we sure it is
>> > > 0 in relevant cases.
>> >
>> > It's fine. I just thought we'd chosen a different convention bit there's
>> > nothing wrong with it. I was just surprised. That's all.
>> >
>> > >
>> > >>>
>> > >>>
>> > >>> +break;
>> > >>> + }
>> > >>> +
>> > >>> nir_ssa_def *group_size = build_local_group_size(b);
>> > >>> nir_ssa_def *group_id = nir_load_work_group_id(b);
>> > >>> nir_ssa_def *local_id = nir_load_local_invocation_id(b);
>> > >>> @@ -115,6 +124,11 @@ convert_block(nir_block *block, nir_builder *b)
>> > >>> }
>> > >>>
>> > >>>
>> > >>> case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
>> > >>> + /* If the local work group size is variable we can't
>> lower it here */
>> > >>> + if (b->shader->info.cs.local_size_variable) {
>> > >>> +break;
>> > >>> + }
>> > >>> +
>> > >>> sysval = build_local_group_size(b);
>> > >>> break;
>> > >>> }
>> > >>> @@ -189,6 +203,14 @@ convert_block(nir_block *block, nir_builder *b)
>> > >>> break;
>> > >>>
>> > >>>
>> > >>> case SYSTEM_VALUE_GLOBAL_GROUP_SIZE: {
>> > >>> + /*
>> > >>> + * If the local work group size is variable we can't
>> lower the global
>> > >>> + * group size here.
>> > >>> + */
>> > >>> + if (b->shader->info.cs.local_size_variable) {
>> > >>> +break;
>> > >>> + }
>> > >>
>> > >>
>> > >> Why can't we lower the global size? It seems like we would want the
>> below
>> > >> multiplication regardless of whether the local size is constant.
>> > >
>> > > well I am not sure about ARB_compute_variable_group_size, but at least
>> > > in CL you know nothing about it at compile time as you specify
>> > > everything when you enqueue the kernel. Could be that the number of
>> > > work_groups is fixed with ARB_compute_variable_group_size though
>> >
>> > Why can't you multiply two non-constant things together? Maybe the
>> driver
>> > can do something now efficient but it's not clear to me that this isn't
>> > what we want.
>> >
>>
>> oh, you meant it that way. Mhh, I actually wrote the patch adding
>> build_local_group_size to handle that inside that function (lower to
>> constants if known, intrinsics otherwise) as we need the local_size
>> for three different system values. I have a
Re: [Mesa-dev] [PATCH] intel, nir: Move gl_LocalInvocationID lowering to nir_lower_system_values
Looks good to me :)
Reviewed-by: Plamena Manolova
On Fri, Nov 16, 2018 at 7:02 AM Jason Ekstrand wrote:
> It's not at all intel-specific; the formula is dictated by OpenGL and
> Vulkan. The only intel-specific thing is that we need the lowering. As
> a nice side-effect, the new version is variable-group-size ready.
>
> Cc: Plamena Manolova
> ---
> src/compiler/nir/nir.h| 1 +
> src/compiler/nir/nir_lower_system_values.c| 49 ++-
> src/intel/compiler/brw_compiler.c | 1 +
> .../compiler/brw_nir_lower_cs_intrinsics.c| 33 -
> 4 files changed, 50 insertions(+), 34 deletions(-)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index b0cff50eaf2..1dd605010f6 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -2178,6 +2178,7 @@ typedef struct nir_shader_compiler_options {
> bool lower_helper_invocation;
>
> bool lower_cs_local_index_from_id;
> + bool lower_cs_local_id_from_index;
>
> bool lower_device_index_to_zero;
>
> diff --git a/src/compiler/nir/nir_lower_system_values.c
> b/src/compiler/nir/nir_lower_system_values.c
> index fbc40573579..08a9e8be44a 100644
> --- a/src/compiler/nir/nir_lower_system_values.c
> +++ b/src/compiler/nir/nir_lower_system_values.c
> @@ -51,6 +51,45 @@ build_local_group_size(nir_builder *b)
> return local_size;
> }
>
> +static nir_ssa_def *
> +build_local_invocation_id(nir_builder *b)
> +{
> + if (b->shader->options->lower_cs_local_id_from_index) {
> + /* We lower gl_LocalInvocationID from gl_LocalInvocationIndex based
> + * on this formula:
> + *
> + *gl_LocalInvocationID.x =
> + * gl_LocalInvocationIndex % gl_WorkGroupSize.x;
> + *gl_LocalInvocationID.y =
> + * (gl_LocalInvocationIndex / gl_WorkGroupSize.x) %
> + * gl_WorkGroupSize.y;
> + *gl_LocalInvocationID.z =
> + * (gl_LocalInvocationIndex /
> + *(gl_WorkGroupSize.x * gl_WorkGroupSize.y)) %
> + * gl_WorkGroupSize.z;
> + *
> + * However, the final % gl_WorkGroupSize.z does nothing unless we
> + * accidentally end up with a gl_LocalInvocationIndex that is too
> + * large so it can safely be omitted.
> + */
> + nir_ssa_def *local_index = nir_load_local_invocation_index(b);
> + nir_ssa_def *local_size = build_local_group_size(b);
> +
> + nir_ssa_def *id_x, *id_y, *id_z;
> + id_x = nir_umod(b, local_index,
> + nir_channel(b, local_size, 0));
> + id_y = nir_umod(b, nir_udiv(b, local_index,
> + nir_channel(b, local_size, 0)),
> + nir_channel(b, local_size, 1));
> + id_z = nir_udiv(b, local_index,
> + nir_imul(b, nir_channel(b, local_size, 0),
> + nir_channel(b, local_size, 1)));
> + return nir_vec3(b, id_x, id_y, id_z);
> + } else {
> + return nir_load_local_invocation_id(b);
> + }
> +}
> +
> static bool
> convert_block(nir_block *block, nir_builder *b)
> {
> @@ -91,7 +130,7 @@ convert_block(nir_block *block, nir_builder *b)
>*/
> nir_ssa_def *group_size = build_local_group_size(b);
> nir_ssa_def *group_id = nir_load_work_group_id(b);
> - nir_ssa_def *local_id = nir_load_local_invocation_id(b);
> + nir_ssa_def *local_id = build_local_invocation_id(b);
>
> sysval = nir_iadd(b, nir_imul(b, group_id, group_size),
> local_id);
> break;
> @@ -126,6 +165,14 @@ convert_block(nir_block *block, nir_builder *b)
> break;
>}
>
> + case SYSTEM_VALUE_LOCAL_INVOCATION_ID:
> + /* If lower_cs_local_id_from_index is true, then we derive the
> local
> + * index from the local id.
> + */
> + if (b->shader->options->lower_cs_local_id_from_index)
> +sysval = build_local_invocation_id(b);
> + break;
> +
>case SYSTEM_VALUE_LOCAL_GROUP_SIZE: {
> sysval = build_local_group_size(b);
> break;
> diff --git a/src/intel/compiler/brw_compiler.c
> b/src/intel/compiler/brw_compiler.c
> index e863b08b991..fe632c5badc 100644
> --- a/src/intel/compiler/brw_compiler.c
> +++ b/src/intel/compiler/brw_compiler.c
> @@ -42,6 +42,7 @@
> .lower_fdiv = true,
> \
> .lower_flrp64 = true,
> \
> .lower_ldexp = true,
> \
> + .lower_cs_local_id_from_index = true,
> \
> .lower_device_index_to_zero = true,
> \
> .native_integers = true,
> \
> .use_interpolated_input_intrinsics = true,
> \
> diff --git a/src/intel/compiler/brw_nir_lower_cs_intrinsics.c
> b/src/intel/compiler/brw_nir_lower_cs_intrinsics.c
> index bfbdea0e8fa..fab5edc893f 100644
> --- a/src/intel/compiler/brw_nir_lower_cs_intrinsics.c
> +++ b/src/intel/compiler/brw_nir_lower_cs_intrinsics.c
> @@ -70,39 +70,6 @@ lower_cs_
Re: [Mesa-dev] [PATCH] egl: Check if API is supported when using eglBindAPI.
Hi Daniel,
Thanks for reviewing!
On Fri, May 13, 2016 at 5:09 PM, Daniel Stone wrote:
> Hi,
>
> On 13 May 2016 at 17:03, Plamena Manolova
> wrote:
> > @@ -444,6 +444,8 @@ _eglCreateAPIsString(_EGLDisplay *dpy)
> >strcat(dpy->ClientAPIsString, "OpenVG ");
> >
> > assert(strlen(dpy->ClientAPIsString) <
> sizeof(dpy->ClientAPIsString));
> > +
> > + _eglGlobal.ClientAPIsString = dpy->ClientAPIsString;
>
> What happens when the display is destroyed and this is freed? Or when
> different displays have different supported APIs?
>
You're right this would cause trouble. I think we have two alternatives
here:
1: We could copy the string, but that wouldn't address the case in which
different displays have different APIs.
2: We could fetch the current context and use the ClientAPIsString of the
display associated with it. If there's no current
context we could simply return EGL_FALSE since we'll have no way of
verifying whether the API is supported.
>
> > @@ -69,7 +70,26 @@ struct _egl_thread_info
> > static inline EGLBoolean
> > _eglIsApiValid(EGLenum api)
> > {
> > - return (api >= _EGL_API_FIRST_API && api <= _EGL_API_LAST_API);
> > + char *api_string;
> > + switch (api) {
> > + case EGL_OPENGL_API:
> > + api_string = "OpenGL";
> > + break;
> > + case EGL_OPENGL_ES_API:
> > + api_string = "OpenGL_ES";
> > + break;
> > + case EGL_OPENVG_API:
> > + api_string = "OpenVG";
> > + break;
> > + default:
> > + return EGL_FALSE;
> > + break;
> > + }
> > +
> > + if (strstr(api_string, _eglGlobal.ClientAPIsString))
> > + return EGL_TRUE;
> > + else
> > + return EGL_FALSE;
>
> This is trivially broken: it returns TRUE if a display only supports
> OpenGL ES, but you request to bind OpenGL.
>
>
Thank you for catching this! Silly mistake on my part.
> Cheers,
> Daniel
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: Check if API is supported when using eglBindAPI.
Hi Emil,
Thanks for reviewing!
On Wed, May 18, 2016 at 12:11 PM, Emil Velikov
wrote:
> Hi Plamena,
>
> On 17 May 2016 at 17:35, Plamena Manolova
> wrote:
> > According to the EGL specifications before binding an API
> > we must check whether it's supported first. If not eglBindAPI
> > should return EGL_FALSE and generate a EGL_BAD_PARAMETER error.
> >
> > Signed-off-by: Plamena Manolova
> > ---
> > src/egl/main/eglcurrent.h | 33 ++---
> > 1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/egl/main/eglcurrent.h b/src/egl/main/eglcurrent.h
> > index 1e386ac..f2e19cc 100644
> > --- a/src/egl/main/eglcurrent.h
> > +++ b/src/egl/main/eglcurrent.h
> > @@ -32,7 +32,8 @@
> > #include "c99_compat.h"
> >
> > #include "egltypedefs.h"
> > -
> > +#include "eglglobals.h"
> > +#include "egldisplay.h"
> >
> > #ifdef __cplusplus
> > extern "C" {
> > @@ -62,14 +63,40 @@ struct _egl_thread_info
> > EGLint CurrentAPIIndex;
> > };
> >
> > -
> > +static inline EGLBoolean
> > +_eglDisplaySupportsApi(_EGLDisplay *dpy, EGLenum api)
> > +{
> > + if (!dpy->Initialized) {
> > + return EGL_FALSE;
> > + } else if (api == EGL_OPENGL_API && dpy->ClientAPIs &
> EGL_OPENGL_BIT) {
> > + return EGL_TRUE;
> > + } else if (api == EGL_OPENGL_ES_API &&
> > + (dpy->ClientAPIs & EGL_OPENGL_ES_BIT ||
> > + dpy->ClientAPIs & EGL_OPENGL_ES2_BIT ||
> > + dpy->ClientAPIs & EGL_OPENGL_ES3_BIT_KHR)) {
> > + return EGL_TRUE;
> > + } else if (api == EGL_OPENVG_API && dpy->ClientAPIs &
> EGL_OPENVG_BIT) {
> > + return EGL_TRUE;
> > + } else {
> > + return EGL_FALSE;
> > + }
> Nit: please use a switch (api) statement ?
I'll go ahead and make this change.
>
> > +}
> > /**
> > * Return true if a client API enum is recognized.
> > */
> > static inline EGLBoolean
> > _eglIsApiValid(EGLenum api)
> > {
> > - return (api >= _EGL_API_FIRST_API && api <= _EGL_API_LAST_API);
> > + _EGLDisplay *dpy = _eglGlobal.DisplayList;
> > +
> > + while (dpy) {
> > + if (_eglDisplaySupportsApi(dpy, api) == EGL_TRUE)
> > + return EGL_TRUE;
> > +
> > + dpy = dpy->Next;
> > + }
> > +
> > + return EGL_FALSE;
> Afaict currently this returns true if any display supports the said
> APIs, while the manpage says
>
> "eglBindAPI defines the current rendering API for EGL in the thread it
> is called from"
>
> Haven't looked at the spec, but I'd assume it's a similar thing.
>
> So here we'd want to get the current thread info via
> _eglGetCurrentThread() and derive the display (displays?) based on it.
> I think we don't have a way to the the latter yet, so one will need to
> sort that one first.
>
> I'm not an EGL expert but ^^ makes sense from where I look at it.
>
>
Just checked the spec, you're absolutely right eglBindAPI works on a per
thread basis. I'll have a go at associating displays to their respective
threads and then I'll update this patch.
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: Check if API is supported when using eglBindAPI.
Hi Ian,
Thanks for reviewing!
On Wed, May 18, 2016 at 4:33 PM, Ian Romanick wrote:
> On 05/17/2016 09:35 AM, Plamena Manolova wrote:
> > According to the EGL specifications before binding an API
> > we must check whether it's supported first. If not eglBindAPI
> > should return EGL_FALSE and generate a EGL_BAD_PARAMETER error.
>
> Can you provide a spec quotation?
>
https://www.khronos.org/registry/egl/sdk/docs/man/html/eglBindAPI.xhtml
"EGL_BAD_PARAMETER is generated if api is not one of the accepted tokens,
or if the
specified client API is not supported by the EGL implementation."
> Signed-off-by: Plamena Manolova
> > ---
> > src/egl/main/eglcurrent.h | 33 ++---
> > 1 file changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/egl/main/eglcurrent.h b/src/egl/main/eglcurrent.h
> > index 1e386ac..f2e19cc 100644
> > --- a/src/egl/main/eglcurrent.h
> > +++ b/src/egl/main/eglcurrent.h
> > @@ -32,7 +32,8 @@
> > #include "c99_compat.h"
> >
> > #include "egltypedefs.h"
> > -
> > +#include "eglglobals.h"
> > +#include "egldisplay.h"
> >
> > #ifdef __cplusplus
> > extern "C" {
> > @@ -62,14 +63,40 @@ struct _egl_thread_info
> > EGLint CurrentAPIIndex;
> > };
> >
> > -
> > +static inline EGLBoolean
> > +_eglDisplaySupportsApi(_EGLDisplay *dpy, EGLenum api)
>
> Since this is only used internally, please use bool/true/false. Based
> on my comments at the bottom, I think this function should go directly
> in eglapi.c.
>
> > +{
>
> This is a really complex way of doing something quite simple. How about.
>
>unsigned api_bit;
>
>if (!dpy->Initialized)
> return false:
>
>switch (api) {
>case EGL_OPENGL_API:
> api_bit = EGL_OPENGL_BIT;
> break;
>case EGL_OPENGL_ES_API:
> api_bit = EGL_OPENGL_ES_BIT |
> EGL_OPENGL_ES2_BIT |
> EGL_OPENGL_ES3_BIT_KHR;
> break;
>case EGL_OPENVG_API:
> api_bit = EGL_OPENVG_BIT;
> break;
>default:
> api_bit = 0;
> break;
>}
>
>return (dpy->ClientAPIs & api_bit) != 0;
I'll make those changes.
>
> > + if (!dpy->Initialized) {
> > + return EGL_FALSE;
> > + } else if (api == EGL_OPENGL_API && dpy->ClientAPIs &
> EGL_OPENGL_BIT) {
> > + return EGL_TRUE;
> > + } else if (api == EGL_OPENGL_ES_API &&
> > + (dpy->ClientAPIs & EGL_OPENGL_ES_BIT ||
> > + dpy->ClientAPIs & EGL_OPENGL_ES2_BIT ||
> > + dpy->ClientAPIs & EGL_OPENGL_ES3_BIT_KHR)) {
> > + return EGL_TRUE;
> > + } else if (api == EGL_OPENVG_API && dpy->ClientAPIs &
> EGL_OPENVG_BIT) {
> > + return EGL_TRUE;
> > + } else {
> > + return EGL_FALSE;
> > + }
> > +}
> > /**
> > * Return true if a client API enum is recognized.
> > */
> > static inline EGLBoolean
> > _eglIsApiValid(EGLenum api)
>
> There's only one user of this function, and I suspect there will only
> every be that one user. I would do a first patch that moves this
> function into eglapi.c... maybe even just move this function into the
> only caller. Then this patch would change the way it works.
>
>
Makes sense I'll go ahead and move it to the actual caller.
> > {
> > - return (api >= _EGL_API_FIRST_API && api <= _EGL_API_LAST_API);
> > + _EGLDisplay *dpy = _eglGlobal.DisplayList;
> > +
> > + while (dpy) {
> > + if (_eglDisplaySupportsApi(dpy, api) == EGL_TRUE)
>
> Even without changing _eglDisplaySupportsApi to return bool, you can
> drop the '== EGL_TRUE'.
>
> > + return EGL_TRUE;
> > +
> > + dpy = dpy->Next;
> > + }
> > +
> > + return EGL_FALSE;
> > }
> >
> >
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] egl: Check if API is supported when using eglBindAPI.
On Wed, May 18, 2016 at 8:44 PM, Ian Romanick wrote:
> On 05/18/2016 11:45 AM, Manolova, Plamena wrote:
> > Hi Ian,
> > Thanks for reviewing!
> >
> > On Wed, May 18, 2016 at 4:33 PM, Ian Romanick > <mailto:i...@freedesktop.org>> wrote:
> >
> > On 05/17/2016 09:35 AM, Plamena Manolova wrote:
> > > According to the EGL specifications before binding an API
> > > we must check whether it's supported first. If not eglBindAPI
> > > should return EGL_FALSE and generate a EGL_BAD_PARAMETER error.
> >
> > Can you provide a spec quotation?
> >
> >
> > https://www.khronos.org/registry/egl/sdk/docs/man/html/eglBindAPI.xhtml
> >
> > "EGL_BAD_PARAMETER is generated if api is not one of the accepted
> > tokens, or if the
> > specified client API is not supported by the EGL implementation."
>
> That's the man page, not the spec. We have found a few problems over
> the years in using man page quotations, so we generally prefer to use
> the spec. The biggest issue is that it's harder to track changes in the
> man pages, but we can pretty easily tell when something changed between,
> say, EGL 1.2 and EGL 1.5. If nothing else, it makes the quotation
> practice more consistent to always use the same kind of source.
>
> In Mesa, changes like this should be accompanied by a quotation, in a
> canonical format, from the specification. Ideally, the quotation should
> go in the code being changed. It may also be acceptable to include the
> quotation in the commit message. The next person to look at this code
> is either going to just look at the code or look at the commit that
> added it (after using git-blame).
>
> For this case, the proper spec quotation would be:
>
> Section 3.7 (Rendering Contexts) of the EGL 1.5 spec says:
>
> "api must specify one of the supported client APIs, either
> EGL_OPENGL_API, EGL_OPENGL_ES_API, or EGL_OPENVG_API... If api
> is not one of the values specified above, or if the client API
> specified by api is not supported by the implementation, an
> EGL_BAD_PARAMETER error is generated."
>
>
Thanks for clearing this up for me Ian, I'm pretty new to Mesa so pointers
like this
are really appreciated. I'll keep it mind for next time. I'll also include
the quotation in
the code for my follow up patch.
> > > Signed-off-by: Plamena Manolova > <mailto:plamena.manol...@intel.com>>
> > > ---
> > > src/egl/main/eglcurrent.h | 33 ++---
> > > 1 file changed, 30 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/egl/main/eglcurrent.h b/src/egl/main/eglcurrent.h
> > > index 1e386ac..f2e19cc 100644
> > > --- a/src/egl/main/eglcurrent.h
> > > +++ b/src/egl/main/eglcurrent.h
> > > @@ -32,7 +32,8 @@
> > > #include "c99_compat.h"
> > >
> > > #include "egltypedefs.h"
> > > -
> > > +#include "eglglobals.h"
> > > +#include "egldisplay.h"
> > >
> > > #ifdef __cplusplus
> > > extern "C" {
> > > @@ -62,14 +63,40 @@ struct _egl_thread_info
> > > EGLint CurrentAPIIndex;
> > > };
> > >
> > > -
> > > +static inline EGLBoolean
> > > +_eglDisplaySupportsApi(_EGLDisplay *dpy, EGLenum api)
> >
> > Since this is only used internally, please use bool/true/false.
> Based
> > on my comments at the bottom, I think this function should go
> directly
> > in eglapi.c.
> >
> > > +{
> >
> > This is a really complex way of doing something quite simple. How
> > about.
> >
> >unsigned api_bit;
> >
> >if (!dpy->Initialized)
> > return false:
> >
> >switch (api) {
> >case EGL_OPENGL_API:
> > api_bit = EGL_OPENGL_BIT;
> > break;
> >case EGL_OPENGL_ES_API:
> > api_bit = EGL_OPENGL_ES_BIT |
> > EGL_OPENGL_ES2_BIT |
> > EGL_OPENGL_ES3_BIT_KHR;
> > break;
> >case EGL_OPENVG_API:
> > api_bit = EGL_OPENVG_BIT;
> > break;
> >default:
> > api_bit = 0;
> > break;
> >}
> >
> >return (dpy->ClientAPIs & api_bit) != 0;
> >
> >
> > I'll make those chan
Re: [Mesa-dev] [PATCH] egl: Add OpenGL_ES to API string regardless of GLES version
I just thought that it might be useful to note that due to the ES2/ES3 strings being present in the APIs string some dEQP tests are currently failing. Namely dEQP-EGL.functional.negative_api* and dEQP-EGL.functional.query_context.simple.query_api On Wed, May 18, 2016 at 12:03 PM, Erik Faye-Lund wrote: > On Wed, May 18, 2016 at 1:01 PM, Emil Velikov > wrote: > > On 18 May 2016 at 09:27, Erik Faye-Lund wrote: > >> On Wed, May 18, 2016 at 10:12 AM, Daniel Stone > wrote: > >>> Hi, > >>> > >>> On 18 May 2016 at 00:00, Ian Romanick wrote: > On 05/17/2016 09:59 AM, Ben Widawsky wrote: > > I think you misstated this. It's not invalid to have any other > value. It's > > invalid to not have one of the 3 values, which I suppose is > technically possible > > if you say support ES2, but not ES or GL (for example) > > > > "Returns a string describing which client rendering APIs are > supported. The > > string contains a space-separate list of API names. The list must > include at > > least one of OpenGL, OpenGL_ES, or OpenVG. These strings correspond > > respectively to values EGL_OPENGL_API, EGL_OPENGL_ES_API, and > EGL_OPENVG_API > > of the eglBindAPI, api argument." > > > > I am concerned by this change since I genuinely have no clue how EGL > clients > > might currently be depending on this, and as such could I request > that you not > > change the existing behavior (spit out when ES2 or ES3). At the > bottom I put an > > untested version of what i would have done. > > I think this might be right. Outside of Mesa sources, I can't find > any > mention of OpenGL_ES2 or OpenGL_ES3 strings anywhere on the Internet. > At least VLC > (http://www.videolan.org/developers/vlc/modules/video_output/egl.c) > uses > OpenGL_ES for both OpenGL ES 1.x and 2.x. > >>> > >>> Yes, and they'd be foolish not to: the proprietary Mali, PVR and > >>> Vivante drivers don't expose these strings, just OpenGL_ES, OpenGL and > >>> OpenVG. No idea what the proprietary Tegra drivers do, but I'd be > >>> surprised if they were different. > >> > >> The proprietary NV driver on Tegra 2 reports "OpenGL_ES2 OpenGL_ES > >> OpenGL OpenVG" for me :/ > > > > Thanks Erik, I wish you had better news. > > > > So perhaps we could keep the ES2/ES3 strings for now, until we check > > with newer tegra 3/4 drivers (perhaps they removed it) or a few other > > programs than VLC ? > > > > I also checked with my Tegra 3 device, and it has the same issue. > > But do note that this string seems completely garbage to me: Tegra 2/3 > doesn't support full OpenGL, yet they return it in the client query. > So I have doubts anyone is using this string for anything useful in > the first place. > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] egl: Check if API is supported when using eglBindAPI.
Hi Brian,
On Mon, May 23, 2016 at 9:51 PM, Brian Paul wrote:
> Hi Plamena,
>
> Some style nitpicks below. Feel free to take 'em or leave 'em.
> I'm not too involved in EGL.
>
>
>
> On 05/23/2016 10:27 AM, Plamena Manolova wrote:
>
>> According to the EGL specifications before binding an API
>> we must check whether it's supported first. If not eglBindAPI
>> should return EGL_FALSE and generate a EGL_BAD_PARAMETER error.
>>
>> Signed-off-by: Plamena Manolova
>> ---
>> src/egl/main/eglapi.c | 70
>> +++
>> src/egl/main/eglcurrent.h | 11 +---
>> src/egl/main/egldisplay.c | 5
>> src/egl/main/egldisplay.h | 1 +
>> 4 files changed, 77 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
>> index be2c90f..9dcee79 100644
>> --- a/src/egl/main/eglapi.c
>> +++ b/src/egl/main/eglapi.c
>> @@ -1196,6 +1196,66 @@ eglGetError(void)
>> }
>>
>>
>> +static bool
>> +_eglDisplaySupportsApi(_EGLDisplay *dpy, EGLenum api)
>> +{
>> + if (!dpy->Initialized) {
>> + return false;
>> + }
>> +
>> + switch (api) {
>> + case EGL_OPENGL_API:
>> + if (dpy->ClientAPIs & EGL_OPENGL_BIT)
>> +return true;
>>
> > + break;
>
> I'd probably do:
> return !!(dpy->ClientAPIs & EGL_OPENGL_BIT);
>
> and similar for these cases to make it a little more concise.
>
>
This would make the switch much tidier, I'll go ahead and make that change.
>
> + case EGL_OPENGL_ES_API:
>> + if (dpy->ClientAPIs & EGL_OPENGL_ES_BIT ||
>> + dpy->ClientAPIs & EGL_OPENGL_ES2_BIT ||
>> + dpy->ClientAPIs & EGL_OPENGL_ES3_BIT_KHR) {
>> + return true;
>> + }
>> + break;
>> + case EGL_OPENVG_API:
>> + if (dpy->ClientAPIs & EGL_OPENVG_BIT)
>> +return true;
>> + break;
>> + default:
>> + return false;
>> + break;
>>
>
> break not needed. Should the default case raise an invalid enum error or
> something? I don't recall what the EGL spec says about that stuff.
Yes a EGL_BAD_PARAMETER error is generated if for whatever reason this
function returns false
for all displays associated with the current thread. Actually it would make
more sense for the
default case to be evaluated in the caller. I'll go ahead and move it.
>
Whitespace note: we normally indent 'case' the same amount as the 'switch'.
>
>
>
> + }
>> +
>> + return false;
>> +}
>> +
>> +
>> +/**
>> + * Return true if a client API enum is recognized.
>> + */
>> +static bool
>> +_eglIsApiValid(EGLenum api)
>> +{
>> + _EGLDisplay *dpy = _eglGlobal.DisplayList;
>> + _EGLThreadInfo *current_thread = _eglGetCurrentThread();
>> +
>> + while (dpy) {
>> + _EGLThreadInfo *thread = dpy->ThreadList;
>> +
>> + while (thread) {
>> + if (thread == current_thread) {
>> +if (_eglDisplaySupportsApi(dpy, api))
>> + return true;
>> + }
>> +
>> + thread = thread->Next;
>> + }
>> +
>> + dpy = dpy->Next;
>> + }
>> +
>> + return false;
>> +}
>> +
>> +
>> /**
>>** EGL 1.2
>>**/
>> @@ -1211,6 +1271,16 @@ eglGetError(void)
>>* eglWaitNative()
>>* See section 3.7 "Rendering Context" in the EGL specification for
>> details.
>>*/
>> +
>> + /**
>> + * Section 3.7 (Rendering Contexts) of the EGL 1.5 spec says:
>> + *
>> + * "api must specify one of the supported client APIs, either
>> + * EGL_OPENGL_API, EGL_OPENGL_ES_API, or EGL_OPENVG_API... If api
>> + * is not one of the values specified above, or if the client API
>> + * specified by api is not supported by the implementation, an
>> + * EGL_BAD_PARAMETER error is generated."
>> + */
>> EGLBoolean EGLAPIENTRY
>> eglBindAPI(EGLenum api)
>> {
>> diff --git a/src/egl/main/eglcurrent.h b/src/egl/main/eglcurrent.h
>> index 1e386ac..6c203be 100644
>> --- a/src/egl/main/eglcurrent.h
>> +++ b/src/egl/main/eglcurrent.h
>> @@ -56,6 +56,7 @@ extern "C" {
>>*/
>> struct _egl_thread_info
>> {
>> + _EGLThreadInfo *Next; /* used to link threads */
>> EGLint LastError;
>> _EGLContext *CurrentContexts[_EGL_API_NUM_APIS];
>> /* use index for fast access to current context */
>> @@ -64,16 +65,6 @@ struct _egl_thread_info
>>
>>
>> /**
>> - * Return true if a client API enum is recognized.
>> - */
>> -static inline EGLBoolean
>> -_eglIsApiValid(EGLenum api)
>> -{
>> - return (api >= _EGL_API_FIRST_API && api <= _EGL_API_LAST_API);
>> -}
>> -
>> -
>> -/**
>>* Convert a client API enum to an index, for use by thread info.
>>* The client API enum is assumed to be valid.
>>*/
>> diff --git a/src/egl/main/egldisplay.c b/src/egl/main/egldisplay.c
>> index f6db03a..907a607 100644
>> --- a/src/egl/main/egldisplay.c
>> +++ b/src/egl/main/egldisplay.c
>> @@ -240,6 +240,7 @@ _EGLDisplay *
>> _eglFindDisplay(_EGLPlatformType plat, void *plat_dpy)
>> {
>> _EGLDisplay *dpy;
>> +
Re: [Mesa-dev] iris now officially OpenGL 4.6 conformant
That's so awesome, congrats! \O/ On Wed, Nov 20, 2019 at 6:37 PM Kenneth Graunke wrote: > Hi all, > > iris is now officially a conformant OpenGL 4.6 implementation! > > > https://www.khronos.org/conformance/adopters/conformant-products/opengl#submission_253 > > This is on Gen9. I've also submitted for Gen8, but that's still under > review; Gen11 is 99% of the way there but I'm hunting down one last bug. > > --Ken > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
