Nouveau - Feb 2016 - Dealing with opencl kernel parameters in nouveau now that RES support is gone

On Mon, Feb 22, 2016 at 9:17 AM, Hans de Goede <hdegoede at redhat.com>
wrote:
> Hi,
>
> On 22-02-16 14:47, Ilia Mirkin wrote:
>>
>> On Mon, Feb 22, 2016 at 8:45 AM, Ilia Mirkin <imirkin at
alum.mit.edu> wrote:
>>>
>>> INPUT is for shader inputs which come from fixed function loaders.
>>> This is not what you want. You want CONST. Stick the input params
into
>>> a constbuf, and you're done.
>>
>>
>> Oh, and in case it's not clear, I think this should be done by the
st,
>> not by the driver. Not a big fan of the current interface where the
>> driver is responsible for uploading the kernel input parameters.
>
>
> Moving this to the state-tracker will likely break clover for amd
> cards, also what is the right place to stick the input params my
Not really. Could just have a PIPE_CAP. Could make it part of the
whole TGSI logic in the first place. This functionality isn't used for
the OpenGL compute shader, and it'd be nice to bring OpenCL in line
with the OpenGL stuff.
> differ from one gpu to the other, also the opencl -> tgsi compiler
> will need to know how to "address" input params without it
needing to
> know too much details of the targetted gpu. So of INPUT is not suitable,
> then I think we are going to need MEMORY[#], INPUT for this, which
> nouveau can then just treat as CONST.
That doesn't make sense... MEMORY is for... memory. Perhaps there's
something I don't understand about kernel inputs that would make my
suggestion unworkable? The way I see it is that you stick them into a
user constbuf (i.e. pipe->set_constant_buffer({cb.user = pointer to
your thing})), and then launch the grid. Your TGSI would have been
composed such that it would expect the user params to show up in the
first constbuf.

You *really* want to be using constbufs here -- they're designed for this.

  -ilia

Hi,

On 22-02-16 15:22, Ilia Mirkin wrote:
> On Mon, Feb 22, 2016 at 9:17 AM, Hans de Goede <hdegoede at
redhat.com> wrote:
>> Hi,
>>
>> On 22-02-16 14:47, Ilia Mirkin wrote:
>>>
>>> On Mon, Feb 22, 2016 at 8:45 AM, Ilia Mirkin <imirkin at
alum.mit.edu> wrote:
>>>>
>>>> INPUT is for shader inputs which come from fixed function
loaders.
>>>> This is not what you want. You want CONST. Stick the input
params into
>>>> a constbuf, and you're done.
>>>
>>>
>>> Oh, and in case it's not clear, I think this should be done by
the st,
>>> not by the driver. Not a big fan of the current interface where the
>>> driver is responsible for uploading the kernel input parameters.
>>
>>
>> Moving this to the state-tracker will likely break clover for amd
>> cards, also what is the right place to stick the input params my
>
> Not really. Could just have a PIPE_CAP. Could make it part of the
> whole TGSI logic in the first place. This functionality isn't used for
> the OpenGL compute shader, and it'd be nice to bring OpenCL in line
> with the OpenGL stuff.
>
>> differ from one gpu to the other, also the opencl -> tgsi compiler
>> will need to know how to "address" input params without it
needing to
>> know too much details of the targetted gpu. So of INPUT is not
suitable,
>> then I think we are going to need MEMORY[#], INPUT for this, which
>> nouveau can then just treat as CONST.
>
> That doesn't make sense... MEMORY is for... memory. Perhaps there's
> something I don't understand about kernel inputs that would make my
> suggestion unworkable? The way I see it is that you stick them into a
> user constbuf (i.e. pipe->set_constant_buffer({cb.user = pointer to
> your thing})), and then launch the grid. Your TGSI would have been
> composed such that it would expect the user params to show up in the
> first constbuf.
Keep in mind the flat address space issue opencl has, there is no
such thing as the first constbuf, there is a const address space,
which is shared by all the const buffers, and the kernel gets
passed in offsets in the single address space to find different
const buffers. I'm not saying that this cannot work with your suggestion,
but it is something to keep in mind.

I've not yet looked closely at const bufs, I've only been working with
global buffers so far, here is how I understand those to work:

-clover calls pipe->set_global_binding() before calling launch_grid()
-clover has set up the uint32_t **handles pointer array to point to
  one uint32_t in the buffer which it is going to pass as "input" to
  launch_grid for each global buffer
-In the tgsi code for the kernel I can get to the global buffer pointed
  to by the first uint32_t in the input data by doing:
  LOAD TEMP[#].x, RES[#], IMM[0] # IMM[0] == 0

And then using TEMP[#].x as offset into RES[#]

If I understand your proposal correctly then you're suggestion that
getting the offset would become:

  LOAD TEMP[#].x, CONST[0], IMM[0] # IMM[0] == 0

That would work fine for me, the question then becomes what do we do
with const input parameters ? use CONST[1] for those ?

It is also not entirely clear to me yet how things are going to
work with regards to the handles returned by pipe->set_global_binding()
with RES these are offsets which can be used directly to address
the "RES" address-space. I assume that the intent with
the MEMORY register file is that each global buffer gets its own
MEMORY[#] and that to address say the first byte of the second buffer
allocated one would use:

LOAD TEMP[#].x, MEMORY[1], IMM[0] # IMM[0] == 0

But I cannot do that in OpenCL generated TGSI, as at the
tgsi llvm backend level I've no idea about which global buffer it is,
I only get a base-offset for the global buffer and all global buffers
are assumed to be in one memory space, e.g. I will always be using
MEMORY[0] for all the global buffers.

Does this mean that the state-tracker now needs to figure out the size
of all global buffers combined, and then request a single global memory
pool and set the offsets which it passes as input to the kernel itself,
dividing that memory pool into multiple buffers itself ?

Or will there still be a way to pass "flat" addresses in tgsi ?

Regards,

Hans

Hello,
> On 22 Feb 2016, at 15:22, Ilia Mirkin <imirkin at alum.mit.edu>
wrote:
> 
>> On Mon, Feb 22, 2016 at 9:17 AM, Hans de Goede <hdegoede at
redhat.com> wrote:
>> Hi,
>> 
>>> On 22-02-16 14:47, Ilia Mirkin wrote:
>>> 
>>>> On Mon, Feb 22, 2016 at 8:45 AM, Ilia Mirkin <imirkin at
alum.mit.edu> wrote:
>>>> 
>>>> INPUT is for shader inputs which come from fixed function
loaders.
>>>> This is not what you want. You want CONST. Stick the input
params into
>>>> a constbuf, and you're done.
>>> 
>>> 
>>> Oh, and in case it's not clear, I think this should be done by
the st,
>>> not by the driver. Not a big fan of the current interface where the
>>> driver is responsible for uploading the kernel input parameters.
>> 
>> 
>> Moving this to the state-tracker will likely break clover for amd
>> cards, also what is the right place to stick the input params my
> 
> Not really. Could just have a PIPE_CAP. Could make it part of the
> whole TGSI logic in the first place. This functionality isn't used for
> the OpenGL compute shader, and it'd be nice to bring OpenCL in line
> with the OpenGL stuff.
> 
>> differ from one gpu to the other, also the opencl -> tgsi compiler
>> will need to know how to "address" input params without it
needing to
>> know too much details of the targetted gpu. So of INPUT is not
suitable,
>> then I think we are going to need MEMORY[#], INPUT for this, which
>> nouveau can then just treat as CONST.
> 
> That doesn't make sense... MEMORY is for... memory. Perhaps there's
> something I don't understand about kernel inputs that would make my
> suggestion unworkable? The way I see it is that you stick them into a
> user constbuf (i.e. pipe->set_constant_buffer({cb.user = pointer to
> your thing})), and then launch the grid. Your TGSI would have been
> composed such that it would expect the user params to show up in the
> first constbuf.
Apart from Tesla which uses shared memory for passing the OpenCL user params,
Fermi+ use const memory for that, and this is what Samuel's patches are
doing (at least from what I remember).

Regards,
Pierre
> 
> You *really* want to be using constbufs here -- they're designed for
this.
> 
>  -ilia
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau

On Mon, Feb 22, 2016 at 9:50 AM, Hans de Goede <hdegoede at redhat.com>
wrote:
> Hi,
>
>
> On 22-02-16 15:22, Ilia Mirkin wrote:
>>
>> On Mon, Feb 22, 2016 at 9:17 AM, Hans de Goede <hdegoede at
redhat.com>
>> wrote:
>>>
>>> Hi,
>>>
>>> On 22-02-16 14:47, Ilia Mirkin wrote:
>>>>
>>>>
>>>> On Mon, Feb 22, 2016 at 8:45 AM, Ilia Mirkin <imirkin at
alum.mit.edu>
>>>> wrote:
>>>>>
>>>>>
>>>>> INPUT is for shader inputs which come from fixed function
loaders.
>>>>> This is not what you want. You want CONST. Stick the input
params into
>>>>> a constbuf, and you're done.
>>>>
>>>>
>>>>
>>>> Oh, and in case it's not clear, I think this should be done
by the st,
>>>> not by the driver. Not a big fan of the current interface where
the
>>>> driver is responsible for uploading the kernel input
parameters.
>>>
>>>
>>>
>>> Moving this to the state-tracker will likely break clover for amd
>>> cards, also what is the right place to stick the input params my
>>
>>
>> Not really. Could just have a PIPE_CAP. Could make it part of the
>> whole TGSI logic in the first place. This functionality isn't used
for
>> the OpenGL compute shader, and it'd be nice to bring OpenCL in line
>> with the OpenGL stuff.
>>
>>> differ from one gpu to the other, also the opencl -> tgsi
compiler
>>> will need to know how to "address" input params without
it needing to
>>> know too much details of the targetted gpu. So of INPUT is not
suitable,
>>> then I think we are going to need MEMORY[#], INPUT for this, which
>>> nouveau can then just treat as CONST.
>>
>>
>> That doesn't make sense... MEMORY is for... memory. Perhaps
there's
>> something I don't understand about kernel inputs that would make my
>> suggestion unworkable? The way I see it is that you stick them into a
>> user constbuf (i.e. pipe->set_constant_buffer({cb.user = pointer to
>> your thing})), and then launch the grid. Your TGSI would have been
>> composed such that it would expect the user params to show up in the
>> first constbuf.
>
>
> Keep in mind the flat address space issue opencl has, there is no
> such thing as the first constbuf, there is a const address space,
> which is shared by all the const buffers, and the kernel gets
> passed in offsets in the single address space to find different
> const buffers. I'm not saying that this cannot work with your
suggestion,
> but it is something to keep in mind.
>
> I've not yet looked closely at const bufs, I've only been working
with
> global buffers so far, here is how I understand those to work:
>
> -clover calls pipe->set_global_binding() before calling launch_grid()
> -clover has set up the uint32_t **handles pointer array to point to
>  one uint32_t in the buffer which it is going to pass as "input"
to
>  launch_grid for each global buffer
> -In the tgsi code for the kernel I can get to the global buffer pointed
>  to by the first uint32_t in the input data by doing:
>  LOAD TEMP[#].x, RES[#], IMM[0] # IMM[0] == 0
But inputs are somehow special in OpenCL, right? In other words, you
know when you want to load an input vs when you want to load
not-an-input, right? And those inputs aren't in the flat, global
memory space, they're just a list of 32-bit values (or at least
expressible as such). If not, then ignore everything I've said and
I'll come up with another alternative.

But assuming I'm right, what I'm proposing is that instead of passing
the input in as a global buffer, to instead pass it in as a const
buffer. As such instead of sticking it into ->set_global_binding,
you'd stick it into ->set_constant_buffer, and then you'll be able to
refer to it as CONST[0], CONST[1], etc. (Which are, implicitly,
CONST[0][0], CONST[0][1], etc -- it doesn't print the second dim when
it's 0.) You don't even have to load these, you can use them as args
directly anywhere you like (except as indirect addresses).

The old code would actually take the supplied inputs, stick them into
a constbuf, and then lower RINPUT accesses to load from that constbuf.
I'm suggesting we cut out the middleman.

By the way, another term for "constant buffer" is "uniform
buffer", on
the off chance it helps. Basically it's super-cached by the shader for
values that never change across shader invocations. [And there's
special stuff in the hw to allow running multiple sets of shader
invocations with different "constant" values... or so we think.]

  -ilia

On Mon, Feb 22, 2016 at 10:15 AM, Pierre Moreau <pierre.morrow at free.fr>
wrote:
> Apart from Tesla which uses shared memory for passing the OpenCL user
params, Fermi+ use const memory for that, and this is what Samuel's patches
are doing (at least from what I remember).
Hmmm.... that puts a bit of a hole in my theory of "let's just use
constbufs". That said, constbufs *will* work on Tesla as well.

Well, in that case, perhaps a separate input memory *is* the right
thing to do? Ugh :( This definitely needs to be discussed with the
wider gallium/tgsi community.

  -ilia