Re: [Mesa-dev] Workflow Proposal

[apinheiro]

On 12/10/21 13:55, Alyssa Rosenzweig wrote:

I would love to see this be the process across Mesa.  We already don't
rewrite commit messages for freedreno and i915g, and I only have to do
the rebase (busy-)work for my projects in other areas of the tree.

Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost
devs do, which I'm fine with. But it's not a requirement to merging.

The arguments about "who can help support this years from now?" are moot
at our scale... the team is small enough that the name on the reviewer
is likely the code owner / maintainer, and patches regularly go in
unreviewed for lack of review bandwidth.

There is another reason to the Rb tag, that is to measure the quantity
of patch review people do.

This was well summarized some years ago by Matt Turner, as it was
minimized (even suggested to be removed) on a different thread:

https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html

I was part of the Intel team when people started doing this r-b
counting.  I believe that it was being done due to Intel management's
failure to understand who was doing the work on the team and credit
them appropriately, and also to encourage those doing less to step up.



That's basically the same problem with trying to measure and compare 
developers just by commit count. In theory commit count is a bad measure 
for that. In practice it is used somehow.



Unfortunately, the problem with Intel management wasn't a lack of
available information, and I didn't see publishing the counts change
reviews either.



Upstream should do what's best for upstream, not for Intel's "unique"
management.



Not sure how from Emma explaining how Rb tags were used by Intel 
management it came the conclusion that it were used in that way only by 
Intel management. Spoiler: it is not.


Replying both, that's is one of the reasons I pointed original Matt 
Turner email. He never mentioned explicitly Intel management, neither 
pointed this as an accurate measure of the use. Quoting:


"The number of R-b tags is not a 100% accurate picture of the
situation, but it gives at least a good overview of who is doing the
tedious work of patch review. "

In any case, just to be clear here: Im not saying that the Rb tags main 
use is this one. Just saying that is one of their uses, and the value 
for such use can be debatable, but it is not zero.


BR

Re: [Mesa-dev] Workflow Proposal

[apinheiro]

Answering here, as it is the second time it is mentioned that Rb is only 
for "who can help support this years from now?", but not specifically to 
this email.


On 7/10/21 15:00, Alyssa Rosenzweig wrote:

I would love to see this be the process across Mesa.  We already don't
rewrite commit messages for freedreno and i915g, and I only have to do
the rebase (busy-)work for my projects in other areas of the tree.

Likewise for Panfrost. At least, I don't do the rewriting. Some Panfrost
devs do, which I'm fine with. But it's not a requirement to merging.

The arguments about "who can help support this years from now?" are moot
at our scale... the team is small enough that the name on the reviewer
is likely the code owner / maintainer, and patches regularly go in
unreviewed for lack of review bandwidth.


There is another reason to the Rb tag, that is to measure the quantity 
of patch review people do.


This was well summarized some years ago by Matt Turner, as it was 
minimized (even suggested to be removed) on a different thread:


https://lists.freedesktop.org/archives/mesa-dev/2019-January/213586.html



I think it's reasonable that small driver teams and large common
components have different review needs, and it's ok for the process to
reflect that.

[Mesa-dev] v3dv is Vulkan 1.0 conformant

[apinheiro]

So just a for your information, we submitted v3dv for Vulkan 1.0 
conformance around one month ago, in behalf of the Raspberry Foundation, 
and it is not official [1]. Here the Foundation blog post [2].


We want to thanks everybody that helped with this, but specially to Eric 
Anholt, as without the foundation of the v3d driver he wrote, all the 
answers to our questions and doubts, and all his patch reviews, we would 
not be able to do get this done in less than a year, that is (imho) a 
really nice time frame.


Right now we are working on improving the current stack, with several 
clean-ups, performance improvements, and adding some optional 1.0 
features. 1.1 is one the table but still not the main development focus. 
We will see as we keep adding "popular" optional 1.0 features.


BR

[1] 
https://www.khronos.org/conformance/adopters/conformant-products#submission_530


[2] https://www.raspberrypi.org/blog/vulkan-update-were-conformant/

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

Re: [Mesa-dev] Plan for merging v3dv in mesa

[apinheiro]

On 17/9/20 19:13, Jason Ekstrand wrote:

One more comment:  Could you make a big WIP MR with the whole driver
to act as a pointer to the branch for now?


Here you are:

https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6766



  Then it can be the thing
that gets merged once the stuff in a and b land.



Trying to make things easier, I listed the commits for the a) and b) 
categories:


https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6766#note_628333


I hope those links makes skim them easy (and provide feedback about 
which MR to create)





--Jason

On Thu, Sep 17, 2020 at 12:10 PM Jason Ekstrand  wrote:

Good work, all!  I'm super-happy to see another Vulkan driver land in Mesa.

On Thu, Sep 17, 2020 at 8:52 AM apinheiro  wrote:

Our development branch is ~525 patches on top of master, categorized as follows:
a) Patches that touch common Mesa infrastructure (NIR, Vulkan WSI, Meson, 
etc):  ~5 patches.
b) Patches that touch common Broadcom infrastructure under src/broadcom 
(V3D backend compiler mostly): ~20 patches
c) Patches that are independent and specific to the V3D Vulkan driver 
(under src/broadcom/vulkan): ~500 patches.

Since we are talking about a very large amount of patches, we are expecting 
that we can merge most of them without a review, particularly those in c) that 
implement the bulk of the Vulkan driver.

I think that's fine.


The patches in b) are mostly about extending our compiler backend to support 
Vulkan intrinsics and requirements as well a a few more general fixes or 
improvements. Our plan is to at least have someone in our team review them 
internally and grant Rbs, I think the only other person who might want to 
review these would be Eric if he has the time and is interested in doing so. We 
have sent some of these for early review [1][2] when we found they were more 
generic fixes or improvements to the compiler, but we might not want to do this 
for each and every one of them unless we see there is interest in reviewing 
them separately.

An ACK from Eric or someone other v3d devs would be good.  Not my
area, though, so I'll let others talk.


For the patches in a) we would like to at least get an Ack from other Mesa 
devs. They are mostly very simple things that just add an option to a NIR pass 
or a new intrinsic for use in our driver backend, so maybe it is not needed to 
create dedicated MRs and it is fine to just ping specific Mesa devs for reviews 
on those patches when we propose the large MR for the bulk of the driver. We 
did send one of these as an RFC some time ago [3] and it would be nice to get 
some more feedback there if possible.

I'd definitely like to see the patches in a) get real review.  I don't
know how many MRs you want to make there; do what makes sense.


Does all this sound sensible?

Yup.

--Jason

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

[Mesa-dev] Plan for merging v3dv in mesa

[apinheiro]

Hi everybody,

As some of you already know, we have been working on a Vulkan driver 
(v3dv) for the Broadcom V3D GPU present in the Raspberry Pi 4. So far we 
have beenworking on a personal branch, rebasing regularly, and we would 
like to start discussing about the process to merge the driver in Mesa.


First, here is some data about the current state of the driver:

Currently, we are targeting Vulkan 1.0 as our first milestone. At the 
moment, our Vulkan CTS results look like this:
[701251/701251] Pass: 106776 Fail: 18 Skip: 594454 ExpectedFail: 0 
UnexpectedPass: 0 Crash: 0 Timeout: 1 Missing: 0 Flake: 2


So we we are hoping to be able to submit the driver for conformance soon.

We have not done much testing beyond CTS yet, however, we know that we 
can run all Vulkan ports of the Quake 1-3 classics as well as OpenArena 
and we also know that there is a PSP simulator that uses Vulkan that 
some people have used to run some games on the Rpi4 as well. We can also 
run many of the Sascha Willem's demos.


So all in all, it seems that the driver can be useful already to people 
who want to start playing around with Vulkan on the Rpi4, and we would 
also like to start seeing more people doing exactly that so we can get 
feedback to continue polishing and improving the driver for real world 
usage.


As for our proposal to merge the driver, following are our initial 
thoughts. We would like to know if this sounds reasonable before we 
start making preparations.


Our development branch is ~525 patches on top of master, categorized as 
follows:
   a) Patches that touch common Mesa infrastructure (NIR, Vulkan WSI, 
Meson, etc): ~5 patches.
   b) Patches that touch common Broadcom infrastructure under 
src/broadcom (V3D backend compiler mostly): ~20 patches
   c) Patches that are independent and specific to the V3D Vulkan 
driver (under src/broadcom/vulkan): ~500 patches.


Since we are talking about a very large amount of patches, we are 
expecting that we can merge most of them without a review, particularly 
those in c) that implement the bulk of the Vulkan driver.


The patches in b) are mostly about extending our compiler backend to 
support Vulkan intrinsics and requirements as well a a few more general 
fixes or improvements. Our plan is to at least have someone in our team 
review them internally and grant Rbs, I think the only other person who 
might want to review these would be Eric if he has the time and is 
interested in doing so. We have sent some of these for early review 
[1][2] when we found they were more generic fixes or improvements to the 
compiler, but we might not want to do this for each and every one of 
them unless we see there is interest in reviewing them separately.


For the patches in a) we would like to at least get an Ack from other 
Mesa devs. They are mostly very simple things that just add an option to 
a NIR pass or a new intrinsic for use in our driver backend, so maybe it 
is not needed to create dedicated MRs and it is fine to just ping 
specific Mesa devs for reviews on those patches when we propose the 
large MR for the bulk of the driver. We did send one of these as an RFC 
some time ago [3] and it would be nice to get some more feedback there 
if possible.


Does all this sound sensible?

BR

[1] "v3d/compiler: allow to batch spills" (MR#6632)
[2] "broadcom/compiler: Allow spills of temporaries from TMU reads" 
(MR#6606)
[3] "[RFC] vulkan/wsi: allow non-PCI devices to avoid the prime blit 
path" (MR#5917)
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [Libre-soc-dev] Loading Vulkan Driver

[apinheiro]

On 23/8/20 8:11, Luke Kenneth Casson Leighton wrote:

On Sun, Aug 23, 2020 at 1:56 AM apinheiro  wrote:


On 22/8/20 23:59, Luke Kenneth Casson Leighton wrote:

constructive feedback on this approach greatly appreciated:
https://bugs.libre-soc.org/show_bug.cgi?id=251#c36

In any case, those steps look good enough, although I lack any context
of the final target.

(if i can fill in this bit)

the context is: to augment the PowerISA Instruction set to add 3D
opcodes *directly* to PowerISA, with the guidance, full knowledge and
under the supervision of the OpenPOWER Foundation.

this will include:  adding sin, cos, atan2... *to PowerISA*.  adding
texture interpolation instructions... *to PowerISA*.  adding YUV2RGB
and FP32-to-RGB conversion instructions... *to PowerISA*.

therefore, whilst the initial target is "general purpose scalar
non-accelerated non-vectorised soft-3D", this is simply because the
next step is to add 3D opcodes *directly* to the POWER9 core that we
are developing.



Then, in addition to the drivers you already looking as reference, it 
would make sense if you take a look to Vallium, that have just landed on 
Mesa master


https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/6082




i mention this because normally, GPUs are considered completely
separate entities - completely separate processors.  LibreSOC will be
the first ever libre-licensed *hybrid* 3D-capable CPU, i.e. where the
*main processor* ISA is augmented to no longer need a completely
separate GPU.

i appreciate that i have said the same thing in several different
ways.  this is because hybrid CPU/VPU/GPUs are so unusual and so "not
normal industry practice" (because they're easier to sell if they're
separate) that it takes a while to sink in.

i know of only one commercial company that has developed a hybrid
CPU/VPU/GPU (they called it a GPGPU - general purpose GPU) - by
ICubeCorp. back around 2012-2015 they received government funding
sufficient to complete the IC3128 and associated VLIW compiler.  an
SGI Engineer who went on to work for both AMD and NVidia created the
ISA and designed the architecture.

l.
___
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] [Libre-soc-dev] Loading Vulkan Driver

[apinheiro]

On 22/8/20 23:59, Luke Kenneth Casson Leighton wrote:

On Sat, Aug 22, 2020 at 10:34 PM apinheiro  wrote:


As Jason mentioned, to get a Vulkan driver started, you would need more
than just one method. If you want a reference:

https://gitlab.freedesktop.org/apinheiro/mesa/-/commit/07d01ebf6aae2f9ae71a8bea13a5d8acccb6280e

This commit added the basic skeleton for v3dv (broadcom mesa vulkan
driver).

fantastic this is extremely helpful and much appreciated, thank you.

some background: vivek has kindly agreed to start the NLNet-funded
LibreSOC MESA Vulkan driver.  given that the LibreSOC CPU is a hybrid
CPU/VPU/GPU with an augmented POWER9 ISA (rather than "a totally
separate custom GPU with a foreign ISA completely incompatible with
POWER9"), the first step is actually a general-purpose non-accelerated
*software* Vulkan Driver, very similar to SwiftShader in concept
except:

* utilising NIR in order to take advantage of the 3D shader passes and
the information it can hold
* retaining vector, predication and texturisation intrinsics right up
to the very last second when handing over to LLVM-IR
* relying on "general" LLVM-IR to perform translation for *native*
(host) execution - not a "totally separate custom GPU..."
* initially entirely targetting *host* (native) scalar instructions
[or, if general-purpose LLVM-IR happens to use NEON, SSE etc. that's
great but not our concern]

in other words it should be possible for the LibreSOC Vulkan driver to
run on native non-accelerated CPUs - x86, POWER9, ARM, MIPS and so on.

the second step will be to add custom 3D instructions *to POWER9*, at
which point whilst we hope to retain the ability to still run on
unaccelerated hardware, it is a secondary priority, albeit a very
important one.

as there may be questions "why not start from a different point, why
not use SwiftShader or gallium" and so on, that evaluation took place
here:
https://bugs.libre-soc.org/show_bug.cgi?id=251

constructive feedback on this approach greatly appreciated:
https://bugs.libre-soc.org/show_bug.cgi?id=251#c36



Quoting from there:

>  it should just return some error code as VkResult.

Not sure why it should return an error code. Even if initially is a 
no-op pipeline, if the method is able to create the object I think it is 
ok to return success, and let errors to be for real errors (out of 
memory, etc). But I guess that's ok if you prefer that until it starts 
to do something real.


> Test this setup with by forcing application to use this driver. Need 
to figure out way how to force it. May be through VK_ICD_FILENAMES.


Yes you can do that with the VK_ICD_FILENAMES envvar.


> Step3: Once we have above broken driver ready we can start enabling 
code required to process "COMPUTE" shaders.


Any specific reason to start with compute shaders? (mostly curiosity).

In any case, those steps look good enough, although I lack any context 
of the final target. The only thing that I miss is something like 
"writing the most basic vulkan program we can, and get it working with 
the driver". FWIW, when we did that for v3dv, we didn't even rendered 
anything, we just used clear/stores and confirmed that the output was 
correct. This allowed us starting with a basic driver that didn't even 
need a working backend compiler.


BR




with thanks and gratitude,

l.
___
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] Loading Vulkan Driver

[apinheiro]

On 21/8/20 7:27, vivek pandya wrote:
Thanks Jason for your time to reply. I understand the error but I am 
not much familiar with entry points generation code.



