[Mesa-dev] A question about nir_lower_wpos_ytransform

[Alejandro Piñeiro]

On 07/05/18 21:21, Rob Clark wrote:
> On Mon, May 7, 2018 at 2:36 PM, Jason Ekstrand <jason at jlekstrand.net> wrote:
>> On Mon, May 7, 2018 at 11:02 AM, Rob Clark <robdclark at gmail.com> wrote:
>>> On Mon, May 7, 2018 at 11:53 AM, Jason Ekstrand <jason at jlekstrand.net>
>>> wrote:
>>>> On Mon, May 7, 2018 at 8:02 AM, Alejandro Piñeiro <apinheiro at igalia.com>
>>>> wrote:
>>>>> Hi Jason,
>>>>>
>>>>> as part of the ARB_gl_spirv work, we are doing the linking based on the
>>>>> nir shader that comes from spirv_to_nir. On some cases,
>>>>> nir_lower_wpos_ytransform introduces a new uniform,
>>>>> "gl_FbWposYTransform". So as the uniform is there when we start to do
>>>>> the linking, it is treated as any other uniform, being added to the
>>>>> UniformStorage, accessible through the OpenGL API and so on. Something
>>>>> that I understand that we don't want. On GLSL this is not a problem
>>>>> because the linking is done based on MESA/IR, then the IR is converted
>>>>> to NIR, and the lowering adds the uniform based on the NIR shader,
>>>>> really after the linking.
>>>>>
>>>>> The first solution I thought was just mark the uniform with a special
>>>>> location [1]. It would need to be a negative value, not -1 that is
>>>>> already reserved for not-assigned locations (we still have some cases
>>>>> for those, even if in general ARB_gl_spirv makes explicit location
>>>>> mandatory). So it would be a new negative value, like lets say -5. So
>>>>> then on the linking, if the uniform location has this value, the
>>>>> uniform
>>>>> is skipped.
>>>>>
>>>>> After checking that this solution worked (although I only tested with
>>>>> some tests), I started to search for an alternative, as this solution
>>>>> seemed too hacky.
>>>>
>>>> It does seem a bit hacky, yes.
>>>>
>>>>> But this lowering needs to be run before nir_lower_io,
>>>>> and nir_lower_io is part of the uniform lowering. So I didn't see
>>>>> really
>>>>> reasonable to do a nir_lower_io without nir_lower_wpos_ytransform, do
>>>>> the linking, and then execute again
>>>>> nir_lower_io/nir_lower_wpos_transform.
>>>>
>>>> This is all a bit sticky.  One observation is that the newly added
>>>> uniform
>>>> makes usage of the token system.  Are tokens no longer used with SPIR-V?
>>>> I
>>>> guess they are kind-of an old mechanism to handle the crufty ARB program
>>>> parameters.  However, you could have -5 mean "It uses a token"
>>>>
>>>> Another option would be to use a system value instead of a uniform.  We
>>>> can
>>>> then convert it to a uniform in the back-end.  We have a few places
>>>> where we
>>>> do things like this for compute shaders.
>>>>
>>> fwiw, I originally used the token mechanism because mesa/st was
>>> already injecting the value into the uniform state (and I'm not sure
>>> the pipe driver is even aware where it is offhand)
>>>
>>> I suppose an easy thing to do for now is make this configurable to
>>> either use gl_FbWposYTransform or a system value.  This way the
>>> gallium drivers could continue to use gl_FbWposYTransform.
>>
>> What if those gallium drivers want SPIR-V?  :-P
> maybe tarceri has given that some thought?  It isn't really something
> I've thought about at all yet.

Timothy mentioned that eventually Gallium would support ARB_gl_spirv
too, although it is not a high priority at this moment. He is reviewing
a lot of the patches we are sending though (really appreciated!)

Also, Nicolai mentioned their interest of supporting this extension on
radeonsi during this year FOSDEM [1], and listed it as one of the
reasons to add NIR support on Gallium. In fact, we didn't start our work
from scratch, but from Nicolai's initial tackle to the extension (really
appreciated too!)

[1] https://fosdem.org/2018/schedule/event/radeonsi/

>
> Probably the right thing to do is expose the value to the pipe_driver
> and migrate over to using a sysval.. which I guess wouldn't be *that*
> hard.
>
> Short term, probably just supporting either sysval or current
> mechanism in the lowering pass is the easy way.  And long term move
> everything to sysval.  That would at least unblock i965.
>
> BR,
> -R
>
>> I agree that using the slot mechanism is really nice because it hides it
>> from the driver.  It's less practical with Vulkan but there we can usually
>> compile the value in directly so it's not a big deal.
>>
>>> BR,
>>> -R
>>>
>>>> I don't have a lot of good ideas or strong opinions, I'm afraid. :-(
>>>> Maybe
>>>> Ken has some?
>>>>
>>>> --Jason
>>>>
>>>> _______________________________________________
>>>> mesa-dev mailing list
>>>> mesa-dev at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>>>
>>