Re: [Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue

[Mathias Fröhlich]

Hi Brian,

On Thursday, 14 March 2019 15:04:28 CET Brian Paul wrote:
> > Ok, so basically you are saying that you improve the current situation with 
> > this
> > current series, still leaving a race condition open. But a good final fix 
> > to the race
> > condition is underway.
> 
> Yeah, I can probably stage the multi-thread/race patch series before 
> this context-fix-up series.


Ok, combining that all with the rest of the fix makes highly sense!
Thanks for explaining!
And sorry for my late replies. Here without family and sickness.

best

Mathias


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/8] st/mesa: implement "zombie" sampler views

[Mathias Fröhlich]

Hi Brian,

the full package looks great and makes a lot of sense!

Reviewed-by: Mathias Fröhlich 

best

Mathias



On Thursday, 14 March 2019 20:37:09 CET Brian Paul wrote:
> When st_texture_release_all_sampler_views() is called the texture may
> have sampler views belonging to several contexts.  If we unreference a
> sampler view and its refcount hits zero, we need to be sure to destroy
> the sampler view with the same context which created it.
> 
> This was not the case with the previous code which used
> pipe_sampler_view_release().  That function could end up freeing a
> sampler view with a context different than the one which created it.
> In the case of the VMware svga driver, we detected this but leaked the
> sampler view.  This led to a crash with google-chrome when the kernel
> module had too many sampler views.  VMware bug 2274734.
> 
> Alternately, if we try to delete a sampler view with the correct
> context, we may be "reaching into" a context which is active on
> another thread.  That's not safe.
> 
> To fix these issues this patch adds a per-context list of "zombie"
> sampler views.  These are views which are to be freed at some point
> when the context is active.  Other contexts may safely add sampler
> views to the zombie list at any time (it's mutex protected).  This
> avoids the context/view ownership mix-ups we had before.
> 
> Tested with: google-chrome, google earth, Redway3D Watch/Turbine demos
> a few Linux games.  If anyone can recomment some other multi-threaded,
> multi-context GL apps to test, please let me know.
> ---
>  src/mesa/state_tracker/st_cb_flush.c |  6 +++
>  src/mesa/state_tracker/st_context.c  | 81 
> 
>  src/mesa/state_tracker/st_context.h  | 25 ++
>  src/mesa/state_tracker/st_sampler_view.c | 27 +--
>  src/mesa/state_tracker/st_texture.h  |  3 ++
>  5 files changed, 138 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/state_tracker/st_cb_flush.c 
> b/src/mesa/state_tracker/st_cb_flush.c
> index 5b3188c..81e5338 100644
> --- a/src/mesa/state_tracker/st_cb_flush.c
> +++ b/src/mesa/state_tracker/st_cb_flush.c
> @@ -39,6 +39,7 @@
>  #include "st_cb_flush.h"
>  #include "st_cb_clear.h"
>  #include "st_cb_fbo.h"
> +#include "st_context.h"
>  #include "st_manager.h"
>  #include "pipe/p_context.h"
>  #include "pipe/p_defines.h"
> @@ -53,6 +54,11 @@ st_flush(struct st_context *st,
>  {
> st_flush_bitmap_cache(st);
>  
> +   /* We want to call this function periodically.
> +* Typically, it has nothing to do so it shouldn't be expensive.
> +*/
> +   st_context_free_zombie_objects(st);
> +
> st->pipe->flush(st->pipe, fence, flags);
>  }
>  
> diff --git a/src/mesa/state_tracker/st_context.c 
> b/src/mesa/state_tracker/st_context.c
> index 2898279..bd919da 100644
> --- a/src/mesa/state_tracker/st_context.c
> +++ b/src/mesa/state_tracker/st_context.c
> @@ -261,6 +261,80 @@ st_invalidate_state(struct gl_context *ctx)
>  }
>  
>  
> +/*
> + * In some circumstances (such as running google-chrome) the state
> + * tracker may try to delete a resource view from a context different
> + * than when it was created.  We don't want to do that.
> + * In that situation, st_texture_release_all_sampler_views() calls this
> + * function to save the view for later deletion.  The context here is
> + * expected to be the context which created the view.
> + */
> +void
> +st_save_zombie_sampler_view(struct st_context *st,
> +struct pipe_sampler_view *view)
> +{
> +   struct st_zombie_sampler_view_node *entry;
> +
> +   assert(view->context == st->pipe);
> +   assert(view->reference.count == 1);
> +
> +   entry = MALLOC_STRUCT(st_zombie_sampler_view_node);
> +   if (!entry)
> +  return;
> +
> +   entry->view = view;
> +
> +   /* We need a mutex since this function may be called from one thread
> +* while free_zombie_resource_views() is called from another.
> +*/
> +   mtx_lock(>zombie_sampler_views.mutex);
> +   LIST_ADDTAIL(>node, >zombie_sampler_views.list.node);
> +   mtx_unlock(>zombie_sampler_views.mutex);
> +}
> +
> +
> +/*
> + * Free any zombie sampler views that may be attached to this context.
> + */
> +static void
> +free_zombie_sampler_views(struct st_context *st)
> +{
> +   struct st_zombie_sampler_view_node *entry, *next;
> +
> +   if (LIST_IS_EMPTY(>zombie_sampler_views.list.node)) {
> +  return;
> +   }
> +
> +   mtx_lock(>zombie_sampler_views.mutex);
> +
> +   LIST_FOR_EACH_ENTRY_SAFE(entry, next,
> +>zombie_sampler_views.list.node, node) {
> +  LIST_DEL(>node);  // remove this entry from the list
> +
> +  assert(entry->view->context == st->pipe);
> +  assert(entry->view->reference.count == 1);
> +  pipe_sampler_view_reference(>view, NULL);
> +
> +  free(entry);
> +   }
> +
> +   assert(LIST_IS_EMPTY(>zombie_sampler_views.list.node));
> +
> +   mtx_unlock(>zombie_sampler_views.mutex);
> +}
> +
> +
> +/*
> 

[Mesa-dev] [PATCH 4/5] panfrost/decode: Respect primitive size pointers

[Alyssa Rosenzweig]

Signed-off-by: Alyssa Rosenzweig 
---
 src/gallium/drivers/panfrost/pandecode/decode.c | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/gallium/drivers/panfrost/pandecode/decode.c 
b/src/gallium/drivers/panfrost/pandecode/decode.c
index e6932744939..28bc0509feb 100644
--- a/src/gallium/drivers/panfrost/pandecode/decode.c
+++ b/src/gallium/drivers/panfrost/pandecode/decode.c
@@ -1692,7 +1692,11 @@ pandecode_replay_primitive_size(union 
midgard_primitive_size u, bool constant)
 pandecode_log(".primitive_size = {\n");
 pandecode_indent++;
 
-pandecode_prop("constant = %f", u.constant);
+if (constant) {
+pandecode_prop("constant = %f", u.constant);
+} else {
+MEMORY_PROP((), pointer);
+}
 
 pandecode_indent--;
 pandecode_log("},\n");
@@ -1802,8 +1806,8 @@ pandecode_replay_vertex_or_tiler_job_mdg(const struct 
mali_job_descriptor_header
 pandecode_log("struct midgard_payload_vertex_tiler payload_%d = {\n", 
job_no);
 pandecode_indent++;
 
-/* TODO: gl_PointSize */
-pandecode_replay_primitive_size(v->primitive_size, true);
+bool has_primitive_pointer = v->prefix.unknown_draw & 
MALI_DRAW_VARYING_SIZE;
+pandecode_replay_primitive_size(v->primitive_size, 
!has_primitive_pointer);
 
 pandecode_log(".prefix = ");
 pandecode_replay_vertex_tiler_prefix(>prefix, job_no);
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/5] panfrost: Workaround buffer overrun with mip level

[Alyssa Rosenzweig]

Mipmaps are still broken, but at least this way we don't crash on some
apps using mipmaps.

Signed-off-by: Alyssa Rosenzweig 
---
 src/gallium/drivers/panfrost/pan_resource.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/gallium/drivers/panfrost/pan_resource.c 
b/src/gallium/drivers/panfrost/pan_resource.c
index ffb65fc52a9..9e83c9bcb51 100644
--- a/src/gallium/drivers/panfrost/pan_resource.c
+++ b/src/gallium/drivers/panfrost/pan_resource.c
@@ -235,7 +235,6 @@ panfrost_create_bo(struct panfrost_screen *screen, const 
struct pipe_resource *t
 for (int l = 0; l < (template->last_level + 1); ++l) {
 bo->cpu[l] = malloc(sz);
 bo->size[l] = sz;
-sz >>= 2;
 }
 } else {
 /* For a linear resource, allocate a block of memory from
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 5/5] panfrost: Replay more varying buffers

[Alyssa Rosenzweig]

This is required for gl_PointCoord to show up on decodes.

Signed-off-by: Alyssa Rosenzweig 
---
 src/gallium/drivers/panfrost/pandecode/decode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/panfrost/pandecode/decode.c 
b/src/gallium/drivers/panfrost/pandecode/decode.c
index 28bc0509feb..86fb968e334 100644
--- a/src/gallium/drivers/panfrost/pandecode/decode.c
+++ b/src/gallium/drivers/panfrost/pandecode/decode.c
@@ -1357,7 +1357,7 @@ pandecode_replay_vertex_tiler_postfix_pre(const struct 
mali_vertex_tiler_postfix
 /* Number of descriptors depends on whether there are
  * non-internal varyings */
 
-pandecode_replay_attributes(attr_mem, p->varyings, job_no, 
suffix, varying_count > 1 ? 2 : 1, true);
+pandecode_replay_attributes(attr_mem, p->varyings, job_no, 
suffix, varying_count > 1 ? 4 : 1, true);
 }
 
 if (p->varying_meta) {
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/5] panfrost: Fix primconvert check

[Alyssa Rosenzweig]

In addition to fixing actual primconvert bugs, this prevents an infinite
loop when trying to draw POINTS.

Signed-off-by: Alyssa Rosenzweig 
---
 src/gallium/drivers/panfrost/pan_context.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/panfrost/pan_context.c 
b/src/gallium/drivers/panfrost/pan_context.c
index 378a631410b..b6cf5302cae 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -1362,7 +1362,7 @@ panfrost_draw_vbo(
 
 /* Fallback for unsupported modes */
 
-if (!(ctx->draw_modes & mode)) {
+if (!(ctx->draw_modes & (1 << mode))) {
 if (mode == PIPE_PRIM_QUADS && info->count == 4 && 
ctx->rasterizer && !ctx->rasterizer->base.flatshade) {
 mode = PIPE_PRIM_TRIANGLE_FAN;
 } else {
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/5] panfrost: Disable PIPE_CAP_TGSI_TEXCOORD

[Alyssa Rosenzweig]

I don't know why this was on to begin with...?

Signed-off-by: Alyssa Rosenzweig 
---
 src/gallium/drivers/panfrost/pan_screen.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/src/gallium/drivers/panfrost/pan_screen.c 
b/src/gallium/drivers/panfrost/pan_screen.c
index 9cd65ca8ff8..9672048bca8 100644
--- a/src/gallium/drivers/panfrost/pan_screen.c
+++ b/src/gallium/drivers/panfrost/pan_screen.c
@@ -195,9 +195,6 @@ panfrost_get_param(struct pipe_screen *screen, enum 
pipe_cap param)
 case PIPE_CAP_TEXTURE_BUFFER_OFFSET_ALIGNMENT:
 return 0;
 
-case PIPE_CAP_TGSI_TEXCOORD:
-return 1; /* XXX: What should this me exactly? */
-
 case PIPE_CAP_PREFER_BLIT_BASED_TEXTURE_TRANSFER:
 return 0;
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 110116] Neverball particles are broken (GL_POINT_SPRITE)

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=110116

QwertyChouskie  changed:

   What|Removed |Added

 OS|All |Linux (All)
 CC||asdfghrbljz...@outlook.com
   Hardware|Other   |x86-64 (AMD64)

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 110116] Neverball particles are broken (GL_POINT_SPRITE)

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=110116

Bug ID: 110116
   Summary: Neverball particles are broken (GL_POINT_SPRITE)
   Product: Mesa
   Version: 18.3
  Hardware: Other
OS: All
Status: NEW
  Severity: normal
  Priority: medium
 Component: Mesa core
  Assignee: mesa-dev@lists.freedesktop.org
  Reporter: asdfghrbljz...@outlook.com
QA Contact: mesa-dev@lists.freedesktop.org

>From https://github.com/Neverball/neverball/issues/170

The particles in the goal posts in Neverball 1.6.0 (from the Ubuntu archives)
render as squares on my system.  I have Intel HD 4000 graphics, Mesa 18.3.3,
Linux 4.18.0-16-generic, and Ubuntu 18.04.  Note that this also happens with
llvmpipe and softpipe, so probably not an Intel-specific issue.

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Rob Clark]

On Thu, Mar 14, 2019 at 9:58 PM Roland Scheidegger  wrote:
>
> Am 15.03.19 um 02:18 schrieb Rob Clark:
> > On Thu, Mar 14, 2019 at 8:28 PM Roland Scheidegger  
> > wrote:
> >>
> >> Am 14.03.19 um 22:06 schrieb Rob Clark:
> >>> On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger  
> >>> wrote:
> 
>  Am 14.03.19 um 14:13 schrieb Rob Clark:
> > On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  
> > wrote:
> >>
> >> Am 12.03.19 um 16:16 schrieb Rob Clark:
> >>> This previously was not called out clearly, but based on a survey of 
> >>> the
> >>> code, it seems the expected behavior is to release the reference to 
> >>> any
> >>> sampler views beyond the new range being bound.
> >>
> >> That isn't really true. This was designed to work like d3d10, where
> >> other views are unmodified.
> >> The cso code will actually unset all views which previously were set 
> >> and
> >> are above the num_views in the call (this wouldn't be necessary if the
> >> pipe function itself would work like this).
> >> However, it will only do this for fragment textures, and pass through
> >> the parameters unmodified otherwise. Which means behavior might not be
> >> very consistent for the different stages...
> >
> > Any opinion about whether views==NULL should be allowed?  Currently I
> > have something like:
> >
> > 
> > diff --git a/src/gallium/docs/source/context.rst
> > b/src/gallium/docs/source/context.rst
> > index f89d9e1005e..06d30bfb38b 100644
> > --- a/src/gallium/docs/source/context.rst
> > +++ b/src/gallium/docs/source/context.rst
> > @@ -143,6 +143,11 @@ to the array index which is used for sampling.
> >to a respective sampler view and releases a reference to the previous
> >sampler view.
> >
> > +  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
> > +  unmodified.  If ``views`` is NULL, the behavior is the same as if
> > +  ``views[n]`` was NULL for the entire range, ie. releasing the 
> > reference
> > +  for all the sampler views in the specified range.
> > +
> >  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> > associated
> >with the sampler view which results in sampler view holding a 
> > reference
> >to the texture. Format specified in template must be compatible
> > 
> >
> > But going thru the other drivers, a lot of them also don't handle the
> > views==NULL case.  This case doesn't seem to come up with mesa/st, but
> > does with XA and nine, and some of the test code.
> 
>  I think this should be illegal. As you've noted some drivers can't
>  handle it, and I don't see a particularly good reason to allow it. Well
>  I guess it trades some complexity in state trackers with some complexity
>  in drivers...
> >>>
> >>> fwiw, going with the idea that it should be legal, I fixed that in the
> >>> drivers that didn't handle it in:
> >>>
> >>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2Fmerge_requests%2F449data=02%7C01%7Csroland%40vmware.com%7C2fe81dea2d9d4de1974f08d6a8e42caa%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636882095286989477sdata=qd1z5iv8dvt2z16ZlT2OPngoDGofvCM%2F%2F0hsddqAbO4%3Dreserved=0
> >>>
> >>> (planning to send to list, I just pushed a WIP MR to run it thru the CI 
> >>> system)
> >>
> >> I'm pretty sure both softpipe and llvmpipe would crash too, they
> >> dereference this without checking if it's null.
> >> So effectively all drivers but one thought it was illegal?
> >> I still see no point in allowing it (or rather, changing this to be
> >> allowed - per se there's nothing really wrong with this to be allowed).
> >> That said, it appears that set_shader_images and set_shader_buffers
> >> allow it, so there's some precedence for this.
> >
> > hmm, I'd assumed llvmpipe was used with xa somewhere so I didn't
> > really look at it and assumed it handled this..
> xa only sets fragment sampler views, and those only through cso.
> cso will turn this into a non-null views parameter.
> (cso itself also won't tolerate null views parameter, unless the count
> is zero, but that should be alright since its semantics are that it will
> unbind all views above the count - well for fragment sampler views...)
> nine also sets vertex sampler views through cso, which will get passed
> through to drivers as-is. However, the NULL views used there is always
> accompanied by a 0 count, so for drivers interpreting things as range to
> change rather than unbind things outside it is a natural no-op, and
> they'll never even look at views in their loop. (Of course, that's not
> quite what nine actually wanted to do...)
> And yes things are very inconsistent when passed through cso (for
> drivers interpreting it as range to change), since cso will unbind the
> views 

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Roland Scheidegger]

Am 15.03.19 um 02:18 schrieb Rob Clark:
> On Thu, Mar 14, 2019 at 8:28 PM Roland Scheidegger  wrote:
>>
>> Am 14.03.19 um 22:06 schrieb Rob Clark:
>>> On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger  
>>> wrote:

 Am 14.03.19 um 14:13 schrieb Rob Clark:
> On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  
> wrote:
>>
>> Am 12.03.19 um 16:16 schrieb Rob Clark:
>>> This previously was not called out clearly, but based on a survey of the
>>> code, it seems the expected behavior is to release the reference to any
>>> sampler views beyond the new range being bound.
>>
>> That isn't really true. This was designed to work like d3d10, where
>> other views are unmodified.
>> The cso code will actually unset all views which previously were set and
>> are above the num_views in the call (this wouldn't be necessary if the
>> pipe function itself would work like this).
>> However, it will only do this for fragment textures, and pass through
>> the parameters unmodified otherwise. Which means behavior might not be
>> very consistent for the different stages...
>
> Any opinion about whether views==NULL should be allowed?  Currently I
> have something like:
>
> 
> diff --git a/src/gallium/docs/source/context.rst
> b/src/gallium/docs/source/context.rst
> index f89d9e1005e..06d30bfb38b 100644
> --- a/src/gallium/docs/source/context.rst
> +++ b/src/gallium/docs/source/context.rst
> @@ -143,6 +143,11 @@ to the array index which is used for sampling.
>to a respective sampler view and releases a reference to the previous
>sampler view.
>
> +  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
> +  unmodified.  If ``views`` is NULL, the behavior is the same as if
> +  ``views[n]`` was NULL for the entire range, ie. releasing the reference
> +  for all the sampler views in the specified range.
> +
>  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> associated
>with the sampler view which results in sampler view holding a reference
>to the texture. Format specified in template must be compatible
> 
>
> But going thru the other drivers, a lot of them also don't handle the
> views==NULL case.  This case doesn't seem to come up with mesa/st, but
> does with XA and nine, and some of the test code.

 I think this should be illegal. As you've noted some drivers can't
 handle it, and I don't see a particularly good reason to allow it. Well
 I guess it trades some complexity in state trackers with some complexity
 in drivers...
>>>
>>> fwiw, going with the idea that it should be legal, I fixed that in the
>>> drivers that didn't handle it in:
>>>
>>> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2Fmerge_requests%2F449data=02%7C01%7Csroland%40vmware.com%7C2fe81dea2d9d4de1974f08d6a8e42caa%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636882095286989477sdata=qd1z5iv8dvt2z16ZlT2OPngoDGofvCM%2F%2F0hsddqAbO4%3Dreserved=0
>>>
>>> (planning to send to list, I just pushed a WIP MR to run it thru the CI 
>>> system)
>>
>> I'm pretty sure both softpipe and llvmpipe would crash too, they
>> dereference this without checking if it's null.
>> So effectively all drivers but one thought it was illegal?
>> I still see no point in allowing it (or rather, changing this to be
>> allowed - per se there's nothing really wrong with this to be allowed).
>> That said, it appears that set_shader_images and set_shader_buffers
>> allow it, so there's some precedence for this.
> 
> hmm, I'd assumed llvmpipe was used with xa somewhere so I didn't
> really look at it and assumed it handled this..
xa only sets fragment sampler views, and those only through cso.
cso will turn this into a non-null views parameter.
(cso itself also won't tolerate null views parameter, unless the count
is zero, but that should be alright since its semantics are that it will
unbind all views above the count - well for fragment sampler views...)
nine also sets vertex sampler views through cso, which will get passed
through to drivers as-is. However, the NULL views used there is always
accompanied by a 0 count, so for drivers interpreting things as range to
change rather than unbind things outside it is a natural no-op, and
they'll never even look at views in their loop. (Of course, that's not
quite what nine actually wanted to do...)
And yes things are very inconsistent when passed through cso (for
drivers interpreting it as range to change), since cso will unbind the
views above count for fragment shader views explicitly, but won't do
anything for any other shader stage...



> 
> but as you mentioned below, if set_shader_buffers and
> set_shader_images allow null, for consistency (and since I'm already
> fixing up a bunch of set_shader_buffer implementations, so 

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Rob Clark]

On Thu, Mar 14, 2019 at 8:28 PM Roland Scheidegger  wrote:
>
> Am 14.03.19 um 22:06 schrieb Rob Clark:
> > On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger  
> > wrote:
> >>
> >> Am 14.03.19 um 14:13 schrieb Rob Clark:
> >>> On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  
> >>> wrote:
> 
>  Am 12.03.19 um 16:16 schrieb Rob Clark:
> > This previously was not called out clearly, but based on a survey of the
> > code, it seems the expected behavior is to release the reference to any
> > sampler views beyond the new range being bound.
> 
>  That isn't really true. This was designed to work like d3d10, where
>  other views are unmodified.
>  The cso code will actually unset all views which previously were set and
>  are above the num_views in the call (this wouldn't be necessary if the
>  pipe function itself would work like this).
>  However, it will only do this for fragment textures, and pass through
>  the parameters unmodified otherwise. Which means behavior might not be
>  very consistent for the different stages...
> >>>
> >>> Any opinion about whether views==NULL should be allowed?  Currently I
> >>> have something like:
> >>>
> >>> 
> >>> diff --git a/src/gallium/docs/source/context.rst
> >>> b/src/gallium/docs/source/context.rst
> >>> index f89d9e1005e..06d30bfb38b 100644
> >>> --- a/src/gallium/docs/source/context.rst
> >>> +++ b/src/gallium/docs/source/context.rst
> >>> @@ -143,6 +143,11 @@ to the array index which is used for sampling.
> >>>to a respective sampler view and releases a reference to the previous
> >>>sampler view.
> >>>
> >>> +  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
> >>> +  unmodified.  If ``views`` is NULL, the behavior is the same as if
> >>> +  ``views[n]`` was NULL for the entire range, ie. releasing the reference
> >>> +  for all the sampler views in the specified range.
> >>> +
> >>>  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> >>> associated
> >>>with the sampler view which results in sampler view holding a reference
> >>>to the texture. Format specified in template must be compatible
> >>> 
> >>>
> >>> But going thru the other drivers, a lot of them also don't handle the
> >>> views==NULL case.  This case doesn't seem to come up with mesa/st, but
> >>> does with XA and nine, and some of the test code.
> >>
> >> I think this should be illegal. As you've noted some drivers can't
> >> handle it, and I don't see a particularly good reason to allow it. Well
> >> I guess it trades some complexity in state trackers with some complexity
> >> in drivers...
> >
> > fwiw, going with the idea that it should be legal, I fixed that in the
> > drivers that didn't handle it in:
> >
> > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2Fmerge_requests%2F449data=02%7C01%7Csroland%40vmware.com%7C503a661358114ccf08d208d6a8c0eadc%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636881943862256444sdata=4c6ehFiS676ZwbneR6T0CBhBHHq7zoL5efQ7E9e%2Fd9E%3Dreserved=0
> >
> > (planning to send to list, I just pushed a WIP MR to run it thru the CI 
> > system)
>
> I'm pretty sure both softpipe and llvmpipe would crash too, they
> dereference this without checking if it's null.
> So effectively all drivers but one thought it was illegal?
> I still see no point in allowing it (or rather, changing this to be
> allowed - per se there's nothing really wrong with this to be allowed).
> That said, it appears that set_shader_images and set_shader_buffers
> allow it, so there's some precedence for this.

hmm, I'd assumed llvmpipe was used with xa somewhere so I didn't
really look at it and assumed it handled this..

but as you mentioned below, if set_shader_buffers and
set_shader_images allow null, for consistency (and since I'm already
fixing up a bunch of set_shader_buffer implementations, so handling
the ==NULL case isn't a big deal), I'd lean towards allowing NULL.  I
guess if we are going to do API cleanup, then consistency is a useful
thing.. I can check llvmpipe and softpipe and add patches to fix them
if needed.

BR,
-R

>
> >
> > I was on the fence between making st handle it vs making driver handle
> > it, but it is trivial for the driver to handle it if it knows it is
> > supposed to.  And I had to fixup the drivers for various things
> > already (most hadn't been updated to handle the `start_slot` param,
> > for ex).
> Yes, I think in particular because when going through cso things will
> always start at slot 0, so some drivers got sloppy...
> But well for views not being allowed to be null that's also pretty
> trivial for state trackers to handle...
>
> >
> > Eric suggested (on the MR) introducing a helper for this, which might
> > be a better approach to cut down on the boilerplate..  I'll play with
> > that idea.
> >
> > (btw, from a quick look, set_sampler_views isn't the only problem
> > slot.. I 

Re: [Mesa-dev] [PATCH 8/8] gallium/util: remove pipe_sampler_view_release()

[Roland Scheidegger]

This looks all good to me.
For the series:
Reviewed-by: Roland Scheidegger 

Am 14.03.19 um 20:37 schrieb Brian Paul:
> It's no longer used.
> ---
>  src/gallium/auxiliary/util/u_inlines.h | 20 
>  1 file changed, 20 deletions(-)
> 
> diff --git a/src/gallium/auxiliary/util/u_inlines.h 
> b/src/gallium/auxiliary/util/u_inlines.h
> index fa1e920..567d3d0 100644
> --- a/src/gallium/auxiliary/util/u_inlines.h
> +++ b/src/gallium/auxiliary/util/u_inlines.h
> @@ -192,26 +192,6 @@ pipe_sampler_view_reference(struct pipe_sampler_view 
> **dst,
> *dst = src;
>  }
>  
> -/**
> - * Similar to pipe_sampler_view_reference() but always set the pointer to
> - * NULL and pass in the current context explicitly.
> - *
> - * If *ptr is non-NULL, it may refer to a view that was created in a 
> different
> - * context (however, that context must still be alive).
> - */
> -static inline void
> -pipe_sampler_view_release(struct pipe_context *ctx,
> -  struct pipe_sampler_view **ptr)
> -{
> -   struct pipe_sampler_view *old_view = *ptr;
> -
> -   if (pipe_reference_described(_view->reference, NULL,
> -
> (debug_reference_descriptor)debug_describe_sampler_view)) {
> -  ctx->sampler_view_destroy(ctx, old_view);
> -   }
> -   *ptr = NULL;
> -}
> -
>  static inline void
>  pipe_so_target_reference(struct pipe_stream_output_target **dst,
>   struct pipe_stream_output_target *src)
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v5 08/11] anv: Added support for dynamic sample locations

[Jason Ekstrand]

On Thu, Mar 14, 2019 at 7:08 PM Jason Ekstrand  wrote:

> From: Eleni Maria Stea 
>
> Added support for setting the locations when the pipeline has been
> created with the dynamic state bit enabled according to the Vulkan
> Specification section [26.5. Custom Sample Locations] for the function:
>
> 'vkCmdSetSampleLocationsEXT'
>
> The reason that we preferred to store the boolean valid inside the
> dynamic state struct for locations instead of using a dirty bit
> (ANV_CMD_DIRTY_SAMPLE_LOCATIONS for example) is that other functions
> can modify the value of the dirty bits causing unexpected behavior.
>
> v2: Removed all the anv* structs used with sample locations to store
> the locations in order for dynamic case. (see also the patch for
> the non-dynamic case. (Jason Ekstrand)
> ---
>  src/intel/vulkan/anv_cmd_buffer.c  | 17 +
>  src/intel/vulkan/anv_private.h |  6 ++
>  src/intel/vulkan/genX_cmd_buffer.c | 11 +++
>  3 files changed, 34 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_cmd_buffer.c
> b/src/intel/vulkan/anv_cmd_buffer.c
> index 1b34644a434..7f31300857b 100644
> --- a/src/intel/vulkan/anv_cmd_buffer.c
> +++ b/src/intel/vulkan/anv_cmd_buffer.c
> @@ -558,6 +558,23 @@ void anv_CmdSetStencilReference(
> cmd_buffer->state.gfx.dirty |= ANV_CMD_DIRTY_DYNAMIC_STENCIL_REFERENCE;
>  }
>
> +void
> +anv_CmdSetSampleLocationsEXT(VkCommandBuffer commandBuffer,
> + const VkSampleLocationsInfoEXT
> *pSampleLocationsInfo)
> +{
> +   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
> +
> +   struct anv_dynamic_state *dyn_state = _buffer->state.gfx.dynamic;
> +   uint32_t samples = pSampleLocationsInfo->sampleLocationsPerPixel;
> +
> +   assert(pSampleLocationsInfo);
> +   dyn_state->sample_locations.samples = samples;
> +   typed_memcpy(dyn_state->sample_locations.locations,
> +pSampleLocationsInfo->pSampleLocations, samples);
> +
> +   dyn_state->sample_locations.valid = true;
> +}
> +
>  static void
>  anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
> VkPipelineBindPoint bind_point,
> diff --git a/src/intel/vulkan/anv_private.h
> b/src/intel/vulkan/anv_private.h
> index a39195733cd..94c9db14028 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -2124,6 +2124,12 @@ struct anv_dynamic_state {
>uint32_t  front;
>uint32_t  back;
> } stencil_reference;
> +
> +   struct {
> +  bool  valid;
> +  uint32_t  samples;
> +  VkSampleLocationEXT
>  locations[MAX_SAMPLE_LOCATIONS];
> +   } sample_locations;
>  };
>
>  extern const struct anv_dynamic_state default_dynamic_state;
> diff --git a/src/intel/vulkan/genX_cmd_buffer.c
> b/src/intel/vulkan/genX_cmd_buffer.c
> index 7687507e6b7..5d2b17cf8ae 100644
> --- a/src/intel/vulkan/genX_cmd_buffer.c
> +++ b/src/intel/vulkan/genX_cmd_buffer.c
> @@ -2796,6 +2796,17 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer
> *cmd_buffer)
>ANV_CMD_DIRTY_RENDER_TARGETS))
>gen7_cmd_buffer_emit_scissor(cmd_buffer);
>
> +   if (cmd_buffer->state.gfx.dynamic.sample_locations.valid) {
> +  uint32_t samples =
> cmd_buffer->state.gfx.dynamic.sample_locations.samples;
> +  const VkSampleLocationEXT *locations =
> + cmd_buffer->state.gfx.dynamic.sample_locations.locations;
> +  genX(emit_multisample)(_buffer->batch, samples, locations);
> +#if GEN_GEN >= 8
> +  genX(emit_sample_pattern)(_buffer->batch, samples, locations);
> +#endif
> +  cmd_buffer->state.gfx.dynamic.sample_locations.valid = false;
>

I'm not sure this is actually going to be correct.  With dynamic state,
you're required to set it before you use it.  With pipeline state, it gets
set every time the pipeline is bound.  Effectively, the pipeline state is a
big bag of dynamic state.  With both of these, however, there are no
defaults and you're required to bind a pipeline containing the state or
explicitly set it on the command buffer before it gets used.
VK_EXT_sample_locations is different though because it does have a
default.  So the question I'm coming around to is: When does the default
get applied?  The only sensible thing I can think of is at the top of the
command buffer or maybe the top of the subpass.  If this is the case, then
we need to emit sample positions at the start of every subpass.  Does the
spec talk about this at all?

--Jason

>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Roland Scheidegger]