As Jason mentioned, to get a Vulkan driver started, you would need more 
than just one method. If you want a reference:


https://gitlab.freedesktop.org/apinheiro/mesa/-/commit/07d01ebf6aae2f9ae71a8bea13a5d8acccb6280e

This commit added the basic skeleton for v3dv (broadcom mesa vulkan 
driver). It C and adapted the python scripts that Jason wrote for anv 
(that was that radv and turnip also did, afaik), and then provided 
several empty implementation for several methods, and dummy structs for 
several objects. It is possible that you could wrote something more 
simplified as a starting point (like no debug methods), but not too much.



Currently I just make it compile (I just want to develop a broken 
pipeline quickly that just returns from the entry point) .



As Jason mentioned, a device and instance would be required. I really 
doubt that any vulkan program would be able to skip calling the methods 
that create them. Although those could be dummy too.



I will study the code. Is there any document to read about that? I 
want to understand how loaders and icd interact.


On Thu, Aug 20, 2020 at 9:46 PM Jason Ekstrand <mailto:ja...@jlekstrand.net>> wrote:


The error says pretty clearly what went wrong.  The loader looked for
the `vk_icdGetInstanceProcAddr` symbol and couldn't find it in your
so.  You need at least the basic Get*ProcAddr symbols or else the
loader can't do anything.  You'll also need device and instance
creation functions and possibly some of the queries before anything
will work.

On Thu, Aug 20, 2020 at 10:43 AM vivek pandya
mailto:vivekvpan...@gmail.com>> wrote:
>
> Hello,
>
> I have started building mesa Vulkan driver.
> I have started by copying amd/vulkan driver however I have just
kept only one file in build
> libresoc_pipeline.c
> I have only one method
>
> VkResult libresoc_CreateGraphicsPipelines(
>         VkDevice _device,
>         VkPipelineCache  pipelineCache,
>         uint32_t count,
>         const VkGraphicsPipelineCreateInfo*  pCreateInfos,
>         const VkAllocationCallbacks* pAllocator,
>         VkPipeline*  pPipelines)
> {
>         return VK_ERROR_UNKNOWN;
> }
>
> with few edits/commenting out code into files I am able to build
libvulkan_libresoc.so
>  but when I forced loaded driver with VK_ICD_FILENAMES I am
getting following error:
> however I was expecting to hit VK_ERROR_UNKNOWN. Anyone have any
ideas? Am I missing any file in the build setting?
>
> vivek@vivek-VirtualBox:~/install/share/vulkan/icd.d$ vulkaninfo
> ERROR: [Loader Message] Code 0 : loader_scanned_icd_add: Attempt
to retrieve either 'vkGetInstanceProcAddr' or
'vk_icdGetInstanceProcAddr' from ICD
/home/vivek/install/lib/x86_64-linux-gnu/libvulkan_libresoc.so failed.
> Cannot create Vulkan instance.
> This problem is often caused by a faulty installation of the
Vulkan driver or attempting to use a GPU that does not support Vulkan.
>

/build/vulkan-tools-KEbD_A/vulkan-tools-1.2.131.1+dfsg1/vulkaninfo/vulkaninfo.h:371:
failed with ERROR_INCOMPATIBLE_DRIVER
>
> Thanks,
> Vivek
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
<mailto:mesa-dev@lists.freedesktop.org>
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


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

Re: [Mesa-dev] piglit/mesa marge access

[apinheiro]

On 31/7/20 18:35, Erik Faye-Lund wrote:

I second the request :-)

+1


Jul 31, 2020 16:15:05 Mike Blumenkrantz :


Hi,

I'd like to request marge access for the piglit and mesa gitlab projects. I've 
been contributing a number of patches here (primarily to zink/gallium), and 
this would be useful in my continued work.

Regards,

Mike (zmike)


___
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] Drop scons for 20.1?

[apinheiro]

On 26/2/20 5:15, Jason Ekstrand wrote:
> +Jose & Brian
>
> I'm not personally opposed but I also can't remember the last time I
> had to fix the scons build. I think it's been years. Maybe that's
> because I don't work on GL anymore? 


I bet that the latter. I remember some years ago pushing new code (I
think that was the initial ARB_gl_spirv code), updating the meson and
autotools files (we were still on transition), and found when the code
was I pushed broke the scons build. Moving from three build systems to
two was good. If possible I really think that it would be a good idea to
move from two to one.


> In any case, I don't know that it's really costing us that much given
> that basically none of the drivers actually build with it. But fat
> meh, I guess.
>
> --Jason
>
> On February 25, 2020 21:56:30 Rob Clark  wrote:
>
>> It looks like we have 4 scons build jobs in CI.. I'm not sure how much
>> that costs us, but I guess those cycles could be put to better use?
>> So even ignoring the developer-cycles issue (ie. someone making
>> changes that effects scons build, and has to setup a scons build env
>> to fix breakage of their MR) I guess there is at least an argument to
>> remove scons from CI.  Whether it is worth keeping a dead build system
>> after it is removed from CI is an issue that I'm ambivalent about.
>>
>> BR,
>> -R
>>
>> On Tue, Feb 25, 2020 at 3:42 PM Kristian Høgsberg
>>  wrote:
>>>
>>> It's been a while since Dylan did the work to make meson support
>>> Windows and there's been plenty of time to provide feedback or improve
>>> argue why we still need scons. I haven't seen any such discussion and
>>> I think we've waited long enough.
>>>
>>> Let's drop scons for the next release and move things forward?
>>>
>>> Kristian
>>> ___
>>> mesa-dev mailing list
>>> mesa-dev@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Requiring a full author name when contributing to mesa?

[apinheiro]

On 12/12/19 5:46, Mark Janes wrote:

Ian Romanick  writes:


On 12/11/19 2:27 PM, Timothy Arceri wrote:

Hi,

So it seems lately we have been increasingly merging patches with made
up names, or single names etc [1]. The latest submitted patch has the
name Icecream95. This seems wrong to me from a point of keeping up the
integrity of the project. I'm not a legal expert but it doesn't seem
ideal to be amassing commits with these type of author tags from that
point of view either.

Is it just me or do others agree we should at least require a proper
name on the commits (as fake as that may be also)? Seems like a low bar
to me.

First we don't allow single Unicode characters or emojis, and now you
want to require realistic looking names.  Where does it end, Tim?!?
WHERE DOES IT END???

藍

But seriously... I definitely agree with the sentiment.  It seems really
lame to have a bunch of commits from clearly nonsense names.  Who are
these randos?

Perhaps they are graphics developers working at corporations where
management is not enthusiastic about contributions to mesa?

Or, agents seeking to damage mesa by submitting vulnerable or
IP-entangled code?

The kernel does not allow anonymous contributions, to ensure all
contributions are compatible with the GPL.



Out of curiosity: and how far they go to ensure that they are not 
anonymous contributions? So for this case, why Icecream95 is not 
anonymous ? It is a nickname that after a quick googling he has been 
using for a long time now, so he is already known by that nickname.






Where's the accountability?

As far as any possible legal aspects go, since we don't require (as far
as I'm aware) submitters to sign any sort of certificate of origin, I
don't know that Icecream95 is any better or worse than Ralphio Grant (a
realistic looking name that I just made up) or Ian Romanck.

It seems to me that this is a social problem, so it likely has a social
solution.  If we don't want people to be anonymous cowards with clearly
phony names, we should try to make the alternatives of being involved in
the community and using a real name more attractive.  We should do our
best to encourage people to "do the right thing."

I don't think it's very realistic for us to try to compel people to do
so, and I don't think there's much value in it... especially if it
drives away people making technically competent contributions.


[1]
https://gitlab.freedesktop.org/mesa/mesa/merge_requests/3050#note_361924
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.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] Requiring a full author name when contributing to mesa?

[apinheiro]

On 12/12/19 0:38, Eric Engestrom wrote:

On 2019-12-11 at 23:09, Eric Anholt  wrote:

On Wed, Dec 11, 2019 at 2:35 PM Timothy Arceri  wrote:

Hi,

So it seems lately we have been increasingly merging patches with made
up names, or single names etc [1]. The latest submitted patch has the
name Icecream95. This seems wrong to me from a point of keeping up the
integrity of the project. I'm not a legal expert but it doesn't seem
ideal to be amassing commits with these type of author tags from that
point of view either.

Is it just me or do others agree we should at least require a proper
name on the commits (as fake as that may be also)? Seems like a low bar
to me.

I'm of the opinion that in fact all names are made up,

Whole heartedly agreed.

Remember that many different cultures exist, and they have different customs
around names. As an example, a teacher of mine had a single name, but the school
required two separate "first name" and "last name" fields so he wrote his name 
twice,
which appeared on every form we got from the school, yet everyone knew he didn't
have what we called a "last name"/"family name".


Just adding another example: or like in Spain where we have two surnames 
(the usual naming template is  
 ), but we usually just use 
the first one on commits because it is what everybody else does.


And similar to what Eric mentioned on that paragraph, any immigrant that 
gets the spanish nationality needs to provide two surnames, even in 
their country of origin they didn't have that, so it is usual that the 
second one is somewhat made-up.



Another example is people from Asia who often assume a made up Western-sounding
pseudonym to use when communicating with Western people, and those often don't
look like real names to us.







What looks like a real name to you?
How would you even start to define such a rule?


and we don't
want to be getting into the business of requiring legal names for
committing.  If legal names were what you were getting at: have you
checked the legal names of your fellow contributors match what they're
contributing under?

I don't know what legal risk you might be thinking of, that seems like
spreading fear for no reason to me.

___
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] Remove classic drivers or fork src/mesa for gallium?

[apinheiro]

On 5/12/19 8:48, Kenneth Graunke wrote:

On Tuesday, December 3, 2019 4:39:15 PM PST Marek Olšák wrote:

Hi,

Here are 2 proposals to simplify and better optimize the GL->Gallium
translation.

1) Move classic drivers to a fork of Mesa, and remove them from master.
Classic drivers won't share any code with master. glvnd will load them, but
glvnd is not ready for this yet.

2) Keep classic drivers. Fork src/mesa for Gallium. I think only mesa/main,
mesa/vbo, mesa/program, and drivers/dri/common need to be forked and
mesa/state_tracker moved. src/gallium/state-trackers/gl/ can be the target
location.

Option 2 is more acceptable to people who want to keep classic drivers in
the tree and it can be done right now.

Opinions?

Thanks,
Marek

FWIW, I am not in favor of either plan for the time being.

- I agree with Eric that we're finally starting to clean up and
   de-duplicate things, and pay off technical debt we've put off for
   a long time.  I think forking everything in-tree would just be a
   giant mess.

- I agree with Dave that this seems to be a giant hassle for our
   downstreams with little benefit for them in the short term.

- Shuffling r100/r200/i915/nouveau_vieux off to a legacy fork seems
   like a fine plan.  They're ancient hardware that can't (or barely
   can) do GL 2.x.  Nothing much has happened with them in years,
   and I'm not sure all of them even really have maintainers.

   The main blocker here I think would be ironing out all the glvnd
   issues and making sure that is working solidly for everyone.
   (FWIW, glvnd is not even enabled by default in our build system!)

- Shuffling i965 off to legacy is really premature I think.  Iris
   isn't even shipping in distros yet (hopefully soon!), and even
   then, we have a _ton_ of Haswell users.  Check the Phoronix
   comments if you want to see how pissed off people are about even
   bringing up this topic.  (Or better yet, don't...basic human
   decency toward others seems to be lacking.  Hooray, internet...)

- Writing a Gallium driver for Intel Gen4-7.5 would be interesting.

   I've been thinking about this some, and it might be possible to
   retain some of the niceties of the iris memory model even on older
   hardware, by relying on a modern kernel and possibly making some
   (hopefully minor) improvements.  Even if we didn't, going back to
   the i965 model wouldn't be the worst thing.  The new driver would
   almost certainly be faster than i965, if not as good as iris.

   ajax and I were planning to call it crocus, if I wrote one.  I don't
   think it would take nearly as long.  But it's still a bunch of time
   that I don't think I can justify spending.  The new hardware can do
   so much more, and is so much faster, and much lower power.  I think
   it would be better for me to spend my time on Gen11+.

- Vulkan has really taken off, and OpenGL feels increasingly like a
   dead end.  DXVK has brought more games than we had with native ports.
   Feral is even reworking some native OpenGL ports to use Vulkan.  New
   graphics features are happening in the Vulkan space.

   So, I kind of wonder whether it's worth our time to go through and
   massively clean up and rearchitecture the OpenGL drivers at this
   point.  By the time we're finished, will anyone care?

- Are there concrete things that are prohibitively expensive with
   classic around?  Otherwise, Eric's suggestion of "so go do that!"
   sounds like a good idea.  We've started finally merging a bunch
   of code and I think this is a positive direction.

I'd say let's shelve this for now and think about it again later.
I suspect in a year or so the calculus will look fairly different.



Although I think that it is somewhat early to think that OpenGL is 
nearly a dead end, FWIW, I agree in general with Kenneth analysis and 
suggestions here.


BR

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

Re: [Mesa-dev] iris now officially OpenGL 4.6 conformant

[apinheiro]

On 20/11/19 19:31, Kenneth Graunke wrote:

Hi all,

iris is now officially a conformant OpenGL 4.6 implementation!

https://www.khronos.org/conformance/adopters/conformant-products/opengl#submission_253

This is on Gen9.  I've also submitted for Gen8, but that's still under
review; Gen11 is 99% of the way there but I'm hunting down one last bug.



Hey, congrats \o/



--Ken

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

Re: [Mesa-dev] [ANNOUNCE] mesa 19.2.0-rc2

[apinheiro]

On 5/9/19 0:57, Dylan Baker wrote:

Hi List,

I'd like to announce the availability of mesa-19.2.0-rc2. This is the
culmination of two weeks worth of work. Due to maintenance the Intel CI is not
running, but I've built and tested this locally. I would have preferred to get
more testing, but being two weeks out from -rc1 I wanted to get a release out.

Dylan



I would like to nominate the following v3d patch:

"broadcom/v3d: Allow importing linear BOs with arbitrary offset/stride" [1]

I already mentioned that patch on the "[Mesa-dev] Mesa 19.2.0 release 
plan" thread, but I forgot to CC mesa-stable. Sorry for that.


FWIW, the patch fixes the following piglit tests:

spec/ext_image_dma_buf_import/ext_image_dma_buf_import-sample_nv12
spec/ext_image_dma_buf_import/ext_image_dma_buf_import-sample_yuv420
spec/ext_image_dma_buf_import/ext_image_dma_buf_import-sample_yvu420

[1] 
https://gitlab.igalia.com/graphics/mesa/commit/873b092e9110a0605293db7bc1c5bcb749cf9a28






Shortlog:


Alex Smith (1):
   radv: Change memory type order for GPUs without dedicated VRAM

Alyssa Rosenzweig (1):
   pan/midgard: Fix writeout combining

Andres Rodriguez (1):
   radv: additional query fixes

Bas Nieuwenhuizen (3):
   radv: Use correct vgpr_comp_cnt for VS if both prim_id and instance_id 
are needed.
   radv: Emit VGT_GS_ONCHIP_CNTL for tess on GFX10.
   radv: Disable NGG for geometry shaders.

Danylo Piliaiev (1):
   nir/loop_unroll: Prepare loop for unrolling in wrapper_unroll

Dave Airlie (2):
   virgl: fix format conversion for recent gallium changes.
   gallivm: fix atomic compare-and-swap

Dylan Baker (1):
   bump version to 19.2-rc2

Ian Romanick (7):
   nir/algrbraic: Don't optimize open-coded bitfield reverse when lowering 
is enabled
   intel/compiler: Request bitfield_reverse lowering on pre-Gen7 hardware
   nir/algebraic: Mark some value range analysis-based optimizations 
imprecise
   nir/range-analysis: Adjust result range of exp2 to account for 
flush-to-zero
   nir/range-analysis: Adjust result range of multiplication to account for 
flush-to-zero
   nir/range-analysis: Fix incorrect fadd range result for (ne_zero, 
ne_zero)
   nir/range-analysis: Handle constants in nir_op_mov just like nir_op_bcsel

Ilia Mirkin (1):
   gallium/vl: use compute preference for all multimedia, not just blit

Jose Maria Casanova Crespo (1):
   mesa: recover target_check before get_current_tex_objects

Kenneth Graunke (15):
   gallium/ddebug: Wrap resource_get_param if available
   gallium/trace: Wrap resource_get_param if available
   gallium/rbug: Wrap resource_get_param if available
   gallium/noop: Implement resource_get_param
   iris: Replace devinfo->gen with GEN_GEN
   iris: Fix broken aux.possible/sampler_usages bitmask handling
   iris: Update fast clear colors on Gen9 with direct immediate writes.
   iris: Drop copy format hacks from copy region based transfer path.
   iris: Avoid unnecessary resolves on transfer maps
   iris: Fix large timeout handling in rel2abs()
   isl: Drop UnormPathInColorPipe for buffer surfaces.
   isl: Don't set UnormPathInColorPipe for integer surfaces.
   util: Add a _mesa_i64roundevenf() helper.
   mesa: Fix _mesa_float_to_unorm() on 32-bit systems.
   iris: Fix partial fast clear checks to account for miplevel.

Lionel Landwerlin (2):
   util/timespec: use unsigned 64 bit integers for nsec values
   util: fix compilation on macos

Marek Olšák (18):
   radeonsi/gfx10: fix the legacy pipeline by storing as_ngg in the shader 
cache
   radeonsi: move some global shader cache flags to per-binary flags
   radeonsi/gfx10: fix tessellation for the legacy pipeline
   radeonsi/gfx10: fix the PRIMITIVES_GENERATED query if using legacy 
streamout
   radeonsi/gfx10: create the GS copy shader if using legacy streamout
   radeonsi/gfx10: add as_ngg variant for VS as ES to select Wave32/64
   radeonsi/gfx10: fix InstanceID for legacy VS+GS
   radeonsi/gfx10: don't initialize VGT_INSTANCE_STEP_RATE_0
   radeonsi/gfx10: always use the legacy pipeline for streamout
   radeonsi/gfx10: finish up Navi14, add PCI ID
   radeonsi/gfx10: add AMD_DEBUG=nongg
   winsys/amdgpu+radeon: process AMD_DEBUG in addition to R600_DEBUG
   radeonsi: add PKT3_CONTEXT_REG_RMW
   radeonsi/gfx10: remove incorrect ngg/pos_writes_edgeflag variables
   radeonsi/gfx10: set PA_CL_VS_OUT_CNTL with CONTEXT_REG_RMW to fix edge 
flags
   radeonsi: consolidate determining VGPR_COMP_CNT for API VS
   radeonsi: unbind blend/DSA/rasterizer state correctly in delete functions
   radeonsi: fix scratch buffer WAVESIZE setting leading to corruption

Paulo Zanoni (1):
   intel/fs: grab fail_msg from v32 instead of v16 when v32->run_cs fails

Pierre-Eric Pelloux-Prayer (1):
   glsl: replace 'x + (-x)' with constant 0

Re: [Mesa-dev] [PATCH v3] broadcom/vc4: Expand width of dst surface

[apinheiro]

Just pushed, thanks for the patch.

On 3/9/19 4:58, Zhaowei Yuan wrote:

Four bytes of src_surf will be compressed into a 32-bits data
and stored into dst_surf, and dst_surf is read as z-order,so
its width must be aligned to multiples of 8(4x2) before divided
by 2.

Signed-off-by: Zhaowei Yuan 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111266
---
  src/gallium/drivers/vc4/vc4_blit.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/vc4/vc4_blit.c 
b/src/gallium/drivers/vc4/vc4_blit.c
index d3cc515..d379bcc 100644
--- a/src/gallium/drivers/vc4/vc4_blit.c
+++ b/src/gallium/drivers/vc4/vc4_blit.c
@@ -360,7 +360,7 @@ vc4_yuv_blit(struct pipe_context *pctx, const struct 
pipe_blit_info *info)
  util_blitter_unset_running_flag(vc4->blitter);
  return false;
  }
-dst_surf->width /= 2;
+dst_surf->width = align(dst_surf->width, 8) / 2;
  if (dst->cpp == 1)
  dst_surf->height /= 2;
  

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

Re: [Mesa-dev] [PATCH v2] broadcom/vc4: Expand width of dst surface

[apinheiro]

On 28/8/19 3:16, Zhaowei Yuan wrote:

Four bytes of src_surf will be compressed into a 32-bits data
and stored into dst_surf, and dst_surf is read as z-order,so
its width must be aligned to multiples of 8(4x2) before devided


Nitpick: typo, "devided" instead of "divided".


With that change:

Reviewed-by: Alejandro Piñeiro 


Do you have permissions to push this change by yourself, or do you need 
me to push it?




by 2.

Signed-off-by: Zhaowei Yuan 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111266
---
  src/gallium/drivers/vc4/vc4_blit.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/vc4/vc4_blit.c 
b/src/gallium/drivers/vc4/vc4_blit.c
index d3cc515..d379bcc 100644
--- a/src/gallium/drivers/vc4/vc4_blit.c
+++ b/src/gallium/drivers/vc4/vc4_blit.c
@@ -360,7 +360,7 @@ vc4_yuv_blit(struct pipe_context *pctx, const struct 
pipe_blit_info *info)
  util_blitter_unset_running_flag(vc4->blitter);
  return false;
  }
-dst_surf->width /= 2;
+dst_surf->width = align(dst_surf->width, 8) / 2;
  if (dst->cpp == 1)
  dst_surf->height /= 2;
  

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

Re: [Mesa-dev] Mesa 19.2.0 release plan

[apinheiro]

On 30/8/19 0:52, apinheiro wrote:


On 7/8/19 10:44, Emil Velikov wrote:

Hi all,



Hi Emil,




On Wed, 31 Jul 2019 at 09:37, Emil Velikov  
wrote:

Hi all,

Here is the tentative release plan for 19.2.0.

As many of you are well aware, it's time to the next branch point.
The calendar is already updated, so these are the tentative dates:

  Aug 06 2019 - Feature freeze/Release candidate 1
  Aug 13 2019 - Release candidate 2
  Aug 20 2019 - Release candidate 3
  Aug 27 2019 - Release candidate 4/final release


With multiple teams finalising the final features for their drivers,
I've decided to push the branch point by one week.


Are we still in time to add this v3d patch:

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

It is not still on master, but I would try to get it merged on master 
tomorrow Friday. 



Just landed on master.


But if there isn't any possibility to get it included on 19.2.X at 
this point, I would not bother anyone to get an extra Rb/Ack.


Thanks in advance, and sorry if at this point the answer is evident




I would like to remind teams that they are welcome to merge
functionality early and keep it disabled by default.
This way they can address outstanding issues and enable, during the
stabilisation period.

Thanks
Emil
___
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] Mesa 19.2.0 release plan

[apinheiro]

On 7/8/19 10:44, Emil Velikov wrote:

Hi all,



Hi Emil,




On Wed, 31 Jul 2019 at 09:37, Emil Velikov  wrote:

Hi all,

Here is the tentative release plan for 19.2.0.

As many of you are well aware, it's time to the next branch point.
The calendar is already updated, so these are the tentative dates:

  Aug 06 2019 - Feature freeze/Release candidate 1
  Aug 13 2019 - Release candidate 2
  Aug 20 2019 - Release candidate 3
  Aug 27 2019 - Release candidate 4/final release


With multiple teams finalising the final features for their drivers,
I've decided to push the branch point by one week.


Are we still in time to add this v3d patch:

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

It is not still on master, but I would try to get it merged on master 
tomorrow Friday. But if there isn't any possibility to get it included 
on 19.2.X at this point, I would not bother anyone to get an extra Rb/Ack.


Thanks in advance, and sorry if at this point the answer is evident




I would like to remind teams that they are welcome to merge
functionality early and keep it disabled by default.
This way they can address outstanding issues and enable, during the
stabilisation period.

Thanks
Emil
___
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] broadcom/vc4: Expand width of dst surface

[apinheiro]

On 27/8/19 11:13, Zhaowei Yuan wrote:

Four bytes of src_surf will be compressed into a 32-bits data
and stored into dst_surf, and dst_surf is read as z-order,so
its width must be aligned to multiples of 8(4x2) before devided
by 2.

Signed-off-by: Zhaowei Yuan 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=111266
---
  src/gallium/drivers/vc4/vc4_blit.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/gallium/drivers/vc4/vc4_blit.c 
b/src/gallium/drivers/vc4/vc4_blit.c
index d3cc515..7697bfa 100644
--- a/src/gallium/drivers/vc4/vc4_blit.c
+++ b/src/gallium/drivers/vc4/vc4_blit.c
@@ -361,6 +361,7 @@ vc4_yuv_blit(struct pipe_context *pctx, const struct 
pipe_blit_info *info)
  return false;
  }
  dst_surf->width /= 2;
+dst_surf->width = align(dst_surf->width, 8) / 2;



The commit message and what you are doing here is somewhat inconsistent, 
as you are dividing by two twice. Shouldn't the previous width division 
being replaced instead of adding a new aligned one?  It is somewhat 
confusing to divide by two, and then align and divide by two again.


FWIW, at this point it would be advisable if you send vc4/v3d patches as 
gitlab MR, as lately we are using it more that the mesa list.



BR


  if (dst->cpp == 1)
  dst_surf->height /= 2;
  

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

Re: [Mesa-dev] v3d: Difference between TransformFeedback Gallium <-> Vulkan

[apinheiro]

On 26/8/19 13:28, abergme...@gmx.net wrote:
For a few weeks now I am working on implementing Vulkan for VideoCore 
6 AKA 42 (using V3D/DRM). Don't hold you breath ;)


Currently I am trying to understand what is necessary or how to 
interact with V3D. So I am looking at TransformFeedback because it 
interacts with quite a few other parts of the pipeline and still seems 
less mangled into the big logic than other features. So I am comparing 
how Gallium (V3D) is handling TF in the state tracker VS how Vulkan 
(Intel) is handling the Extension.


The following is what I so far think I gathered:
1. GV3D is handling TransformFeedback directly with other bound parts 
of the pipeline (e.g. `emit_state` in _v3dx_emit.c_). It seems to look 
into the shader and associated TF specs. It seems to use "streamout 
targets", although I do not yet understand what these are really. Then 
it passes all the offsets, indices and finally resources to V3D.
2. The Vulkan Extension only knows about CounterBuffers and iterates 
over these. Intel seems to call TF -> XF? and subsequently the buffers 
XFB. Have also not yet gathered what is the difference and what all 
the gazillion acronyms mean.



XFB is a really common acronym for transform feedback. So yes, XFB and 
TF are acronyms fro the same.




So far my idea would be to implement TF similar to Intel and instead 
of iterating over "streamout targets" I would iterate XFBs.
The problem with this approach is, that it will not be easy to mimic 
`cl_emit` calls similar to Gallium.

My question now is which parts of V3D emits have a dependency.

I would assume that I can move TRANSFORM_FEEDBACK_SPECS and 
TRANSFORM_FEEDBACK_ENABLE to cmd state flush in Vulkan.
`vkCmdBeginTransformFeedbackEXT` shoudl then only 
need TRANSFORM_FEEDBACK_BUFFER and TRANSFORM_FEEDBACK_OUTPUT_ADDRESS.


Sorry if this is a bit confusing - I am really just trying to figure 
this out one by one.


Any information would be appreciated.

___
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] Mesa 19.2.0 release plan

[apinheiro]

(Thanks Chema for pointing me this thread)

On 21/8/19 9:09, Emil wrote:

Hi Jason,

On 21 August 2019 01:22:00 EEST, Jason Ekstrand  wrote:

Sorry for the late breaking hold but I just realized that
GL_ARB_gl_spirv
and OpenGL 4.6 for Intel is 1 regression (I think it's not even a
regression) away from landing.  Can I have 24 hours?


As you have noticed I've rolled our RC1 just after your email went out.

 From a quick look we are talking about 20 or so patches. I'm fine with 
cherry-picking the work for RC2 if we get a couple of +1 from other developers.

We had the odd exception in the past, I see no reason why we cannot do one here 
- GL4.6 is a worthy addition :-)



I have just added some comments on the gitlab, among other things 
clarifying the "I think it's not even a regression" thing. In my 
opinion, both extension have been tested long using CTS and piglit, and 
it is time to get some real world testing (even if the piglit branch is 
under review). So as soon as Jason and Caio agrees with push those 3 
patches to mesa master,  +1 from me to include it on the release 
(although I guess that I'm a subjective opinion).


BR


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

Re: [Mesa-dev] [PATCH] st/mesa: accelerate glCopyPixels(STENCIL)

[apinheiro]

On 11/7/19 20:12, Marek Olšák wrote:
The test passes here. I wouldn't push a commit that doesn't pass. It 
looks like v3d can't blit stencil.



FWIW, just in case you are curious: I was able to take a look today to 
this. This problem only affects the fbo-stencil case with 
GL_DEPTH32F_STENCIL8. So for example, the following ones:


./bin/fbo-stencil copypixels GL_DEPTH24_STENCIL8 -auto -fbo

./bin/fbo-depthstencil copypixels GL_DEPTH32F_STENCIL8 -auto -fbo

works fine, so I guess that we need to do something special with that 
format on v3d.





Marek

On Thu, Jul 11, 2019 at 6:29 AM apinheiro <mailto:apinhe...@igalia.com>> wrote:


Hi, the following piglit test:

./bin/fbo-stencil copypixels GL_DEPTH32F_STENCIL8 -auto -fbo

regressed after this patch landed master with the v3d driver. So
Marek
and anyone reading this email, could you execute that test and
confirms
if only regress with v3d?

Thanks in advance.

On 25/6/19 2:12, Marek Olšák wrote:
> From: Marek Olšák mailto:marek.ol...@amd.com>>
>
> ---
>   src/mesa/state_tracker/st_cb_drawpixels.c | 58
+++
>   1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
b/src/mesa/state_tracker/st_cb_drawpixels.c
> index 59868d3ff1d..26d3cc33e5c 100644
> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
> @@ -1508,35 +1508,35 @@ static GLboolean
>   blit_copy_pixels(struct gl_context *ctx, GLint srcx, GLint srcy,
>                    GLsizei width, GLsizei height,
>                    GLint dstx, GLint dsty, GLenum type)
>   {
>      struct st_context *st = st_context(ctx);
>      struct pipe_context *pipe = st->pipe;
>      struct pipe_screen *screen = pipe->screen;
>      struct gl_pixelstore_attrib pack, unpack;
>      GLint readX, readY, readW, readH, drawX, drawY, drawW, drawH;
>
> -   if (type == GL_COLOR &&
> -       ctx->Pixel.ZoomX == 1.0 &&
> +   if (ctx->Pixel.ZoomX == 1.0 &&
>          ctx->Pixel.ZoomY == 1.0 &&
> -       ctx->_ImageTransferState == 0x0 &&
> -       !ctx->Color.BlendEnabled &&
> -       !ctx->Color.AlphaEnabled &&
> -       (!ctx->Color.ColorLogicOpEnabled || ctx->Color.LogicOp
== GL_COPY) &&
> -       !ctx->Depth.Test &&
> -       !ctx->Fog.Enabled &&
> -       !ctx->Stencil.Enabled &&
> -       !ctx->FragmentProgram.Enabled &&
> -       !ctx->VertexProgram.Enabled &&
> -  !ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT] &&
> -       !_mesa_ati_fragment_shader_enabled(ctx) &&
> -       ctx->DrawBuffer->_NumColorDrawBuffers == 1 &&
> +       (type != GL_COLOR ||
> +        (ctx->_ImageTransferState == 0x0 &&
> +         !ctx->Color.BlendEnabled &&
> +         !ctx->Color.AlphaEnabled &&
> +         (!ctx->Color.ColorLogicOpEnabled || ctx->Color.LogicOp
== GL_COPY) &&
> +         !ctx->Depth.Test &&
> +         !ctx->Fog.Enabled &&
> +         !ctx->Stencil.Enabled &&
> +         !ctx->FragmentProgram.Enabled &&
> +         !ctx->VertexProgram.Enabled &&
> +  !ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT] &&
> +         !_mesa_ati_fragment_shader_enabled(ctx) &&
> +         ctx->DrawBuffer->_NumColorDrawBuffers == 1)) &&
>          !ctx->Query.CondRenderQuery &&
>          !ctx->Query.CurrentOcclusionObject) {
>         struct st_renderbuffer *rbRead, *rbDraw;
>
>         /*
>          * Clip the read region against the src buffer bounds.
>          * We'll still allocate a temporary buffer/texture for
the original
>          * src region size but we'll only read the region which
is on-screen.
>          * This may mean that we draw garbage pixels into the
dest region, but
>          * that's expected.
> @@ -1555,22 +1555,32 @@ blit_copy_pixels(struct gl_context *ctx,
GLint srcx, GLint srcy,
>         unpack = pack;
>         if (!_mesa_clip_drawpixels(ctx, , , ,
, ))
>            return GL_TRUE; /* all done */
>
>         readX = readX - pack.SkipPixels + unpack.SkipPixels;
>         readY = readY - pack.SkipRows + unpack.SkipRows;
>
>         drawW = readW;
>         drawH = readH;
>
&g

Re: [Mesa-dev] [PATCH] st/mesa: accelerate glCopyPixels(STENCIL)

[apinheiro]

On 11/7/19 20:12, Marek Olšák wrote:

The test passes here. I wouldn't push a commit that doesn't pass.



Ok, thanks for confirming.



It looks like v3d can't blit stencil.


Ok, thanks for the hint.




Marek

On Thu, Jul 11, 2019 at 6:29 AM apinheiro <mailto:apinhe...@igalia.com>> wrote:


Hi, the following piglit test:

./bin/fbo-stencil copypixels GL_DEPTH32F_STENCIL8 -auto -fbo

regressed after this patch landed master with the v3d driver. So
Marek
and anyone reading this email, could you execute that test and
confirms
if only regress with v3d?

Thanks in advance.

On 25/6/19 2:12, Marek Olšák wrote:
> From: Marek Olšák mailto:marek.ol...@amd.com>>
>
> ---
>   src/mesa/state_tracker/st_cb_drawpixels.c | 58
+++
>   1 file changed, 38 insertions(+), 20 deletions(-)
>
> diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c
b/src/mesa/state_tracker/st_cb_drawpixels.c
> index 59868d3ff1d..26d3cc33e5c 100644
> --- a/src/mesa/state_tracker/st_cb_drawpixels.c
> +++ b/src/mesa/state_tracker/st_cb_drawpixels.c
> @@ -1508,35 +1508,35 @@ static GLboolean
>   blit_copy_pixels(struct gl_context *ctx, GLint srcx, GLint srcy,
>                    GLsizei width, GLsizei height,
>                    GLint dstx, GLint dsty, GLenum type)
>   {
>      struct st_context *st = st_context(ctx);
>      struct pipe_context *pipe = st->pipe;
>      struct pipe_screen *screen = pipe->screen;
>      struct gl_pixelstore_attrib pack, unpack;
>      GLint readX, readY, readW, readH, drawX, drawY, drawW, drawH;
>
> -   if (type == GL_COLOR &&
> -       ctx->Pixel.ZoomX == 1.0 &&
> +   if (ctx->Pixel.ZoomX == 1.0 &&
>          ctx->Pixel.ZoomY == 1.0 &&
> -       ctx->_ImageTransferState == 0x0 &&
> -       !ctx->Color.BlendEnabled &&
> -       !ctx->Color.AlphaEnabled &&
> -       (!ctx->Color.ColorLogicOpEnabled || ctx->Color.LogicOp
== GL_COPY) &&
> -       !ctx->Depth.Test &&
> -       !ctx->Fog.Enabled &&
> -       !ctx->Stencil.Enabled &&
> -       !ctx->FragmentProgram.Enabled &&
> -       !ctx->VertexProgram.Enabled &&
> -  !ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT] &&
> -       !_mesa_ati_fragment_shader_enabled(ctx) &&
> -       ctx->DrawBuffer->_NumColorDrawBuffers == 1 &&
> +       (type != GL_COLOR ||
> +        (ctx->_ImageTransferState == 0x0 &&
> +         !ctx->Color.BlendEnabled &&
> +         !ctx->Color.AlphaEnabled &&
> +         (!ctx->Color.ColorLogicOpEnabled || ctx->Color.LogicOp
== GL_COPY) &&
> +         !ctx->Depth.Test &&
> +         !ctx->Fog.Enabled &&
> +         !ctx->Stencil.Enabled &&
> +         !ctx->FragmentProgram.Enabled &&
> +         !ctx->VertexProgram.Enabled &&
> +  !ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT] &&
> +         !_mesa_ati_fragment_shader_enabled(ctx) &&
> +         ctx->DrawBuffer->_NumColorDrawBuffers == 1)) &&
>          !ctx->Query.CondRenderQuery &&
>          !ctx->Query.CurrentOcclusionObject) {
>         struct st_renderbuffer *rbRead, *rbDraw;
>
>         /*
>          * Clip the read region against the src buffer bounds.
>          * We'll still allocate a temporary buffer/texture for
the original
>          * src region size but we'll only read the region which
is on-screen.
>          * This may mean that we draw garbage pixels into the
dest region, but
>          * that's expected.
> @@ -1555,22 +1555,32 @@ blit_copy_pixels(struct gl_context *ctx,
GLint srcx, GLint srcy,
>         unpack = pack;
>         if (!_mesa_clip_drawpixels(ctx, , , ,
, ))
>            return GL_TRUE; /* all done */
>
>         readX = readX - pack.SkipPixels + unpack.SkipPixels;
>         readY = readY - pack.SkipRows + unpack.SkipRows;
>
>         drawW = readW;
>         drawH = readH;
>
> -      rbRead = st_get_color_read_renderbuffer(ctx);
> -      rbDraw =
st_renderbuffer(ctx->DrawBuffer->_ColorDrawBuffers[0]);
> +      if (type == GL_COLOR) {
> +         rbRead = st_get_color_read_renderbuffer(ctx);
> +         rbDraw =
st_renderbuffer(ctx->DrawBuffer->_ColorDrawB

Re: [Mesa-dev] [PATCH] st/mesa: accelerate glCopyPixels(STENCIL)

[apinheiro]

Hi, the following piglit test:

./bin/fbo-stencil copypixels GL_DEPTH32F_STENCIL8 -auto -fbo

regressed after this patch landed master with the v3d driver. So Marek 
and anyone reading this email, could you execute that test and confirms 
if only regress with v3d?


Thanks in advance.

On 25/6/19 2:12, Marek Olšák wrote:

From: Marek Olšák 

---
  src/mesa/state_tracker/st_cb_drawpixels.c | 58 +++
  1 file changed, 38 insertions(+), 20 deletions(-)

diff --git a/src/mesa/state_tracker/st_cb_drawpixels.c 
b/src/mesa/state_tracker/st_cb_drawpixels.c
index 59868d3ff1d..26d3cc33e5c 100644
--- a/src/mesa/state_tracker/st_cb_drawpixels.c
+++ b/src/mesa/state_tracker/st_cb_drawpixels.c
@@ -1508,35 +1508,35 @@ static GLboolean
  blit_copy_pixels(struct gl_context *ctx, GLint srcx, GLint srcy,
   GLsizei width, GLsizei height,
   GLint dstx, GLint dsty, GLenum type)
  {
 struct st_context *st = st_context(ctx);
 struct pipe_context *pipe = st->pipe;
 struct pipe_screen *screen = pipe->screen;
 struct gl_pixelstore_attrib pack, unpack;
 GLint readX, readY, readW, readH, drawX, drawY, drawW, drawH;
  
-   if (type == GL_COLOR &&

-   ctx->Pixel.ZoomX == 1.0 &&
+   if (ctx->Pixel.ZoomX == 1.0 &&
 ctx->Pixel.ZoomY == 1.0 &&
-   ctx->_ImageTransferState == 0x0 &&
-   !ctx->Color.BlendEnabled &&
-   !ctx->Color.AlphaEnabled &&
-   (!ctx->Color.ColorLogicOpEnabled || ctx->Color.LogicOp == GL_COPY) &&
-   !ctx->Depth.Test &&
-   !ctx->Fog.Enabled &&
-   !ctx->Stencil.Enabled &&
-   !ctx->FragmentProgram.Enabled &&
-   !ctx->VertexProgram.Enabled &&
-   !ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT] &&
-   !_mesa_ati_fragment_shader_enabled(ctx) &&
-   ctx->DrawBuffer->_NumColorDrawBuffers == 1 &&
+   (type != GL_COLOR ||
+(ctx->_ImageTransferState == 0x0 &&
+ !ctx->Color.BlendEnabled &&
+ !ctx->Color.AlphaEnabled &&
+ (!ctx->Color.ColorLogicOpEnabled || ctx->Color.LogicOp == GL_COPY) &&
+ !ctx->Depth.Test &&
+ !ctx->Fog.Enabled &&
+ !ctx->Stencil.Enabled &&
+ !ctx->FragmentProgram.Enabled &&
+ !ctx->VertexProgram.Enabled &&
+ !ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT] &&
+ !_mesa_ati_fragment_shader_enabled(ctx) &&
+ ctx->DrawBuffer->_NumColorDrawBuffers == 1)) &&
 !ctx->Query.CondRenderQuery &&
 !ctx->Query.CurrentOcclusionObject) {
struct st_renderbuffer *rbRead, *rbDraw;
  
/*

 * Clip the read region against the src buffer bounds.
 * We'll still allocate a temporary buffer/texture for the original
 * src region size but we'll only read the region which is on-screen.
 * This may mean that we draw garbage pixels into the dest region, but
 * that's expected.
@@ -1555,22 +1555,32 @@ blit_copy_pixels(struct gl_context *ctx, GLint srcx, 
GLint srcy,
unpack = pack;
if (!_mesa_clip_drawpixels(ctx, , , , , 
))
   return GL_TRUE; /* all done */
  
readX = readX - pack.SkipPixels + unpack.SkipPixels;

readY = readY - pack.SkipRows + unpack.SkipRows;
  
drawW = readW;

drawH = readH;
  
-  rbRead = st_get_color_read_renderbuffer(ctx);

-  rbDraw = st_renderbuffer(ctx->DrawBuffer->_ColorDrawBuffers[0]);
+  if (type == GL_COLOR) {
+ rbRead = st_get_color_read_renderbuffer(ctx);
+ rbDraw = st_renderbuffer(ctx->DrawBuffer->_ColorDrawBuffers[0]);
+  } else if (type == GL_DEPTH || type == GL_DEPTH_STENCIL) {
+ rbRead = 
st_renderbuffer(ctx->ReadBuffer->Attachment[BUFFER_DEPTH].Renderbuffer);
+ rbDraw = 
st_renderbuffer(ctx->DrawBuffer->Attachment[BUFFER_DEPTH].Renderbuffer);
+  } else if (type == GL_STENCIL) {
+ rbRead = 
st_renderbuffer(ctx->ReadBuffer->Attachment[BUFFER_STENCIL].Renderbuffer);
+ rbDraw = 
st_renderbuffer(ctx->DrawBuffer->Attachment[BUFFER_STENCIL].Renderbuffer);
+  } else {
+ return false;
+  }
  
/* Flip src/dst position depending on the orientation of buffers. */

if (st_fb_orientation(ctx->ReadBuffer) == Y_0_TOP) {
   readY = rbRead->Base.Height - readY;
   readH = -readH;
}
  
if (st_fb_orientation(ctx->DrawBuffer) == Y_0_TOP) {

   /* We can't flip the destination for pipe->blit, so we only adjust
* its position and flip the source.
@@ -1597,23 +1607,31 @@ blit_copy_pixels(struct gl_context *ctx, GLint srcx, 
GLint srcy,
   blit.src.box.depth = 1;
   blit.dst.resource = rbDraw->texture;
   blit.dst.level = rbDraw->surface->u.tex.level;
   blit.dst.format = rbDraw->texture->format;
   blit.dst.box.x = drawX;
   blit.dst.box.y = drawY;
   blit.dst.box.z = rbDraw->surface->u.tex.first_layer;
   blit.dst.box.width = drawW;
   

Re: [Mesa-dev] What does WIP really mean in an MR?

[apinheiro]

On 29/6/19 2:30, Rob Clark wrote:

I had interpreted it as literally the "block the gitlab merge button"
option, ie. "I want to get feedback but it is not ready to merge and
I'll drop the WIP tag when I think it is"..





(comments inline)

On Fri, Jun 28, 2019 at 5:12 PM Ian Romanick  wrote:

After a conversation yesterday with a couple of the other Intel devs,
I've come to the conclusion that *everyone* interprets WIP to mean
something different.  I heard no less than four interpretations.

* This series is good.  It hasn't been reviewed, so don't click "merge."

isn't that the point of a MR.. doesn't seem like a reason for "WIP"



I agree with Rob here. What I understand of WIP is that there is some 
reason that prevents a MR to be merged, even if I still want it to be 
reviewed, or if it is fully reviewed. For example, I send a MR with 
patches that I think that are correct, so people can start to review it. 
But on a rebase, I found that the branch causes regressions on 
piglit/cts/whatever. I still think that the patches are correct, but 
those regressions need to be investigated before pushing. So I just add 
a WIP on the MR to prevent to be merged by mistake.





* This series has some sketchy bits.  It probably isn't ready for review
unless you've been tagged for design feedback.

I guess I'd also use WIP for "I want some early feedback, but it isn't
ready yet".. but in this case I'd also poke people who I wanted to
look at it



I thought that in this case RFC was used. Or RFC was dropped on gitlab MRs?





* This series has been reviewed.  Incorporation of detailed feedback is
in progress, but it's going to take some time.

I suppose also a case for "WIP"..


* This series is good, but there are some questionable patches at the end.

I guess in this case, I'd reform things into multiple MR's, one with
the parts ready to go, and one w/ the remaining WIP bits

BR,
-R


Due to this lack of common understanding, we discovered at least one MR
that was ready to go but had been ignored for months. :(  This makes me
wonder if other MRs have similarly languished for no good reason.

Can we formulate some guidelines for how people should apply WIP to
their MRs and how people should interpret WIP when they see it on an MR?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

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

Re: [Mesa-dev] [PATCH] docs: advice to resolve discussion on gitlab MR doc

[apinheiro]

On 16/5/19 13:35, Eric Engestrom wrote:

On Thursday, 2019-05-16 13:14:46 +0200, Connor Abbott wrote:

Some grammar nits:

- "Resolve Discussion" goes before "button" as it modifies it.
- It's either "This way..." or "In this manner...", not "In this
way...", although the latter is a little too stilted/over-formal here.
- This isn't a hypothetical or another situation where "would know..."
is appropriate.
- "...which didn't" (since it's short for "which didn't get handled").

The corrected text is:

After an update, for the feedback you handled, close the feedback
discussion with the "Resolve Discussion" button. This way the reviewer
knows which feedback got handled and which didn't.

I definitely didn't notice this when I started using Gitlab either.
With the fixed text:

Reviewed-by: Connor Abbott 

I agree with the message, and the fixed up text by Connor is:
Reviewed-by: Eric Engestrom 

If this isn't too bikeshed-y, I would also say "the reviewers know" as
there are potentially (and usually) more than one, but this really
doesn't matter that much.



I pushed the patch with Connor text plus this suggestion. Thanks for the 
quick review! (and for basically rewrite my broken grammar english text)





On Thu, May 16, 2019 at 11:36 AM Alejandro Piñeiro  wrote:

For newcomers to gitlab, it is not evident that it is better to press
the "Resolve Discussion" button when you update your branch handling
feedback.
---

As the commit message says, it is not always evident. I was pointed to
do that when I started to use gitlab, and just today I mentioned it to
two different people that didn't know about that.

Having said so, I feel that the specific text needs some poulishing
first, so any suggestion is welcome.

  docs/submittingpatches.html | 4 
  1 file changed, 4 insertions(+)

diff --git a/docs/submittingpatches.html b/docs/submittingpatches.html
index 020e73d09ec..147b97d76e1 100644
--- a/docs/submittingpatches.html
+++ b/docs/submittingpatches.html
@@ -258,6 +258,10 @@ your email administrator for this.)
  
  
Make changes and update your branch based on feedback
+  After an update, for the feedback you handled, close the
+  feedback discussion with the button "Resolve Discussion". In this
+  way the reviewer would know which feedback got handled and which
+  not.
Old, stale MR may be closed, but you can reopen it if you
  still want to pursue the changes
You should periodically check to see if your MR needs to be
--
2.19.1

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

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

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

[Mesa-dev] shader-db MR: run: set NULL as initial value for binding_list

[apinheiro]

Without it, under some specific compilation options, it can be 
initialized to NULL or to garbage. On the latter case, if the shader 
doesn't require a binding_list, would cause a crash later when it 
attempts to be used.


https://gitlab.freedesktop.org/mesa/shader-db/merge_requests/7

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

[Mesa-dev] [MR] anv: fix use after free when copying nir_xfb_info

[apinheiro]

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

After adding varyings to nir_xfb_info, I added varyings as a pointer, 
and replaced outputs[0] for also a pointer, so now both needed to be 
allocated. But anv was copying such xfb info as: *xfb_info = *xfb_info_in


So after my changes, that line was assigning the outputs pointer, 
instead of copying. Then xfb_info_in was freed, and that included their 
outputs, that now are xfb_info outputs too. Unfourtunately, as as with 
other use after free crashes, the crash didn't happen always, just with 
some configurations. And that included not crashing on Intel CI.


Thanks @jasuarez  for pinging 
me with this issue.


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

Re: [Mesa-dev] [PATCH] nir/serialize: Make preserving names optional

[apinheiro]

Reviewed-by: Alejandro Piñeiro 

Out of curiosity: right now it is always used as true (so maintain 
current behaviour). How it is intended to be used? With an envvar?


On 7/3/19 22:38, Jason Ekstrand wrote:

---
  src/compiler/nir/nir_serialize.c  | 56 ---
  src/compiler/nir/nir_serialize.h  |  3 +-
  .../drivers/radeonsi/si_state_shaders.c   |  2 +-
  src/intel/vulkan/anv_pipeline_cache.c |  2 +-
  .../drivers/dri/i965/brw_program_binary.c |  2 +-
  src/mesa/state_tracker/st_shader_cache.c  |  2 +-
  6 files changed, 42 insertions(+), 25 deletions(-)

diff --git a/src/compiler/nir/nir_serialize.c b/src/compiler/nir/nir_serialize.c
index 743eeaed3d5..ad094beb1f4 100644
--- a/src/compiler/nir/nir_serialize.c
+++ b/src/compiler/nir/nir_serialize.c
@@ -42,6 +42,9 @@ typedef struct {
 /* the next index to assign to a NIR in-memory object */
 uintptr_t next_idx;
  
+   /* Whether or not to preserve variable and SSA def names */

+   bool preserve_names;
+
 /* Array of write_phi_fixup structs representing phi sources that need to
  * be resolved in the second pass.
  */
@@ -136,9 +139,13 @@ write_variable(write_ctx *ctx, const nir_variable *var)
  {
 write_add_object(ctx, var);
 encode_type_to_blob(ctx->blob, var->type);
-   blob_write_uint32(ctx->blob, !!(var->name));
-   if (var->name)
-  blob_write_string(ctx->blob, var->name);
+   if (ctx->preserve_names) {
+  blob_write_uint32(ctx->blob, !!(var->name));
+  if (var->name)
+ blob_write_string(ctx->blob, var->name);
+   } else {
+  blob_write_uint32(ctx->blob, 0);
+   }
 blob_write_bytes(ctx->blob, (uint8_t *) >data, sizeof(var->data));
 blob_write_uint32(ctx->blob, var->num_state_slots);
 blob_write_bytes(ctx->blob, (uint8_t *) var->state_slots,
@@ -224,9 +231,13 @@ write_register(write_ctx *ctx, const nir_register *reg)
 blob_write_uint32(ctx->blob, reg->bit_size);
 blob_write_uint32(ctx->blob, reg->num_array_elems);
 blob_write_uint32(ctx->blob, reg->index);
-   blob_write_uint32(ctx->blob, !!(reg->name));
-   if (reg->name)
-  blob_write_string(ctx->blob, reg->name);
+   if (ctx->preserve_names) {
+  blob_write_uint32(ctx->blob, !!(reg->name));
+  if (reg->name)
+ blob_write_string(ctx->blob, reg->name);
+   } else {
+  blob_write_uint32(ctx->blob, 0);
+   }
 blob_write_uint32(ctx->blob, reg->is_global << 1 | reg->is_packed);
  }
  
@@ -327,7 +338,7 @@ write_dest(write_ctx *ctx, const nir_dest *dst)

  {
 uint32_t val = dst->is_ssa;
 if (dst->is_ssa) {
-  val |= !!(dst->ssa.name) << 1;
+  val |= (ctx->preserve_names && dst->ssa.name) << 1;
val |= dst->ssa.num_components << 2;
val |= dst->ssa.bit_size << 5;
 } else {
@@ -336,7 +347,7 @@ write_dest(write_ctx *ctx, const nir_dest *dst)
 blob_write_uint32(ctx->blob, val);
 if (dst->is_ssa) {
write_add_object(ctx, >ssa);
-  if (dst->ssa.name)
+  if (ctx->preserve_names && dst->ssa.name)
   blob_write_string(ctx->blob, dst->ssa.name);
 } else {
blob_write_intptr(ctx->blob, write_lookup_object(ctx, dst->reg.reg));
@@ -1079,28 +1090,33 @@ read_function(read_ctx *ctx)
  }
  
  void

-nir_serialize(struct blob *blob, const nir_shader *nir)
+nir_serialize(struct blob *blob, const nir_shader *nir, bool preserve_names)
  {
 write_ctx ctx;
 ctx.remap_table = _mesa_pointer_hash_table_create(NULL);
 ctx.next_idx = 0;
 ctx.blob = blob;
 ctx.nir = nir;
+   ctx.preserve_names = preserve_names;
 util_dynarray_init(_fixups, NULL);
  
 size_t idx_size_offset = blob_reserve_intptr(blob);
  
 struct shader_info info = nir->info;

-   uint32_t strings = 0;
-   if (info.name)
-  strings |= 0x1;
-   if (info.label)
-  strings |= 0x2;
-   blob_write_uint32(blob, strings);
-   if (info.name)
-  blob_write_string(blob, info.name);
-   if (info.label)
-  blob_write_string(blob, info.label);
+   if (ctx.preserve_names) {
+  uint32_t strings = 0;
+  if (info.name)
+ strings |= 0x1;
+  if (info.label)
+ strings |= 0x2;
+  blob_write_uint32(blob, strings);
+  if (info.name)
+ blob_write_string(blob, info.name);
+  if (info.label)
+ blob_write_string(blob, info.label);
+   } else {
+  blob_write_uint32(blob, 0);
+   }
 info.name = info.label = NULL;
 blob_write_bytes(blob, (uint8_t *) , sizeof(info));
  
@@ -1204,7 +1220,7 @@ nir_shader_serialize_deserialize(void *mem_ctx, nir_shader *s)
  
 struct blob writer;

 blob_init();
-   nir_serialize(, s);
+   nir_serialize(, s, true);
 ralloc_free(s);
  
 struct blob_reader reader;

diff --git a/src/compiler/nir/nir_serialize.h b/src/compiler/nir/nir_serialize.h
index f77d8e367ff..22e5fa88ee1 100644
--- a/src/compiler/nir/nir_serialize.h
+++ b/src/compiler/nir/nir_serialize.h
@@ -31,7 +31,8 @@
  extern "C" {
  #endif
  

Re: [Mesa-dev] [PATCH] nir/split_vars: Don't compact vectors unnecissarily

[apinheiro]

On 23/2/19 12:07, apinheiro wrote:

Reviewed-by: Alejandro Piñeiro 



Well, just in case, shouldn't be unnecessarily instead of unnecissarily?




On 23/2/19 6:14, Jason Ekstrand wrote:

---
  src/compiler/nir/nir_split_vars.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/src/compiler/nir/nir_split_vars.c 
b/src/compiler/nir/nir_split_vars.c

index 244ffd6dcf0..96b6042e6d9 100644
--- a/src/compiler/nir/nir_split_vars.c
+++ b/src/compiler/nir/nir_split_vars.c
@@ -1423,6 +1423,12 @@ shrink_vec_var_access_impl(nir_function_impl 
*impl,

 continue;
  }
  +    /* If we're not dropping any components, there's no 
need to

+ * compact vectors.
+ */
+    if (usage->comps_kept == usage->all_comps)
+   continue;
+
  if (intrin->intrinsic == nir_intrinsic_load_deref) {
 b.cursor = nir_after_instr(>instr);

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

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

Re: [Mesa-dev] [PATCH] nir/split_vars: Don't compact vectors unnecissarily

[apinheiro]

Reviewed-by: Alejandro Piñeiro 

On 23/2/19 6:14, Jason Ekstrand wrote:

---
  src/compiler/nir/nir_split_vars.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/src/compiler/nir/nir_split_vars.c 
b/src/compiler/nir/nir_split_vars.c
index 244ffd6dcf0..96b6042e6d9 100644
--- a/src/compiler/nir/nir_split_vars.c
+++ b/src/compiler/nir/nir_split_vars.c
@@ -1423,6 +1423,12 @@ shrink_vec_var_access_impl(nir_function_impl *impl,
 continue;
  }
  
+/* If we're not dropping any components, there's no need to

+ * compact vectors.
+ */
+if (usage->comps_kept == usage->all_comps)
+   continue;
+
  if (intrin->intrinsic == nir_intrinsic_load_deref) {
 b.cursor = nir_after_instr(>instr);
  

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

Re: [Mesa-dev] [PATCH] nir/xfb: Properly align 64-bit values

[apinheiro]

I vaguely remember glslang doing something similar when computing 
offsets. This, and other xfb corner cases, were the reason I 
thought/concluded it would made sense to let glslang (or any other 
frontend) to do the offset assignment also for structs, so SPIR-V 
consuming drivers didn't need to handle all that mess, and could just 
expect to everything having his offset/stride assigned. In any case, we 
also found that doing that can be problematic, and Im just rambling... 
The patch LGTM:


Reviewed-by: Alejandro Piñeiro 

On 12/2/19 20:22, Jason Ekstrand wrote:

Fixes: 19064b8c "nir: Add a pass for gathering transform feedback info"
Cc: Alejandro Piñeiro 
---
  src/compiler/nir/nir_gather_xfb_info.c | 44 ++
  1 file changed, 44 insertions(+)

diff --git a/src/compiler/nir/nir_gather_xfb_info.c 
b/src/compiler/nir/nir_gather_xfb_info.c
index 96f0ece5e75..fb736dfeb17 100644
--- a/src/compiler/nir/nir_gather_xfb_info.c
+++ b/src/compiler/nir/nir_gather_xfb_info.c
@@ -72,6 +72,50 @@ add_var_xfb_outputs(nir_xfb_info *xfb,
assert(var->data.location_frac + comp_slots <= 8);
uint8_t comp_mask = ((1 << comp_slots) - 1) << var->data.location_frac;
  
+  /* From version 4.60 of the GLSL spec:

+   *
+   *"Variables and block members qualified with xfb_offset can be
+   *scalars, vectors, matrices, structures, and (sized) arrays of
+   *these. The offset must be a multiple of the size of the first
+   *component of the first qualified variable or block member, or a
+   *compile-time error results. Further, if applied to an aggregate
+   *containing a double, the offset must also be a multiple of 8, and
+   *the space taken in the buffer will be a multiple of 8. The given
+   *offset applies to the first component of the first member of the
+   *qualified entity. Then, within the qualified entity, subsequent
+   *components are each assigned, in order, to the next available
+   *offset aligned to a multiple of that component's size. Aggregate
+   *types are flattened down to the component level to get this
+   *sequence of components."
+   *
+   * We need to align each element to the component size in order to get
+   * the correct layout.  We do this at the component level and don't try
+   * to align entire aggregate types such as structs because of the last
+   * sentence which says that aggregate types are treated as flattened to
+   * components.  In other words, if we have
+   *
+   *struct A {
+   *   int a;
+   *   double b;
+   *};
+   *
+   *struct B {
+   *   int b
+   *   A a;
+   *};
+   *
+   *layout (...) out B o;
+   *
+   * then we treat it as if struct A was embedded struct B and o.a.b has
+   * an offset of 8.  If we tried to apply the alignment rule to nested
+   * structs and didn't flatten, o.a would have an offset of 8 because it
+   * contains a double and o.a.b would then have an offset of 16.
+   * However, thanks to the above GLSL rule, o.b and o.a.a are tightly
+   * packed and there is no gap.
+   */
+  if (glsl_type_is_64bit(type))
+ *offset = ALIGN_POT(*offset, 8);
+
assert(attrib_slots <= 2);
for (unsigned s = 0; s < attrib_slots; s++) {
   nir_xfb_output_info *output = >outputs[xfb->output_count++];

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

[Mesa-dev] [MR] nir: move pixel_center_integer/origin_upper_left to shader_info.fs

[apinheiro]

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

New version of the thread that I sent recently, showing two initial 
versions to solve the regression I found on MR #144:


https://lists.freedesktop.org/archives/mesa-dev/2019-February/214808.html

This MR includes a v2 of the second option. It includes Jason's feedback 
plus some extra cleaning-ups that I found while re-checking the GLSL 
linker (like removing PixelCenterInteger/OriginUpperLeft from 
gl_program, as that info is tracked at gl_program.info.fs.xxx). Due 
that, it touches a lot of places on Mesa.


This MR also includes a second patch, that removes 
pixel_center_integer/origin_upper_left from the ir variable. Basically 
because that is already tracked in a lot of places, so it is not really 
needed. It is on a different patch because I initially though of it as 
something optional, as it is not really required to fix the regression. 
But after finishing it, the only reason to keep it as a different patch 
is to make easier the review. I think that it would be better to squash 
both patches, although I don't have a strong opinion, I would let the 
reviewer give his opinion.



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

Re: [Mesa-dev] [PATCH] nir/deref: Rematerialize parents in rematerialize_derefs_in_use_blocks

[apinheiro]

Reviewed-by: Alejandro Piñeiro 

On 11/2/19 5:28, Jason Ekstrand wrote:

When nir_rematerialize_derefs_in_use_blocks_impl was first written, I
attempted to optimize things a bit by not bothering to re-materialize
the sources of deref instructions figuring that the final caller would
take care of that.  However, in the case of more complex deref chains
where the first link or two lives in block A and then another link and
the load/store_deref intrinsic live in block B it doesn't work.  The
code in rematerialize_deref_in_block looks at the tail of the chain,
sees that it's already in block B and skips it, not realizing that part
of the chain also lives in block A.

The easy solution here is to just rematerialize deref sources of deref
instructions as well.  This may potentially lead to a few more deref
instructions being created by the conditions required for that to
actually happen are fairly unlikely and, thanks to the caching, it's all
linear time regardless.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109603
---
  src/compiler/nir/nir_deref.c | 5 ++---
  1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/compiler/nir/nir_deref.c b/src/compiler/nir/nir_deref.c
index 13aa10c7532..7b56611915d 100644
--- a/src/compiler/nir/nir_deref.c
+++ b/src/compiler/nir/nir_deref.c
@@ -574,10 +574,9 @@ 
nir_rematerialize_derefs_in_use_blocks_impl(nir_function_impl *impl)
   _mesa_hash_table_clear(state.cache, NULL);
  
nir_foreach_instr_safe(instr, block) {

- if (instr->type == nir_instr_type_deref) {
-nir_deref_instr_remove_if_unused(nir_instr_as_deref(instr));
+ if (instr->type == nir_instr_type_deref &&
+ nir_deref_instr_remove_if_unused(nir_instr_as_deref(instr)))
  continue;
- }
  
   state.builder.cursor = nir_before_instr(instr);

   nir_foreach_src(instr, rematerialize_deref_src, );

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

Re: [Mesa-dev] [PATCH] glsl: glsl to nir fix uninit static class member.

[apinheiro]

Reviewed-by: Alejandro Piñeiro 

On 8/2/19 6:24, Dave Airlie wrote:

From: Dave Airlie 

The constructor should init this to NULL
---
  src/compiler/glsl/glsl_to_nir.cpp | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/compiler/glsl/glsl_to_nir.cpp 
b/src/compiler/glsl/glsl_to_nir.cpp
index 09599e4cee7..d62de862fac 100644
--- a/src/compiler/glsl/glsl_to_nir.cpp
+++ b/src/compiler/glsl/glsl_to_nir.cpp
@@ -180,6 +180,7 @@ nir_visitor::nir_visitor(nir_shader *shader)
 this->overload_table = _mesa_pointer_hash_table_create(NULL);
 this->result = NULL;
 this->impl = NULL;
+   this->deref = NULL;
 memset(>b, 0, sizeof(this->b));
  }
  

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

[Mesa-dev] [MR] mesa/i965: ARB_gl_spirv and ARB_spirv_extensions implementation, plus 4.6

[apinheiro]

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

TL; DR; This series implements all the mesa bits needed to enable 
ARB_gl_spirv and ARB_spirv_extensions on the i965 driver, and with that, 
expose OpenGL 4.6 on that driver.


Detailed explanation: we reached a point where we consider our 
development branch good enough to enable both extensions on i965, so we 
preferred to send the full series, instead of keeping sending subseries 
of specific sub-features. As a collateral effect of enabling both 
extensions we can also expose OpenGL 4.6 on i965.


It is worth to note that some of those patches could be easily squashed 
with some others, but we preferred to keep them, to make easier the 
review process.


Although on this MR the patch numbering will get quickly obsolete, here 
an overview of the the patches at the moment this MR got sent:


 *

   Patches 1-5 provides improvements over current ARB_gl_spirv xfb
   support. It basically extends the xfb nir gathering pass recently
   added for Vulkan needs, and then replaces the ARB_gl_spirv custom
   one and uses it instead.

 *

   Patches 6-24 implemens UBO/SSBO support, and calls to link them on
   the i965 driver. It is worth to note that it is using the new deref
   based UBO/SSBO path, so we could get arrays of arrays properly
   supported on the spirv to nir pass.

 *

   Patches 25-33 adds the different resources already supported by the
   ARB_gl_spirv linker to the resource list (input/output, ubo/ssbo,
   etc), and fill up properly some data for such resources, in
   preparation to the program interface queries support.

 *

   Patches 34-41 fixes and extends several program interfaces queries.
   In most of the cases they are just to handling the fact that now
   resource name can be NULL, and applies the value expected for such
   case defined on the ARB_gl_spirv spec.

 *

   Patches 42-45 gets serialization working when using SPIR-V shaders.
   As the previous case, most patches are about handling the fact that
   names can be NULL, plus initializing UniformDataDefaults when using
   the ARB_gl_spirv nir linker. All this got interaction with
   ARB_get_program_binary working (at least with the tests we had an hand).

 *

   Patch 46 prevents to load the shader cache. Even if we handle the
   serialization with previous patches, shader cache is still not
   working with SPIR-V shaders, so we prevent the cache to be used on
   that case. Taking into account that the ARB_gl_spirv spec doesn't
   require the cache to be working, we thought that we shouldn't wait
   to enable the extension until finishing it, and clearly we shouldn't
   wait for it to send to review.

 *

   Patches 47-48 add some validations. It is worth to note that
   ARB_gl_spirv doesn't require too much validation. That includes the
   validations added with those patches. As with Vulkan, it is assumed
   that the SPIR-V shaders should be correct. Having said so, the spec
   also allows to raise them as link errors if you want. We implemented
   those validations during development, and we think that they are
   fine and simple enough validations to be included.

 *

   Patch 49: enable ARB_gl_spirv on i965.

 *

   Patches 50-55 adds the support for ARB_spirv_extensions, and enable
   it on i965. FWIW, we sent this several months ago to review. It
   didn't change anything since then (except minor changes due rebases).

 *

   Patch 56: exposes OpenGL 4.6 on i965.

So about how this series got tested. First, this series gets all the 
ARB_gl_spirv and ARB_spirv_extensions CTS tests passing. So in theory 
that should be enough to enable both extensions. In the practice, those 
tests were not enough, so we tested this series with piglit patches. For 
that we had two types of tests:


 *

   Barebone tests: tests we were adding as we implemented the
   extension. Piglit master already have some of them.

 *

   Borrowed tests: recycled tests from other specs. For this we use a
   heavily modified version of a script wrote by Nicolai Hähnle, that
   do some fixes on shader_test tests, and then uses glslang to convert
   them to SPIR-V.

Unfourtunately, right now the previous two are not ready for review 
(except those that we already sent), as we were focusing on getting the 
Mesa support done. We are cleaning both this week, so we will send to 
review all those on the following weeks.


Having said so, right now we get the following outcome when run all the 
shader piglit tests on SPIR-V mode:


[34596/34596] skip: 6020, pass: 28559, fail: 17, crash: 0

FWIW, this is the outcome on GLSL mode:

[34596/34596] skip: 4877, pass: 29714, fail: 3, crash: 2

Probably the main difference is the greater amount of skipped tests on 
the SPIR-V run (for example, those crashed tests on the GLSL run are 
skipped on the SPIR-V run). This increase comes from:


 *

   Tests that doesn't make sense on ARB_gl_spirv, like old GLSL
   functionality that is not supported, or name-based 

[Mesa-dev] [MR] spirv/nir: handle location decorations on block interface members, plus cleaning

[apinheiro]

While doing a review of the current ARB_gl_spirv series, I found that 
this one also applies to Vulkan, and fixes some Vulkan tests, so I 
preferred to send it in advance, and keep any ARB_gl_spirv series to 
review to only apply to OpenGL.


FWIW, this was sent a long time ago [1], but since then the patch needed 
several updates. We are also dropping the third patch of that series, as 
right now it is not working/fixing as intended (investigate it just 
added to my TODO, but will not have too much priority).


As part of this, I also took the Vulkan tests from a custom vkrunner 
branch, and imported to piglit. You can find them here [2], but I will 
send in short to the piglit mailing list to be reviewed.


[1] https://patchwork.freedesktop.org/series/40797/

[2] 
https://github.com/Igalia/piglit/tree/review/handle-block-member-location



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

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

Re: [Mesa-dev] [PATCH v1] nir: Length of boolean vtn_value now is 1

[apinheiro]

I was waiting Jason to chime in, but just in case he is too busy I took 
a look in detail to the patch. It LGTM, so assuming it passes intel CI:


Reviewed-by: Alejandro Piñeiro 

On 15/1/19 12:08, Sergii Romantsov wrote:

During conversion type-length was lost due to math.

CC: Jason Ekstrand 
Fixes: 44227453ec03 (nir: Switch to using 1-bit Booleans for almost everything)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109353
Signed-off-by: Sergii Romantsov 
---
  src/compiler/spirv/spirv_to_nir.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/compiler/spirv/spirv_to_nir.c 
b/src/compiler/spirv/spirv_to_nir.c
index e3dc619..faad771 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1042,14 +1042,16 @@ vtn_type_layout_std430(struct vtn_builder *b, struct 
vtn_type *type,
  {
 switch (type->base_type) {
 case vtn_base_type_scalar: {
-  uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
+  uint32_t comp_size = glsl_type_is_boolean(type->type)
+ ? 1 : glsl_get_bit_size(type->type) / 8;
*size_out = comp_size;
*align_out = comp_size;
return type;
 }
  
 case vtn_base_type_vector: {

-  uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
+  uint32_t comp_size = glsl_type_is_boolean(type->type)
+ ? 1 : glsl_get_bit_size(type->type) / 8;
unsigned align_comps = type->length == 3 ? 4 : type->length;
*size_out = comp_size * type->length,
*align_out = comp_size * align_comps;
@@ -1168,7 +1170,8 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
val->type->base_type = vtn_base_type_vector;
val->type->type = glsl_vector_type(glsl_get_base_type(base->type), 
elems);
val->type->length = elems;
-  val->type->stride = glsl_get_bit_size(base->type) / 8;
+  val->type->stride = glsl_type_is_boolean(val->type->type)
+ ? 1 : glsl_get_bit_size(base->type) / 8;
val->type->array_element = base;
break;
 }

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

Re: [Mesa-dev] Thoughts after hitting 100 merge requests?

[apinheiro]

On 17/1/19 9:39, Kenneth Graunke wrote:

On Wednesday, January 16, 2019 11:38:05 PM PST Erik Faye-Lund wrote:

On Fri, 2019-01-11 at 10:57 -0600, Jason Ekstrand wrote:

All,

The mesa project has now hit 100 merge requests (36 are still open).
I (and I'm sure others) would be curious to hear people's initial
thoughts on the process.  What's working well?  What's not working?
Is it total fail and should we go back to mailing lists?


So, overall I think it works pretty well. I have some things I think
maybe we could do better, some of which has already been pointed out:

1. New MRs should probably get their cover-letter automatically sent to
the mailing list for incrased visibility.

2. Perhaps we should ban sending MRs from the main mesa repo? With
gitlab, it's trivial to make your own fork, and you can delegate
permissions to other users for collaborators. I don't think there's any
reason to clutter up the main mesa repo with all kinds of branches. But
it seems some people send their MRs from the main-repo anyway. Perhaps
we should document that this isn't how to send MRs?

I agree, I would much rather see MRs sent from personal forks.
That's what I've been doing, and it works well.  If you see people
doing that, feel free to say something - I assume they just don't
realize they can do that.



As one of the people doing it wrongly (using a branch of the main repo, 
instead of a personal fork): yes please, let's document that, as I was 
unaware that was a problem.


What Im not sure if it is possible to update a existing MR in order to 
use a different repo/branch. If that is not possible, should I close my 
MR and reopen it with a fork?





--Ken

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

Re: [Mesa-dev] [PATCH v1] nir: Length of boolean vtn_value now is 1

[apinheiro]

Just tested it with the ARB_gl_spirv tests where I found this:

Tested-by: Alejandro Piñeiro 


On 15/1/19 12:08, Sergii Romantsov wrote:

During conversion type-length was lost due to math.

CC: Jason Ekstrand 
Fixes: 44227453ec03 (nir: Switch to using 1-bit Booleans for almost everything)
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=109353
Signed-off-by: Sergii Romantsov 
---
  src/compiler/spirv/spirv_to_nir.c | 9 ++---
  1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/src/compiler/spirv/spirv_to_nir.c 
b/src/compiler/spirv/spirv_to_nir.c
index e3dc619..faad771 100644
--- a/src/compiler/spirv/spirv_to_nir.c
+++ b/src/compiler/spirv/spirv_to_nir.c
@@ -1042,14 +1042,16 @@ vtn_type_layout_std430(struct vtn_builder *b, struct 
vtn_type *type,
  {
 switch (type->base_type) {
 case vtn_base_type_scalar: {
-  uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
+  uint32_t comp_size = glsl_type_is_boolean(type->type)
+ ? 1 : glsl_get_bit_size(type->type) / 8;
*size_out = comp_size;
*align_out = comp_size;
return type;
 }
  
 case vtn_base_type_vector: {

-  uint32_t comp_size = glsl_get_bit_size(type->type) / 8;
+  uint32_t comp_size = glsl_type_is_boolean(type->type)
+ ? 1 : glsl_get_bit_size(type->type) / 8;
unsigned align_comps = type->length == 3 ? 4 : type->length;
*size_out = comp_size * type->length,
*align_out = comp_size * align_comps;
@@ -1168,7 +1170,8 @@ vtn_handle_type(struct vtn_builder *b, SpvOp opcode,
val->type->base_type = vtn_base_type_vector;
val->type->type = glsl_vector_type(glsl_get_base_type(base->type), 
elems);
val->type->length = elems;
-  val->type->stride = glsl_get_bit_size(base->type) / 8;
+  val->type->stride = glsl_type_is_boolean(val->type->type)
+ ? 1 : glsl_get_bit_size(base->type) / 8;
val->type->array_element = base;
break;
 }

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

Re: [Mesa-dev] Thoughts after hitting 100 merge requests?

[apinheiro]

On 15/1/19 7:01, Tapani Pälli wrote:
>
>
> On 1/14/19 2:36 PM, Daniel Stone wrote:
>> Hi,
>>
>> On Fri, 11 Jan 2019 at 17:05, Jason Ekstrand 
>> wrote:
>>>   5. There's no way with gitlab for Reviewed-by tags to get
>>> automatically applied as part of the merging process.  This makes
>>> merging a bit more manual than it needs to be but is really no worse
>>> than it was before.
>>
>> I'm still on the side of not seeing the value in them. Most of the
>> time when I go to pursue someone who reviewed a commit, I'll go to see
>> what came up in review anyway. Maybe someone had the same comment
>> which was found to be not applicable or otherwise explained away.
>> Reviewed-by and Acked-by are also pretty lossy anyway, and freeform
>> text descriptors in a comment can much better capture the intent (e.g.
>> 'I'm strongly OK with the driver changes and weakly OK with the core
>> changes as it's not really my area of expertise').
>>
>> In other projects, we looked for ways to apply the tags and ended up
>> concluding that they didn't bring enough value to make it worthwhile.
>> I don't know if that holds for Mesa, but it would be better to start
>> with an actual problem statement - what value does R-b bring and how?
>> - then look at ways to solve that problem, rather than just very
>> directly finding a way to insert that literal text string into every
>> commit message.
>
> IMO it brings some 'shared responsibility' for correctness of the
> patch and quickly accessible information on who were looking at the
> change. So ideally later when filing bug against commit/series there
> would be more people than just the committer that should take a look
> at the possible regressions. At least in my experience people filing
> bugs tend to often also CC the reviewer.


In addition to that, it is also useful for big series that are updated
several times, as is a way to know which patches were already reviewed
and which not, so reviewer can focus on the latter.


>
>> FWIW, if you go to
>> https://gitlab.freedesktop.org/mesa/mesa/commit/SHA1 then you get a
>> hyperlink from the web UI which points you to the MR. The API to do
>> this is pretty straightforward and amenable to piping through jq:
>> https://docs.gitlab.com/ce/api/commits.html#list-merge-requests-associated-with-a-commit
>>
>
> I guess if we would move issue tracking to gitlab then we could
> possibly automate the CC list generation based on commit?
>
> // Tapani
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Thoughts after hitting 100 merge requests?

[apinheiro]

I mostly agree with your thoughts below. Will add some additional
comments inline.

On 11/1/19 18:05, Jason Ekstrand wrote:
> I'm putting my own thoughts in a reply for some reason.  Here's what
> I've seen.
>
>  1. I really like GitLab "discussions".  It provides a very good way
> for both the author and the reviewers to keep track of what review
> comments have been dealt with and what comments are still outstanding.


Yes, I agree that the general discussion for a series has improved. But ...

>
>  2. GitLab is currently missing a good way to comment on commit
> messages which makes giving review tags rather painful.  There is a
> GitLab issue opened about this:
> https://gitlab.com/gitlab-org/gitlab-ce/issues/38602
>
>  3. GitLab has a bug regarding per-commit comments where they tend to
> get lost while you're looking at the commit itself:
> https://gitlab.com/gitlab-org/gitlab-ce/issues/53175


... as you mention here, there are some per-commit bugs. This makes
per-commit discussion and tagging (as you mention below) harder, so the
general discussion gets somewhat messy with individual commit messages.
And in addition to what your comment here, I miss the possibility to add
an annotate section on individual commits. For example, the usual
annotate section "I have this, but I'm not happy of X due Y, what do you
think", or in other words, a placeholder for starting a
discussion/debate for such commit. I guess that if those bugs are fixed,
it would be just doing the push, and then adding those "annotate
sections" on the commits.

>
>  4. At least two of those merge requests were small bug fixes by brand
> new contributors who I've never seen on the mailing list.
>
>  5. There's no way with gitlab for Reviewed-by tags to get
> automatically applied as part of the merging process.  This makes
> merging a bit more manual than it needs to be but is really no worse
> than it was before.


Well, I would say that it slightly worse. For small series, it is true
that I manually added the Rb when I got a review. But for big series,
when it got reviewed, I used patchwork to get back the series, but with
the Rb in place.

>
> Ok, there you have my thoughts.  I'd be happy to hear others.
>
> --Jason
>
> On Fri, Jan 11, 2019 at 10:57 AM Jason Ekstrand  > wrote:
>
> All,
>
> The mesa project has now hit 100 merge requests (36 are still
> open).  I (and I'm sure others) would be curious to hear people's
> initial thoughts on the process.  What's working well?  What's not
> working?  Is it total fail and should we go back to mailing lists?
>
> --Jason
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] MR: ARB_gl_spirv xfb improvement

[apinheiro]

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

This series started as just trying to use the general NIR xfb gathering
pass, added for Vulkan consumption, and ended fixing some corner cases
on such gathering, and on the ARB_gl_spirv support for xfb itself. All
that can be tested on piglit here:
https://github.com/Igalia/piglit/tree/apinheiro/xfb

For now a personal branch. It is mostly done, but it is pending the
double checking before sending to review.

As part of this series, we also change how input arrays of blocks are
splitted, in order to support interpolators qualifiers for such case.



pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/3] nir: make nir_opt_remove_phis_impl() static

[apinheiro]

The three patches:

Reviewed-by: Alejandro Piñeiro 

On 2/1/19 6:00, Timothy Arceri wrote:
> ---
>  src/compiler/nir/nir.h | 1 -
>  src/compiler/nir/nir_opt_remove_phis.c | 2 +-
>  2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index 4b8de4bb01..94d6578620 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -3221,7 +3221,6 @@ bool nir_opt_move_load_ubo(nir_shader *shader);
>  bool nir_opt_peephole_select(nir_shader *shader, unsigned limit,
>   bool indirect_load_ok, bool expensive_alu_ok);
>  
> -bool nir_opt_remove_phis_impl(nir_function_impl *impl);
>  bool nir_opt_remove_phis(nir_shader *shader);
>  
>  bool nir_opt_shrink_load(nir_shader *shader);
> diff --git a/src/compiler/nir/nir_opt_remove_phis.c 
> b/src/compiler/nir/nir_opt_remove_phis.c
> index e2d3994c49..d7ca2fe717 100644
> --- a/src/compiler/nir/nir_opt_remove_phis.c
> +++ b/src/compiler/nir/nir_opt_remove_phis.c
> @@ -139,7 +139,7 @@ remove_phis_block(nir_block *block, nir_builder *b)
> return progress;
>  }
>  
> -bool
> +static bool
>  nir_opt_remove_phis_impl(nir_function_impl *impl)
>  {
> bool progress = false;


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] docs: Document and *require* usage of Signed-off-by

[apinheiro]

On 30/11/18 17:32, Ian Romanick wrote:
> On 11/29/2018 03:53 PM, Eric Anholt wrote:
>> e<#secure method=pgpmime mode=sign>
>> Erik Faye-Lund  writes:
>>
>>> On Wed, 2018-11-28 at 13:43 -0800, Eric Anholt wrote:
 Jordan Justen  writes:

> This adds the "Developer's Certificate of Origin 1.1" from the
> Linux
> kernel. It indicates that by using Signed-off-by you are certifying
> that your patch meets the DCO 1.1 guidelines.
>
> It also changes Signed-off-by from being optional to being
> required.
>
> Signed-off-by: Jordan Justen 
> Cc: Matt Turner 
 What problem for our project is solved by signed-off-by?  Do you
 think
 that it has actually reduced the incidence of people submitting code
 they don't have permission to submit in the projects where it's been
 used?  I know the behavior in the kernel is that people submit a
 patch,
 it's missing s-o-b, so it gets bounced, then they maybe add s-o-b or
 just give up.  I don't think anyone stops and says "Wow, that's a
 good
 question.  Maybe I don't have permission to distribute this after
 all?"

 /me sees s-o-b as basically just a linux kernel hazing ritual
>>> I don't think that's the purpose of s-o-b in the Kernel. AFAIK, the
>>> reason for the s-o-b is to have a paper-trail for how a patch landed in
>>> the kernel; who it went through etc.
>> I get the *idea*, I just believe the idea fails in practice.  The S-O-B
>> idea came from "wouldn't it be nice if we could make everyone think
>> about this issue that is important to us" but what we actually got
>> instead is "your patch gets bounced if you don't have a s-o-b on until
>> you slap one on and resend."
> You're thinking like an engineer, but the s-o-b is the spawn of lawyers.
>  Supposedly, having someone say "I had the rights to contribute this
> code," even if they didn't think about what that meant, enables you to
> pass that blame along because that person showed intent.  You may not
> have read and thought about every page of your mortgage, but you can
> still be held accountable for every word.  Having an author tag or a
> committer tag does not show that same intent.  In a legal context, the
> s-o-b shifts the onus of auditing code ownership from the distributor to
> the submitter.  When a distributor, like IBM in the SCO case, get sued,
> they can plausibly claim that all of the code they distributed was
> vouched for.  If someone submits code that they do not have the right to
> submit and provides false testimony, then that's on the submitter.
>
> S-o-b for that purpose always seemed pretty lame to me (from any
> perspective) because patches aren't digitally signed.  Anyone can add
> any s-o-b to any patch, spoof the sending address, etc.
>
> The s-o-b is also mostly an anachronism of taking patches via mailing
> list.  Projects that exclusively use GitHub or similar have everyone
> sign an SLA, and every patch that comes in through the submission
> mechanism, guarded by a login, is implicitly signed.
>
>> We've already got 1-2 people to contact if there's a question about
>> authorship, from the committer and author fields.
> That's one thing that annoys me from time to time about git.  Five
> people may have worked on a patch, but it only shows one author.


FWIW, although I usually don't use the s-o-b, I tend to use it when more
than one person works on a patch. So for some of our work, we keep the
author as the one that initially created the patch, and if we have
several people touching it, we add several s-o-b on the patch. Now
reading the thread, perhaps that was somewhat silly, but looked like the
best/easier solution to give some authorship to everyone.


BR





pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] spirv/nir: adjust location assignment for the case of arrays of blocks

[apinheiro]

On 14/12/18 0:54, Timothy Arceri wrote:
>
>
> On 13/12/18 11:11 pm, Alejandro Piñeiro wrote:
>> This is needed due how the types get rearranged after the struct
>> splitting.
>>
>> So for example, this array of blocks:
>>
>>    layout(location = 0) out block {
>>  vec4 v;
>>  vec3 v2;
>>    } x[2];
>>
>> Would be splitted on two nir variables with the following types:
>>    * vec4 v[2]
>>    * vec3 v2[2]
>>
>> So we need to take into account the length of the array to avoid
>> locations overlaps one with the other.
>> ---
>>
>>
>> Hi Jason,
>>
>> again, sending in advance patches, just in case you are working on the
>> same.
>>
>> I was able to fix the location overlapping without all those crazy
>> ideas about lowering array of blocks into individual blocks, by just
>> adjusting the locations as this patch shows.
>>
>> FWIW, the resulting locations are equivalent to those that we get with
>> GLSL IR, that results on a similar splitting.
>>
>> With this change I got the following working:
>>     * SPIR-V simple arrays of blocks input/outputs
>>
>>     * The arrays of blocks inputs/outputs + interpolator qualifiers
>>   test I mentioned to you last week [1] when run its SPIR-V
>>   equivalent.
>>
>>     * SPIR-V xfb tests using arrays of blocks, where the xfb offset are
>>   assigned to all block members.
>>
>>     * SPIR-V xfb tests using arrays of blocks, where the xfb offset is
>>   assigned to just one member, so just that member is captured,
>>   although as many times as the array length (yes! afaiu by spec
>>   that needs to work)
>>
>> So now, the only pending thing is a cleanup and send the series to
>> review. Specifically, I think that this series can be put on top of
>> current master instead of the arb_gl_spirv. Will try that and send a
>> final series this week or early next week.
>>
>> BR
>>
>>
>> [1]
>> https://github.com/Igalia/piglit/blob/master/tests/spec/glsl-1.50/execution/interface-block-interpolation-array.shader_test
>>
>>
>>
>>   src/compiler/spirv/vtn_variables.c | 14 +-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/compiler/spirv/vtn_variables.c
>> b/src/compiler/spirv/vtn_variables.c
>> index a8f2fdfa534..87386cee42f 100644
>> --- a/src/compiler/spirv/vtn_variables.c
>> +++ b/src/compiler/spirv/vtn_variables.c
>> @@ -1672,6 +1672,14 @@ add_missing_member_locations(struct
>> vtn_variable *var,
>>     glsl_get_length(glsl_without_array(var->type->type));
>>  int location = var->base_location;
>>   +   /* To know if it is a interface block we can't ask directly for
>> +    * var->type->block because on the case of arrays of blocks,
>> block is set
>> +    * on the array_element.
>> +    */
>> +   bool is_array_block = var->var->interface_type != NULL &&
>> +  glsl_type_is_array(var->type->type);
>> +   int adjustment = is_array_block ?
>> glsl_get_length(var->type->type) : 1;
>> +
>
> I think you probably want to use glsl_get_aoa_size() here instead of
> glsl_get_length() ?


Ok, thanks for the suggestion, I will try it, and add some extra tests
to verify it.

Having said so, after cleaning this RFC series, I got some regressions
with tessellation cts vulkan tests with Intel CI, starting from this
commit. So unfourtunately, this is still WIP, and probably the final
patch will be different.

Again, thanks for the hint.


>
>>  for (unsigned i = 0; i < length; i++) {
>>     /* From the Vulkan spec:
>>  *
>> @@ -1702,8 +1710,12 @@ add_missing_member_locations(struct
>> vtn_variable *var,
>>     const struct glsl_type *member_type =
>>    glsl_get_struct_field(glsl_without_array(var->type->type),
>> i);
>>   +  /* For arrays of interface blocks we can't just add the
>> attribute slots
>> +   * of a member type due how the splitting would rearrange the
>> types, so
>> +   * we need to adjust for the array length in that case.
>> +   */
>>     location +=
>> - glsl_count_attribute_slots(member_type, is_vertex_input);
>> + glsl_count_attribute_slots(member_type, is_vertex_input) *
>> adjustment;
>>  }
>>   }
>>  
>


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 25/28] i965/fs: add support for shader float control to remove_extra_rounding_modes()

[apinheiro]

On 6/12/18 8:37, apinheiro wrote:
> On 5/12/18 16:55, Samuel Iglesias Gonsálvez wrote:
>> The remove_extra_rounding_modes() optimization will remove duplicated
>> rounding mode changes.
>>
>> Signed-off-by: Samuel Iglesias Gonsálvez 
>> ---
>>  src/intel/compiler/brw_fs.cpp | 9 +++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
>> index 18dcd92219c..eb253679930 100644
>> --- a/src/intel/compiler/brw_fs.cpp
>> +++ b/src/intel/compiler/brw_fs.cpp
>> @@ -3457,10 +3457,15 @@ bool
>>  fs_visitor::remove_extra_rounding_modes()
>>  {
>> bool progress = false;
>> +   unsigned execution_mode = 
>> this->nir->info.shader_float_controls_execution_mode;
>>  
>> -   foreach_block (block, cfg) {
>> -  brw_rnd_mode prev_mode = BRW_RND_MODE_UNSPECIFIED;
>> +   brw_rnd_mode prev_mode = BRW_RND_MODE_UNSPECIFIED;
>> +   if (execution_mode & SHADER_ROUNDING_MODE_RTE)
>> +  prev_mode = BRW_RND_MODE_RTNE;
>> +   if (execution_mode & SHADER_ROUNDING_MODE_RTZ)
>> +  prev_mode = BRW_RND_MODE_RTZ;
> I see that you move prev_mode reset out of block. This needs to be reset
> for each block, and it is a mistake that I also committed for the v1 of
> this optimization. See original review:
>
> https://lists.freedesktop.org/archives/mesa-dev/2017-September/168970.html
>
> Unless I'm missing something, Jason comments still applies.


Having said so, most of that code is block-independent, so perhaps you
could add a new variable "base_mode" or something, so that code
initializes base_mode, and prev_mode remains at the beginning of the
block, but initialized to base_mode instead to UNSPECIFIED.


>
>
>>  
>> +   foreach_block (block, cfg) {
>>foreach_inst_in_block_safe (fs_inst, inst, block) {
>>   if (inst->opcode == SHADER_OPCODE_RND_MODE) {
>>  assert(inst->src[0].file == BRW_IMMEDIATE_VALUE);


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 25/28] i965/fs: add support for shader float control to remove_extra_rounding_modes()

[apinheiro]

On 5/12/18 16:55, Samuel Iglesias Gonsálvez wrote:
> The remove_extra_rounding_modes() optimization will remove duplicated
> rounding mode changes.
>
> Signed-off-by: Samuel Iglesias Gonsálvez 
> ---
>  src/intel/compiler/brw_fs.cpp | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs.cpp b/src/intel/compiler/brw_fs.cpp
> index 18dcd92219c..eb253679930 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -3457,10 +3457,15 @@ bool
>  fs_visitor::remove_extra_rounding_modes()
>  {
> bool progress = false;
> +   unsigned execution_mode = 
> this->nir->info.shader_float_controls_execution_mode;
>  
> -   foreach_block (block, cfg) {
> -  brw_rnd_mode prev_mode = BRW_RND_MODE_UNSPECIFIED;
> +   brw_rnd_mode prev_mode = BRW_RND_MODE_UNSPECIFIED;
> +   if (execution_mode & SHADER_ROUNDING_MODE_RTE)
> +  prev_mode = BRW_RND_MODE_RTNE;
> +   if (execution_mode & SHADER_ROUNDING_MODE_RTZ)
> +  prev_mode = BRW_RND_MODE_RTZ;

I see that you move prev_mode reset out of block. This needs to be reset
for each block, and it is a mistake that I also committed for the v1 of
this optimization. See original review:

https://lists.freedesktop.org/archives/mesa-dev/2017-September/168970.html

Unless I'm missing something, Jason comments still applies.


>  
> +   foreach_block (block, cfg) {
>foreach_inst_in_block_safe (fs_inst, inst, block) {
>   if (inst->opcode == SHADER_OPCODE_RND_MODE) {
>  assert(inst->src[0].file == BRW_IMMEDIATE_VALUE);


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] Representing explicit memory layouts in NIR

[apinheiro]

On 1/12/18 14:34, Jason Ekstrand wrote:
> On Sat, Dec 1, 2018 at 4:06 AM apinheiro  <mailto:apinhe...@igalia.com>> wrote:
>
> On 30/11/18 23:52, Jason Ekstrand wrote:
>> On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri
>> mailto:tarc...@itsqueeze.com>> wrote:
>>
>> On 1/12/18 9:11 am, Jason Ekstrand wrote:
>> > All,
>> >
>> > This week, I've been working on trying to move UBO and SSBO
>> access in
>> > NIR over to deref instructions.  I'm hoping that this will
>> allow us to
>> > start doing alias analysis and copy-propagation on it.  The
>> passes we
>> > have in NIR *should* be able to work with SSBOs as long as
>> > nir_compare_derefs does the right thing.
>> >
>> > # A story about derefs
>> >
>> > In that effort, I've run into a bit of a snag with how to
>> represent the
>> > layout information.  What we get in from SPIR-V for Vulkan
>> is a byte
>> > offset for every struct member and a byte stride for every
>> array (and
>> > pointer in the OpPtrAccessChain case).  For matrices, there
>> is an
>> > additional RowMajor boolean we need to track somewhere. 
>> With OpenCL
>> > memory access, you don't get these decorations but it's
>> trivial to
>> > translate the OpenCL layout (It's the same as C) to
>> offset/stride when
>> > creating the type.  I've come up with three different ways
>> to represent
>> > the information and they all have their own downsides:
>> >
>> > ## 1. Put the information on the glsl_type similar to how
>> it's done in
>> > SPIR-V
>> >
>> > This has the advantage of being fairly non-invasive to
>> glsl_type.  A lot
>> > of the fields we need are already there and the only real
>> change is to
>> > allow array types to have strides.  The downside is that
>> the information
>> > is often not where you want.  Arrays and structs are ok
>> but, for
>> > matrices, you have to go fishing all the way back to the
>> struct type to
>> > get the RowMajor and MatrixStride decorations.  (Thanks,
>> SPIR-V...) 
>> > While this seems like a local annoyance, it actually
>> destroys basically
>> > all the advantages of having the information on the type
>> and makes
>> > lower_io a real pain.
>> >
>> > ## 2. Put the information on the type but do it properly
>> >
>> > In this version, we would put the matrix stride and
>> RowMajor decoration
>> > directly on the matrix type.  One obvious advantage here is
>> that it
>> > means no fishing for matrix type information.  Another is
>> that, by
>> > having the types specialized like this, the only way to
>> change layouts
>> > mid-deref-chain would be to have a cast.  Option 1 doesn't
>> provide this
>> > because matrix types are the same regardless of whether or
>> not they're
>> > declared RowMajor in the struct.  The downside to this
>> option is that it
>> > requires glsl_type surgery to make it work.  More on that
>> in a bit.
>>
>
> FWIW, while working on ARB_gl_spirv I also found annoying the need
> to fish for the struct member information in order to get RowMajor
> and MatrixStride information, as that lead to the need to track
> the parent_type while you were processing the current type. As a
> example, this is a extract of the code from _get_type_size (new
> method from ARB_gl_spirv ubo/ssbo support, used to compute the
> ubo/ssbo size):
>
>    /* Matrices must have a matrix stride and either RowMajor or
> ColMajor */
>    if (glsl_type_is_matrix(type)) {
>   unsigned matrix_stride =
>  glsl_get_struct_field_explicit_matrix_stride(parent_type,
> index_in_parent);
>
>   bool row_major =
>  glsl_get_struct_field_matrix_layout(parent_type,
> index_in_parent) ==
>  GLSL_MATRIX_LAYOUT_ROW_MAJOR;
>
> 

Re: [Mesa-dev] Representing explicit memory layouts in NIR

[apinheiro]

On 30/11/18 23:52, Jason Ekstrand wrote:
> On Fri, Nov 30, 2018 at 4:39 PM Timothy Arceri  > wrote:
>
> On 1/12/18 9:11 am, Jason Ekstrand wrote:
> > All,
> >
> > This week, I've been working on trying to move UBO and SSBO
> access in
> > NIR over to deref instructions.  I'm hoping that this will allow
> us to
> > start doing alias analysis and copy-propagation on it.  The
> passes we
> > have in NIR *should* be able to work with SSBOs as long as
> > nir_compare_derefs does the right thing.
> >
> > # A story about derefs
> >
> > In that effort, I've run into a bit of a snag with how to
> represent the
> > layout information.  What we get in from SPIR-V for Vulkan is a
> byte
> > offset for every struct member and a byte stride for every array
> (and
> > pointer in the OpPtrAccessChain case).  For matrices, there is an
> > additional RowMajor boolean we need to track somewhere.  With
> OpenCL
> > memory access, you don't get these decorations but it's trivial to
> > translate the OpenCL layout (It's the same as C) to
> offset/stride when
> > creating the type.  I've come up with three different ways to
> represent
> > the information and they all have their own downsides:
> >
> > ## 1. Put the information on the glsl_type similar to how it's
> done in
> > SPIR-V
> >
> > This has the advantage of being fairly non-invasive to
> glsl_type.  A lot
> > of the fields we need are already there and the only real change
> is to
> > allow array types to have strides.  The downside is that the
> information
> > is often not where you want.  Arrays and structs are ok but, for
> > matrices, you have to go fishing all the way back to the struct
> type to
> > get the RowMajor and MatrixStride decorations.  (Thanks,
> SPIR-V...) 
> > While this seems like a local annoyance, it actually destroys
> basically
> > all the advantages of having the information on the type and makes
> > lower_io a real pain.
> >
> > ## 2. Put the information on the type but do it properly
> >
> > In this version, we would put the matrix stride and RowMajor
> decoration
> > directly on the matrix type.  One obvious advantage here is that it
> > means no fishing for matrix type information.  Another is that, by
> > having the types specialized like this, the only way to change
> layouts
> > mid-deref-chain would be to have a cast.  Option 1 doesn't
> provide this
> > because matrix types are the same regardless of whether or not
> they're
> > declared RowMajor in the struct.  The downside to this option is
> that it
> > requires glsl_type surgery to make it work.  More on that in a bit.
>

FWIW, while working on ARB_gl_spirv I also found annoying the need to
fish for the struct member information in order to get RowMajor and
MatrixStride information, as that lead to the need to track the
parent_type while you were processing the current type. As a example,
this is a extract of the code from _get_type_size (new method from
ARB_gl_spirv ubo/ssbo support, used to compute the ubo/ssbo size):

   /* Matrices must have a matrix stride and either RowMajor or ColMajor */
   if (glsl_type_is_matrix(type)) {
  unsigned matrix_stride =
 glsl_get_struct_field_explicit_matrix_stride(parent_type,
index_in_parent);

  bool row_major =
 glsl_get_struct_field_matrix_layout(parent_type,
index_in_parent) ==
 GLSL_MATRIX_LAYOUT_ROW_MAJOR;

  unsigned length = row_major ? glsl_get_vector_elements(type)
 : glsl_get_length(type);

  /* We don't really need to compute the type_size of the matrix element
   * type. That should be already included as part of matrix_stride
   */
  return matrix_stride * length;
   }


So for a given type, sometimes we are using the info from the current
type, and sometimes we need to go up to the parent (so in addition to
the parent_type we also need to track the index of the current type on
its parent).

At some point I was tempted to move matrix stride and row major from the
struct field to glsl type. But as Jason mentioned, that would need a lot
of changes, and I felt that doing that only for ARB_gl_spirv convenience
was an overkill. But if we have now more reasons to do the move, I'm on
that ship.


> >
> > ## 3. Put the information directly on the deref
> >
> > Instead of putting the stride/offset information on the type, we
> just
> > put it on the deref as we build the deref chain.  This is easy
> enough to
> > do in spirv_to_nir and someone could also do it easily enough as a
> > lowering pass based on a type_size function.  This has the
> advantage of
> > simplicity because you don't have to modify glsl_type at all and
> > lowering is 

Re: [Mesa-dev] [PATCH] intel/fs,vec4: Fix a compiler warning

[apinheiro]

Reviewed-by: Alejandro Piñeiro 

On 16/11/18 16:25, Jason Ekstrand wrote:
> ../src/intel/compiler/brw_fs_nir.cpp:3534:46: warning: comparison of integer 
> expressions of different signedness: ‘unsigned int’ and ‘int’ [-Wsign-compare]
>assert(nir_intrinsic_write_mask(instr) ==
>   ^~
>   (1 << instr->num_components) - 1);
>   
>
> This was caused by 6339aba775ecdc which added these completely valid
> checks.  However clang likes to complain about signedness mismatches.
>
> Fixes: 6339aba775ecdc "intel/compiler: Lower SSBO and shared..."
> ---
>  src/intel/compiler/brw_fs_nir.cpp   | 4 ++--
>  src/intel/compiler/brw_vec4_nir.cpp | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp 
> b/src/intel/compiler/brw_fs_nir.cpp
> index 84d0c6be6c3..6eb68794f58 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -3514,7 +3514,7 @@ fs_visitor::nir_emit_cs_intrinsic(const fs_builder ,
>val_reg.type = brw_reg_type_from_bit_size(bit_size, 
> BRW_REGISTER_TYPE_UD);
>  
>assert(nir_intrinsic_write_mask(instr) ==
> - (1 << instr->num_components) - 1);
> + (1u << instr->num_components) - 1);
>if (nir_intrinsic_align(instr) >= 4) {
>   assert(nir_src_bit_size(instr->src[0]) == 32);
>   assert(nir_src_num_components(instr->src[0]) <= 4);
> @@ -4070,7 +4070,7 @@ fs_visitor::nir_emit_intrinsic(const fs_builder , 
> nir_intrinsic_instr *instr
>val_reg.type = brw_reg_type_from_bit_size(bit_size, 
> BRW_REGISTER_TYPE_UD);
>  
>assert(nir_intrinsic_write_mask(instr) ==
> - (1 << instr->num_components) - 1);
> + (1u << instr->num_components) - 1);
>if (nir_intrinsic_align(instr) >= 4) {
>   assert(nir_src_bit_size(instr->src[0]) == 32);
>   assert(nir_src_num_components(instr->src[0]) <= 4);
> diff --git a/src/intel/compiler/brw_vec4_nir.cpp 
> b/src/intel/compiler/brw_vec4_nir.cpp
> index 26ca2ddd8dc..4bb4d0d4074 100644
> --- a/src/intel/compiler/brw_vec4_nir.cpp
> +++ b/src/intel/compiler/brw_vec4_nir.cpp
> @@ -503,7 +503,7 @@ vec4_visitor::nir_emit_intrinsic(nir_intrinsic_instr 
> *instr)
>/* brw_nir_lower_mem_access_bit_sizes takes care of this */
>assert(nir_src_bit_size(instr->src[0]) == 32);
>assert(nir_intrinsic_write_mask(instr) ==
> - (1 << instr->num_components) - 1);
> + (1u << instr->num_components) - 1);
>  
>src_reg surf_index = get_nir_ssbo_intrinsic_index(instr);
>src_reg offset_reg = retype(get_nir_src_imm(instr->src[2]),


pEpkey.asc
Description: application/pgp-keys
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev