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

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

Hi,

On 22-02-16 16:24, Ilia Mirkin wrote:
> 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?
Correct, the best way to view them is as them being in their own
address-space, which is why I suggested using MEMORY[#], INPUT.
> 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.
More or less, the address-space is typically addressed with byte
addresses, so the second 32 bit value is addresses with address 4,
etc.
> 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.]
I'm fine with using constant buffers for the input, it is not the
mechanism I'm worried about it is the tgsi syntax to express things,
I think it would be beneficial for the tgsi syntax to be abstract, and
not worry about the underlying mechanism, this will i.e. allow us
to use shared memory for input on tesla and const bufs on later generations
without the part generating the tgsi code needing to worry about this.

###

Somewhat unrelated to the input problem, I'm also somewhat worried
about the addressing method for MEMORY type registers.

Looking at the old RES stuff then the "index" passed into say a LOAD
was not as much an index as it was simply a 32 bit GPU virtual memory
address, which fits well with the OpenCL ways of doing things (the
register number as in the 55 in RES[55] was more or less ignored).

Where as, e.g. the new BUFFER style "registers" the index really
is an index, e.g. doing:
LOAD TEMP[0].x, BUFFER[0], IMM[0]
resp.
LOAD TEMP[0].x, BUFFER[1], IMM[0]

Will read from a different memory address, correct ?

So how will this work for MEMORY type registers ? For OpenCL having the
1-dimensional behavior of RES really is quite useful, and having the
address be composed of a hidden base address which gets determined under
the hood from the register number, and then adding an index on top of
it does not fit so well.

As said this could be fixed by making the state-tracker add the size
of all buffers of a certain type together and then do one large
request for them, but that seems sub-optimal.

Regards,

Hans

On Mon, Feb 22, 2016 at 10:50 AM, Hans de Goede <hdegoede at redhat.com>
wrote:
>> 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.]
>
>
> I'm fine with using constant buffers for the input, it is not the
> mechanism I'm worried about it is the tgsi syntax to express things,
> I think it would be beneficial for the tgsi syntax to be abstract, and
> not worry about the underlying mechanism, this will i.e. allow us
> to use shared memory for input on tesla and const bufs on later generations
> without the part generating the tgsi code needing to worry about this.
Yeah, I think you're right. I didn't realize that tesla had a special
form of input for user params, I assumed it was just the usual thing.
So forget about constbufs, go with the INPUT thing. Which is great,
since we had one value left over in that (future) 2-bit field :)
>
> ###
>
> Somewhat unrelated to the input problem, I'm also somewhat worried
> about the addressing method for MEMORY type registers.
>
> Looking at the old RES stuff then the "index" passed into say a
LOAD
> was not as much an index as it was simply a 32 bit GPU virtual memory
> address, which fits well with the OpenCL ways of doing things (the
> register number as in the 55 in RES[55] was more or less ignored).
>
> Where as, e.g. the new BUFFER style "registers" the index really
> is an index, e.g. doing:
> LOAD TEMP[0].x, BUFFER[0], IMM[0]
> resp.
> LOAD TEMP[0].x, BUFFER[1], IMM[0]
>
> Will read from a different memory address, correct ?
Correct -- BUFFER[0] refers to the buffer at binding point 0, and
BUFFER[1] refers to the buffer at binding point 1. They might, in
fact, overlap, or even be the same buffer. But the code doesn't know
about that.
>
> So how will this work for MEMORY type registers ? For OpenCL having the
> 1-dimensional behavior of RES really is quite useful, and having the
> address be composed of a hidden base address which gets determined under
> the hood from the register number, and then adding an index on top of
> it does not fit so well.
Not sure what the question is... you have code like

int *foo = [pointer value from user input];
*foo = *(foo + 5);

right?

So that'd just become

MOV TEMP[0].x, <val from user input, whereever it is>
ADD TEMP[0].y, TEMP[0].x, 5 * 4
LOAD TEMP[1].x, MEMORY[0] (which is global), TEMP[0].y
STORE MEMORY[0], TEMP[0].x, TEMP[1].x

or perhaps I'm misunderstanding something?

MEMORY, GLOBAL == the global virtual memory address space, not some
specific buffer. Trying to load address 0 from it will likely lead to
sadness, unless you happen to have something mapped there. BUFFER has
an implied base address, based on the binding point, but MEMORY has no
such thing.

  -ilia