Am 14.03.19 um 22:06 schrieb Rob Clark:
> On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger  wrote:
>>
>> Am 14.03.19 um 14:13 schrieb Rob Clark:
>>> On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  
>>> wrote:

 Am 12.03.19 um 16:16 schrieb Rob Clark:
> This previously was not called out clearly, but based on a survey of the
> code, it seems the expected behavior is to release the reference to any
> sampler views beyond the new range being bound.

 That isn't really true. This was designed to work like d3d10, where
 other views are unmodified.
 The cso code will actually unset all views which previously were set and
 are above the num_views in the call (this wouldn't be necessary if the
 pipe function itself would work like this).
 However, it will only do this for fragment textures, and pass through
 the parameters unmodified otherwise. Which means behavior might not be
 very consistent for the different stages...
>>>
>>> Any opinion about whether views==NULL should be allowed?  Currently I
>>> have something like:
>>>
>>> 
>>> diff --git a/src/gallium/docs/source/context.rst
>>> b/src/gallium/docs/source/context.rst
>>> index f89d9e1005e..06d30bfb38b 100644
>>> --- a/src/gallium/docs/source/context.rst
>>> +++ b/src/gallium/docs/source/context.rst
>>> @@ -143,6 +143,11 @@ to the array index which is used for sampling.
>>>to a respective sampler view and releases a reference to the previous
>>>sampler view.
>>>
>>> +  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
>>> +  unmodified.  If ``views`` is NULL, the behavior is the same as if
>>> +  ``views[n]`` was NULL for the entire range, ie. releasing the reference
>>> +  for all the sampler views in the specified range.
>>> +
>>>  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
>>> associated
>>>with the sampler view which results in sampler view holding a reference
>>>to the texture. Format specified in template must be compatible
>>> 
>>>
>>> But going thru the other drivers, a lot of them also don't handle the
>>> views==NULL case.  This case doesn't seem to come up with mesa/st, but
>>> does with XA and nine, and some of the test code.
>>
>> I think this should be illegal. As you've noted some drivers can't
>> handle it, and I don't see a particularly good reason to allow it. Well
>> I guess it trades some complexity in state trackers with some complexity
>> in drivers...
> 
> fwiw, going with the idea that it should be legal, I fixed that in the
> drivers that didn't handle it in:
> 
> https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgitlab.freedesktop.org%2Fmesa%2Fmesa%2Fmerge_requests%2F449data=02%7C01%7Csroland%40vmware.com%7C503a661358114ccf08d208d6a8c0eadc%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636881943862256444sdata=4c6ehFiS676ZwbneR6T0CBhBHHq7zoL5efQ7E9e%2Fd9E%3Dreserved=0
> 
> (planning to send to list, I just pushed a WIP MR to run it thru the CI 
> system)

I'm pretty sure both softpipe and llvmpipe would crash too, they
dereference this without checking if it's null.
So effectively all drivers but one thought it was illegal?
I still see no point in allowing it (or rather, changing this to be
allowed - per se there's nothing really wrong with this to be allowed).
That said, it appears that set_shader_images and set_shader_buffers
allow it, so there's some precedence for this.

> 
> I was on the fence between making st handle it vs making driver handle
> it, but it is trivial for the driver to handle it if it knows it is
> supposed to.  And I had to fixup the drivers for various things
> already (most hadn't been updated to handle the `start_slot` param,
> for ex).
Yes, I think in particular because when going through cso things will
always start at slot 0, so some drivers got sloppy...
But well for views not being allowed to be null that's also pretty
trivial for state trackers to handle...

> 
> Eric suggested (on the MR) introducing a helper for this, which might
> be a better approach to cut down on the boilerplate..  I'll play with
> that idea.
> 
> (btw, from a quick look, set_sampler_views isn't the only problem
> slot.. I noticed set_shader_buffers has the same issue.. but maybe
> I'll try to fix one thing at a time and worry more about that when
> panfrost or etnaviv gets closer to the point of supporting SSBO and
> compute shaders..)
Both set_shader_buffers and set_shader_images say it's allowed to be
null (as per their function description in p_context.h). They are much
newer functions though and not everybody implements them, so there might
not be much of an issue there (though it doesn't actually look mesa/st
would make use of that neither, so maybe it isn't all that useful...).

Roland


> 
> BR,
> -R
> 
>>
>> Roland
>>
>>
>>
>>> BR,
>>> -R
>>>


>
> I think radeonsi and freedreno were the only ones not doing this.  Which
> could probably temporarily leak a 

Re: [Mesa-dev] [PATCH v5 00/11] anv: Implement VK_EXT_sample_locations

[Jason Ekstrand]

On Thu, Mar 14, 2019 at 7:08 PM Jason Ekstrand  wrote:

> Eleni,
>
> I made a comment on one of your patches and then I started getting lost in
> the churn.  It's easier to see what's going on if things are reordered and
> refactored a bit.  Unfortunately, what needed to be done was hard to
> explain and it was easier for me to just do the code motion and re-send the
> series.  I hope you understand.  This is one of those cases where it's
> easier for me to write code than English.
>
> One of the results of this refactor is that gen8 and gen7 changes happen at
> the same time.
>
> --Jason
>
>
>
> Eleni Maria Stea (9):
>   anv: Added the VK_EXT_sample_locations extension to the anv_extensions
> list
>   anv: Set the values for the
> VkPhysicalDeviceSampleLocationsPropertiesEXT
>   anv: Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT
>   anv/state: Take explicit sample locations in emit helpers
>   anv: Add support for non-dynamic sample locations
>

The above patches are all

Reviewed-by: Jason Ekstrand 


>   anv: Added support for dynamic sample locations
>   anv: Optimized the emission of the default locations on Gen8+
>

I've got some concerns about these two.


>   anv: Removed unused header file
>   anv: Enabled the VK_EXT_sample_locations extension
>

These two are also

Reviewed-by: Jason Ekstrand 


>
> Jason Ekstrand (2):
>   anv/pipeline: Refactor 3DSTATE_SAMPLE_MASK setup
>   anv: Break SAMPLE_PATTERN and MULTISAMPLE emit into helpers
>
>  src/intel/common/gen_sample_positions.h |  57 +
>  src/intel/vulkan/anv_cmd_buffer.c   |  17 +++
>  src/intel/vulkan/anv_device.c   |  45 
>  src/intel/vulkan/anv_extensions.py  |   1 +
>  src/intel/vulkan/anv_genX.h |   6 +
>  src/intel/vulkan/anv_private.h  |   7 ++
>  src/intel/vulkan/genX_blorp_exec.c  |   1 -
>  src/intel/vulkan/genX_cmd_buffer.c  |  12 ++
>  src/intel/vulkan/genX_pipeline.c|  93 +++
>  src/intel/vulkan/genX_state.c   | 147 
>  10 files changed, 315 insertions(+), 71 deletions(-)
>
> --
> 2.20.1
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 02/11] anv: Set the values for the VkPhysicalDeviceSampleLocationsPropertiesEXT

[Jason Ekstrand]

From: Eleni Maria Stea 

The VkPhysicalDeviceSampleLocationPropertiesEXT struct is filled with
implementation dependent values and according to the table from the
Vulkan Specification section [36.1. Limit Requirements]:

pname | max | min
pname:sampleLocationSampleCounts   |-|ename:VK_SAMPLE_COUNT_4_BIT
pname:maxSampleLocationGridSize|-|(1, 1)
pname:sampleLocationCoordinateRange|(0.0, 0.9375)|(0.0, 0.9375)
pname:sampleLocationSubPixelBits   |-|4
pname:variableSampleLocations  | false   |implementation dependent

The hardware only supports setting the same sample location for all the
pixels, so we only support 1x1 grids.

Also, variableSampleLocations is set to false because we don't support the
feature.

v2: 1- Replaced false with VK_FALSE for consistency. (Sagar Ghuge)
2- Used the isl_device_sample_count to take the number of samples
per platform to avoid extra checks. (Sagar Ghuge)

v3: 1- Replaced VK_FALSE with false as Jason has sent a patch to replace
VK_FALSE with false in other places. (Jason Ekstrand)
2- Removed unecessary defines and set the grid size to 1 (Jason Ekstrand)

Reviewed-by: Sagar Ghuge 
---
 src/intel/vulkan/anv_device.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 83fa3936c19..52ea058bdd5 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1401,6 +1401,26 @@ void anv_GetPhysicalDeviceProperties2(
  break;
   }
 
+  case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SAMPLE_LOCATIONS_PROPERTIES_EXT: {
+ VkPhysicalDeviceSampleLocationsPropertiesEXT *props =
+(VkPhysicalDeviceSampleLocationsPropertiesEXT *)ext;
+
+ props->sampleLocationSampleCounts =
+isl_device_get_sample_counts(>isl_dev);
+
+ /* See also anv_GetPhysicalDeviceMultisamplePropertiesEXT */
+ props->maxSampleLocationGridSize.width = 1;
+ props->maxSampleLocationGridSize.height = 1;
+
+ props->sampleLocationCoordinateRange[0] = 0;
+ props->sampleLocationCoordinateRange[1] = 0.9375;
+ props->sampleLocationSubPixelBits = 4;
+
+ props->variableSampleLocations = false;
+
+ break;
+  }
+
   default:
  anv_debug_ignored_stype(ext->sType);
  break;
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 03/11] anv: Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT

[Jason Ekstrand]

From: Eleni Maria Stea 

Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT according to
the Vulkan Specification section [36.2. Additional Multisampling
Capabilities].

v2: 1- Moved the vkGetPhysicalDeviceMultisamplePropertiesEXT from the
   anv_sample_locations.c to the anv_device.c (Jason Ekstrand)
2- Simplified the code that sets the grid size (Jason Ekstrand)
3- Instead of filling the whole struct, we only fill the parts we
   should override (sType, grid size) and we call
   anv_debug_ignored_stype to any pNext elements (Jason Ekstrand)
---
 src/intel/vulkan/anv_device.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 52ea058bdd5..0bfff7e0b30 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -3557,6 +3557,31 @@ VkResult anv_GetCalibratedTimestampsEXT(
return VK_SUCCESS;
 }
 
+void
+anv_GetPhysicalDeviceMultisamplePropertiesEXT(VkPhysicalDevice physicalDevice,
+  VkSampleCountFlagBits samples,
+  VkMultisamplePropertiesEXT
+  *pMultisampleProperties)
+{
+   ANV_FROM_HANDLE(anv_physical_device, physical_device, physicalDevice);
+
+   VkExtent2D grid_size;
+   if (samples & isl_device_get_sample_counts(_device->isl_dev)) {
+  grid_size.width = 1;
+  grid_size.height = 1;
+   } else {
+  grid_size.width = 0;
+  grid_size.height = 0;
+   }
+
+   pMultisampleProperties->sType =
+  VK_STRUCTURE_TYPE_MULTISAMPLE_PROPERTIES_EXT;
+   pMultisampleProperties->maxSampleLocationGridSize = grid_size;
+
+   vk_foreach_struct(ext, pMultisampleProperties->pNext)
+  anv_debug_ignored_stype(ext->sType);
+}
+
 /* vk_icd.h does not declare this function, so we declare it here to
  * suppress Wmissing-prototypes.
  */
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 10/11] anv: Removed unused header file

[Jason Ekstrand]

From: Eleni Maria Stea 

In src/intel/vulkan/genX_blorp_exec.c we included the file:
common/gen_sample_positions.h but not use it. Removed.

Reviewed-by: Sagar Ghuge 
Reviewed-by: Jason Ekstrand 
---
 src/intel/vulkan/genX_blorp_exec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/intel/vulkan/genX_blorp_exec.c 
b/src/intel/vulkan/genX_blorp_exec.c
index e9c85d56d5f..0eeefaaa9d6 100644
--- a/src/intel/vulkan/genX_blorp_exec.c
+++ b/src/intel/vulkan/genX_blorp_exec.c
@@ -31,7 +31,6 @@
 #undef __gen_combine_address
 
 #include "common/gen_l3_config.h"
-#include "common/gen_sample_positions.h"
 #include "blorp/blorp_genX_exec.h"
 
 static void *
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 05/11] anv: Break SAMPLE_PATTERN and MULTISAMPLE emit into helpers

[Jason Ekstrand]

---
 src/intel/vulkan/anv_genX.h  |  4 ++
 src/intel/vulkan/genX_pipeline.c | 40 +--
 src/intel/vulkan/genX_state.c| 84 
 3 files changed, 70 insertions(+), 58 deletions(-)

diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
index 8fd32cabf1e..55226179124 100644
--- a/src/intel/vulkan/anv_genX.h
+++ b/src/intel/vulkan/anv_genX.h
@@ -74,6 +74,10 @@ genX(emit_urb_setup)(struct anv_device *device, struct 
anv_batch *batch,
  VkShaderStageFlags active_stages,
  const unsigned entry_size[4]);
 
+void genX(emit_multisample)(struct anv_batch *batch, uint32_t samples);
+
+void genX(emit_sample_pattern)(struct anv_batch *batch);
+
 void genX(cmd_buffer_so_memcpy)(struct anv_cmd_buffer *cmd_buffer,
 struct anv_address dst, struct anv_address src,
 uint32_t size);
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 8ba206ed8c4..7eb05333dad 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -551,45 +551,9 @@ static void
 emit_ms_state(struct anv_pipeline *pipeline,
   const VkPipelineMultisampleStateCreateInfo *info)
 {
-   uint32_t samples = 1;
-   uint32_t log2_samples = 0;
+   uint32_t samples = info ? info->rasterizationSamples : 1;
 
-   if (info) {
-  samples = info->rasterizationSamples;
-  log2_samples = __builtin_ffs(samples) - 1;
-   }
-
-   anv_batch_emit(>batch, GENX(3DSTATE_MULTISAMPLE), ms) {
-  ms.NumberofMultisamples   = log2_samples;
-
-  ms.PixelLocation  = CENTER;
-#if GEN_GEN >= 8
-  /* The PRM says that this bit is valid only for DX9:
-   *
-   *SW can choose to set this bit only for DX9 API. DX10/OGL API's
-   *should not have any effect by setting or not setting this bit.
-   */
-  ms.PixelPositionOffsetEnable  = false;
-#else
-
-  switch (samples) {
-  case 1:
- GEN_SAMPLE_POS_1X(ms.Sample);
- break;
-  case 2:
- GEN_SAMPLE_POS_2X(ms.Sample);
- break;
-  case 4:
- GEN_SAMPLE_POS_4X(ms.Sample);
- break;
-  case 8:
- GEN_SAMPLE_POS_8X(ms.Sample);
- break;
-  default:
- break;
-  }
-#endif
-   }
+   genX(emit_multisample)(>batch, samples);
 
anv_batch_emit(>batch, GENX(3DSTATE_SAMPLE_MASK), sm) {
   /* From the Vulkan 1.0 spec:
diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
index cffd1e47247..cc066e68f2c 100644
--- a/src/intel/vulkan/genX_state.c
+++ b/src/intel/vulkan/genX_state.c
@@ -139,22 +139,7 @@ genX(init_device_state)(struct anv_device *device)
 #if GEN_GEN >= 8
anv_batch_emit(, GENX(3DSTATE_WM_CHROMAKEY), ck);
 
-#if GEN_GEN == 10
-   gen10_emit_wa_cs_stall_flush();
-#endif
-
-   /* See the Vulkan 1.0 spec Table 24.1 "Standard sample locations" and
-* VkPhysicalDeviceFeatures::standardSampleLocations.
-*/
-   anv_batch_emit(, GENX(3DSTATE_SAMPLE_PATTERN), sp) {
-  GEN_SAMPLE_POS_1X(sp._1xSample);
-  GEN_SAMPLE_POS_2X(sp._2xSample);
-  GEN_SAMPLE_POS_4X(sp._4xSample);
-  GEN_SAMPLE_POS_8X(sp._8xSample);
-#if GEN_GEN >= 9
-  GEN_SAMPLE_POS_16X(sp._16xSample);
-#endif
-   }
+   genX(emit_sample_pattern)();
 
/* The BDW+ docs describe how to use the 3DSTATE_WM_HZ_OP instruction in the
 * section titled, "Optimized Depth Buffer Clear and/or Stencil Buffer
@@ -167,10 +152,6 @@ genX(init_device_state)(struct anv_device *device)
anv_batch_emit(, GENX(3DSTATE_WM_HZ_OP), hzp);
 #endif
 
-#if GEN_GEN == 10
-   gen10_emit_wa_lri_to_cache_mode_zero();
-#endif
-
 #if GEN_GEN == 11
/* The default behavior of bit 5 "Headerless Message for Pre-emptable
 * Contexts" in SAMPLER MODE register is set to 0, which means
@@ -236,6 +217,69 @@ genX(init_device_state)(struct anv_device *device)
return anv_device_submit_simple_batch(device, );
 }
 
+void
+genX(emit_multisample)(struct anv_batch *batch, uint32_t samples)
+{
+   anv_batch_emit(batch, GENX(3DSTATE_MULTISAMPLE), ms) {
+  ms.NumberofMultisamples   = __builtin_ffs(samples) - 1;
+
+  ms.PixelLocation  = CENTER;
+#if GEN_GEN >= 8
+  /* The PRM says that this bit is valid only for DX9:
+   *
+   *SW can choose to set this bit only for DX9 API. DX10/OGL API's
+   *should not have any effect by setting or not setting this bit.
+   */
+  ms.PixelPositionOffsetEnable  = false;
+#else
+
+  switch (samples) {
+  case 1:
+ GEN_SAMPLE_POS_1X(ms.Sample);
+ break;
+  case 2:
+ GEN_SAMPLE_POS_2X(ms.Sample);
+ break;
+  case 4:
+ GEN_SAMPLE_POS_4X(ms.Sample);
+ break;
+  case 8:
+ GEN_SAMPLE_POS_8X(ms.Sample);
+ break;
+  default:
+ break;
+  }
+#endif
+   }
+}
+
+#if GEN_GEN >= 8
+void

[Mesa-dev] [PATCH v5 09/11] anv: Optimized the emission of the default locations on Gen8+

[Jason Ekstrand]

From: Eleni Maria Stea 

We only emit sample locations when the extension is enabled by the user.
In all other cases the default locations are emitted once when the device
is initialized to increase performance.
---
 src/intel/vulkan/genX_cmd_buffer.c | 1 +
 src/intel/vulkan/genX_pipeline.c   | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index 5d2b17cf8ae..ca684f6aa72 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -2797,6 +2797,7 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
   gen7_cmd_buffer_emit_scissor(cmd_buffer);
 
if (cmd_buffer->state.gfx.dynamic.sample_locations.valid) {
+  assert(cmd_buffer->device->enabled_extensions.EXT_sample_locations);
   uint32_t samples = 
cmd_buffer->state.gfx.dynamic.sample_locations.samples;
   const VkSampleLocationEXT *locations =
  cmd_buffer->state.gfx.dynamic.sample_locations.locations;
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 828fcb17fcc..ffd5c7c6409 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -580,7 +580,8 @@ emit_ms_state(struct anv_pipeline *pipeline,
}
 
 #if GEN_GEN >= 8
-   genX(emit_sample_pattern)(>batch, samples, locations);
+   if (pipeline->device->enabled_extensions.EXT_sample_locations)
+  genX(emit_sample_pattern)(>batch, samples, locations);
 #endif
 
genX(emit_multisample)(>batch, samples, locations);
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 07/11] anv: Add support for non-dynamic sample locations

[Jason Ekstrand]

From: Eleni Maria Stea 

Allowing the user to set custom sample locations non-dynamically, by
filling the extension structs and chaining them to the pipeline structs
according to the Vulkan specification section [26.5. Custom Sample
Locations] for the following structures:

'VkPipelineSampleLocationsStateCreateInfoEXT'
'VkSampleLocationsInfoEXT'
'VkSampleLocationEXT'

Once custom locations are used, the default locations are lost and need
to be re-emitted again in the next pipeline creation. For that, we emit
the 3DSTATE_SAMPLE_PATTERN at every pipeline creation.

v2: In v1, we used the custom anv_sample struct to store the location
and the distance from the pixel center because we would then use
this distance to sort the locations and send them in increasing
monotonical order to the GPU. That was because the Skylake PRM Vol.
2a "3DSTATE_SAMPLE_PATTERN" says that the samples must have
monotonically increasing distance from the pixel center to get the
correct centroid computation in the device. However, the Vulkan
spec seems to require that the samples occur in the order provided
through the API and this requirement is only for the standard
locations. As long as this only affects centroid calculations as
the docs say, we should be ok because OpenGL and Vulkan only
require that the centroid be some lit sample and that it's the same
for all samples in a pixel; they have no requirement that it be the
one closest to center. (Jason Ekstrand)
For that we made the following changes:
1- We removed the custom structs and functions from anv_private.h
   and anv_sample_locations.h and anv_sample_locations.c (the last
   two files were removed). (Jason Ekstrand)
2- We modified the macros used to take also the array as parameter
   and we renamed them to start by GEN_. (Jason Ekstrand)
3- We don't sort the samples anymore. (Jason Ekstrand)

v3 (Jason Ekstrand):
Break the refactoring out into multiple commits
---
 src/intel/vulkan/anv_private.h   |  1 +
 src/intel/vulkan/genX_pipeline.c | 37 +---
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index eed282ff985..a39195733cd 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -165,6 +165,7 @@ struct gen_l3_config;
 #define MAX_PUSH_DESCRIPTORS 32 /* Minimum requirement */
 #define MAX_INLINE_UNIFORM_BLOCK_SIZE 4096
 #define MAX_INLINE_UNIFORM_BLOCK_DESCRIPTORS 32
+#define MAX_SAMPLE_LOCATIONS 16
 
 /* The kernel relocation API has a limitation of a 32-bit delta value
  * applied to the address before it is written which, in spite of it being
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 430b095c5b1..828fcb17fcc 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -547,13 +547,43 @@ emit_rs_state(struct anv_pipeline *pipeline,
 #endif
 }
 
+static bool
+is_dynamic_state(VkDynamicState state,
+ const VkPipelineDynamicStateCreateInfo *dinfo)
+{
+   for (uint32_t i = 0; i < dinfo->dynamicStateCount; i++) {
+  if (state == dinfo->pDynamicStates[i])
+ return true;
+   }
+
+   return false;
+}
+
 static void
 emit_ms_state(struct anv_pipeline *pipeline,
-  const VkPipelineMultisampleStateCreateInfo *info)
+  const VkPipelineMultisampleStateCreateInfo *info,
+  const VkPipelineDynamicStateCreateInfo *dinfo)
 {
uint32_t samples = info ? info->rasterizationSamples : 1;
 
-   genX(emit_multisample)(>batch, samples, NULL);
+   const VkSampleLocationEXT *locations = NULL;
+   const VkPipelineSampleLocationsStateCreateInfoEXT *sl_info =
+  vk_find_struct_const(info, 
PIPELINE_SAMPLE_LOCATIONS_STATE_CREATE_INFO_EXT);
+   if (sl_info && sl_info->sampleLocationsEnable &&
+   !is_dynamic_state(VK_DYNAMIC_STATE_SAMPLE_LOCATIONS_EXT, dinfo)) {
+
+  assert(sl_info->sampleLocationsInfo.sampleLocationsPerPixel == samples);
+  assert(sl_info->sampleLocationsInfo.sampleLocationGridSize.width == 1);
+  assert(sl_info->sampleLocationsInfo.sampleLocationGridSize.height == 1);
+  assert(sl_info->sampleLocationsInfo.sampleLocationsCount == samples);
+  locations = sl_info->sampleLocationsInfo.pSampleLocations;
+   }
+
+#if GEN_GEN >= 8
+   genX(emit_sample_pattern)(>batch, samples, locations);
+#endif
+
+   genX(emit_multisample)(>batch, samples, locations);
 
anv_batch_emit(>batch, GENX(3DSTATE_SAMPLE_MASK), sm) {
   /* From the Vulkan 1.0 spec:
@@ -1899,7 +1929,8 @@ genX(graphics_pipeline_create)(
assert(pCreateInfo->pRasterizationState);
emit_rs_state(pipeline, pCreateInfo->pRasterizationState,
  pCreateInfo->pMultisampleState, pass, subpass);
-   emit_ms_state(pipeline, pCreateInfo->pMultisampleState);
+   emit_ms_state(pipeline, pCreateInfo->pMultisampleState,
+  

[Mesa-dev] [PATCH v5 01/11] anv: Added the VK_EXT_sample_locations extension to the anv_extensions list

[Jason Ekstrand]

From: Eleni Maria Stea 

Added the VK_EXT_sample_locations to the anv_extensions.py list to
generate the related entrypoints.

Reviewed-by: Sagar Ghuge 
---
 src/intel/vulkan/anv_extensions.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/intel/vulkan/anv_extensions.py 
b/src/intel/vulkan/anv_extensions.py
index 6fff293dee4..9e4e03e46df 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -129,6 +129,7 @@ EXTENSIONS = [
 Extension('VK_EXT_inline_uniform_block',  1, True),
 Extension('VK_EXT_pci_bus_info',  2, True),
 Extension('VK_EXT_post_depth_coverage',   1, 'device->info.gen 
>= 9'),
+Extension('VK_EXT_sample_locations',  1, False),
 Extension('VK_EXT_sampler_filter_minmax', 1, 'device->info.gen 
>= 9'),
 Extension('VK_EXT_scalar_block_layout',   1, True),
 Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen 
>= 9'),
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 00/11] anv: Implement VK_EXT_sample_locations

[Jason Ekstrand]

Eleni,

I made a comment on one of your patches and then I started getting lost in
the churn.  It's easier to see what's going on if things are reordered and
refactored a bit.  Unfortunately, what needed to be done was hard to
explain and it was easier for me to just do the code motion and re-send the
series.  I hope you understand.  This is one of those cases where it's
easier for me to write code than English.

One of the results of this refactor is that gen8 and gen7 changes happen at
the same time.

--Jason



Eleni Maria Stea (9):
  anv: Added the VK_EXT_sample_locations extension to the anv_extensions
list
  anv: Set the values for the
VkPhysicalDeviceSampleLocationsPropertiesEXT
  anv: Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT
  anv/state: Take explicit sample locations in emit helpers
  anv: Add support for non-dynamic sample locations
  anv: Added support for dynamic sample locations
  anv: Optimized the emission of the default locations on Gen8+
  anv: Removed unused header file
  anv: Enabled the VK_EXT_sample_locations extension

Jason Ekstrand (2):
  anv/pipeline: Refactor 3DSTATE_SAMPLE_MASK setup
  anv: Break SAMPLE_PATTERN and MULTISAMPLE emit into helpers

 src/intel/common/gen_sample_positions.h |  57 +
 src/intel/vulkan/anv_cmd_buffer.c   |  17 +++
 src/intel/vulkan/anv_device.c   |  45 
 src/intel/vulkan/anv_extensions.py  |   1 +
 src/intel/vulkan/anv_genX.h |   6 +
 src/intel/vulkan/anv_private.h  |   7 ++
 src/intel/vulkan/genX_blorp_exec.c  |   1 -
 src/intel/vulkan/genX_cmd_buffer.c  |  12 ++
 src/intel/vulkan/genX_pipeline.c|  93 +++
 src/intel/vulkan/genX_state.c   | 147 
 10 files changed, 315 insertions(+), 71 deletions(-)

-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 11/11] anv: Enabled the VK_EXT_sample_locations extension

[Jason Ekstrand]

From: Eleni Maria Stea 

Enabled the VK_EXT_sample_locations for Intel Gen >= 7.

v2: Replaced device.info->gen >= 7 with True, as Anv doesn't support
anything below Gen7. (Lionel Landwerlin)

Reviewed-by: Sagar Ghuge 
---
 src/intel/vulkan/anv_extensions.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/intel/vulkan/anv_extensions.py 
b/src/intel/vulkan/anv_extensions.py
index 9e4e03e46df..5a30c733c5c 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -129,7 +129,7 @@ EXTENSIONS = [
 Extension('VK_EXT_inline_uniform_block',  1, True),
 Extension('VK_EXT_pci_bus_info',  2, True),
 Extension('VK_EXT_post_depth_coverage',   1, 'device->info.gen 
>= 9'),
-Extension('VK_EXT_sample_locations',  1, False),
+Extension('VK_EXT_sample_locations',  1, True),
 Extension('VK_EXT_sampler_filter_minmax', 1, 'device->info.gen 
>= 9'),
 Extension('VK_EXT_scalar_block_layout',   1, True),
 Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen 
>= 9'),
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 06/11] anv/state: Take explicit sample locations in emit helpers

[Jason Ekstrand]

From: Eleni Maria Stea 

This commit adds a "locations" parameter to emit_multisample and
emit_sample_pattern which, if provided, will override the default
sample locations.
---
 src/intel/common/gen_sample_positions.h |  57 +
 src/intel/vulkan/anv_genX.h |   6 +-
 src/intel/vulkan/genX_pipeline.c|   2 +-
 src/intel/vulkan/genX_state.c   | 109 +++-
 4 files changed, 148 insertions(+), 26 deletions(-)

diff --git a/src/intel/common/gen_sample_positions.h 
b/src/intel/common/gen_sample_positions.h
index da48dcb5ed0..850661931cf 100644
--- a/src/intel/common/gen_sample_positions.h
+++ b/src/intel/common/gen_sample_positions.h
@@ -160,4 +160,61 @@ prefix##14YOffset  = 0.9375; \
 prefix##15XOffset  = 0.0625; \
 prefix##15YOffset  = 0.;
 
+/* Examples:
+ * in case of GEN_GEN < 8:
+ * GEN_SAMPLE_POS_ELEM(ms.Sample, info->pSampleLocations, 0); expands to:
+ *ms.Sample0XOffset = info->pSampleLocations[0].pos.x;
+ *ms.Sample0YOffset = info->pSampleLocations[0].y;
+ *
+ * in case of GEN_GEN >= 8:
+ * GEN_SAMPLE_POS_ELEM(sp._16xSample, info->pSampleLocations, 0); expands to:
+ *sp._16xSample0XOffset = info->pSampleLocations[0].x;
+ *sp._16xSample0YOffset = info->pSampleLocations[0].y;
+ */
+
+#define GEN_SAMPLE_POS_ELEM(prefix, arr, sample_idx) \
+prefix##sample_idx##XOffset = arr[sample_idx].x; \
+prefix##sample_idx##YOffset = arr[sample_idx].y;
+
+#define GEN_SAMPLE_POS_1X_ARRAY(prefix, arr)\
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0);
+
+#define GEN_SAMPLE_POS_2X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1);
+
+#define GEN_SAMPLE_POS_4X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 3);
+
+#define GEN_SAMPLE_POS_8X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 3); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 4); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 5); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 6); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 7);
+
+#define GEN_SAMPLE_POS_16X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 3); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 4); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 5); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 6); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 7); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 8); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 9); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 10); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 11); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 12); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 13); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 14); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 15);
+
 #endif /* GEN_SAMPLE_POSITIONS_H */
diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
index 55226179124..608954d1a2c 100644
--- a/src/intel/vulkan/anv_genX.h
+++ b/src/intel/vulkan/anv_genX.h
@@ -74,9 +74,11 @@ genX(emit_urb_setup)(struct anv_device *device, struct 
anv_batch *batch,
  VkShaderStageFlags active_stages,
  const unsigned entry_size[4]);
 
-void genX(emit_multisample)(struct anv_batch *batch, uint32_t samples);
+void genX(emit_multisample)(struct anv_batch *batch, uint32_t samples,
+const VkSampleLocationEXT *locations);
 
-void genX(emit_sample_pattern)(struct anv_batch *batch);
+void genX(emit_sample_pattern)(struct anv_batch *batch, uint32_t samples,
+   const VkSampleLocationEXT *locations);
 
 void genX(cmd_buffer_so_memcpy)(struct anv_cmd_buffer *cmd_buffer,
 struct anv_address dst, struct anv_address src,
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 7eb05333dad..430b095c5b1 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -553,7 +553,7 @@ emit_ms_state(struct anv_pipeline *pipeline,
 {
uint32_t samples = info ? info->rasterizationSamples : 1;
 
-   genX(emit_multisample)(>batch, samples);
+   genX(emit_multisample)(>batch, samples, NULL);
 
anv_batch_emit(>batch, GENX(3DSTATE_SAMPLE_MASK), sm) {
   /* From the Vulkan 1.0 spec:
diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
index cc066e68f2c..4054f9c6024 100644
--- a/src/intel/vulkan/genX_state.c
+++ b/src/intel/vulkan/genX_state.c
@@ -139,7 +139,7 @@ genX(init_device_state)(struct anv_device *device)
 #if GEN_GEN >= 8
anv_batch_emit(, GENX(3DSTATE_WM_CHROMAKEY), ck);
 
-   genX(emit_sample_pattern)();
+   genX(emit_sample_pattern)(, 0, NULL);
 
/* The BDW+ docs describe how to use the 3DSTATE_WM_HZ_OP instruction in the
 * section titled, "Optimized Depth Buffer Clear and/or Stencil Buffer

[Mesa-dev] [PATCH v5 08/11] anv: Added support for dynamic sample locations

[Jason Ekstrand]

From: Eleni Maria Stea 

Added support for setting the locations when the pipeline has been
created with the dynamic state bit enabled according to the Vulkan
Specification section [26.5. Custom Sample Locations] for the function:

'vkCmdSetSampleLocationsEXT'

The reason that we preferred to store the boolean valid inside the
dynamic state struct for locations instead of using a dirty bit
(ANV_CMD_DIRTY_SAMPLE_LOCATIONS for example) is that other functions
can modify the value of the dirty bits causing unexpected behavior.

v2: Removed all the anv* structs used with sample locations to store
the locations in order for dynamic case. (see also the patch for
the non-dynamic case. (Jason Ekstrand)
---
 src/intel/vulkan/anv_cmd_buffer.c  | 17 +
 src/intel/vulkan/anv_private.h |  6 ++
 src/intel/vulkan/genX_cmd_buffer.c | 11 +++
 3 files changed, 34 insertions(+)

diff --git a/src/intel/vulkan/anv_cmd_buffer.c 
b/src/intel/vulkan/anv_cmd_buffer.c
index 1b34644a434..7f31300857b 100644
--- a/src/intel/vulkan/anv_cmd_buffer.c
+++ b/src/intel/vulkan/anv_cmd_buffer.c
@@ -558,6 +558,23 @@ void anv_CmdSetStencilReference(
cmd_buffer->state.gfx.dirty |= ANV_CMD_DIRTY_DYNAMIC_STENCIL_REFERENCE;
 }
 
+void
+anv_CmdSetSampleLocationsEXT(VkCommandBuffer commandBuffer,
+ const VkSampleLocationsInfoEXT 
*pSampleLocationsInfo)
+{
+   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
+
+   struct anv_dynamic_state *dyn_state = _buffer->state.gfx.dynamic;
+   uint32_t samples = pSampleLocationsInfo->sampleLocationsPerPixel;
+
+   assert(pSampleLocationsInfo);
+   dyn_state->sample_locations.samples = samples;
+   typed_memcpy(dyn_state->sample_locations.locations,
+pSampleLocationsInfo->pSampleLocations, samples);
+
+   dyn_state->sample_locations.valid = true;
+}
+
 static void
 anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
VkPipelineBindPoint bind_point,
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index a39195733cd..94c9db14028 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -2124,6 +2124,12 @@ struct anv_dynamic_state {
   uint32_t  front;
   uint32_t  back;
} stencil_reference;
+
+   struct {
+  bool  valid;
+  uint32_t  samples;
+  VkSampleLocationEXT   
locations[MAX_SAMPLE_LOCATIONS];
+   } sample_locations;
 };
 
 extern const struct anv_dynamic_state default_dynamic_state;
diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index 7687507e6b7..5d2b17cf8ae 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -2796,6 +2796,17 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
   ANV_CMD_DIRTY_RENDER_TARGETS))
   gen7_cmd_buffer_emit_scissor(cmd_buffer);
 
+   if (cmd_buffer->state.gfx.dynamic.sample_locations.valid) {
+  uint32_t samples = 
cmd_buffer->state.gfx.dynamic.sample_locations.samples;
+  const VkSampleLocationEXT *locations =
+ cmd_buffer->state.gfx.dynamic.sample_locations.locations;
+  genX(emit_multisample)(_buffer->batch, samples, locations);
+#if GEN_GEN >= 8
+  genX(emit_sample_pattern)(_buffer->batch, samples, locations);
+#endif
+  cmd_buffer->state.gfx.dynamic.sample_locations.valid = false;
+   }
+
genX(cmd_buffer_flush_dynamic_state)(cmd_buffer);
 
genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v5 04/11] anv/pipeline: Refactor 3DSTATE_SAMPLE_MASK setup

[Jason Ekstrand]

---
 src/intel/vulkan/genX_pipeline.c | 29 +
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 975052deb79..8ba206ed8c4 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -554,26 +554,11 @@ emit_ms_state(struct anv_pipeline *pipeline,
uint32_t samples = 1;
uint32_t log2_samples = 0;
 
-   /* From the Vulkan 1.0 spec:
-*If pSampleMask is NULL, it is treated as if the mask has all bits
-*enabled, i.e. no coverage is removed from fragments.
-*
-* 3DSTATE_SAMPLE_MASK.SampleMask is 16 bits.
-*/
-#if GEN_GEN >= 8
-   uint32_t sample_mask = 0x;
-#else
-   uint32_t sample_mask = 0xff;
-#endif
-
if (info) {
   samples = info->rasterizationSamples;
   log2_samples = __builtin_ffs(samples) - 1;
}
 
-   if (info && info->pSampleMask)
-  sample_mask &= info->pSampleMask[0];
-
anv_batch_emit(>batch, GENX(3DSTATE_MULTISAMPLE), ms) {
   ms.NumberofMultisamples   = log2_samples;
 
@@ -607,7 +592,19 @@ emit_ms_state(struct anv_pipeline *pipeline,
}
 
anv_batch_emit(>batch, GENX(3DSTATE_SAMPLE_MASK), sm) {
-  sm.SampleMask = sample_mask;
+  /* From the Vulkan 1.0 spec:
+   *If pSampleMask is NULL, it is treated as if the mask has all bits
+   *enabled, i.e. no coverage is removed from fragments.
+   *
+   * 3DSTATE_SAMPLE_MASK.SampleMask is 16 bits.
+   */
+#if GEN_GEN >= 8
+  sm.SampleMask = 0x;
+#else
+  sm.SampleMask = 0xff;
+#endif
+  if (info && info->pSampleMask)
+ sm.SampleMask &= info->pSampleMask[0];
}
 }
 
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 100316] Linking GLSL 1.30 shaders with invariant and deprecated variables triggers an 'mismatching invariant qualifiers' error

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=100316

--- Comment #4 from Jordan Justen  ---
(In reply to Danylo from comment #3)
> I've sent the patch for more general manifestation of the issue:
> https://patchwork.freedesktop.org/patch/247067/

Just noting that Danylo submitted a merge request for this patch:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/84

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 8/8] gallium/util: remove pipe_sampler_view_release()

[Neha Bhende]

Looks great!

For series,
Reviewed-by: Neha Bhende 

Regards,
Neha


From: Brian Paul 
Sent: Thursday, March 14, 2019 12:37 PM
To: mesa-dev@lists.freedesktop.org
Cc: Neha Bhende; Jose Fonseca; Roland Scheidegger; Mathias Fröhlich
Subject: [PATCH 8/8] gallium/util: remove pipe_sampler_view_release()

It's no longer used.
---
 src/gallium/auxiliary/util/u_inlines.h | 20 
 1 file changed, 20 deletions(-)

diff --git a/src/gallium/auxiliary/util/u_inlines.h 
b/src/gallium/auxiliary/util/u_inlines.h
index fa1e920..567d3d0 100644
--- a/src/gallium/auxiliary/util/u_inlines.h
+++ b/src/gallium/auxiliary/util/u_inlines.h
@@ -192,26 +192,6 @@ pipe_sampler_view_reference(struct pipe_sampler_view **dst,
*dst = src;
 }

-/**
- * Similar to pipe_sampler_view_reference() but always set the pointer to
- * NULL and pass in the current context explicitly.
- *
- * If *ptr is non-NULL, it may refer to a view that was created in a different
- * context (however, that context must still be alive).
- */
-static inline void
-pipe_sampler_view_release(struct pipe_context *ctx,
-  struct pipe_sampler_view **ptr)
-{
-   struct pipe_sampler_view *old_view = *ptr;
-
-   if (pipe_reference_described(_view->reference, NULL,
-(debug_reference_descriptor)debug_describe_sampler_view)) {
-  ctx->sampler_view_destroy(ctx, old_view);
-   }
-   *ptr = NULL;
-}
-
 static inline void
 pipe_so_target_reference(struct pipe_stream_output_target **dst,
  struct pipe_stream_output_target *src)
--
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/u_transfer_helper: do not call resource_create(..) directly

[Eric Anholt]

Rob Clark  writes:

> On Thu, Mar 14, 2019 at 3:35 PM Eric Anholt  wrote:
>>
>> Rob Clark  writes:
>>
>> > On Fri, Mar 1, 2019 at 10:54 AM Christian Gmeiner
>> >  wrote:
>> >>
>> >> Use u_transfer_helper_resource_create(..) instead which uses the
>> >> resource_create(..) function specified in u_transfer_vtbl.
>> >>
>> >> Signed-off-by: Christian Gmeiner 
>> >> ---
>> >>  src/gallium/auxiliary/util/u_transfer_helper.c | 2 +-
>> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> >>
>> >> diff --git a/src/gallium/auxiliary/util/u_transfer_helper.c 
>> >> b/src/gallium/auxiliary/util/u_transfer_helper.c
>> >> index 14c4d56392d..a5c3c026e71 100644
>> >> --- a/src/gallium/auxiliary/util/u_transfer_helper.c
>> >> +++ b/src/gallium/auxiliary/util/u_transfer_helper.c
>> >> @@ -182,7 +182,7 @@ transfer_map_msaa(struct pipe_context *pctx,
>> >>   .depth0 = 1,
>> >>   .array_size = 1,
>> >> };
>> >> -   trans->ss = pscreen->resource_create(pscreen, );
>> >> +   trans->ss = u_transfer_helper_resource_create(pscreen, );
>> >
>> >
>> > So I *think* the thinking here was to use pscreen->resource_create()
>> > in case there are multiple things the transfer-helper has to deal
>> > with, like MSAA+RGTC..
>> >
>> > (I can't guarantee that actually works.. but that was the reasoning..)
>>
>> So, I believe that pscreen->resource_create should be set to
>> u_transfer_helper_resource_create if you're using u_transfer_helper.
>> freedreno, v3d, iris, panfrost all do this.  vc4 doesn't but that's just
>> a bug that doesn't matter becuase it doesn't do rgtc or z32fs8.
>>
>> If you've wrapped something around pscreen->resource_create's call of
>> u_transfer_helper_resource_create for some reason, then I think you'd
>> still want to have that called from the MSAA map path.
>
> Ahh, good point.. so I guess this change should be fine.  Although
> also not entirely clear about what it is fixing?

What I'm saying is I think the unpatched code is actually best, though.


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v4 4/9] anv: Added support for non-dynamic sample locations on Gen8+

[Jason Ekstrand]

On Thu, Mar 14, 2019 at 2:52 PM Eleni Maria Stea  wrote:

> Allowing the user to set custom sample locations non-dynamically, by
> filling the extension structs and chaining them to the pipeline structs
> according to the Vulkan specification section [26.5. Custom Sample
> Locations] for the following structures:
>
> 'VkPipelineSampleLocationsStateCreateInfoEXT'
> 'VkSampleLocationsInfoEXT'
> 'VkSampleLocationEXT'
>
> Once custom locations are used, the default locations are lost and need
> to be re-emitted again in the next pipeline creation. For that, we emit
> the 3DSTATE_SAMPLE_PATTERN at every pipeline creation.
>
> v2: In v1, we used the custom anv_sample struct to store the location
> and the distance from the pixel center because we would then use
> this distance to sort the locations and send them in increasing
> monotonical order to the GPU. That was because the Skylake PRM Vol.
> 2a "3DSTATE_SAMPLE_PATTERN" says that the samples must have
> monotonically increasing distance from the pixel center to get the
> correct centroid computation in the device. However, the Vulkan
> spec seems to require that the samples occur in the order provided
> through the API and this requirement is only for the standard
> locations. As long as this only affects centroid calculations as
> the docs say, we should be ok because OpenGL and Vulkan only
> require that the centroid be some lit sample and that it's the same
> for all samples in a pixel; they have no requirement that it be the
> one closest to center. (Jason Ekstrand)
> For that we made the following changes:
> 1- We removed the custom structs and functions from anv_private.h
>and anv_sample_locations.h and anv_sample_locations.c (the last
>two files were removed). (Jason Ekstrand)
> 2- We modified the macros used to take also the array as parameter
>and we renamed them to start by GEN_. (Jason Ekstrand)
> 3- We don't sort the samples anymore. (Jason Ekstrand)
> ---
>  src/intel/common/gen_sample_positions.h | 57 ++
>  src/intel/vulkan/anv_genX.h |  5 ++
>  src/intel/vulkan/anv_private.h  |  1 +
>  src/intel/vulkan/genX_pipeline.c| 79 +
>  src/intel/vulkan/genX_state.c   | 72 ++
>  5 files changed, 201 insertions(+), 13 deletions(-)
>
> diff --git a/src/intel/common/gen_sample_positions.h
> b/src/intel/common/gen_sample_positions.h
> index da48dcb5ed0..850661931cf 100644
> --- a/src/intel/common/gen_sample_positions.h
> +++ b/src/intel/common/gen_sample_positions.h
> @@ -160,4 +160,61 @@ prefix##14YOffset  = 0.9375; \
>  prefix##15XOffset  = 0.0625; \
>  prefix##15YOffset  = 0.;
>
> +/* Examples:
> + * in case of GEN_GEN < 8:
> + * GEN_SAMPLE_POS_ELEM(ms.Sample, info->pSampleLocations, 0); expands to:
> + *ms.Sample0XOffset = info->pSampleLocations[0].pos.x;
> + *ms.Sample0YOffset = info->pSampleLocations[0].y;
> + *
> + * in case of GEN_GEN >= 8:
> + * GEN_SAMPLE_POS_ELEM(sp._16xSample, info->pSampleLocations, 0); expands
> to:
> + *sp._16xSample0XOffset = info->pSampleLocations[0].x;
> + *sp._16xSample0YOffset = info->pSampleLocations[0].y;
> + */
> +
> +#define GEN_SAMPLE_POS_ELEM(prefix, arr, sample_idx) \
> +prefix##sample_idx##XOffset = arr[sample_idx].x; \
> +prefix##sample_idx##YOffset = arr[sample_idx].y;
> +
> +#define GEN_SAMPLE_POS_1X_ARRAY(prefix, arr)\
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 0);
> +
> +#define GEN_SAMPLE_POS_2X_ARRAY(prefix, arr) \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 1);
> +
> +#define GEN_SAMPLE_POS_4X_ARRAY(prefix, arr) \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 3);
> +
> +#define GEN_SAMPLE_POS_8X_ARRAY(prefix, arr) \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 3); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 4); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 5); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 6); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 7);
> +
> +#define GEN_SAMPLE_POS_16X_ARRAY(prefix, arr) \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 3); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 4); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 5); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 6); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 7); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 8); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 9); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 10); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 11); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 12); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 13); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 14); \
> +GEN_SAMPLE_POS_ELEM(prefix, arr, 15);

[Mesa-dev] 2019 X.Org Foundation Election Delayed

[Wentland, Harry]

To all X.Org Foundation Members:

In order to give members more time to process the proposed changes to the bylaw 
the elections committee decided to delay the start of voting by one week.

Updated Schedule:
  Nomination period Start: Jan 31 00:00 UTC
  Nomination period End: Feb 28 23:59 UTC
  Deadline of X.Org membership application or renewal: Mar 07 23:59 UTC
  Publication of Candidates & start of Candidate QA: Mar 11
  Election Start: Mar 21 00:00 UTC
  Election End: Apr 4 23:59 UTC

Please take some time to familiarize yourself with the proposed bylaw changes 
and the candidates. Details are at 
https://www.x.org/wiki/BoardOfDirectors/Elections/2019/

Harry, on behalf of the X.Org elections committee
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] Bylaw Changes Vote

[Wentland, Harry]

To all X.Org Foundation Members:

My previous emails about the 2019 X.Org elections failed to mention on 
important ballot items that we'd like to vote on, in addition to the new board 
members: the Bylaw change 

The updated Bylaws to be voted on can be found here: 
https://gitlab.freedesktop.org/xorgfoundation/bylaws/blob/bylaw-updates/bylaws.pdf
The individual changes can be seen here: 
https://gitlab.freedesktop.org/xorgfoundation/bylaws/commit/06e7f04f79131df2c86e9cdfedc00aa1d1ec3f52

Please take a look.

To give members more time to understand and discuss this the elections 
committee decided to move the election out by a week. Voting will start Mar 21.

Harry, on behalf of the X.Org elections committee
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/u_transfer_helper: do not call resource_create(..) directly

[Rob Clark]

On Thu, Mar 14, 2019 at 3:35 PM Eric Anholt  wrote:
>
> Rob Clark  writes:
>
> > On Fri, Mar 1, 2019 at 10:54 AM Christian Gmeiner
> >  wrote:
> >>
> >> Use u_transfer_helper_resource_create(..) instead which uses the
> >> resource_create(..) function specified in u_transfer_vtbl.
> >>
> >> Signed-off-by: Christian Gmeiner 
> >> ---
> >>  src/gallium/auxiliary/util/u_transfer_helper.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/src/gallium/auxiliary/util/u_transfer_helper.c 
> >> b/src/gallium/auxiliary/util/u_transfer_helper.c
> >> index 14c4d56392d..a5c3c026e71 100644
> >> --- a/src/gallium/auxiliary/util/u_transfer_helper.c
> >> +++ b/src/gallium/auxiliary/util/u_transfer_helper.c
> >> @@ -182,7 +182,7 @@ transfer_map_msaa(struct pipe_context *pctx,
> >>   .depth0 = 1,
> >>   .array_size = 1,
> >> };
> >> -   trans->ss = pscreen->resource_create(pscreen, );
> >> +   trans->ss = u_transfer_helper_resource_create(pscreen, );
> >
> >
> > So I *think* the thinking here was to use pscreen->resource_create()
> > in case there are multiple things the transfer-helper has to deal
> > with, like MSAA+RGTC..
> >
> > (I can't guarantee that actually works.. but that was the reasoning..)
>
> So, I believe that pscreen->resource_create should be set to
> u_transfer_helper_resource_create if you're using u_transfer_helper.
> freedreno, v3d, iris, panfrost all do this.  vc4 doesn't but that's just
> a bug that doesn't matter becuase it doesn't do rgtc or z32fs8.
>
> If you've wrapped something around pscreen->resource_create's call of
> u_transfer_helper_resource_create for some reason, then I think you'd
> still want to have that called from the MSAA map path.

Ahh, good point.. so I guess this change should be fine.  Although
also not entirely clear about what it is fixing?

BR,
-R

> -BEGIN PGP SIGNATURE-
>
> iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlyKrPoACgkQtdYpNtH8
> nuj64g//ZbahuV6CRilGDBPUNbKCJP9Pp7GErDqgNWZgrc0tAJ+xLLFLyJ/WZXzz
> ThH4OfLgjcUQ1k+tHC/hftJBpqWsFzFYG+d9LsDLdqlbGB0anzILCaNtpzhj9AoW
> 0GXahC724XlXU+V6qVxTPPXzvaAaDqZHfyBAgiUAorS/Wvdkc2ECtZuFk+1LcfHx
> xdlyKankMuo0sghMBOF+npncW9J2fgya3UW43GD/DvcHckzd0eUuGkUNPOZduvAU
> BaD3xlQI2RcwxPPFatrQejDBYcOBxF1FFa6oUN6E5MuOWULKcYBvrdNxIpLvBEMv
> cC+dRqTDdoVZtXeTmB6BegFYo2ujq9xk+jy+OosAcIrJl9mhHi4ghbFGCVzs45ei
> WeiN13RXH5jfBDdO/Yj3KmI1UzDGLL5Ef1THTD3mm8RpoYBndZ5HGUvYjpw3GGeS
> 83RVOhVoNaA0ZmU9APj3ohkAnFNPcWWUTti7VPkZECznwD+OC/4fbFMAqGtGwfPM
> gS12v5L3+JoYUpM49SbEEMi60mM9goGyag/AMAfa55ffbgc9r6ddZitG0+uwQ7/E
> iLBlYxCHnpx5mtpmfEi4riLMCTq5R668K+2dHX1C6h2hizzmfnNuzYVWnZJOflZ3
> rKbBg6DeNkTBLry6y8o+UqQPzezbvtzu/Iu0EMHIWBUM8yX18ZI=
> =u3ZV
> -END PGP SIGNATURE-
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Rob Clark]

On Thu, Mar 14, 2019 at 3:58 PM Roland Scheidegger  wrote:
>
> Am 14.03.19 um 14:13 schrieb Rob Clark:
> > On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  
> > wrote:
> >>
> >> Am 12.03.19 um 16:16 schrieb Rob Clark:
> >>> This previously was not called out clearly, but based on a survey of the
> >>> code, it seems the expected behavior is to release the reference to any
> >>> sampler views beyond the new range being bound.
> >>
> >> That isn't really true. This was designed to work like d3d10, where
> >> other views are unmodified.
> >> The cso code will actually unset all views which previously were set and
> >> are above the num_views in the call (this wouldn't be necessary if the
> >> pipe function itself would work like this).
> >> However, it will only do this for fragment textures, and pass through
> >> the parameters unmodified otherwise. Which means behavior might not be
> >> very consistent for the different stages...
> >
> > Any opinion about whether views==NULL should be allowed?  Currently I
> > have something like:
> >
> > 
> > diff --git a/src/gallium/docs/source/context.rst
> > b/src/gallium/docs/source/context.rst
> > index f89d9e1005e..06d30bfb38b 100644
> > --- a/src/gallium/docs/source/context.rst
> > +++ b/src/gallium/docs/source/context.rst
> > @@ -143,6 +143,11 @@ to the array index which is used for sampling.
> >to a respective sampler view and releases a reference to the previous
> >sampler view.
> >
> > +  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
> > +  unmodified.  If ``views`` is NULL, the behavior is the same as if
> > +  ``views[n]`` was NULL for the entire range, ie. releasing the reference
> > +  for all the sampler views in the specified range.
> > +
> >  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> > associated
> >with the sampler view which results in sampler view holding a reference
> >to the texture. Format specified in template must be compatible
> > 
> >
> > But going thru the other drivers, a lot of them also don't handle the
> > views==NULL case.  This case doesn't seem to come up with mesa/st, but
> > does with XA and nine, and some of the test code.
>
> I think this should be illegal. As you've noted some drivers can't
> handle it, and I don't see a particularly good reason to allow it. Well
> I guess it trades some complexity in state trackers with some complexity
> in drivers...

fwiw, going with the idea that it should be legal, I fixed that in the
drivers that didn't handle it in:

https://gitlab.freedesktop.org/mesa/mesa/merge_requests/449

(planning to send to list, I just pushed a WIP MR to run it thru the CI system)

I was on the fence between making st handle it vs making driver handle
it, but it is trivial for the driver to handle it if it knows it is
supposed to.  And I had to fixup the drivers for various things
already (most hadn't been updated to handle the `start_slot` param,
for ex).

Eric suggested (on the MR) introducing a helper for this, which might
be a better approach to cut down on the boilerplate..  I'll play with
that idea.

(btw, from a quick look, set_sampler_views isn't the only problem
slot.. I noticed set_shader_buffers has the same issue.. but maybe
I'll try to fix one thing at a time and worry more about that when
panfrost or etnaviv gets closer to the point of supporting SSBO and
compute shaders..)

BR,
-R

>
> Roland
>
>
>
> > BR,
> > -R
> >
> >>
> >>
> >>>
> >>> I think radeonsi and freedreno were the only ones not doing this.  Which
> >>> could probably temporarily leak a bit of memory by holding on to the
> >>> sampler view reference.
> >> Not sure about other drivers, but llvmpipe will not do this neither.
> >>
> >> Roland
> >>
> >>
> >>>
> >>> Signed-off-by: Rob Clark 
> >>> ---
> >>>  src/gallium/docs/source/context.rst | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/src/gallium/docs/source/context.rst 
> >>> b/src/gallium/docs/source/context.rst
> >>> index f89d9e1005e..199d335f8f4 100644
> >>> --- a/src/gallium/docs/source/context.rst
> >>> +++ b/src/gallium/docs/source/context.rst
> >>> @@ -143,6 +143,9 @@ to the array index which is used for sampling.
> >>>to a respective sampler view and releases a reference to the previous
> >>>sampler view.
> >>>
> >>> +  Previously bound samplers with index ``>= num_views`` are unbound 
> >>> rather
> >>> +  than unmodified.
> >>> +
> >>>  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> >>> associated
> >>>with the sampler view which results in sampler view holding a reference
> >>>to the texture. Format specified in template must be compatible
> >>>
> >>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] 2019 X.Org Foundation Election Candidates

[Wentland, Harry]

On 2019-03-14 4:40 p.m., Luc Verhaegen wrote:
> On Thu, Mar 14, 2019 at 08:36:13PM +, Wentland, Harry wrote:
>> On 2019-03-12 6:49 a.m., Luc Verhaegen wrote:
>>>
>>> Are these candidates the only thing we will be voting on?
>>>
>>
>> You're right. The FDO question totally slipped me mind.
>>
>> We'll also be voting on the bylaw change I sent out sometime toward the end 
>> of last year to facilitate the marriage of fdo and x.org.
>>
>> I'll send out details shortly.
> 
> Cool, thanks!
> 
> This way members can actually review and perhaps discuss this instead of 
> being confrontend with this when voting for just candidates, without 
> preparation, which could've perhaps invalidated the result.
> 

Definitely a big oversight on my part. Details are now on 
https://www.x.org/wiki/BoardOfDirectors/Elections/2019/

I'll bring this up at today's board meeting. Not sure if this warrants delaying 
start of voting but I'd consider extending the end date.

Harry

> Luc Verhaegen.
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] 2019 X.Org Foundation Election Candidates

[Luc Verhaegen]

On Thu, Mar 14, 2019 at 08:36:13PM +, Wentland, Harry wrote:
> On 2019-03-12 6:49 a.m., Luc Verhaegen wrote:
> > 
> > Are these candidates the only thing we will be voting on?
> > 
> 
> You're right. The FDO question totally slipped me mind.
> 
> We'll also be voting on the bylaw change I sent out sometime toward the end 
> of last year to facilitate the marriage of fdo and x.org.
> 
> I'll send out details shortly.

Cool, thanks!

This way members can actually review and perhaps discuss this instead of 
being confrontend with this when voting for just candidates, without 
preparation, which could've perhaps invalidated the result.

Luc Verhaegen.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] 2019 X.Org Foundation Election Candidates

[Wentland, Harry]

On 2019-03-12 6:49 a.m., Luc Verhaegen wrote:
> On Tue, Mar 12, 2019 at 12:24:00AM +, Wentland, Harry wrote:
>> To all X.Org Foundation Members:
>>
>> The election for the X.Org Foundation Board of Directors will begin on 
>> 14 March 2019. We have 6 candidates who are running for 4 seats. They 
>> are (in alphabetical order):
> 
>> Attached below are the Personal Statements each candidate submitted 
>> for your consideration along with their Statements of Contribution 
>> that they submitted with the membership application. Please review 
>> each of the candidates' statements to help you decide whom to vote for 
>> during the upcoming election.
>>
>> If you have questions of the candidates, you should feel free to ask them 
>> here on the mailing list.
>>
>> The election committee will provide detailed instructions on how the voting 
>> system will work when the voting period begins.
>>
>> Harry, on behalf of the X.Org elections committee
> 
> Are these candidates the only thing we will be voting on?
> 

You're right. The FDO question totally slipped me mind.

We'll also be voting on the bylaw change I sent out sometime toward the end of 
last year to facilitate the marriage of fdo and x.org.

I'll send out details shortly.

Harry

> Luc Verhaegen.
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Roland Scheidegger]

Am 14.03.19 um 14:13 schrieb Rob Clark:
> On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  wrote:
>>
>> Am 12.03.19 um 16:16 schrieb Rob Clark:
>>> This previously was not called out clearly, but based on a survey of the
>>> code, it seems the expected behavior is to release the reference to any
>>> sampler views beyond the new range being bound.
>>
>> That isn't really true. This was designed to work like d3d10, where
>> other views are unmodified.
>> The cso code will actually unset all views which previously were set and
>> are above the num_views in the call (this wouldn't be necessary if the
>> pipe function itself would work like this).
>> However, it will only do this for fragment textures, and pass through
>> the parameters unmodified otherwise. Which means behavior might not be
>> very consistent for the different stages...
> 
> Any opinion about whether views==NULL should be allowed?  Currently I
> have something like:
> 
> 
> diff --git a/src/gallium/docs/source/context.rst
> b/src/gallium/docs/source/context.rst
> index f89d9e1005e..06d30bfb38b 100644
> --- a/src/gallium/docs/source/context.rst
> +++ b/src/gallium/docs/source/context.rst
> @@ -143,6 +143,11 @@ to the array index which is used for sampling.
>to a respective sampler view and releases a reference to the previous
>sampler view.
> 
> +  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
> +  unmodified.  If ``views`` is NULL, the behavior is the same as if
> +  ``views[n]`` was NULL for the entire range, ie. releasing the reference
> +  for all the sampler views in the specified range.
> +
>  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> associated
>with the sampler view which results in sampler view holding a reference
>to the texture. Format specified in template must be compatible
> 
> 
> But going thru the other drivers, a lot of them also don't handle the
> views==NULL case.  This case doesn't seem to come up with mesa/st, but
> does with XA and nine, and some of the test code.

I think this should be illegal. As you've noted some drivers can't
handle it, and I don't see a particularly good reason to allow it. Well
I guess it trades some complexity in state trackers with some complexity
in drivers...

Roland



> BR,
> -R
> 
>>
>>
>>>
>>> I think radeonsi and freedreno were the only ones not doing this.  Which
>>> could probably temporarily leak a bit of memory by holding on to the
>>> sampler view reference.
>> Not sure about other drivers, but llvmpipe will not do this neither.
>>
>> Roland
>>
>>
>>>
>>> Signed-off-by: Rob Clark 
>>> ---
>>>  src/gallium/docs/source/context.rst | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/src/gallium/docs/source/context.rst 
>>> b/src/gallium/docs/source/context.rst
>>> index f89d9e1005e..199d335f8f4 100644
>>> --- a/src/gallium/docs/source/context.rst
>>> +++ b/src/gallium/docs/source/context.rst
>>> @@ -143,6 +143,9 @@ to the array index which is used for sampling.
>>>to a respective sampler view and releases a reference to the previous
>>>sampler view.
>>>
>>> +  Previously bound samplers with index ``>= num_views`` are unbound rather
>>> +  than unmodified.
>>> +
>>>  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
>>> associated
>>>with the sampler view which results in sampler view holding a reference
>>>to the texture. Format specified in template must be compatible
>>>
>>

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 3/9] anv: Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT

[Eleni Maria Stea]

Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT according to
the Vulkan Specification section [36.2. Additional Multisampling
Capabilities].

v2: 1- Moved the vkGetPhysicalDeviceMultisamplePropertiesEXT from the
   anv_sample_locations.c to the anv_device.c (Jason Ekstrand)
2- Simplified the code that sets the grid size (Jason Ekstrand)
3- Instead of filling the whole struct, we only fill the parts we
   should override (sType, grid size) and we call
   anv_debug_ignored_stype to any pNext elements (Jason Ekstrand)
---
 src/intel/vulkan/anv_device.c | 25 +
 1 file changed, 25 insertions(+)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 52ea058bdd5..0bfff7e0b30 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -3557,6 +3557,31 @@ VkResult anv_GetCalibratedTimestampsEXT(
return VK_SUCCESS;
 }
 
+void
+anv_GetPhysicalDeviceMultisamplePropertiesEXT(VkPhysicalDevice physicalDevice,
+  VkSampleCountFlagBits samples,
+  VkMultisamplePropertiesEXT
+  *pMultisampleProperties)
+{
+   ANV_FROM_HANDLE(anv_physical_device, physical_device, physicalDevice);
+
+   VkExtent2D grid_size;
+   if (samples & isl_device_get_sample_counts(_device->isl_dev)) {
+  grid_size.width = 1;
+  grid_size.height = 1;
+   } else {
+  grid_size.width = 0;
+  grid_size.height = 0;
+   }
+
+   pMultisampleProperties->sType =
+  VK_STRUCTURE_TYPE_MULTISAMPLE_PROPERTIES_EXT;
+   pMultisampleProperties->maxSampleLocationGridSize = grid_size;
+
+   vk_foreach_struct(ext, pMultisampleProperties->pNext)
+  anv_debug_ignored_stype(ext->sType);
+}
+
 /* vk_icd.h does not declare this function, so we declare it here to
  * suppress Wmissing-prototypes.
  */
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 7/9] anv: Optimized the emission of the default locations on Gen8+

[Eleni Maria Stea]

We only emit sample locations when the extension is enabled by the user.
In all other cases the default locations are emitted once when the device
is initialized to increase performance.
---
 src/intel/vulkan/anv_genX.h|  3 ++-
 src/intel/vulkan/genX_cmd_buffer.c |  2 +-
 src/intel/vulkan/genX_pipeline.c   | 13 -
 src/intel/vulkan/genX_state.c  |  8 +---
 4 files changed, 16 insertions(+), 10 deletions(-)

diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
index 82fe5cc93bf..f28ee0b1a76 100644
--- a/src/intel/vulkan/anv_genX.h
+++ b/src/intel/vulkan/anv_genX.h
@@ -93,4 +93,5 @@ void genX(emit_ms_state)(struct anv_batch *batch,
  const VkSampleLocationEXT *sl,
  uint32_t num_samples,
  uint32_t log2_samples,
- bool custom_sample_locations);
+ bool custom_sample_locations,
+ bool sample_locations_ext_enabled);
diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index 57dd94bfbd7..63913dd0668 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -2651,7 +2651,7 @@ cmd_buffer_emit_sample_locations(struct anv_cmd_buffer 
*cmd_buffer)
 
genX(emit_ms_state)(_buffer->batch,
dyn_state->sample_locations.positions,
-   samples, log2_samples, true);
+   samples, log2_samples, true, true);
 }
 
 void
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index 21b21a719da..1245090386c 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -572,10 +572,12 @@ emit_sample_mask(struct anv_pipeline *pipeline,
 }
 
 static void
-emit_ms_state(struct anv_pipeline *pipeline,
+emit_ms_state(struct anv_device *device,
+  struct anv_pipeline *pipeline,
   const VkPipelineMultisampleStateCreateInfo *info,
   const VkPipelineDynamicStateCreateInfo *dinfo)
 {
+   bool sample_loc_enabled = device->enabled_extensions.EXT_sample_locations;
VkSampleLocationsInfoEXT *sl;
bool custom_locations = false;
uint32_t samples = 1;
@@ -586,7 +588,7 @@ emit_ms_state(struct anv_pipeline *pipeline,
if (info) {
   samples = info->rasterizationSamples;
 
-  if (info->pNext) {
+  if (sample_loc_enabled && info->pNext) {
  VkPipelineSampleLocationsStateCreateInfoEXT *slinfo =
 (VkPipelineSampleLocationsStateCreateInfoEXT *)info->pNext;
 
@@ -613,8 +615,8 @@ emit_ms_state(struct anv_pipeline *pipeline,
   log2_samples = __builtin_ffs(samples) - 1;
}
 
-   genX(emit_ms_state)(>batch, sl->pSampleLocations, samples, 
log2_samples,
-   custom_locations);
+   genX(emit_ms_state)(>batch, sl->pSampleLocations, samples,
+   log2_samples, custom_locations, sample_loc_enabled);
 }
 
 static const uint32_t vk_to_gen_logic_op[] = {
@@ -1944,7 +1946,8 @@ genX(graphics_pipeline_create)(
assert(pCreateInfo->pRasterizationState);
emit_rs_state(pipeline, pCreateInfo->pRasterizationState,
  pCreateInfo->pMultisampleState, pass, subpass);
-   emit_ms_state(pipeline, pCreateInfo->pMultisampleState, 
pCreateInfo->pDynamicState);
+   emit_ms_state(device, pipeline, pCreateInfo->pMultisampleState,
+ pCreateInfo->pDynamicState);
emit_ds_state(pipeline, pCreateInfo->pDepthStencilState, pass, subpass);
emit_cb_state(pipeline, pCreateInfo->pColorBlendState,
pCreateInfo->pMultisampleState);
diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
index 9b05506f3af..6e13001b74f 100644
--- a/src/intel/vulkan/genX_state.c
+++ b/src/intel/vulkan/genX_state.c
@@ -568,12 +568,14 @@ genX(emit_ms_state)(struct anv_batch *batch,
 const VkSampleLocationEXT *sl,
 uint32_t num_samples,
 uint32_t log2_samples,
-bool custom_sample_locations)
+bool custom_sample_locations,
+bool sample_locations_ext_enabled)
 {
emit_multisample(batch, sl, num_samples, log2_samples,
 custom_sample_locations);
 #if GEN_GEN >= 8
-   emit_sample_locations(batch, sl, num_samples,
- custom_sample_locations);
+   if (sample_locations_ext_enabled)
+  emit_sample_locations(batch, sl, num_samples,
+custom_sample_locations);
 #endif
 }
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 8/9] anv: Removed unused header file

[Eleni Maria Stea]

In src/intel/vulkan/genX_blorp_exec.c we included the file:
common/gen_sample_positions.h but not use it. Removed.

Reviewed-by: Sagar Ghuge 
Reviewed-by: Jason Ekstrand 
---
 src/intel/vulkan/genX_blorp_exec.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/src/intel/vulkan/genX_blorp_exec.c 
b/src/intel/vulkan/genX_blorp_exec.c
index e9c85d56d5f..0eeefaaa9d6 100644
--- a/src/intel/vulkan/genX_blorp_exec.c
+++ b/src/intel/vulkan/genX_blorp_exec.c
@@ -31,7 +31,6 @@
 #undef __gen_combine_address
 
 #include "common/gen_l3_config.h"
-#include "common/gen_sample_positions.h"
 #include "blorp/blorp_genX_exec.h"
 
 static void *
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 6/9] anv: Added support for dynamic and non-dynamic sample locations on Gen7

[Eleni Maria Stea]

Allowing setting dynamic and non-dynamic sample locations on Gen7.

v2: Similarly to the previous patches, removed structs and functions
that were used to sort and store the sorted sample positions (Jason
Ekstrand)
---
 src/intel/vulkan/anv_genX.h| 13 ++---
 src/intel/vulkan/genX_cmd_buffer.c |  9 ++--
 src/intel/vulkan/genX_pipeline.c   | 13 +
 src/intel/vulkan/genX_state.c  | 86 +-
 4 files changed, 70 insertions(+), 51 deletions(-)

diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
index 5c618ab..82fe5cc93bf 100644
--- a/src/intel/vulkan/anv_genX.h
+++ b/src/intel/vulkan/anv_genX.h
@@ -89,11 +89,8 @@ void genX(cmd_buffer_mi_memset)(struct anv_cmd_buffer 
*cmd_buffer,
 void genX(blorp_exec)(struct blorp_batch *batch,
   const struct blorp_params *params);
 
-void genX(emit_multisample)(struct anv_batch *batch,
-uint32_t samples,
-uint32_t log2_samples);
-
-void genX(emit_sample_locations)(struct anv_batch *batch,
- const VkSampleLocationEXT *sl,
- uint32_t num_samples,
- bool custom_locations);
+void genX(emit_ms_state)(struct anv_batch *batch,
+ const VkSampleLocationEXT *sl,
+ uint32_t num_samples,
+ uint32_t log2_samples,
+ bool custom_sample_locations);
diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index 5d7c9b51a84..57dd94bfbd7 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -2642,7 +2642,6 @@ cmd_buffer_flush_push_constants(struct anv_cmd_buffer 
*cmd_buffer,
 static void
 cmd_buffer_emit_sample_locations(struct anv_cmd_buffer *cmd_buffer)
 {
-#if GEN_GEN >= 8
struct anv_dynamic_state *dyn_state = _buffer->state.gfx.dynamic;
uint32_t samples = dyn_state->sample_locations.num_samples;
uint32_t log2_samples;
@@ -2650,11 +2649,9 @@ cmd_buffer_emit_sample_locations(struct anv_cmd_buffer 
*cmd_buffer)
assert(samples > 0);
log2_samples = __builtin_ffs(samples) - 1;
 
-   genX(emit_multisample)(_buffer->batch, samples, log2_samples);
-   genX(emit_sample_locations)(_buffer->batch,
-   dyn_state->sample_locations.positions,
-   samples, true);
-#endif
+   genX(emit_ms_state)(_buffer->batch,
+   dyn_state->sample_locations.positions,
+   samples, log2_samples, true);
 }
 
 void
diff --git a/src/intel/vulkan/genX_pipeline.c b/src/intel/vulkan/genX_pipeline.c
index ada022620d1..21b21a719da 100644
--- a/src/intel/vulkan/genX_pipeline.c
+++ b/src/intel/vulkan/genX_pipeline.c
@@ -576,11 +576,8 @@ emit_ms_state(struct anv_pipeline *pipeline,
   const VkPipelineMultisampleStateCreateInfo *info,
   const VkPipelineDynamicStateCreateInfo *dinfo)
 {
-#if GEN_GEN >= 8
VkSampleLocationsInfoEXT *sl;
bool custom_locations = false;
-#endif
-
uint32_t samples = 1;
uint32_t log2_samples = 0;
 
@@ -589,7 +586,6 @@ emit_ms_state(struct anv_pipeline *pipeline,
if (info) {
   samples = info->rasterizationSamples;
 
-#if GEN_GEN >= 8
   if (info->pNext) {
  VkPipelineSampleLocationsStateCreateInfoEXT *slinfo =
 (VkPipelineSampleLocationsStateCreateInfoEXT *)info->pNext;
@@ -613,17 +609,12 @@ emit_ms_state(struct anv_pipeline *pipeline,
 }
  }
   }
-#endif
 
   log2_samples = __builtin_ffs(samples) - 1;
}
 
-   genX(emit_multisample(>batch, samples, log2_samples));
-
-#if GEN_GEN >= 8
-   genX(emit_sample_locations)(>batch, sl->pSampleLocations,
-   samples, custom_locations);
-#endif
+   genX(emit_ms_state)(>batch, sl->pSampleLocations, samples, 
log2_samples,
+   custom_locations);
 }
 
 static const uint32_t vk_to_gen_logic_op[] = {
diff --git a/src/intel/vulkan/genX_state.c b/src/intel/vulkan/genX_state.c
index 4fdb74111a5..9b05506f3af 100644
--- a/src/intel/vulkan/genX_state.c
+++ b/src/intel/vulkan/genX_state.c
@@ -436,10 +436,12 @@ VkResult genX(CreateSampler)(
return VK_SUCCESS;
 }
 
-void
-genX(emit_multisample)(struct anv_batch *batch,
-   uint32_t samples,
-   uint32_t log2_samples)
+static void
+emit_multisample(struct anv_batch *batch,
+ const VkSampleLocationEXT *sl,
+ uint32_t samples,
+ uint32_t log2_samples,
+ bool custom_locations)
 {
anv_batch_emit(batch, GENX(3DSTATE_MULTISAMPLE), ms) {
   ms.NumberofMultisamples = log2_samples;
@@ -452,31 +454,51 @@ genX(emit_multisample)(struct anv_batch *batch,
*/
   ms.PixelPositionOffsetEnable  = false;
 #else
-  switch (samples) {
-  case 1:
- 

[Mesa-dev] [PATCH v4 9/9] anv: Enabled the VK_EXT_sample_locations extension

[Eleni Maria Stea]

Enabled the VK_EXT_sample_locations for Intel Gen >= 7.

v2: Replaced device.info->gen >= 7 with True, as Anv doesn't support
anything below Gen7. (Lionel Landwerlin)

Reviewed-by: Sagar Ghuge 
---
 src/intel/vulkan/anv_extensions.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/intel/vulkan/anv_extensions.py 
b/src/intel/vulkan/anv_extensions.py
index 9e4e03e46df..5a30c733c5c 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -129,7 +129,7 @@ EXTENSIONS = [
 Extension('VK_EXT_inline_uniform_block',  1, True),
 Extension('VK_EXT_pci_bus_info',  2, True),
 Extension('VK_EXT_post_depth_coverage',   1, 'device->info.gen 
>= 9'),
-Extension('VK_EXT_sample_locations',  1, False),
+Extension('VK_EXT_sample_locations',  1, True),
 Extension('VK_EXT_sampler_filter_minmax', 1, 'device->info.gen 
>= 9'),
 Extension('VK_EXT_scalar_block_layout',   1, True),
 Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen 
>= 9'),
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 4/9] anv: Added support for non-dynamic sample locations on Gen8+

[Eleni Maria Stea]

Allowing the user to set custom sample locations non-dynamically, by
filling the extension structs and chaining them to the pipeline structs
according to the Vulkan specification section [26.5. Custom Sample
Locations] for the following structures:

'VkPipelineSampleLocationsStateCreateInfoEXT'
'VkSampleLocationsInfoEXT'
'VkSampleLocationEXT'

Once custom locations are used, the default locations are lost and need
to be re-emitted again in the next pipeline creation. For that, we emit
the 3DSTATE_SAMPLE_PATTERN at every pipeline creation.

v2: In v1, we used the custom anv_sample struct to store the location
and the distance from the pixel center because we would then use
this distance to sort the locations and send them in increasing
monotonical order to the GPU. That was because the Skylake PRM Vol.
2a "3DSTATE_SAMPLE_PATTERN" says that the samples must have
monotonically increasing distance from the pixel center to get the
correct centroid computation in the device. However, the Vulkan
spec seems to require that the samples occur in the order provided
through the API and this requirement is only for the standard
locations. As long as this only affects centroid calculations as
the docs say, we should be ok because OpenGL and Vulkan only
require that the centroid be some lit sample and that it's the same
for all samples in a pixel; they have no requirement that it be the
one closest to center. (Jason Ekstrand)
For that we made the following changes:
1- We removed the custom structs and functions from anv_private.h
   and anv_sample_locations.h and anv_sample_locations.c (the last
   two files were removed). (Jason Ekstrand)
2- We modified the macros used to take also the array as parameter
   and we renamed them to start by GEN_. (Jason Ekstrand)
3- We don't sort the samples anymore. (Jason Ekstrand)
---
 src/intel/common/gen_sample_positions.h | 57 ++
 src/intel/vulkan/anv_genX.h |  5 ++
 src/intel/vulkan/anv_private.h  |  1 +
 src/intel/vulkan/genX_pipeline.c| 79 +
 src/intel/vulkan/genX_state.c   | 72 ++
 5 files changed, 201 insertions(+), 13 deletions(-)

diff --git a/src/intel/common/gen_sample_positions.h 
b/src/intel/common/gen_sample_positions.h
index da48dcb5ed0..850661931cf 100644
--- a/src/intel/common/gen_sample_positions.h
+++ b/src/intel/common/gen_sample_positions.h
@@ -160,4 +160,61 @@ prefix##14YOffset  = 0.9375; \
 prefix##15XOffset  = 0.0625; \
 prefix##15YOffset  = 0.;
 
+/* Examples:
+ * in case of GEN_GEN < 8:
+ * GEN_SAMPLE_POS_ELEM(ms.Sample, info->pSampleLocations, 0); expands to:
+ *ms.Sample0XOffset = info->pSampleLocations[0].pos.x;
+ *ms.Sample0YOffset = info->pSampleLocations[0].y;
+ *
+ * in case of GEN_GEN >= 8:
+ * GEN_SAMPLE_POS_ELEM(sp._16xSample, info->pSampleLocations, 0); expands to:
+ *sp._16xSample0XOffset = info->pSampleLocations[0].x;
+ *sp._16xSample0YOffset = info->pSampleLocations[0].y;
+ */
+
+#define GEN_SAMPLE_POS_ELEM(prefix, arr, sample_idx) \
+prefix##sample_idx##XOffset = arr[sample_idx].x; \
+prefix##sample_idx##YOffset = arr[sample_idx].y;
+
+#define GEN_SAMPLE_POS_1X_ARRAY(prefix, arr)\
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0);
+
+#define GEN_SAMPLE_POS_2X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1);
+
+#define GEN_SAMPLE_POS_4X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 3);
+
+#define GEN_SAMPLE_POS_8X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 3); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 4); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 5); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 6); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 7);
+
+#define GEN_SAMPLE_POS_16X_ARRAY(prefix, arr) \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 0); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 1); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 2); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 3); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 4); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 5); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 6); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 7); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 8); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 9); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 10); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 11); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 12); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 13); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 14); \
+GEN_SAMPLE_POS_ELEM(prefix, arr, 15);
+
 #endif /* GEN_SAMPLE_POSITIONS_H */
diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
index 8fd32cabf1e..fb7419b6347 100644
--- a/src/intel/vulkan/anv_genX.h
+++ b/src/intel/vulkan/anv_genX.h
@@ -88,3 +88,8 @@ void 

[Mesa-dev] [PATCH v4 5/9] anv: Added support for dynamic sample locations on Gen8+

[Eleni Maria Stea]

Added support for setting the locations when the pipeline has been
created with the dynamic state bit enabled according to the Vulkan
Specification section [26.5. Custom Sample Locations] for the function:

'vkCmdSetSampleLocationsEXT'

The reason that we preferred to store the boolean valid inside the
dynamic state struct for locations instead of using a dirty bit
(ANV_CMD_DIRTY_SAMPLE_LOCATIONS for example) is that other functions
can modify the value of the dirty bits causing unexpected behavior.

v2: Removed all the anv* structs used with sample locations to store
the locations in order for dynamic case. (see also the patch for
the non-dynamic case. (Jason Ekstrand)
---
 src/intel/vulkan/anv_cmd_buffer.c  | 19 ++
 src/intel/vulkan/anv_genX.h|  4 +++
 src/intel/vulkan/anv_private.h |  6 +
 src/intel/vulkan/genX_cmd_buffer.c | 24 ++
 src/intel/vulkan/genX_pipeline.c   | 40 +-
 src/intel/vulkan/genX_state.c  | 36 +++
 6 files changed, 90 insertions(+), 39 deletions(-)

diff --git a/src/intel/vulkan/anv_cmd_buffer.c 
b/src/intel/vulkan/anv_cmd_buffer.c
index 1b34644a434..866cd03b05e 100644
--- a/src/intel/vulkan/anv_cmd_buffer.c
+++ b/src/intel/vulkan/anv_cmd_buffer.c
@@ -558,6 +558,25 @@ void anv_CmdSetStencilReference(
cmd_buffer->state.gfx.dirty |= ANV_CMD_DIRTY_DYNAMIC_STENCIL_REFERENCE;
 }
 
+void
+anv_CmdSetSampleLocationsEXT(VkCommandBuffer commandBuffer,
+ const VkSampleLocationsInfoEXT 
*pSampleLocationsInfo)
+{
+   ANV_FROM_HANDLE(anv_cmd_buffer, cmd_buffer, commandBuffer);
+
+   struct anv_dynamic_state *dyn_state = _buffer->state.gfx.dynamic;
+   uint32_t num_samples = pSampleLocationsInfo->sampleLocationsPerPixel;
+
+   assert(pSampleLocationsInfo);
+   dyn_state->sample_locations.num_samples = num_samples;
+
+   memcpy(dyn_state->sample_locations.positions,
+  pSampleLocationsInfo->pSampleLocations,
+  num_samples * sizeof *pSampleLocationsInfo->pSampleLocations);
+
+   dyn_state->sample_locations.valid = true;
+}
+
 static void
 anv_cmd_buffer_bind_descriptor_set(struct anv_cmd_buffer *cmd_buffer,
VkPipelineBindPoint bind_point,
diff --git a/src/intel/vulkan/anv_genX.h b/src/intel/vulkan/anv_genX.h
index fb7419b6347..5c618ab 100644
--- a/src/intel/vulkan/anv_genX.h
+++ b/src/intel/vulkan/anv_genX.h
@@ -89,6 +89,10 @@ void genX(cmd_buffer_mi_memset)(struct anv_cmd_buffer 
*cmd_buffer,
 void genX(blorp_exec)(struct blorp_batch *batch,
   const struct blorp_params *params);
 
+void genX(emit_multisample)(struct anv_batch *batch,
+uint32_t samples,
+uint32_t log2_samples);
+
 void genX(emit_sample_locations)(struct anv_batch *batch,
  const VkSampleLocationEXT *sl,
  uint32_t num_samples,
diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index a39195733cd..1e1d2feaa50 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -2124,6 +2124,12 @@ struct anv_dynamic_state {
   uint32_t  front;
   uint32_t  back;
} stencil_reference;
+
+   struct {
+  VkSampleLocationEXT   
positions[MAX_SAMPLE_LOCATIONS];
+  uint32_t  num_samples;
+  bool  valid;
+   } sample_locations;
 };
 
 extern const struct anv_dynamic_state default_dynamic_state;
diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index 7687507e6b7..5d7c9b51a84 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -30,6 +30,7 @@
 #include "util/fast_idiv_by_const.h"
 
 #include "common/gen_l3_config.h"
+#include "common/gen_sample_positions.h"
 #include "genxml/gen_macros.h"
 #include "genxml/genX_pack.h"
 
@@ -2638,6 +2639,24 @@ cmd_buffer_flush_push_constants(struct anv_cmd_buffer 
*cmd_buffer,
cmd_buffer->state.push_constants_dirty &= ~flushed;
 }
 
+static void
+cmd_buffer_emit_sample_locations(struct anv_cmd_buffer *cmd_buffer)
+{
+#if GEN_GEN >= 8
+   struct anv_dynamic_state *dyn_state = _buffer->state.gfx.dynamic;
+   uint32_t samples = dyn_state->sample_locations.num_samples;
+   uint32_t log2_samples;
+
+   assert(samples > 0);
+   log2_samples = __builtin_ffs(samples) - 1;
+
+   genX(emit_multisample)(_buffer->batch, samples, log2_samples);
+   genX(emit_sample_locations)(_buffer->batch,
+   dyn_state->sample_locations.positions,
+   samples, true);
+#endif
+}
+
 void
 genX(cmd_buffer_flush_state)(struct anv_cmd_buffer *cmd_buffer)
 {
@@ -2796,6 +2815,11 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
  

[Mesa-dev] [PATCH v4 0/9] Implementation of the VK_EXT_sample_locations

[Eleni Maria Stea]

Implemented the requirements from the VK_EXT_sample_locations extension
specification to allow setting custom sample locations on Intel Gen >= 7.

Some decisions explained:

The grid size was set to 1x1 because the hardware only supports a single
set of sample locations for the whole framebuffer.

The user can set custom sample locations either per pipeline, by filling
the extension provided structs, or dynamically the way it is described
in sections 26.5, 36.1, 36.2 of the Vulkan specification.

Sections 6.7.3 and 7.4 describe how to use sample locations with images
when a layout transition is about to take place. These sections were
ignored as currently we aren't using sample locations with images in the
driver.

Variable sample locations aren't required and have not been implemented.

(v2): Initially, we were sorting the samples because according to the
  Skylake PRM (vol 2a SAMPLE_PATTERN) the samples should be sent in
  a monotonically increasing distance from the center to get the
  correct centroid computation in the device. However the Vulkan
  spec seems to require that the samples occur in the order provided
  through the API. As long as this requirement only affects centroid
  calculations we should be ok without the ordering because OpenGL
  and Vulkan only require the centroid to be some lit sample and
  that it's the same for all samples in a pixel. They have no
  requirement that it be the one closest to the center. (Jason
  Ekstrand)

We have 754 vk-gl-cts tests for this extension:
690 of the tests pass on Gen >= 9 (where we can support 16 samples).
The remaining 64 tests aren't supported because they test the variable
sample locations.

Eleni Maria Stea (9):
  anv: Added the VK_EXT_sample_locations extension to the anv_extensions
list
  anv: Set the values for the
VkPhysicalDeviceSampleLocationsPropertiesEXT
  anv: Implemented the vkGetPhysicalDeviceMultisamplePropertiesEXT
  anv: Added support for non-dynamic sample locations on Gen8+
  anv: Added support for dynamic sample locations on Gen8+
  anv: Added support for dynamic and non-dynamic sample locations on
Gen7
  anv: Optimized the emission of the default locations on Gen8+
  anv: Removed unused header file
  anv: Enabled the VK_EXT_sample_locations extension

 src/intel/Makefile.sources  |   1 +
 src/intel/common/gen_sample_positions.h |  53 ++
 src/intel/vulkan/anv_cmd_buffer.c   |  19 
 src/intel/vulkan/anv_device.c   |  21 
 src/intel/vulkan/anv_extensions.py  |   1 +
 src/intel/vulkan/anv_genX.h |   7 ++
 src/intel/vulkan/anv_private.h  |  18 
 src/intel/vulkan/anv_sample_locations.c |  96 ++
 src/intel/vulkan/anv_sample_locations.h |  29 ++
 src/intel/vulkan/genX_blorp_exec.c  |   1 -
 src/intel/vulkan/genX_cmd_buffer.c  |  24 +
 src/intel/vulkan/genX_pipeline.c|  92 +
 src/intel/vulkan/genX_state.c   | 128 
 src/intel/vulkan/meson.build|   1 +
 14 files changed, 450 insertions(+), 41 deletions(-)
 create mode 100644 src/intel/vulkan/anv_sample_locations.c
 create mode 100644 src/intel/vulkan/anv_sample_locations.h

-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 2/9] anv: Set the values for the VkPhysicalDeviceSampleLocationsPropertiesEXT

[Eleni Maria Stea]

The VkPhysicalDeviceSampleLocationPropertiesEXT struct is filled with
implementation dependent values and according to the table from the
Vulkan Specification section [36.1. Limit Requirements]:

pname | max | min
pname:sampleLocationSampleCounts   |-|ename:VK_SAMPLE_COUNT_4_BIT
pname:maxSampleLocationGridSize|-|(1, 1)
pname:sampleLocationCoordinateRange|(0.0, 0.9375)|(0.0, 0.9375)
pname:sampleLocationSubPixelBits   |-|4
pname:variableSampleLocations  | false   |implementation dependent

The hardware only supports setting the same sample location for all the
pixels, so we only support 1x1 grids.

Also, variableSampleLocations is set to false because we don't support the
feature.

v2: 1- Replaced false with VK_FALSE for consistency. (Sagar Ghuge)
2- Used the isl_device_sample_count to take the number of samples
per platform to avoid extra checks. (Sagar Ghuge)

v3: 1- Replaced VK_FALSE with false as Jason has sent a patch to replace
VK_FALSE with false in other places. (Jason Ekstrand)
2- Removed unecessary defines and set the grid size to 1 (Jason Ekstrand)

Reviewed-by: Sagar Ghuge 
---
 src/intel/vulkan/anv_device.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 83fa3936c19..52ea058bdd5 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1401,6 +1401,26 @@ void anv_GetPhysicalDeviceProperties2(
  break;
   }
 
+  case VK_STRUCTURE_TYPE_PHYSICAL_DEVICE_SAMPLE_LOCATIONS_PROPERTIES_EXT: {
+ VkPhysicalDeviceSampleLocationsPropertiesEXT *props =
+(VkPhysicalDeviceSampleLocationsPropertiesEXT *)ext;
+
+ props->sampleLocationSampleCounts =
+isl_device_get_sample_counts(>isl_dev);
+
+ /* See also anv_GetPhysicalDeviceMultisamplePropertiesEXT */
+ props->maxSampleLocationGridSize.width = 1;
+ props->maxSampleLocationGridSize.height = 1;
+
+ props->sampleLocationCoordinateRange[0] = 0;
+ props->sampleLocationCoordinateRange[1] = 0.9375;
+ props->sampleLocationSubPixelBits = 4;
+
+ props->variableSampleLocations = false;
+
+ break;
+  }
+
   default:
  anv_debug_ignored_stype(ext->sType);
  break;
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v4 1/9] anv: Added the VK_EXT_sample_locations extension to the anv_extensions list

[Eleni Maria Stea]

Added the VK_EXT_sample_locations to the anv_extensions.py list to
generate the related entrypoints.

Reviewed-by: Sagar Ghuge 
---
 src/intel/vulkan/anv_extensions.py | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/intel/vulkan/anv_extensions.py 
b/src/intel/vulkan/anv_extensions.py
index 6fff293dee4..9e4e03e46df 100644
--- a/src/intel/vulkan/anv_extensions.py
+++ b/src/intel/vulkan/anv_extensions.py
@@ -129,6 +129,7 @@ EXTENSIONS = [
 Extension('VK_EXT_inline_uniform_block',  1, True),
 Extension('VK_EXT_pci_bus_info',  2, True),
 Extension('VK_EXT_post_depth_coverage',   1, 'device->info.gen 
>= 9'),
+Extension('VK_EXT_sample_locations',  1, False),
 Extension('VK_EXT_sampler_filter_minmax', 1, 'device->info.gen 
>= 9'),
 Extension('VK_EXT_scalar_block_layout',   1, True),
 Extension('VK_EXT_shader_stencil_export', 1, 'device->info.gen 
>= 9'),
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/8] st/mesa: implement "zombie" shaders list

[Brian Paul]

As with the preceding patch for sampler views, this patch does
basically the same thing but for shaders.  However, reference counting
isn't needed here (instead of calling cso_delete_XXX_shader() we call
st_save_zombie_shader().

The Redway3D Watch is one app/demo that needs this change.  Otherwise,
the vmwgfx driver generates an error about trying to destroy a shader
ID that doesn't exist in the context.

Note that if PIPE_CAP_SHAREABLE_SHADERS = TRUE, then we can use/delete
any shader with any context and this mechanism is not used.

Tested with: google-chrome, google earth, Redway3D Watch/Turbine demos
and a few Linux games.
---
 src/mesa/state_tracker/st_context.c | 86 +
 src/mesa/state_tracker/st_context.h | 23 ++
 src/mesa/state_tracker/st_program.c | 77 -
 3 files changed, 166 insertions(+), 20 deletions(-)

diff --git a/src/mesa/state_tracker/st_context.c 
b/src/mesa/state_tracker/st_context.c
index bd919da..1e5742b 100644
--- a/src/mesa/state_tracker/st_context.c
+++ b/src/mesa/state_tracker/st_context.c
@@ -294,6 +294,39 @@ st_save_zombie_sampler_view(struct st_context *st,
 
 
 /*
+ * Since OpenGL shaders may be shared among contexts, we can wind up
+ * with variants of a shader created with different contexts.
+ * When we go to destroy a gallium shader, we want to free it with the
+ * same context that it was created with, unless the driver reports
+ * PIPE_CAP_SHAREABLE_SHADERS = TRUE.
+ */
+void
+st_save_zombie_shader(struct st_context *st,
+  enum pipe_shader_type type,
+  struct pipe_shader_state *shader)
+{
+   struct st_zombie_shader_node *entry;
+
+   /* we shouldn't be here if the driver supports shareable shaders */
+   assert(!st->has_shareable_shaders);
+
+   entry = MALLOC_STRUCT(st_zombie_shader_node);
+   if (!entry)
+  return;
+
+   entry->shader = shader;
+   entry->type = type;
+
+   /* We need a mutex since this function may be called from one thread
+* while free_zombie_shaders() is called from another.
+*/
+   mtx_lock(>zombie_shaders.mutex);
+   LIST_ADDTAIL(>node, >zombie_shaders.list.node);
+   mtx_unlock(>zombie_shaders.mutex);
+}
+
+
+/*
  * Free any zombie sampler views that may be attached to this context.
  */
 static void
@@ -325,6 +358,55 @@ free_zombie_sampler_views(struct st_context *st)
 
 
 /*
+ * Free any zombie shaders that may be attached to this context.
+ */
+static void
+free_zombie_shaders(struct st_context *st)
+{
+   struct st_zombie_shader_node *entry, *next;
+
+   if (LIST_IS_EMPTY(>zombie_shaders.list.node)) {
+  return;
+   }
+
+   mtx_lock(>zombie_shaders.mutex);
+
+   LIST_FOR_EACH_ENTRY_SAFE(entry, next,
+>zombie_shaders.list.node, node) {
+  LIST_DEL(>node);  // remove this entry from the list
+
+  switch (entry->type) {
+  case PIPE_SHADER_VERTEX:
+ cso_delete_vertex_shader(st->cso_context, entry->shader);
+ break;
+  case PIPE_SHADER_FRAGMENT:
+ cso_delete_fragment_shader(st->cso_context, entry->shader);
+ break;
+  case PIPE_SHADER_GEOMETRY:
+ cso_delete_geometry_shader(st->cso_context, entry->shader);
+ break;
+  case PIPE_SHADER_TESS_CTRL:
+ cso_delete_tessctrl_shader(st->cso_context, entry->shader);
+ break;
+  case PIPE_SHADER_TESS_EVAL:
+ cso_delete_tesseval_shader(st->cso_context, entry->shader);
+ break;
+  case PIPE_SHADER_COMPUTE:
+ cso_delete_compute_shader(st->cso_context, entry->shader);
+ break;
+  default:
+ unreachable("invalid shader type in free_zombie_shaders()");
+  }
+  free(entry);
+   }
+
+   assert(LIST_IS_EMPTY(>zombie_shaders.list.node));
+
+   mtx_unlock(>zombie_shaders.mutex);
+}
+
+
+/*
  * This function is called periodically to free any zombie objects
  * which are attached to this context.
  */
@@ -332,6 +414,7 @@ void
 st_context_free_zombie_objects(struct st_context *st)
 {
free_zombie_sampler_views(st);
+   free_zombie_shaders(st);
 }
 
 
@@ -644,6 +727,8 @@ st_create_context_priv(struct gl_context *ctx, struct 
pipe_context *pipe,
 
LIST_INITHEAD(>zombie_sampler_views.list.node);
mtx_init(>zombie_sampler_views.mutex, mtx_plain);
+   LIST_INITHEAD(>zombie_shaders.list.node);
+   mtx_init(>zombie_shaders.mutex, mtx_plain);
 
return st;
 }
@@ -848,6 +933,7 @@ st_destroy_context(struct st_context *st)
st_context_free_zombie_objects(st);
 
mtx_destroy(>zombie_sampler_views.mutex);
+   mtx_destroy(>zombie_shaders.mutex);
 
/* This must be called first so that glthread has a chance to finish */
_mesa_glthread_destroy(ctx);
diff --git a/src/mesa/state_tracker/st_context.h 
b/src/mesa/state_tracker/st_context.h
index 1106bb6..c87ff81 100644
--- a/src/mesa/state_tracker/st_context.h
+++ b/src/mesa/state_tracker/st_context.h
@@ -106,6 +106,17 @@ struct st_zombie_sampler_view_node
 };

[Mesa-dev] [PATCH 6/8] swr: remove call to pipe_sampler_view_release()

[Brian Paul]

As with svga, llvmpipe drivers in previous patches.
Compile tested only.
---
 src/gallium/drivers/swr/swr_state.cpp | 5 -
 1 file changed, 5 deletions(-)

diff --git a/src/gallium/drivers/swr/swr_state.cpp 
b/src/gallium/drivers/swr/swr_state.cpp
index d7baa71..42d196f 100644
--- a/src/gallium/drivers/swr/swr_state.cpp
+++ b/src/gallium/drivers/swr/swr_state.cpp
@@ -302,11 +302,6 @@ swr_set_sampler_views(struct pipe_context *pipe,
/* set the new sampler views */
ctx->num_sampler_views[shader] = num;
for (i = 0; i < num; i++) {
-  /* Note: we're using pipe_sampler_view_release() here to work around
-   * a possible crash when the old view belongs to another context that
-   * was already destroyed.
-   */
-  pipe_sampler_view_release(pipe, >sampler_views[shader][start + i]);
   pipe_sampler_view_reference(>sampler_views[shader][start + i],
   views[i]);
}
-- 
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 5/8] llvmpipe: stop using pipe_sampler_view_release()

[Brian Paul]

This was used to avoid freeing a sampler view which was created by a
context that was already deleted.  But the state tracker does not
allow that.
---
 src/gallium/drivers/llvmpipe/lp_state_sampler.c | 6 --
 1 file changed, 6 deletions(-)

diff --git a/src/gallium/drivers/llvmpipe/lp_state_sampler.c 
b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
index c9aba1a..72823e4 100644
--- a/src/gallium/drivers/llvmpipe/lp_state_sampler.c
+++ b/src/gallium/drivers/llvmpipe/lp_state_sampler.c
@@ -123,12 +123,6 @@ llvmpipe_set_sampler_views(struct pipe_context *pipe,
 
/* set the new sampler views */
for (i = 0; i < num; i++) {
-  /* Note: we're using pipe_sampler_view_release() here to work around
-   * a possible crash when the old view belongs to another context that
-   * was already destroyed.
-   */
-  pipe_sampler_view_release(pipe,
->sampler_views[shader][start + i]);
   /*
* Warn if someone tries to set a view created in a different context
* (which is why we need the hack above in the first place).
-- 
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/8] st/mesa: stop using pipe_sampler_view_release()

[Brian Paul]

In all instances here we can replace pipe_sampler_view_release(pipe,
view) with pipe_sampler_view_reference(view, NULL) because the views
in question are private to the state tracker context.  So there's no
danger of freeing a sampler view with the wrong context.

Testing done: google chrome, misc GL demos, games
---
 src/mesa/state_tracker/st_context.c  | 3 +--
 src/mesa/state_tracker/st_sampler_view.c | 6 +++---
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/mesa/state_tracker/st_context.c 
b/src/mesa/state_tracker/st_context.c
index 1e5742b..de98ddc 100644
--- a/src/mesa/state_tracker/st_context.c
+++ b/src/mesa/state_tracker/st_context.c
@@ -435,8 +435,7 @@ st_destroy_context_priv(struct st_context *st, bool 
destroy_pipe)
st_destroy_bound_image_handles(st);
 
for (i = 0; i < ARRAY_SIZE(st->state.frag_sampler_views); i++) {
-  pipe_sampler_view_release(st->pipe,
->state.frag_sampler_views[i]);
+  pipe_sampler_view_reference(>state.frag_sampler_views[i], NULL);
}
 
/* free glReadPixels cache data */
diff --git a/src/mesa/state_tracker/st_sampler_view.c 
b/src/mesa/state_tracker/st_sampler_view.c
index d16d523..23e6c6e 100644
--- a/src/mesa/state_tracker/st_sampler_view.c
+++ b/src/mesa/state_tracker/st_sampler_view.c
@@ -74,7 +74,7 @@ st_texture_set_sampler_view(struct st_context *st,
   if (sv->view) {
  /* check if the context matches */
  if (sv->view->context == st->pipe) {
-pipe_sampler_view_release(st->pipe, >view);
+pipe_sampler_view_reference(>view, NULL);
 goto found;
  }
   } else {
@@ -94,13 +94,13 @@ st_texture_set_sampler_view(struct st_context *st,
 
  if (new_max < views->max ||
  new_max > (UINT_MAX - sizeof(*views)) / sizeof(views->views[0])) {
-pipe_sampler_view_release(st->pipe, );
+pipe_sampler_view_reference(, NULL);
 goto out;
  }
 
  struct st_sampler_views *new_views = malloc(new_size);
  if (!new_views) {
-pipe_sampler_view_release(st->pipe, );
+pipe_sampler_view_reference(, NULL);
 goto out;
  }
 
-- 
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/8] st/mesa: implement "zombie" sampler views

[Brian Paul]

When st_texture_release_all_sampler_views() is called the texture may
have sampler views belonging to several contexts.  If we unreference a
sampler view and its refcount hits zero, we need to be sure to destroy
the sampler view with the same context which created it.

This was not the case with the previous code which used
pipe_sampler_view_release().  That function could end up freeing a
sampler view with a context different than the one which created it.
In the case of the VMware svga driver, we detected this but leaked the
sampler view.  This led to a crash with google-chrome when the kernel
module had too many sampler views.  VMware bug 2274734.

Alternately, if we try to delete a sampler view with the correct
context, we may be "reaching into" a context which is active on
another thread.  That's not safe.

To fix these issues this patch adds a per-context list of "zombie"
sampler views.  These are views which are to be freed at some point
when the context is active.  Other contexts may safely add sampler
views to the zombie list at any time (it's mutex protected).  This
avoids the context/view ownership mix-ups we had before.

Tested with: google-chrome, google earth, Redway3D Watch/Turbine demos
a few Linux games.  If anyone can recomment some other multi-threaded,
multi-context GL apps to test, please let me know.
---
 src/mesa/state_tracker/st_cb_flush.c |  6 +++
 src/mesa/state_tracker/st_context.c  | 81 
 src/mesa/state_tracker/st_context.h  | 25 ++
 src/mesa/state_tracker/st_sampler_view.c | 27 +--
 src/mesa/state_tracker/st_texture.h  |  3 ++
 5 files changed, 138 insertions(+), 4 deletions(-)

diff --git a/src/mesa/state_tracker/st_cb_flush.c 
b/src/mesa/state_tracker/st_cb_flush.c
index 5b3188c..81e5338 100644
--- a/src/mesa/state_tracker/st_cb_flush.c
+++ b/src/mesa/state_tracker/st_cb_flush.c
@@ -39,6 +39,7 @@
 #include "st_cb_flush.h"
 #include "st_cb_clear.h"
 #include "st_cb_fbo.h"
+#include "st_context.h"
 #include "st_manager.h"
 #include "pipe/p_context.h"
 #include "pipe/p_defines.h"
@@ -53,6 +54,11 @@ st_flush(struct st_context *st,
 {
st_flush_bitmap_cache(st);
 
+   /* We want to call this function periodically.
+* Typically, it has nothing to do so it shouldn't be expensive.
+*/
+   st_context_free_zombie_objects(st);
+
st->pipe->flush(st->pipe, fence, flags);
 }
 
diff --git a/src/mesa/state_tracker/st_context.c 
b/src/mesa/state_tracker/st_context.c
index 2898279..bd919da 100644
--- a/src/mesa/state_tracker/st_context.c
+++ b/src/mesa/state_tracker/st_context.c
@@ -261,6 +261,80 @@ st_invalidate_state(struct gl_context *ctx)
 }
 
 
+/*
+ * In some circumstances (such as running google-chrome) the state
+ * tracker may try to delete a resource view from a context different
+ * than when it was created.  We don't want to do that.
+ * In that situation, st_texture_release_all_sampler_views() calls this
+ * function to save the view for later deletion.  The context here is
+ * expected to be the context which created the view.
+ */
+void
+st_save_zombie_sampler_view(struct st_context *st,
+struct pipe_sampler_view *view)
+{
+   struct st_zombie_sampler_view_node *entry;
+
+   assert(view->context == st->pipe);
+   assert(view->reference.count == 1);
+
+   entry = MALLOC_STRUCT(st_zombie_sampler_view_node);
+   if (!entry)
+  return;
+
+   entry->view = view;
+
+   /* We need a mutex since this function may be called from one thread
+* while free_zombie_resource_views() is called from another.
+*/
+   mtx_lock(>zombie_sampler_views.mutex);
+   LIST_ADDTAIL(>node, >zombie_sampler_views.list.node);
+   mtx_unlock(>zombie_sampler_views.mutex);
+}
+
+
+/*
+ * Free any zombie sampler views that may be attached to this context.
+ */
+static void
+free_zombie_sampler_views(struct st_context *st)
+{
+   struct st_zombie_sampler_view_node *entry, *next;
+
+   if (LIST_IS_EMPTY(>zombie_sampler_views.list.node)) {
+  return;
+   }
+
+   mtx_lock(>zombie_sampler_views.mutex);
+
+   LIST_FOR_EACH_ENTRY_SAFE(entry, next,
+>zombie_sampler_views.list.node, node) {
+  LIST_DEL(>node);  // remove this entry from the list
+
+  assert(entry->view->context == st->pipe);
+  assert(entry->view->reference.count == 1);
+  pipe_sampler_view_reference(>view, NULL);
+
+  free(entry);
+   }
+
+   assert(LIST_IS_EMPTY(>zombie_sampler_views.list.node));
+
+   mtx_unlock(>zombie_sampler_views.mutex);
+}
+
+
+/*
+ * This function is called periodically to free any zombie objects
+ * which are attached to this context.
+ */
+void
+st_context_free_zombie_objects(struct st_context *st)
+{
+   free_zombie_sampler_views(st);
+}
+
+
 static void
 st_destroy_context_priv(struct st_context *st, bool destroy_pipe)
 {
@@ -568,6 +642,9 @@ st_create_context_priv(struct gl_context *ctx, struct 
pipe_context *pipe,
/* Initialize context's winsys buffers 

[Mesa-dev] [PATCH 8/8] gallium/util: remove pipe_sampler_view_release()

[Brian Paul]

It's no longer used.
---
 src/gallium/auxiliary/util/u_inlines.h | 20 
 1 file changed, 20 deletions(-)

diff --git a/src/gallium/auxiliary/util/u_inlines.h 
b/src/gallium/auxiliary/util/u_inlines.h
index fa1e920..567d3d0 100644
--- a/src/gallium/auxiliary/util/u_inlines.h
+++ b/src/gallium/auxiliary/util/u_inlines.h
@@ -192,26 +192,6 @@ pipe_sampler_view_reference(struct pipe_sampler_view **dst,
*dst = src;
 }
 
-/**
- * Similar to pipe_sampler_view_reference() but always set the pointer to
- * NULL and pass in the current context explicitly.
- *
- * If *ptr is non-NULL, it may refer to a view that was created in a different
- * context (however, that context must still be alive).
- */
-static inline void
-pipe_sampler_view_release(struct pipe_context *ctx,
-  struct pipe_sampler_view **ptr)
-{
-   struct pipe_sampler_view *old_view = *ptr;
-
-   if (pipe_reference_described(_view->reference, NULL,
-(debug_reference_descriptor)debug_describe_sampler_view)) {
-  ctx->sampler_view_destroy(ctx, old_view);
-   }
-   *ptr = NULL;
-}
-
 static inline void
 pipe_so_target_reference(struct pipe_stream_output_target **dst,
  struct pipe_stream_output_target *src)
-- 
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 4/8] svga: stop using pipe_sampler_view_release()

[Brian Paul]

This function was used in the past to avoid deleting a sampler view
for a context that no longer exists.  But the Mesa state tracker
ensures that cannot happen.  Use the standard refcounting function
instead.

Also, remove the code which checked for context mis-matches in
svga_sampler_view_destroy().  It's no longer needed since implementing
the zombie sampler view code in the state tracker.

Testing Done: google chrome, variety of GL demos/games
---
 src/gallium/drivers/svga/svga_pipe_sampler.c | 38 +---
 src/gallium/drivers/svga/svga_state_tss.c|  4 +--
 2 files changed, 14 insertions(+), 28 deletions(-)

diff --git a/src/gallium/drivers/svga/svga_pipe_sampler.c 
b/src/gallium/drivers/svga/svga_pipe_sampler.c
index b32bb09..f1e68dd 100644
--- a/src/gallium/drivers/svga/svga_pipe_sampler.c
+++ b/src/gallium/drivers/svga/svga_pipe_sampler.c
@@ -405,28 +405,18 @@ svga_sampler_view_destroy(struct pipe_context *pipe,
struct svga_pipe_sampler_view *sv = svga_pipe_sampler_view(view);
 
if (svga_have_vgpu10(svga) && sv->id != SVGA3D_INVALID_ID) {
-  if (view->context != pipe) {
- /* The SVGA3D device will generate an error (and on Linux, cause
-  * us to abort) if we try to destroy a shader resource view from
-  * a context other than the one it was created with.  Skip the
-  * SVGA3D_vgpu10_DestroyShaderResourceView() and leak the sampler
-  * view for now.  This should only sometimes happen when a shared
-  * texture is deleted.
-  */
- _debug_printf("context mismatch in %s\n", __func__);
-  }
-  else {
- enum pipe_error ret;
+  enum pipe_error ret;
+
+  assert(view->context == pipe);
 
- svga_hwtnl_flush_retry(svga); /* XXX is this needed? */
+  svga_hwtnl_flush_retry(svga);
 
+  ret = SVGA3D_vgpu10_DestroyShaderResourceView(svga->swc, sv->id);
+  if (ret != PIPE_OK) {
+ svga_context_flush(svga, NULL);
  ret = SVGA3D_vgpu10_DestroyShaderResourceView(svga->swc, sv->id);
- if (ret != PIPE_OK) {
-svga_context_flush(svga, NULL);
-ret = SVGA3D_vgpu10_DestroyShaderResourceView(svga->swc, sv->id);
- }
- util_bitmask_clear(svga->sampler_view_id_bm, sv->id);
   }
+  util_bitmask_clear(svga->sampler_view_id_bm, sv->id);
}
 
pipe_resource_reference(>base.texture, NULL);
@@ -465,7 +455,8 @@ svga_set_sampler_views(struct pipe_context *pipe,
 */
if (start == 0 && num == 0 && svga->curr.num_sampler_views[shader] > 0) {
   for (i = 0; i < svga->curr.num_sampler_views[shader]; i++) {
- pipe_sampler_view_release(pipe, >curr.sampler_views[shader][i]);
+ pipe_sampler_view_reference(>curr.sampler_views[shader][i],
+ NULL);
   }
   any_change = TRUE;
}
@@ -474,11 +465,6 @@ svga_set_sampler_views(struct pipe_context *pipe,
   enum pipe_texture_target target;
 
   if (svga->curr.sampler_views[shader][start + i] != views[i]) {
- /* Note: we're using pipe_sampler_view_release() here to work around
-  * a possible crash when the old view belongs to another context that
-  * was already destroyed.
-  */
- pipe_sampler_view_release(pipe, 
>curr.sampler_views[shader][start + i]);
  pipe_sampler_view_reference(>curr.sampler_views[shader][start + 
i],
  views[i]);
  any_change = TRUE;
@@ -552,8 +538,8 @@ svga_cleanup_sampler_state(struct svga_context *svga)
   unsigned i;
 
   for (i = 0; i < svga->state.hw_draw.num_sampler_views[shader]; i++) {
- pipe_sampler_view_release(>pipe,
-   
>state.hw_draw.sampler_views[shader][i]);
+ 
pipe_sampler_view_reference(>state.hw_draw.sampler_views[shader][i],
+ NULL);
   }
}

diff --git a/src/gallium/drivers/svga/svga_state_tss.c 
b/src/gallium/drivers/svga/svga_state_tss.c
index d598d23..95b1a9e 100644
--- a/src/gallium/drivers/svga/svga_state_tss.c
+++ b/src/gallium/drivers/svga/svga_state_tss.c
@@ -50,8 +50,8 @@ svga_cleanup_tss_binding(struct svga_context *svga)
   struct svga_hw_view_state *view = >state.hw_draw.views[i];
   if (view) {
  svga_sampler_view_reference(>v, NULL);
- pipe_sampler_view_release(>pipe,
-   >curr.sampler_views[shader][i]);
+ pipe_sampler_view_reference(>curr.sampler_views[shader][i],
+ NULL);
  pipe_resource_reference(>texture, NULL);
  view->dirty = TRUE;
   }
-- 
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 7/8] i915g: remove calls to pipe_sampler_view_release()

[Brian Paul]

As with previous patches for svga, llvmpipe, swr drivers.
Compile tested only.
---
 src/gallium/drivers/i915/i915_state.c | 10 ++
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/src/gallium/drivers/i915/i915_state.c 
b/src/gallium/drivers/i915/i915_state.c
index ddc2709..03d218e 100644
--- a/src/gallium/drivers/i915/i915_state.c
+++ b/src/gallium/drivers/i915/i915_state.c
@@ -745,17 +745,11 @@ static void i915_set_fragment_sampler_views(struct 
pipe_context *pipe,
   return;
 
for (i = 0; i < num; i++) {
-  /* Note: we're using pipe_sampler_view_release() here to work around
-   * a possible crash when the old view belongs to another context that
-   * was already destroyed.
-   */
-  pipe_sampler_view_release(pipe, >fragment_sampler_views[i]);
-  pipe_sampler_view_reference(>fragment_sampler_views[i],
-  views[i]);
+  pipe_sampler_view_reference(>fragment_sampler_views[i], views[i]);
}
 
for (i = num; i < i915->num_fragment_sampler_views; i++)
-  pipe_sampler_view_release(pipe, >fragment_sampler_views[i]);
+  pipe_sampler_view_reference(>fragment_sampler_views[i], NULL);
 
i915->num_fragment_sampler_views = num;
 
-- 
1.8.5.6

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/u_transfer_helper: do not call resource_create(..) directly

[Eric Anholt]

Rob Clark  writes:

> On Fri, Mar 1, 2019 at 10:54 AM Christian Gmeiner
>  wrote:
>>
>> Use u_transfer_helper_resource_create(..) instead which uses the
>> resource_create(..) function specified in u_transfer_vtbl.
>>
>> Signed-off-by: Christian Gmeiner 
>> ---
>>  src/gallium/auxiliary/util/u_transfer_helper.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/util/u_transfer_helper.c 
>> b/src/gallium/auxiliary/util/u_transfer_helper.c
>> index 14c4d56392d..a5c3c026e71 100644
>> --- a/src/gallium/auxiliary/util/u_transfer_helper.c
>> +++ b/src/gallium/auxiliary/util/u_transfer_helper.c
>> @@ -182,7 +182,7 @@ transfer_map_msaa(struct pipe_context *pctx,
>>   .depth0 = 1,
>>   .array_size = 1,
>> };
>> -   trans->ss = pscreen->resource_create(pscreen, );
>> +   trans->ss = u_transfer_helper_resource_create(pscreen, );
>
>
> So I *think* the thinking here was to use pscreen->resource_create()
> in case there are multiple things the transfer-helper has to deal
> with, like MSAA+RGTC..
>
> (I can't guarantee that actually works.. but that was the reasoning..)

So, I believe that pscreen->resource_create should be set to
u_transfer_helper_resource_create if you're using u_transfer_helper.
freedreno, v3d, iris, panfrost all do this.  vc4 doesn't but that's just
a bug that doesn't matter becuase it doesn't do rgtc or z32fs8.

If you've wrapped something around pscreen->resource_create's call of
u_transfer_helper_resource_create for some reason, then I think you'd
still want to have that called from the MSAA map path.


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [virglrenderer-devel] virglrenderer on gles, format conversion

[Erik Faye-Lund]

On Wed, 2019-03-06 at 13:39 +1000, Dave Airlie wrote:
> On Wed, 6 Mar 2019 at 01:01, Erik Faye-Lund
>  wrote:
> > When we try to do a vrend transfer from a renderable texture, we
> > end up
> > using glReadPixels to read the data. However, OpenGL ES has
> > restrictions on what formats are allowed to be used for
> > glReadPixels.
> > In partcular, only GL_UNSIGNED_BYTE + GL_RGBA are guaranteed to be
> > supported (for OpenGL ES 2.0; for OpenGL ES 3.x there's some other
> > variants for integer and float textures - it seems we can rely on
> > the
> > destination format having at least as much range/precision as the
> > source format).
> > 
> > But vrend transfers expect no format conversion at all, they want
> > to
> > read the data verbatim.
> > 
> > I thought the obvious choice was to modify virglrenderer to convert
> > back from RGBA to the source format. After a typing out a lot of
> > code,
> > I realized that even though we have util_format_translate in
> > u_format.h, the actual implementation was deleted in d01f462[1].
> > 
> > OK, so that leaves me with a couple of choices:
> > 
> > 1) Restore the format conversion code in virglrenderer.
> > 2) Use some 3rd party pixel-format conversion code instead.
> > 3) Do format-conversion back to the desired format in Mesa.
> > 
> > I gave 1) a go by reverting the removal, but this conflicted pretty
> > badly due to the 3 years of development that has happened since. It
> > might be possible to continue this effort, but I also don't really
> > like
> > the idea of crash-prone format converion code in virglrenderer. You
> > know, for security reasons; virglrenderer runs inside the virtual
> > machine manager, which makes it pretty high priviledge code. If we
> > don't do 1), I have sent a MR[2] that removes the rest of the
> > format
> > conversion, so others won't be led astray in the future.
> 
> It's likely we should just pull that in anyways, and start from a
> clean base if you decide that is the best path.
> 

OK, I'll untag this with WIP soon. A review would also be nice :)

> I agree I don't think this could should be in the host layer if we
> can avoid it.
> 
> > I have relatively low expectations that we'd find a good fit for 3)
> > as
> > we have a very specific and *big* set of formats we need to
> > support.
> > Format conversion across APIs is often a big pain-point in my
> > experience.
> > 
> > As for 3), it's not quite as easily said as done, because we
> > currently
> > only have protocol commands for reading a format verbatim. So I
> > guess
> > we'd have to add a new format, and inform the Mesa driver that
> > we're
> > limited in what formats we support. With this approach, we don't
> > need
> > conversion code both in mesa *and* in virglrenderer, and we don't
> > have
> > to actively maintain a fork of the pixel-format code separately.
> > 
> > I'm currently leaning towards 3) for now, but I would like input
> > from
> > the list, as I feel this is a rather important directional choice.
> 
> I think we'd need to define a list of formats we can readback, and
> then somehow convince the guest mesa to do the conversions for us, it
> should have all the necessary code already shouldn't it?
> 

Yes, or at least almost. I got this approach working:

https://gitlab.freedesktop.org/virgl/virglrenderer/merge_requests/191

https://gitlab.freedesktop.org/kusma/mesa/tree/virgl-readback-formats

I'm pretty happy with how this worked out.


___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/blorp: Remove unused parameter from blorp_surf_for_miptree.

[Jason Ekstrand]

Yup.  Not getting used at all.

Reviewed-by: Jason Ekstrand 

On Thu, Mar 14, 2019 at 12:10 PM Rafael Antognolli <
rafael.antogno...@intel.com> wrote:

> It seems pretty useless nowadays.
> ---
>  src/mesa/drivers/dri/i965/brw_blorp.c | 36 +--
>  1 file changed, 12 insertions(+), 24 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c
> b/src/mesa/drivers/dri/i965/brw_blorp.c
> index 97a5f6a9937..e09a8cef762 100644
> --- a/src/mesa/drivers/dri/i965/brw_blorp.c
> +++ b/src/mesa/drivers/dri/i965/brw_blorp.c
> @@ -125,8 +125,7 @@ blorp_surf_for_miptree(struct brw_context *brw,
> enum isl_aux_usage aux_usage,
> bool is_render_target,
> unsigned *level,
> -   unsigned start_layer, unsigned num_layers,
> -   struct isl_surf tmp_surfs[1])
> +   unsigned start_layer, unsigned num_layers)
>  {
> const struct gen_device_info *devinfo = >screen->devinfo;
>
> @@ -406,12 +405,11 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
> intel_miptree_prepare_access(brw, dst_mt, dst_level, 1, dst_layer, 1,
>  dst_aux_usage, dst_clear_supported);
>
> -   struct isl_surf tmp_surfs[2];
> struct blorp_surf src_surf, dst_surf;
> blorp_surf_for_miptree(brw, _surf, src_mt, src_aux_usage, false,
> -  _level, src_layer, 1, _surfs[0]);
> +  _level, src_layer, 1);
> blorp_surf_for_miptree(brw, _surf, dst_mt, dst_aux_usage, true,
> -  _level, dst_layer, 1, _surfs[1]);
> +  _level, dst_layer, 1);
>
> struct isl_swizzle src_isl_swizzle = {
>.r = swizzle_to_scs(GET_SWZ(src_swizzle, 0)),
> @@ -497,12 +495,11 @@ brw_blorp_copy_miptrees(struct brw_context *brw,
> intel_miptree_prepare_access(brw, dst_mt, dst_level, 1, dst_layer, 1,
>  dst_aux_usage, dst_clear_supported);
>
> -   struct isl_surf tmp_surfs[2];
> struct blorp_surf src_surf, dst_surf;
> blorp_surf_for_miptree(brw, _surf, src_mt, src_aux_usage, false,
> -  _level, src_layer, 1, _surfs[0]);
> +  _level, src_layer, 1);
> blorp_surf_for_miptree(brw, _surf, dst_mt, dst_aux_usage, true,
> -  _level, dst_layer, 1, _surfs[1]);
> +  _level, dst_layer, 1);
>
> /* The hardware seems to have issues with having a two different format
>  * views of the same texture in the sampler cache at the same time.
> It's
> @@ -1300,10 +1297,9 @@ do_single_blorp_clear(struct brw_context *brw,
> struct gl_framebuffer *fb,
>irb->mt, irb->mt_level, irb->mt_layer, num_layers);
>
>/* We can't setup the blorp_surf until we've allocated the MCS
> above */
> -  struct isl_surf isl_tmp[2];
>struct blorp_surf surf;
>blorp_surf_for_miptree(brw, , irb->mt, irb->mt->aux_usage,
> true,
> - , irb->mt_layer, num_layers, isl_tmp);
> + , irb->mt_layer, num_layers);
>
>/* Ivybrigde PRM Vol 2, Part 1, "11.7 MCS Buffer for Render
> Target(s)":
> *
> @@ -1346,10 +1342,9 @@ do_single_blorp_clear(struct brw_context *brw,
> struct gl_framebuffer *fb,
>intel_miptree_prepare_render(brw, irb->mt, level, irb->mt_layer,
> num_layers, aux_usage);
>
> -  struct isl_surf isl_tmp[2];
>struct blorp_surf surf;
>blorp_surf_for_miptree(brw, , irb->mt, aux_usage, true,
> - , irb->mt_layer, num_layers, isl_tmp);
> + , irb->mt_layer, num_layers);
>
>union isl_color_value clear_color;
>memcpy(clear_color.f32, ctx->Color.ClearColor.f, sizeof(float) * 4);
> @@ -1442,7 +1437,6 @@ brw_blorp_clear_depth_stencil(struct brw_context
> *brw,
>return;
>
> uint32_t level, start_layer, num_layers;
> -   struct isl_surf isl_tmp[4];
> struct blorp_surf depth_surf, stencil_surf;
>
> struct intel_mipmap_tree *depth_mt = NULL;
> @@ -1459,8 +1453,7 @@ brw_blorp_clear_depth_stencil(struct brw_context
> *brw,
>
>unsigned depth_level = level;
>blorp_surf_for_miptree(brw, _surf, depth_mt,
> depth_mt->aux_usage,
> - true, _level, start_layer, num_layers,
> - _tmp[0]);
> + true, _level, start_layer, num_layers);
>assert(depth_level == level);
> }
>
> @@ -1489,8 +1482,7 @@ brw_blorp_clear_depth_stencil(struct brw_context
> *brw,
>unsigned stencil_level = level;
>blorp_surf_for_miptree(brw, _surf, stencil_mt,
>   ISL_AUX_USAGE_NONE, true,
> - _level, start_layer, num_layers,
> - _tmp[2]);
> + 

[Mesa-dev] [PATCH] i965/blorp: Remove unused parameter from blorp_surf_for_miptree.

[Rafael Antognolli]

It seems pretty useless nowadays.
---
 src/mesa/drivers/dri/i965/brw_blorp.c | 36 +--
 1 file changed, 12 insertions(+), 24 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_blorp.c 
b/src/mesa/drivers/dri/i965/brw_blorp.c
index 97a5f6a9937..e09a8cef762 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.c
+++ b/src/mesa/drivers/dri/i965/brw_blorp.c
@@ -125,8 +125,7 @@ blorp_surf_for_miptree(struct brw_context *brw,
enum isl_aux_usage aux_usage,
bool is_render_target,
unsigned *level,
-   unsigned start_layer, unsigned num_layers,
-   struct isl_surf tmp_surfs[1])
+   unsigned start_layer, unsigned num_layers)
 {
const struct gen_device_info *devinfo = >screen->devinfo;
 
@@ -406,12 +405,11 @@ brw_blorp_blit_miptrees(struct brw_context *brw,
intel_miptree_prepare_access(brw, dst_mt, dst_level, 1, dst_layer, 1,
 dst_aux_usage, dst_clear_supported);
 
-   struct isl_surf tmp_surfs[2];
struct blorp_surf src_surf, dst_surf;
blorp_surf_for_miptree(brw, _surf, src_mt, src_aux_usage, false,
-  _level, src_layer, 1, _surfs[0]);
+  _level, src_layer, 1);
blorp_surf_for_miptree(brw, _surf, dst_mt, dst_aux_usage, true,
-  _level, dst_layer, 1, _surfs[1]);
+  _level, dst_layer, 1);
 
struct isl_swizzle src_isl_swizzle = {
   .r = swizzle_to_scs(GET_SWZ(src_swizzle, 0)),
@@ -497,12 +495,11 @@ brw_blorp_copy_miptrees(struct brw_context *brw,
intel_miptree_prepare_access(brw, dst_mt, dst_level, 1, dst_layer, 1,
 dst_aux_usage, dst_clear_supported);
 
-   struct isl_surf tmp_surfs[2];
struct blorp_surf src_surf, dst_surf;
blorp_surf_for_miptree(brw, _surf, src_mt, src_aux_usage, false,
-  _level, src_layer, 1, _surfs[0]);
+  _level, src_layer, 1);
blorp_surf_for_miptree(brw, _surf, dst_mt, dst_aux_usage, true,
-  _level, dst_layer, 1, _surfs[1]);
+  _level, dst_layer, 1);
 
/* The hardware seems to have issues with having a two different format
 * views of the same texture in the sampler cache at the same time.  It's
@@ -1300,10 +1297,9 @@ do_single_blorp_clear(struct brw_context *brw, struct 
gl_framebuffer *fb,
   irb->mt, irb->mt_level, irb->mt_layer, num_layers);
 
   /* We can't setup the blorp_surf until we've allocated the MCS above */
-  struct isl_surf isl_tmp[2];
   struct blorp_surf surf;
   blorp_surf_for_miptree(brw, , irb->mt, irb->mt->aux_usage, true,
- , irb->mt_layer, num_layers, isl_tmp);
+ , irb->mt_layer, num_layers);
 
   /* Ivybrigde PRM Vol 2, Part 1, "11.7 MCS Buffer for Render Target(s)":
*
@@ -1346,10 +1342,9 @@ do_single_blorp_clear(struct brw_context *brw, struct 
gl_framebuffer *fb,
   intel_miptree_prepare_render(brw, irb->mt, level, irb->mt_layer,
num_layers, aux_usage);
 
-  struct isl_surf isl_tmp[2];
   struct blorp_surf surf;
   blorp_surf_for_miptree(brw, , irb->mt, aux_usage, true,
- , irb->mt_layer, num_layers, isl_tmp);
+ , irb->mt_layer, num_layers);
 
   union isl_color_value clear_color;
   memcpy(clear_color.f32, ctx->Color.ClearColor.f, sizeof(float) * 4);
@@ -1442,7 +1437,6 @@ brw_blorp_clear_depth_stencil(struct brw_context *brw,
   return;
 
uint32_t level, start_layer, num_layers;
-   struct isl_surf isl_tmp[4];
struct blorp_surf depth_surf, stencil_surf;
 
struct intel_mipmap_tree *depth_mt = NULL;
@@ -1459,8 +1453,7 @@ brw_blorp_clear_depth_stencil(struct brw_context *brw,
 
   unsigned depth_level = level;
   blorp_surf_for_miptree(brw, _surf, depth_mt, depth_mt->aux_usage,
- true, _level, start_layer, num_layers,
- _tmp[0]);
+ true, _level, start_layer, num_layers);
   assert(depth_level == level);
}
 
@@ -1489,8 +1482,7 @@ brw_blorp_clear_depth_stencil(struct brw_context *brw,
   unsigned stencil_level = level;
   blorp_surf_for_miptree(brw, _surf, stencil_mt,
  ISL_AUX_USAGE_NONE, true,
- _level, start_layer, num_layers,
- _tmp[2]);
+ _level, start_layer, num_layers);
}
 
assert((mask & BUFFER_BIT_DEPTH) || stencil_mask);
@@ -1525,11 +1517,9 @@ brw_blorp_resolve_color(struct brw_context *brw, struct 
intel_mipmap_tree *mt,
 
const mesa_format format = _mesa_get_srgb_format_linear(mt->format);
 
-   struct isl_surf isl_tmp[1];
struct blorp_surf surf;

Re: [Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue

[Jose Fonseca]

On 08/03/2019 22:52, Brian Paul wrote:

After a while of running google-chrome and viewing Bing maps, we'd see
"context mismatch in svga_sampler_view_destroy()" messages and
eventually crash because we leaked too many sampler views (the kernel
module would have too many sampler views).

When a texture object is being deleted, we call
st_texture_release_all_sampler_views() to release all the sampler
views.  In the list, there may sampler views which were created from
other contexts.

Previously, we called pipe_sampler_view_release(pipe, view) which would
use the given pipe context to destroy the view if the refcount hit
zero.  The svga error was triggered because we were calling
pipe->sampler_view_destroy(pipe, view) and the pipe context didn't
match the view's parent context.

Instead, call pipe_sampler_reference(, NULL).  That way, if
the refcount hits zero, we'll use the view's parent context to
destroy the view.  That's what we want.

The pipe_sampler_view_release() function was introduced years ago to
avoid freeing a sampler view with a context that was already deleted.

But since then we've improved sampler view and context tracking.
When a context is destroyed, the state tracker walks over all
texture objects and frees all sampler views which belong to that
context.  So, we should never end up deleting a sampler view after
its context is deleted.

After this, we can remove all calls to pipe_sampler_view_release()
in the drivers.

Finally, it appears that we need to implement a similar tear-down
mechanism for shaders and programs since we may generate per-context
shader variants.

Testing done: google chrome, misc GL demos, games
---
  src/mesa/state_tracker/st_context.c  | 3 +--
  src/mesa/state_tracker/st_sampler_view.c | 8 
  2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/src/mesa/state_tracker/st_context.c 
b/src/mesa/state_tracker/st_context.c
index 2898279..a7464fd 100644
--- a/src/mesa/state_tracker/st_context.c
+++ b/src/mesa/state_tracker/st_context.c
@@ -278,8 +278,7 @@ st_destroy_context_priv(struct st_context *st, bool 
destroy_pipe)
 st_destroy_bound_image_handles(st);
  
 for (i = 0; i < ARRAY_SIZE(st->state.frag_sampler_views); i++) {

-  pipe_sampler_view_release(st->pipe,
->state.frag_sampler_views[i]);
+  pipe_sampler_view_reference(>state.frag_sampler_views[i], NULL);
 }
  
 /* free glReadPixels cache data */

diff --git a/src/mesa/state_tracker/st_sampler_view.c 
b/src/mesa/state_tracker/st_sampler_view.c
index e4eaf39..650a2b0 100644
--- a/src/mesa/state_tracker/st_sampler_view.c
+++ b/src/mesa/state_tracker/st_sampler_view.c
@@ -74,7 +74,7 @@ st_texture_set_sampler_view(struct st_context *st,
if (sv->view) {
   /* check if the context matches */
   if (sv->view->context == st->pipe) {
-pipe_sampler_view_release(st->pipe, >view);
+pipe_sampler_view_reference(>view, NULL);
  goto found;
   }
} else {
@@ -94,13 +94,13 @@ st_texture_set_sampler_view(struct st_context *st,
  
   if (new_max < views->max ||

   new_max > (UINT_MAX - sizeof(*views)) / sizeof(views->views[0])) 
{
-pipe_sampler_view_release(st->pipe, );
+pipe_sampler_view_reference(, NULL);
  goto out;
   }
  
   struct st_sampler_views *new_views = malloc(new_size);

   if (!new_views) {
-pipe_sampler_view_release(st->pipe, );
+pipe_sampler_view_reference(, NULL);
  goto out;
   }
  
@@ -225,7 +225,7 @@ st_texture_release_all_sampler_views(struct st_context *st,

 simple_mtx_lock(>validate_mutex);
 struct st_sampler_views *views = stObj->sampler_views;
 for (i = 0; i < views->count; ++i)
-  pipe_sampler_view_release(st->pipe, >views[i].view);
+  pipe_sampler_view_reference(>views[i].view, NULL);
 simple_mtx_unlock(>validate_mutex);
  }
  



With your upcoming change that prevents the sampler view from ever being 
released with the wrong context (which I suppose should go before this 
one?), this series is


Reviewed-By: Jose Fonseca 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 109159] [KBL-G][vulkan] dEQP-VK.api.version_check.entry_points test failed.

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=109159

--- Comment #4 from Samuel Pitoiset  ---
Any news on this?
This test always pass on my side.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nir/spirv: short-circuit when conditional branch contains end block

[Jason Ekstrand]

Looking at this a bit more, I'm not sure that just short-circuiting
actually covers all the cases.  Unfortunately, we don't know what all the
cases are because the SPIR-V spec doesn't say.  I'm trying to work towards
fixing that right now.

--Jason

On Wed, Mar 6, 2019 at 5:25 AM Juan A. Suarez Romero 
wrote:

> This fixes the case when the SPIR-V code has two nested conditional
> branches, but only one selection merge:
>
> [...]
> %1 = OpLabel
>  OpSelectionMerge %2 None
>  OpBranchConditional %3 %4 %2
> %4 = OpLabel
>  OpBranchConditional %3 %5 %2
> %5 = OpLabel
>  OpBranch %2
> %2 = OpLabel
> [...]
>
> In the second OpBranchConditional, as the else-part is the end
> block (started in the first OpBranchConditional) we can just follow the
> then-part.
>
> This fixes dEQP-VK.vkrunner.controlflow.2-obc-triangle-triangle
>
> CC: Jason Ekstrand 
> ---
>  src/compiler/spirv/vtn_cfg.c | 11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/src/compiler/spirv/vtn_cfg.c b/src/compiler/spirv/vtn_cfg.c
> index 7868eeb60bc..f749118efbe 100644
> --- a/src/compiler/spirv/vtn_cfg.c
> +++ b/src/compiler/spirv/vtn_cfg.c
> @@ -605,7 +605,16 @@ vtn_cfg_walk_blocks(struct vtn_builder *b, struct
> list_head *cf_list,
>  }
>   } else if (if_stmt->then_type == vtn_branch_type_none &&
>  if_stmt->else_type == vtn_branch_type_none) {
> -/* Neither side of the if is something we can short-circuit.
> */
> +/* Neither side of the if is something we can short-circuit,
> + * unless one of the blocks is the end block. */
> +if (then_block == end) {
> +   block = else_block;
> +   continue;
> +} else if (else_block == end) {
> +   block = then_block;
> +   continue;
> +}
> +
>  vtn_assert((*block->merge & SpvOpCodeMask) ==
> SpvOpSelectionMerge);
>  struct vtn_block *merge_block =
> vtn_value(b, block->merge[1], vtn_value_type_block)->block;
> --
> 2.20.1
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 107563] [RADV] Broken rendering in Unity demos

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=107563

Samuel Pitoiset  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #11 from Samuel Pitoiset  ---
I actually found the root cause of the VM faults. Should be fixed with
https://cgit.freedesktop.org/mesa/mesa/commit/?id=3a2e93147f7fa4a6fd17313353113a33291c5ce0

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] util: Add a drm_find_modifier helper

[Eric Anholt]

Alyssa Rosenzweig  writes:

> This function is replicated across vc4/v3d/freedreno and is needed in
> Panfrost; let's make this shared code.
>
> Signed-off-by: Alyssa Rosenzweig 
> ---
>  src/util/u_drm.h | 46 ++
>  1 file changed, 46 insertions(+)
>  create mode 100644 src/util/u_drm.h
>
> diff --git a/src/util/u_drm.h b/src/util/u_drm.h
> new file mode 100644
> index 000..d543c9a7543
> --- /dev/null
> +++ b/src/util/u_drm.h
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright © 2014 Broadcom
> + * Copyright (C) 2012 Rob Clark 
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef U_DRM_H
> +#define U_DRM_H
> +
> +#include 

Maybe add a #include  since you also use bool.

Other than that, the series is:

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 9/9] mesa: Add assert to _mesa_primitive_restart_index.

[Mathias Fröhlich]

Hi Brian,

On Thursday, 14 March 2019 14:55:11 CET Brian Paul wrote:
> yeah, looks good.  Thanks.
> 
> Reviewed-by: Brian Paul 
> 
> I don't remember- do you need me to push your patches for you?
Thanks for the review!

I have rights to push.
Thanks for asking!!

best

Mathias



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] util: Add a drm_find_modifier helper

[Alyssa Rosenzweig]

> hmm, I guess there are already two or three other places that already
> copy/pasta'd this specifically for modifiers.. so I'm ok with the less
> generic name, since it (IMHO) makes the code calling it more clear.

This was my reasoning, yeah... I'm not sure why it is so generic,
honestly (should there be DRM-specific sanity checking? Maybe we trust
the drivers to know what they're doing.. ;) )
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] panfrost: Adapt to uapi changes

[Alyssa Rosenzweig]

Reviewed-by: Alyssa Rosenzweig 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 109929] tgsi_to_nir.c:2111: undefined reference to `gl_nir_lower_samplers_as_deref'

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=109929

--- Comment #5 from Timur Kristóf  ---
Okay, I think I got it. Can you please try with this patch:
https://gitlab.freedesktop.org/Venemo/mesa/commit/c1efd5b26d210b017118d29ecf16aaaeeb34a420

It gets rid of the problem for me.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] panfrost: Adapt to uapi changes

[Tomeu Vizoso]

Two ioctls had wrong DRM_IO* flags.

Signed-off-by: Tomeu Vizoso 
---
 include/drm-uapi/panfrost_drm.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/drm-uapi/panfrost_drm.h b/include/drm-uapi/panfrost_drm.h
index a0ead4979ccc..508b9621d9db 100644
--- a/include/drm-uapi/panfrost_drm.h
+++ b/include/drm-uapi/panfrost_drm.h
@@ -19,8 +19,8 @@ extern "C" {
 #define DRM_PANFROST_GET_PARAM 0x04
 #define DRM_PANFROST_GET_BO_OFFSET 0x05
 
-#define DRM_IOCTL_PANFROST_SUBMIT  DRM_IOWR(DRM_COMMAND_BASE + 
DRM_PANFROST_SUBMIT, struct drm_panfrost_submit)
-#define DRM_IOCTL_PANFROST_WAIT_BO DRM_IOWR(DRM_COMMAND_BASE + 
DRM_PANFROST_WAIT_BO, struct drm_panfrost_wait_bo)
+#define DRM_IOCTL_PANFROST_SUBMIT  DRM_IOW(DRM_COMMAND_BASE + 
DRM_PANFROST_SUBMIT, struct drm_panfrost_submit)
+#define DRM_IOCTL_PANFROST_WAIT_BO DRM_IOW(DRM_COMMAND_BASE + 
DRM_PANFROST_WAIT_BO, struct drm_panfrost_wait_bo)
 #define DRM_IOCTL_PANFROST_CREATE_BO   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_PANFROST_CREATE_BO, struct drm_panfrost_create_bo)
 #define DRM_IOCTL_PANFROST_MMAP_BO DRM_IOWR(DRM_COMMAND_BASE + 
DRM_PANFROST_MMAP_BO, struct drm_panfrost_mmap_bo)
 #define DRM_IOCTL_PANFROST_GET_PARAM   DRM_IOWR(DRM_COMMAND_BASE + 
DRM_PANFROST_GET_PARAM, struct drm_panfrost_get_param)
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nir/spirv: short-circuit when conditional branch contains end block

[Jason Ekstrand]

On Thu, Mar 14, 2019 at 4:09 AM Juan A. Suarez Romero 
wrote:

> On Fri, 2019-03-08 at 13:29 -0600, Jason Ekstrand wrote:
>
> On Fri, Mar 8, 2019 at 9:30 AM Juan A. Suarez Romero 
> wrote:
>
> On Thu, 2019-03-07 at 07:15 -0600, Jason Ekstrand wrote:
> > Woah, is this legal SPIR-V? I think a second OpSelectionMerge is
> required.
>
> I'd say it is legal. The spec does not mandate that each branch has its own
> merge instruction; only that the control flow must be structured for
> shaders.
>
>
> That's exactly the problem.  It says how merge instructions work and how
> they have to be nested but never that or where you have to use them. :-(
> This is a spec bug.  The closest I can find is this line from the
> structured control flow section: "These explicitly declare a header block
> 
> before the control flow diverges and a merge block
> 
> where control flow subsequently converges."  If you read that as "you must
> declare divergence" then it would imply that every OpBranchConditional must
> be preceded by an OpSelectionMerge.  If we don't require this, there are
> lots of weird cases where you can get into diamond patters and other things
> that aren't trivially structurizable.
>
>
> I agree, but I understand in this case the second branch is not diverging,
> but converging to the merge case already defined in the selection merge.
>
> I found a couple of similar questions in public github, which were
> resolved by stating that not all OpBranchConditional must be immediately
> preceded by an OpSelectionMerge or OpLoopMerge.
>
> https://github.com/KhronosGroup/glslang/issues/150#issuecomment-178185325
>
> https://github.com/KhronosGroup/SPIRV-Tools/issues/1185#issuecomment-356457613
>

Grumble grumble This is a backwards-incompatible SPIR-V change

Ok, I'll look at this some more in the fresh light.  I'm not sure if the
short-circuit you're doing is correct but we'll see.



> J.A.
>
> --Jason
>
>
> In section 2.11 ("Structured Control Flow"), about structured control-flow
> constructs:
>
>
> - A structured control-flow construct is then defined as one of:
>   - a selection construct: the set of blocks dominated by a selection
> header,
> minus the set of blocks dominated by the header’s merge block
>   - [...]
>
> - The above structured control-flow constructs must satisfy the following
> rules:
>   - [...]
>   - the only blocks in a construct that can branch outside the construct
> are
> - a block branching to the construct’s merge block
> - a block branching from one case construct to another, for the same
> OpSwitch
> - a back-edge block
> - a continue block for the innermost loop it is nested inside of
> - a break block for the innermost loop it is nested inside of
> - a return block
>
>
> Our selection construct, which contains the two nested conditional
> branches,
> satisfies the rules, as both conditionals branches to the construct's merge
> block.
>
>
> J.A.
>
>
>
>
>
>
> >
>
>
> > --Jason
> >
> >
> > On March 6, 2019 05:25:26 "Juan A. Suarez Romero" 
> wrote:
> >
> > > This fixes the case when the SPIR-V code has two nested conditional
> > > branches, but only one selection merge:
> > >
> > >
> > > [...]
> > > %1 = OpLabel
> > > OpSelectionMerge %2 None
> > > OpBranchConditional %3 %4 %2
> > > %4 = OpLabel
> > > OpBranchConditional %3 %5 %2
> > > %5 = OpLabel
> > > OpBranch %2
> > > %2 = OpLabel
> > > [...]
> > >
> > >
> > > In the second OpBranchConditional, as the else-part is the end
> > > block (started in the first OpBranchConditional) we can just follow the
> > > then-part.
> > >
> > >
> > > This fixes dEQP-VK.vkrunner.controlflow.2-obc-triangle-triangle
> > >
> > >
> > > CC: Jason Ekstrand 
> > > ---
> > > src/compiler/spirv/vtn_cfg.c | 11 ++-
> > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > >
> > > diff --git a/src/compiler/spirv/vtn_cfg.c
> b/src/compiler/spirv/vtn_cfg.c
> > > index 7868eeb60bc..f749118efbe 100644
> > > --- a/src/compiler/spirv/vtn_cfg.c
> > > +++ b/src/compiler/spirv/vtn_cfg.c
> > > @@ -605,7 +605,16 @@ vtn_cfg_walk_blocks(struct vtn_builder *b, struct
> > > list_head *cf_list,
> > > }
> > >  } else if (if_stmt->then_type == vtn_branch_type_none &&
> > > if_stmt->else_type == vtn_branch_type_none) {
> > > -/* Neither side of the if is something we can
> short-circuit. */
> > > +/* Neither side of the if is something we can
> short-circuit,
> > > + * unless one of the blocks is the end block. */
> > > +if (then_block == end) {
> > > +   block = else_block;
> > > +   continue;
> > > +} else if (else_block == end) {
> > > +   block = then_block;
> > > +   continue;
> > > +}
> > > +
> 

Re: [Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue

[Brian Paul]

On 03/14/2019 12:24 AM, Mathias Fröhlich wrote:

Hi Brian,

On Sunday, 10 March 2019 21:32:39 CET Brian Paul wrote:

On Friday, 8 March 2019 23:52:09 CET Brian Paul wrote:

After a while of running google-chrome and viewing Bing maps, we'd see
"context mismatch in svga_sampler_view_destroy()" messages and
eventually crash because we leaked too many sampler views (the kernel
module would have too many sampler views).

When a texture object is being deleted, we call
st_texture_release_all_sampler_views() to release all the sampler
views.  In the list, there may sampler views which were created from
other contexts.

Previously, we called pipe_sampler_view_release(pipe, view) which would
use the given pipe context to destroy the view if the refcount hit
zero.  The svga error was triggered because we were calling
pipe->sampler_view_destroy(pipe, view) and the pipe context didn't
match the view's parent context.

Instead, call pipe_sampler_reference(, NULL).  That way, if
the refcount hits zero, we'll use the view's parent context to
destroy the view.  That's what we want.


That sounds plausible so far.

What I wonder, is calling into an 'arbitrary' different context and do
work there
thread safe for any gallium driver?



Right.  Before this patch there was the problem of trying to destroy a
sampler view with a context different from the one which created it.
That's fixed, but now there's the multi-threading issue as you say, and
which I believe, is a less-common situation.


Hmm, you are saying we tolerate a race condition then?


No, I'm planning to fix that too, as described below.





Especially since pipe_sampler_view_reference in its documentation says
that the
views provided need to originate from the same context and that this must
be the 'current'.

I have been taking a quick look into iris and radeonsi and both seem to be
safe
in terms of threads for dereferencing the views finally.
But either the documentation of pipe_sampler_view_reference should be
updated
or I may miss an other piece of the puzzle to see that it is still save to
do so.



Last week I posted a patch series for the VMware driver which handled the
thread issue with a list of "zombie" sampler views.  Jose suggested moving
that up into the state tracker, and that is a better long-term solution.

I'm almost done with a patch series that does that.  I'll likely post it
tomorrow.


This is taking longer than expected (long testing, plus a sick child and 
wife at home).





Ok, so basically you are saying that you improve the current situation with this
current series, still leaving a race condition open. But a good final fix to 
the race
condition is underway.


Yeah, I can probably stage the multi-thread/race patch series before 
this context-fix-up series.





Anyhow, to me it looks like all the reference counting inlines in gallium seem
to assume being truely thread safe and do not assume that the context that
created the view needs to be current on dereferencing.
Means: is this comment at the pipe_sampler_view_*reference that states
that the owning context needs to be current really correct?
Such a comment implies to me that the context possibly has a non thread safe 
list
of views that may get corrupted if being called from an other current context 
may
be current in an other thread.
... well or something similar to that.
Do you remember where this comment comes from?
Is there a driver that requires the owning context needs to be current property?


I'm not sure I grok all that.  But since pipe_sampler_views are 
per-context objects, I believe they should only be 
created/destroyed/used/referenced from the same context.


-Brian

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: always initialize HTILE when the src layout is UNDEFINED

[Bas Nieuwenhuizen]

r-b

On Thu, Mar 14, 2019 at 2:24 PM Samuel Pitoiset
 wrote:
>
> HTILE should always be initialized when transitioning from
> VK_IMAGE_LAYOUT_UNDEFINED to other image layouts. Otherwise,
> if an app does a transition from UNDEFINED to GENERAL, the
> driver doesn't initialize HTILE and it tries to decompress
> the depth surface. For some reasons, this results in VM faults.
>
> Cc: mesa-sta...@lists.freedesktop.org
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107563
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/amd/vulkan/radv_cmd_buffer.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_cmd_buffer.c 
> b/src/amd/vulkan/radv_cmd_buffer.c
> index 06806ed6fce..ae8f50d0348 100644
> --- a/src/amd/vulkan/radv_cmd_buffer.c
> +++ b/src/amd/vulkan/radv_cmd_buffer.c
> @@ -4477,8 +4477,7 @@ static void radv_handle_depth_image_transition(struct 
> radv_cmd_buffer *cmd_buffe
> if (!radv_image_has_htile(image))
> return;
>
> -   if (src_layout == VK_IMAGE_LAYOUT_UNDEFINED &&
> -  radv_layout_has_htile(image, dst_layout, dst_queue_mask)) {
> +   if (src_layout == VK_IMAGE_LAYOUT_UNDEFINED) {
> /* TODO: merge with the clear if applicable */
> radv_initialize_htile(cmd_buffer, image, range, 0);
> } else if (!radv_layout_is_htile_compressed(image, src_layout, 
> src_queue_mask) &&
> --
> 2.21.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 9/9] mesa: Add assert to _mesa_primitive_restart_index.

[Brian Paul]

On 03/14/2019 06:33 AM, mathias.froehl...@gmx.net wrote:

From: Mathias Fröhlich 

Hi Brian,

You mean an assert like this?


yeah, looks good.  Thanks.

Reviewed-by: Brian Paul 

I don't remember- do you need me to push your patches for you?

-Brian



This patch also made it together with the rest of the series through intels CI.

best

Mathias





Make sure the inde_size parameter is meant to be in bytes.

Signed-off-by: Mathias Fröhlich 
---
  src/mesa/main/varray.h | 3 +++
  1 file changed, 3 insertions(+)

diff --git a/src/mesa/main/varray.h b/src/mesa/main/varray.h
index 0be57971bd7..2831720edfc 100644
--- a/src/mesa/main/varray.h
+++ b/src/mesa/main/varray.h
@@ -316,6 +316,9 @@ static inline unsigned
  _mesa_primitive_restart_index(const struct gl_context *ctx,
unsigned index_size)
  {
+   /* The index_size parameter is menat to be in bytes. */
+   assert(index_size == 1 || index_size == 2 || index_size == 4);
+
 /* From the OpenGL 4.3 core specification, page 302:
  * "If both PRIMITIVE_RESTART and PRIMITIVE_RESTART_FIXED_INDEX are
  *  enabled, the index value determined by PRIMITIVE_RESTART_FIXED_INDEX



___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radv: always initialize HTILE when the src layout is UNDEFINED

[Samuel Pitoiset]

HTILE should always be initialized when transitioning from
VK_IMAGE_LAYOUT_UNDEFINED to other image layouts. Otherwise,
if an app does a transition from UNDEFINED to GENERAL, the
driver doesn't initialize HTILE and it tries to decompress
the depth surface. For some reasons, this results in VM faults.

Cc: mesa-sta...@lists.freedesktop.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107563
Signed-off-by: Samuel Pitoiset 
---
 src/amd/vulkan/radv_cmd_buffer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
index 06806ed6fce..ae8f50d0348 100644
--- a/src/amd/vulkan/radv_cmd_buffer.c
+++ b/src/amd/vulkan/radv_cmd_buffer.c
@@ -4477,8 +4477,7 @@ static void radv_handle_depth_image_transition(struct 
radv_cmd_buffer *cmd_buffe
if (!radv_image_has_htile(image))
return;
 
-   if (src_layout == VK_IMAGE_LAYOUT_UNDEFINED &&
-  radv_layout_has_htile(image, dst_layout, dst_queue_mask)) {
+   if (src_layout == VK_IMAGE_LAYOUT_UNDEFINED) {
/* TODO: merge with the clear if applicable */
radv_initialize_htile(cmd_buffer, image, range, 0);
} else if (!radv_layout_is_htile_compressed(image, src_layout, 
src_queue_mask) &&
-- 
2.21.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] gallium/docs: clarify set_sampler_views

[Rob Clark]

On Tue, Mar 12, 2019 at 1:59 PM Roland Scheidegger  wrote:
>
> Am 12.03.19 um 16:16 schrieb Rob Clark:
> > This previously was not called out clearly, but based on a survey of the
> > code, it seems the expected behavior is to release the reference to any
> > sampler views beyond the new range being bound.
>
> That isn't really true. This was designed to work like d3d10, where
> other views are unmodified.
> The cso code will actually unset all views which previously were set and
> are above the num_views in the call (this wouldn't be necessary if the
> pipe function itself would work like this).
> However, it will only do this for fragment textures, and pass through
> the parameters unmodified otherwise. Which means behavior might not be
> very consistent for the different stages...

Any opinion about whether views==NULL should be allowed?  Currently I
have something like:


diff --git a/src/gallium/docs/source/context.rst
b/src/gallium/docs/source/context.rst
index f89d9e1005e..06d30bfb38b 100644
--- a/src/gallium/docs/source/context.rst
+++ b/src/gallium/docs/source/context.rst
@@ -143,6 +143,11 @@ to the array index which is used for sampling.
   to a respective sampler view and releases a reference to the previous
   sampler view.

+  Sampler views outside of ``[start_slot, start_slot + num_views)`` are
+  unmodified.  If ``views`` is NULL, the behavior is the same as if
+  ``views[n]`` was NULL for the entire range, ie. releasing the reference
+  for all the sampler views in the specified range.
+
 * ``create_sampler_view`` creates a new sampler view. ``texture`` is associated
   with the sampler view which results in sampler view holding a reference
   to the texture. Format specified in template must be compatible


But going thru the other drivers, a lot of them also don't handle the
views==NULL case.  This case doesn't seem to come up with mesa/st, but
does with XA and nine, and some of the test code.

BR,
-R

>
>
> >
> > I think radeonsi and freedreno were the only ones not doing this.  Which
> > could probably temporarily leak a bit of memory by holding on to the
> > sampler view reference.
> Not sure about other drivers, but llvmpipe will not do this neither.
>
> Roland
>
>
> >
> > Signed-off-by: Rob Clark 
> > ---
> >  src/gallium/docs/source/context.rst | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/src/gallium/docs/source/context.rst 
> > b/src/gallium/docs/source/context.rst
> > index f89d9e1005e..199d335f8f4 100644
> > --- a/src/gallium/docs/source/context.rst
> > +++ b/src/gallium/docs/source/context.rst
> > @@ -143,6 +143,9 @@ to the array index which is used for sampling.
> >to a respective sampler view and releases a reference to the previous
> >sampler view.
> >
> > +  Previously bound samplers with index ``>= num_views`` are unbound rather
> > +  than unmodified.
> > +
> >  * ``create_sampler_view`` creates a new sampler view. ``texture`` is 
> > associated
> >with the sampler view which results in sampler view holding a reference
> >to the texture. Format specified in template must be compatible
> >
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/4] freedreno: Use shared drm_find_modifier util

[Rob Clark]

On Thu, Mar 14, 2019 at 12:19 AM Alyssa Rosenzweig  wrote:
>
> Signed-off-by: Alyssa Rosenzweig 
> Cc: Rob Clark 


thanks, r-b

> ---
>  .../drivers/freedreno/freedreno_resource.c| 20 ---
>  1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/src/gallium/drivers/freedreno/freedreno_resource.c 
> b/src/gallium/drivers/freedreno/freedreno_resource.c
> index 36d61d715ef..620ed4cad41 100644
> --- a/src/gallium/drivers/freedreno/freedreno_resource.c
> +++ b/src/gallium/drivers/freedreno/freedreno_resource.c
> @@ -32,6 +32,7 @@
>  #include "util/u_string.h"
>  #include "util/u_surface.h"
>  #include "util/set.h"
> +#include "util/u_drm.h"
>
>  #include "freedreno_resource.h"
>  #include "freedreno_batch_cache.h"
> @@ -830,19 +831,6 @@ has_depth(enum pipe_format format)
> }
>  }
>
> -static bool
> -find_modifier(uint64_t needle, const uint64_t *haystack, int count)
> -{
> -   int i;
> -
> -   for (i = 0; i < count; i++) {
> -   if (haystack[i] == needle)
> -   return true;
> -   }
> -
> -   return false;
> -}
> -
>  /**
>   * Create a new texture object, using the given template info.
>   */
> @@ -906,7 +894,7 @@ fd_resource_create_with_modifiers(struct pipe_screen 
> *pscreen,
>  PIPE_BIND_LINEAR  | \
>  PIPE_BIND_DISPLAY_TARGET)
>
> -   bool linear = find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, count);
> +   bool linear = drm_find_modifier(DRM_FORMAT_MOD_LINEAR, modifiers, 
> count);
> if (tmpl->bind & LINEAR)
> linear = true;
>
> @@ -918,9 +906,9 @@ fd_resource_create_with_modifiers(struct pipe_screen 
> *pscreen,
>  * except we don't have a format modifier for tiled.  (We probably
>  * should.)
>  */
> -   bool allow_ubwc = find_modifier(DRM_FORMAT_MOD_INVALID, modifiers, 
> count);
> +   bool allow_ubwc = drm_find_modifier(DRM_FORMAT_MOD_INVALID, 
> modifiers, count);
> if (tmpl->bind & PIPE_BIND_SHARED)
> -   allow_ubwc = find_modifier(DRM_FORMAT_MOD_QCOM_COMPRESSED, 
> modifiers, count);
> +   allow_ubwc = 
> drm_find_modifier(DRM_FORMAT_MOD_QCOM_COMPRESSED, modifiers, count);
>
> /* TODO turn on UBWC for all internal buffers
>  * Manhattan benchmark shows artifacts when enabled.  Once this
> --
> 2.20.1
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] util: Add a drm_find_modifier helper

[Rob Clark]

On Thu, Mar 14, 2019 at 5:51 AM Eric Engestrom  wrote:
>
> On Thursday, 2019-03-14 04:19:02 +, Alyssa Rosenzweig wrote:
> > This function is replicated across vc4/v3d/freedreno and is needed in
> > Panfrost; let's make this shared code.
> >
> > Signed-off-by: Alyssa Rosenzweig 
> > ---
> >  src/util/u_drm.h | 46 ++
> >  1 file changed, 46 insertions(+)
> >  create mode 100644 src/util/u_drm.h
> >
> > diff --git a/src/util/u_drm.h b/src/util/u_drm.h
> > new file mode 100644
> > index 000..d543c9a7543
> > --- /dev/null
> > +++ b/src/util/u_drm.h
> > @@ -0,0 +1,46 @@
> > +/*
> > + * Copyright © 2014 Broadcom
> > + * Copyright (C) 2012 Rob Clark 
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the 
> > "Software"),
> > + * to deal in the Software without restriction, including without 
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice (including the 
> > next
> > + * paragraph) shall be included in all copies or substantial portions of 
> > the
> > + * Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS 
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> > + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR 
> > OTHER
> > + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> > + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> > DEALINGS
> > + * IN THE SOFTWARE.
> > + */
> > +
> > +#ifndef U_DRM_H
> > +#define U_DRM_H
> > +
> > +#include 
> > +
> > +/* Given a list of `count` DRM modifiers `haystack` and a desired modifier
> > + * `needle`, returns whether the modifier is found */
> > +
> > +static bool
> > +drm_find_modifier(uint64_t needle, const uint64_t *haystack, unsigned 
> > count)
> > +{
> > +unsigned i;
> > +
> > +for (i = 0; i < count; i++) {
> > +if (haystack[i] == needle)
> > +return true;
> > +}
> > +
> > +return false;
> > +}
>
> This is an extremely generic loop; it has nothing to do with modifiers,
> they just happen to be uint64_t.
>
> Can we call it something like this?
>   static inline bool
>   array_contains_u64(uint64_t *array, size_t array_size, uint64_t needle)
>
> There could be other places that could use this function (or its
> u32/s64/s32 equivalents), there's no reason to prevent its adoption with
> a too specific name :)

hmm, I guess there are already two or three other places that already
copy/pasta'd this specifically for modifiers.. so I'm ok with the less
generic name, since it (IMHO) makes the code calling it more clear.

If there are other spots that need to search an array of other u64's,
then we could always do something like:

static inline bool
drm_find_modifier(...)
{
   return array_contains_u64(...);
}

seems a bit overkill..

Either way, w/ missing inline fixed, r-b

BR,
-R

>
> With a generic name (and the missing `inline` Christian also mentioned),
> the series is:
> Reviewed-by: Eric Engestrom 
> ___
> 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] [PATCH 9/9] mesa: Add assert to _mesa_primitive_restart_index.

[Mathias . Froehlich]

From: Mathias Fröhlich 

Hi Brian,

You mean an assert like this?
This patch also made it together with the rest of the series through intels CI.

best

Mathias





Make sure the inde_size parameter is meant to be in bytes.

Signed-off-by: Mathias Fröhlich 
---
 src/mesa/main/varray.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/mesa/main/varray.h b/src/mesa/main/varray.h
index 0be57971bd7..2831720edfc 100644
--- a/src/mesa/main/varray.h
+++ b/src/mesa/main/varray.h
@@ -316,6 +316,9 @@ static inline unsigned
 _mesa_primitive_restart_index(const struct gl_context *ctx,
   unsigned index_size)
 {
+   /* The index_size parameter is menat to be in bytes. */
+   assert(index_size == 1 || index_size == 2 || index_size == 4);
+
/* From the OpenGL 4.3 core specification, page 302:
 * "If both PRIMITIVE_RESTART and PRIMITIVE_RESTART_FIXED_INDEX are
 *  enabled, the index value determined by PRIMITIVE_RESTART_FIXED_INDEX
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/9] mesa: Implement helper functions to map and unmap a VAO.

[Mathias . Froehlich]

From: Mathias Fröhlich 

Brian,

For reference the v2 with the updated comments.

Thanks and best

Mathias

Provide a set of functions that maps or unmaps all VBOs held
in a VAO. The functions will be used in the following patches.

v2: Update comments.

Signed-off-by: Mathias Fröhlich 
---
 src/mesa/main/arrayobj.c | 84 
 src/mesa/main/arrayobj.h | 18 +
 2 files changed, 102 insertions(+)

diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
index 68d30aa9b1f..63138096da6 100644
--- a/src/mesa/main/arrayobj.c
+++ b/src/mesa/main/arrayobj.c
@@ -913,6 +913,90 @@ _mesa_all_buffers_are_unmapped(const struct 
gl_vertex_array_object *vao)
return true;
 }
 
+
+/**
+ * Map buffer objects used in attribute arrays.
+ */
+void
+_mesa_vao_map_arrays(struct gl_context *ctx, struct gl_vertex_array_object 
*vao,
+ GLbitfield access)
+{
+   GLbitfield mask = vao->Enabled & vao->VertexAttribBufferMask;
+   while (mask) {
+  /* Do not use u_bit_scan as we can walk multiple attrib arrays at once */
+  const gl_vert_attrib attr = ffs(mask) - 1;
+  const GLubyte bindex = vao->VertexAttrib[attr].BufferBindingIndex;
+  struct gl_vertex_buffer_binding *binding = >BufferBinding[bindex];
+  mask &= ~binding->_BoundArrays;
+
+  struct gl_buffer_object *bo = binding->BufferObj;
+  assert(_mesa_is_bufferobj(bo));
+  if (_mesa_bufferobj_mapped(bo, MAP_INTERNAL))
+ continue;
+
+  ctx->Driver.MapBufferRange(ctx, 0, bo->Size, access, bo, MAP_INTERNAL);
+   }
+}
+
+
+/**
+ * Map buffer objects used in the vao, attribute arrays and index buffer.
+ */
+void
+_mesa_vao_map(struct gl_context *ctx, struct gl_vertex_array_object *vao,
+  GLbitfield access)
+{
+   struct gl_buffer_object *bo = vao->IndexBufferObj;
+
+   /* map the index buffer, if there is one, and not already mapped */
+   if (_mesa_is_bufferobj(bo) && !_mesa_bufferobj_mapped(bo, MAP_INTERNAL))
+  ctx->Driver.MapBufferRange(ctx, 0, bo->Size, access, bo, MAP_INTERNAL);
+
+   _mesa_vao_map_arrays(ctx, vao, access);
+}
+
+
+/**
+ * Unmap buffer objects used in attribute arrays.
+ */
+void
+_mesa_vao_unmap_arrays(struct gl_context *ctx,
+   struct gl_vertex_array_object *vao)
+{
+   GLbitfield mask = vao->Enabled & vao->VertexAttribBufferMask;
+   while (mask) {
+  /* Do not use u_bit_scan as we can walk multiple attrib arrays at once */
+  const gl_vert_attrib attr = ffs(mask) - 1;
+  const GLubyte bindex = vao->VertexAttrib[attr].BufferBindingIndex;
+  struct gl_vertex_buffer_binding *binding = >BufferBinding[bindex];
+  mask &= ~binding->_BoundArrays;
+
+  struct gl_buffer_object *bo = binding->BufferObj;
+  assert(_mesa_is_bufferobj(bo));
+  if (!_mesa_bufferobj_mapped(bo, MAP_INTERNAL))
+ continue;
+
+  ctx->Driver.UnmapBuffer(ctx, bo, MAP_INTERNAL);
+   }
+}
+
+
+/**
+ * Unmap buffer objects used in the vao, attribute arrays and index buffer.
+ */
+void
+_mesa_vao_unmap(struct gl_context *ctx, struct gl_vertex_array_object *vao)
+{
+   struct gl_buffer_object *bo = vao->IndexBufferObj;
+
+   /* unmap the index buffer, if there is one, and still mapped */
+   if (_mesa_is_bufferobj(bo) && _mesa_bufferobj_mapped(bo, MAP_INTERNAL))
+  ctx->Driver.UnmapBuffer(ctx, bo, MAP_INTERNAL);
+
+   _mesa_vao_unmap_arrays(ctx, vao);
+}
+
+
 /**/
 /* API Functions  */
 /**/
diff --git a/src/mesa/main/arrayobj.h b/src/mesa/main/arrayobj.h
index ee87b4b6ba5..7516bae9e39 100644
--- a/src/mesa/main/arrayobj.h
+++ b/src/mesa/main/arrayobj.h
@@ -100,6 +100,24 @@ extern bool
 _mesa_all_buffers_are_unmapped(const struct gl_vertex_array_object *vao);
 
 
+extern void
+_mesa_vao_map_arrays(struct gl_context *ctx, struct gl_vertex_array_object 
*vao,
+ GLbitfield access);
+
+extern void
+_mesa_vao_map(struct gl_context *ctx, struct gl_vertex_array_object *vao,
+  GLbitfield access);
+
+
+extern void
+_mesa_vao_unmap_arrays(struct gl_context *ctx,
+   struct gl_vertex_array_object *vao);
+
+extern void
+_mesa_vao_unmap(struct gl_context *ctx,
+struct gl_vertex_array_object *vao);
+
+
 /**
  * Array to apply the position/generic0 aliasing map to
  * an attribute value used in vertex processing inputs to an attribute
-- 
2.20.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Mesa-stable] [PATCH 2/3] glsl: TCS outputs can not be transform feedback candidates on GLES

[Chema Casanova]

On 13/3/19 23:17, Emil Velikov wrote:
> Hi Jose,
> 
> On Wed, 21 Nov 2018 at 18:45, Jose Maria Casanova Crespo
>  wrote:
>>
>> Fixes: 
>> KHR-GLES*.core.tessellation_shader.single.xfb_captures_data_from_correct_stage
>>
> This and the follow-up patch "glsl: fix recording of variables for XFB
> in TCS shaders" are explicitly marked as 19.0 only.
> As such I've omitted them from 18.3, let me know if you prefer to include 
> them.

I've checked and both patches apply clearly on 18.3 and they fix the CTS
failure there. So I think it is useful to include them.

Thanks for checking,

Chema
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] util: Add a drm_find_modifier helper

[Eric Engestrom]

On Thursday, 2019-03-14 04:19:02 +, Alyssa Rosenzweig wrote:
> This function is replicated across vc4/v3d/freedreno and is needed in
> Panfrost; let's make this shared code.
> 
> Signed-off-by: Alyssa Rosenzweig 
> ---
>  src/util/u_drm.h | 46 ++
>  1 file changed, 46 insertions(+)
>  create mode 100644 src/util/u_drm.h
> 
> diff --git a/src/util/u_drm.h b/src/util/u_drm.h
> new file mode 100644
> index 000..d543c9a7543
> --- /dev/null
> +++ b/src/util/u_drm.h
> @@ -0,0 +1,46 @@
> +/*
> + * Copyright © 2014 Broadcom
> + * Copyright (C) 2012 Rob Clark 
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER 
> DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#ifndef U_DRM_H
> +#define U_DRM_H
> +
> +#include 
> +
> +/* Given a list of `count` DRM modifiers `haystack` and a desired modifier
> + * `needle`, returns whether the modifier is found */
> +
> +static bool
> +drm_find_modifier(uint64_t needle, const uint64_t *haystack, unsigned count)
> +{
> +unsigned i;
> +
> +for (i = 0; i < count; i++) {
> +if (haystack[i] == needle)
> +return true;
> +}
> +
> +return false;
> +}

This is an extremely generic loop; it has nothing to do with modifiers,
they just happen to be uint64_t.

Can we call it something like this?
  static inline bool
  array_contains_u64(uint64_t *array, size_t array_size, uint64_t needle)

There could be other places that could use this function (or its
u32/s64/s32 equivalents), there's no reason to prevent its adoption with
a too specific name :)

With a generic name (and the missing `inline` Christian also mentioned),
the series is:
Reviewed-by: Eric Engestrom 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] nir/spirv: short-circuit when conditional branch contains end block

[Juan A. Suarez Romero]

On Fri, 2019-03-08 at 13:29 -0600, Jason Ekstrand wrote:
> On Fri, Mar 8, 2019 at 9:30 AM Juan A. Suarez Romero  
> wrote:
> > On Thu, 2019-03-07 at 07:15 -0600, Jason Ekstrand wrote:
> > 
> > > Woah, is this legal SPIR-V? I think a second OpSelectionMerge is required.
> > 
> > 
> > 
> > I'd say it is legal. The spec does not mandate that each branch has its own
> > 
> > merge instruction; only that the control flow must be structured for 
> > shaders.
> 
> That's exactly the problem.  It says how merge instructions work and how they 
> have to be nested but never that or where you have to use them. :-(  This is 
> a spec bug.  The closest I can find is this line from the structured control 
> flow section: "These explicitly declare a header block before the control 
> flow diverges and a merge block where control flow subsequently converges."  
> If you read that as "you must declare divergence" then it would imply that 
> every OpBranchConditional must be preceded by an OpSelectionMerge.  If we 
> don't require this, there are lots of weird cases where you can get into 
> diamond patters and other things that aren't trivially structurizable.
> 

I agree, but I understand in this case the second branch is not diverging, but 
converging to the merge case already defined in the selection merge.
I found a couple of similar questions in public github, which were resolved by 
stating that not all OpBranchConditional must be immediately preceded by an 
OpSelectionMerge or OpLoopMerge.
https://github.com/KhronosGroup/glslang/issues/150#issuecomment-178185325https://github.com/KhronosGroup/SPIRV-Tools/issues/1185#issuecomment-356457613
J.A.
> --Jason
>  
> > In section 2.11 ("Structured Control Flow"), about structured control-flow
> > 
> > constructs:
> > 
> > 
> > 
> > 
> > 
> > - A structured control-flow construct is then defined as one of:
> > 
> >   - a selection construct: the set of blocks dominated by a selection 
> > header,
> > 
> > minus the set of blocks dominated by the header’s merge block
> > 
> >   - [...]
> > 
> > 
> > 
> > - The above structured control-flow constructs must satisfy the following 
> > rules:
> > 
> >   - [...]
> > 
> >   - the only blocks in a construct that can branch outside the construct are
> > 
> > - a block branching to the construct’s merge block
> > 
> > - a block branching from one case construct to another, for the same
> > 
> > OpSwitch
> > 
> > - a back-edge block
> > 
> > - a continue block for the innermost loop it is nested inside of
> > 
> > - a break block for the innermost loop it is nested inside of
> > 
> > - a return block
> > 
> > 
> > 
> > 
> > 
> > Our selection construct, which contains the two nested conditional branches,
> > 
> > satisfies the rules, as both conditionals branches to the construct's merge
> > 
> > block.
> > 
> > 
> > 
> > 
> > 
> > J.A.
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > 
> > > 
> > 
> > 
> > 
> > 
> > 
> > > --Jason
> > 
> > > 
> > 
> > > 
> > 
> > > On March 6, 2019 05:25:26 "Juan A. Suarez Romero"  
> > > wrote:
> > 
> > > 
> > 
> > > > This fixes the case when the SPIR-V code has two nested conditional
> > 
> > > > branches, but only one selection merge:
> > 
> > > > 
> > 
> > > > 
> > 
> > > > [...]
> > 
> > > > %1 = OpLabel
> > 
> > > > OpSelectionMerge %2 None
> > 
> > > > OpBranchConditional %3 %4 %2
> > 
> > > > %4 = OpLabel
> > 
> > > > OpBranchConditional %3 %5 %2
> > 
> > > > %5 = OpLabel
> > 
> > > > OpBranch %2
> > 
> > > > %2 = OpLabel
> > 
> > > > [...]
> > 
> > > > 
> > 
> > > > 
> > 
> > > > In the second OpBranchConditional, as the else-part is the end
> > 
> > > > block (started in the first OpBranchConditional) we can just follow the
> > 
> > > > then-part.
> > 
> > > > 
> > 
> > > > 
> > 
> > > > This fixes dEQP-VK.vkrunner.controlflow.2-obc-triangle-triangle
> > 
> > > > 
> > 
> > > > 
> > 
> > > > CC: Jason Ekstrand 
> > 
> > > > ---
> > 
> > > > src/compiler/spirv/vtn_cfg.c | 11 ++-
> > 
> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > > > 
> > 
> > > > 
> > 
> > > > diff --git a/src/compiler/spirv/vtn_cfg.c b/src/compiler/spirv/vtn_cfg.c
> > 
> > > > index 7868eeb60bc..f749118efbe 100644
> > 
> > > > --- a/src/compiler/spirv/vtn_cfg.c
> > 
> > > > +++ b/src/compiler/spirv/vtn_cfg.c
> > 
> > > > @@ -605,7 +605,16 @@ vtn_cfg_walk_blocks(struct vtn_builder *b, struct 
> > 
> > > > list_head *cf_list,
> > 
> > > > }
> > 
> > > >  } else if (if_stmt->then_type == vtn_branch_type_none &&
> > 
> > > > if_stmt->else_type == vtn_branch_type_none) {
> > 
> > > > -/* Neither side of the if is something we can 
> > > > short-circuit. */
> > 
> > > > +/* Neither side of the if is something we can 
> > > > short-circuit,
> > 
> > > > + * unless one of the blocks is the end block. */
> > 
> > > > +if (then_block == end) {
> > 
> > 

[Mesa-dev] [Bug 109764] [radv] Enable framerate capping

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=109764

soredake  changed:

   What|Removed |Added

 CC||fds...@krutt.org

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 108596] [RADV] Implement a HUD for monitoring GPU/CPU loads, FPS, temperature and more

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=108596

soredake  changed:

   What|Removed |Added

 CC||fds...@krutt.org

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 110078] rtg

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=110078

Michel Dänzer  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
Product|Mesa|Spam
 QA Contact|mesa-dev@lists.freedesktop. |
   |org |
 Resolution|--- |INVALID
   Assignee|mesa-dev@lists.freedesktop. |dan...@fooishbar.org
   |org |
  Component|Demos   |Two

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 105904] Needed to delete mesa shader cache after driver upgrade for 32 bit wine vulkan programs to work.

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=105904

Andre Klapper  changed:

   What|Removed |Added

 QA Contact|dani.ta...@outlook.com  |mesa-dev@lists.freedesktop.
   ||org
   Keywords|bisected|
   Assignee|smhamza3...@gmail.com   |mesa-dev@lists.freedesktop.
   ||org
   Priority|high|medium
 OS|Windows (All)   |Linux (All)
 Blocks|110105, 5435|
 CC|sayyarhas...@gmail.com  |
 Whiteboard|dfghfhj |
   Hardware|PowerPC |x86-64 (AMD64)
URL|www.google.com  |
   Severity|major   |normal
Version|10.0|git


Referenced Bugs:

https://bugs.freedesktop.org/show_bug.cgi?id=5435
[Bug 5435] Suggestion for an alternative phonetic cyrillic layout.
https://bugs.freedesktop.org/show_bug.cgi?id=110105
[Bug 110105] Needed to delete mesa shader cache after driver upgrade for 32 bit
wine vulkan programs to work.
-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98135] dEQP-GLES31.functional.debug.negative_coverage.get_error.shader.transform_feedback_varyings wants a different GL error code

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98135

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98243] dEQP mismatched UBO precision qualifiers

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98243

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 99076] dEQP-GLES3.functional.negative_api.texture#teximage3d fails due to wrong Error code

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=99076

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98223] dEQP GLES3.1 program_interface_query failures w/ error "could not find target resource"

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98223

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98133] GetSynciv should raise an error if bufSize < 0

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98133

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98241] dEQP GLES3.1 failure "Compute Shader should not have compiled with #version 300 es."

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98241

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98245] GLES3.1 link negative dEQP "expected linking to fail, but passed."

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98245

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98242] dEQP mandates preprocessor tests for #line expressions

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98242

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98132] #version 300 es compute shaders should not be possible

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98132

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 94116] program interface queries not returning right data for UBO / GL_BLOCK_INDEX

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=94116

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 98134] dEQP-GLES31.functional.debug.negative_coverage.get_error.buffer.draw_buffers wants a different GL error code

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=98134

Andre Klapper  changed:

   What|Removed |Added

 Blocks|110097  |

-- 
You are receiving this mail because:
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev