Nouveau - Mar 2016 - [PATCH mesa 3/3] nouveau: Add support for clover / OpenCL kernel input parameters

On Thu, Mar 10, 2016 at 10:27 AM, Samuel Pitoiset
<samuel.pitoiset at gmail.com> wrote:
>
>
> On 03/10/2016 04:23 PM, Ilia Mirkin wrote:
>>
>> On Thu, Mar 10, 2016 at 10:14 AM, Hans de Goede <hdegoede at
redhat.com>
>> wrote:
>>>
>>> Add support for clover / OpenCL kernel input parameters.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>   .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp      | 18
>>> +++++++++++++++---
>>>   1 file changed, 15 insertions(+), 3 deletions(-)
>>>
>>> diff --git
a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> index a8258af..de0c72b 100644
>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> @@ -1523,9 +1523,21 @@ Converter::makeSym(uint tgsiFile, int
fileIdx, int
>>> idx, int c, uint32_t address)
>>>
>>>      sym->reg.fileIndex = fileIdx;
>>>
>>> -   if (tgsiFile == TGSI_FILE_MEMORY &&
>>> -       code->memoryFiles[fileIdx].mem_type ==
TGSI_MEMORY_TYPE_SHARED)
>>> -      sym->setFile(FILE_MEMORY_SHARED);
>>> +   if (tgsiFile == TGSI_FILE_MEMORY) {
>>> +      switch (code->memoryFiles[fileIdx].mem_type) {
>>> +      case TGSI_MEMORY_TYPE_SHARED:
>>> +         sym->setFile(FILE_MEMORY_SHARED);
>>> +         break;
>>> +      case TGSI_MEMORY_TYPE_INPUT:
>>> +         assert(prog->getType() == Program::TYPE_COMPUTE);
>>> +         assert(idx == -1);
>>> +         sym->setFile(FILE_SHADER_INPUT);
>>> +         address += info->prop.cp.inputOffset;
>>
>>
>> What's the idea here? i.e. what is the inputOffset, how is it set,
and
>> why?
>
>
> I don't get the idea too, btw.
>
> But prop.cp.inputOffset is only defined for compute on Kepler. It's the
> offset of input parameters in the screen->parm BO but as you already
know,
> it is going to be removed because the idea is to use screen->uniform_bo
> instead. I'll do this change *after* the compute shaders support on
Kepler.
Actually looks like it's only set for nv50 that I can see, shifting
things over by 0x10. It used to be reflected by getResourceBase, but
we broke that abstraction... might be nice to get it back somehow,
perhaps by sending more arguments down to getResourceBase? Either way,
that can be done later. This patch is

Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>

On 03/10/2016 04:43 PM, Ilia Mirkin wrote:
> On Thu, Mar 10, 2016 at 10:27 AM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>>
>>
>> On 03/10/2016 04:23 PM, Ilia Mirkin wrote:
>>>
>>> On Thu, Mar 10, 2016 at 10:14 AM, Hans de Goede <hdegoede at
redhat.com>
>>> wrote:
>>>>
>>>> Add support for clover / OpenCL kernel input parameters.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>>    .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp      | 18
>>>> +++++++++++++++---
>>>>    1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git
a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> index a8258af..de0c72b 100644
>>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> @@ -1523,9 +1523,21 @@ Converter::makeSym(uint tgsiFile, int
fileIdx, int
>>>> idx, int c, uint32_t address)
>>>>
>>>>       sym->reg.fileIndex = fileIdx;
>>>>
>>>> -   if (tgsiFile == TGSI_FILE_MEMORY &&
>>>> -       code->memoryFiles[fileIdx].mem_type ==
TGSI_MEMORY_TYPE_SHARED)
>>>> -      sym->setFile(FILE_MEMORY_SHARED);
>>>> +   if (tgsiFile == TGSI_FILE_MEMORY) {
>>>> +      switch (code->memoryFiles[fileIdx].mem_type) {
>>>> +      case TGSI_MEMORY_TYPE_SHARED:
>>>> +         sym->setFile(FILE_MEMORY_SHARED);
>>>> +         break;
>>>> +      case TGSI_MEMORY_TYPE_INPUT:
>>>> +         assert(prog->getType() == Program::TYPE_COMPUTE);
>>>> +         assert(idx == -1);
>>>> +         sym->setFile(FILE_SHADER_INPUT);
>>>> +         address += info->prop.cp.inputOffset;
>>>
>>>
>>> What's the idea here? i.e. what is the inputOffset, how is it
set, and
>>> why?
>>
>>
>> I don't get the idea too, btw.
>>
>> But prop.cp.inputOffset is only defined for compute on Kepler. It's
the
>> offset of input parameters in the screen->parm BO but as you already
know,
>> it is going to be removed because the idea is to use
screen->uniform_bo
>> instead. I'll do this change *after* the compute shaders support on
Kepler.
>
> Actually looks like it's only set for nv50 that I can see, shifting
> things over by 0x10. It used to be reflected by getResourceBase, but
> we broke that abstraction... might be nice to get it back somehow,
> perhaps by sending more arguments down to getResourceBase? Either way,
> that can be done later. This patch is
Oh yes, I was confused with prop.cp.gridInfoBase on Kepler...
>
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
-- 
-Samuel

Looks fine, except that you will need to lower FILE_SHADER_INPUT to 
FILE_MEMORY_SHARED for Tesla because input kernel parameters are located 
at s[0x10]. No need to do this for Fermi+ because it's already lowered 
to c0[]. Note that input kernel parameters will be probably sticked on 
c7[] after my changes but that doesn't change anything for you.

I already have a patch for the nv50 bits btw, maybe it's the right time 
to send it?

https://cgit.freedesktop.org/~hakzsam/mesa/commit/?h=compute&id=640d68009bcf93c1814cee0b1a12939cb85e5895

Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>

On 03/10/2016 04:43 PM, Ilia Mirkin wrote:
> On Thu, Mar 10, 2016 at 10:27 AM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>>
>>
>> On 03/10/2016 04:23 PM, Ilia Mirkin wrote:
>>>
>>> On Thu, Mar 10, 2016 at 10:14 AM, Hans de Goede <hdegoede at
redhat.com>
>>> wrote:
>>>>
>>>> Add support for clover / OpenCL kernel input parameters.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>>    .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp      | 18
>>>> +++++++++++++++---
>>>>    1 file changed, 15 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git
a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> index a8258af..de0c72b 100644
>>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>> @@ -1523,9 +1523,21 @@ Converter::makeSym(uint tgsiFile, int
fileIdx, int
>>>> idx, int c, uint32_t address)
>>>>
>>>>       sym->reg.fileIndex = fileIdx;
>>>>
>>>> -   if (tgsiFile == TGSI_FILE_MEMORY &&
>>>> -       code->memoryFiles[fileIdx].mem_type ==
TGSI_MEMORY_TYPE_SHARED)
>>>> -      sym->setFile(FILE_MEMORY_SHARED);
>>>> +   if (tgsiFile == TGSI_FILE_MEMORY) {
>>>> +      switch (code->memoryFiles[fileIdx].mem_type) {
>>>> +      case TGSI_MEMORY_TYPE_SHARED:
>>>> +         sym->setFile(FILE_MEMORY_SHARED);
>>>> +         break;
>>>> +      case TGSI_MEMORY_TYPE_INPUT:
>>>> +         assert(prog->getType() == Program::TYPE_COMPUTE);
>>>> +         assert(idx == -1);
>>>> +         sym->setFile(FILE_SHADER_INPUT);
>>>> +         address += info->prop.cp.inputOffset;
>>>
>>>
>>> What's the idea here? i.e. what is the inputOffset, how is it
set, and
>>> why?
>>
>>
>> I don't get the idea too, btw.
>>
>> But prop.cp.inputOffset is only defined for compute on Kepler. It's
the
>> offset of input parameters in the screen->parm BO but as you already
know,
>> it is going to be removed because the idea is to use
screen->uniform_bo
>> instead. I'll do this change *after* the compute shaders support on
Kepler.
>
> Actually looks like it's only set for nv50 that I can see, shifting
> things over by 0x10. It used to be reflected by getResourceBase, but
> we broke that abstraction... might be nice to get it back somehow,
> perhaps by sending more arguments down to getResourceBase? Either way,
> that can be done later. This patch is
>
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
-- 
-Samuel

Hi,

On 10-03-16 17:03, Samuel Pitoiset wrote:
> Looks fine, except that you will need to lower FILE_SHADER_INPUT to
FILE_MEMORY_SHARED for Tesla because input kernel parameters are located at
s[0x10].
Ok, but should this be done in nv50_ir_from_tgsi.cpp ? That feels like the wrong
place to
handle this detail. Not sure where to do it otherwise though, and doing this
later
may make the code more complicated.

 > No need to do this for Fermi+ because it's already lowered to c0[].
Note that input kernel parameters will be probably sticked on c7[] after my
changes but that doesn't change anything for you.

Ack.
>
> I already have a patch for the nv50 bits btw, maybe it's the right time
to send it?
>
>
https://cgit.freedesktop.org/~hakzsam/mesa/commit/?h=compute&id=640d68009bcf93c1814cee0b1a12939cb85e5895
Ah I see that answers my question.

Yes I guess this is the right time to send it, although I've not really
looked
at opencl for nv50 yet.

Regards,

Hans

>
> Reviewed-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>
> On 03/10/2016 04:43 PM, Ilia Mirkin wrote:
>> On Thu, Mar 10, 2016 at 10:27 AM, Samuel Pitoiset
>> <samuel.pitoiset at gmail.com> wrote:
>>>
>>>
>>> On 03/10/2016 04:23 PM, Ilia Mirkin wrote:
>>>>
>>>> On Thu, Mar 10, 2016 at 10:14 AM, Hans de Goede <hdegoede at
redhat.com>
>>>> wrote:
>>>>>
>>>>> Add support for clover / OpenCL kernel input parameters.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>>> ---
>>>>>    .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp      |
18
>>>>> +++++++++++++++---
>>>>>    1 file changed, 15 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git
a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>>> index a8258af..de0c72b 100644
>>>>> ---
a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>>> +++
b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>>>> @@ -1523,9 +1523,21 @@ Converter::makeSym(uint tgsiFile,
int fileIdx, int
>>>>> idx, int c, uint32_t address)
>>>>>
>>>>>       sym->reg.fileIndex = fileIdx;
>>>>>
>>>>> -   if (tgsiFile == TGSI_FILE_MEMORY &&
>>>>> -       code->memoryFiles[fileIdx].mem_type ==
TGSI_MEMORY_TYPE_SHARED)
>>>>> -      sym->setFile(FILE_MEMORY_SHARED);
>>>>> +   if (tgsiFile == TGSI_FILE_MEMORY) {
>>>>> +      switch (code->memoryFiles[fileIdx].mem_type) {
>>>>> +      case TGSI_MEMORY_TYPE_SHARED:
>>>>> +         sym->setFile(FILE_MEMORY_SHARED);
>>>>> +         break;
>>>>> +      case TGSI_MEMORY_TYPE_INPUT:
>>>>> +         assert(prog->getType() ==
Program::TYPE_COMPUTE);
>>>>> +         assert(idx == -1);
>>>>> +         sym->setFile(FILE_SHADER_INPUT);
>>>>> +         address += info->prop.cp.inputOffset;
>>>>
>>>>
>>>> What's the idea here? i.e. what is the inputOffset, how is
it set, and
>>>> why?
>>>
>>>
>>> I don't get the idea too, btw.
>>>
>>> But prop.cp.inputOffset is only defined for compute on Kepler.
It's the
>>> offset of input parameters in the screen->parm BO but as you
already know,
>>> it is going to be removed because the idea is to use
screen->uniform_bo
>>> instead. I'll do this change *after* the compute shaders
support on Kepler.
>>
>> Actually looks like it's only set for nv50 that I can see, shifting
>> things over by 0x10. It used to be reflected by getResourceBase, but
>> we broke that abstraction... might be nice to get it back somehow,
>> perhaps by sending more arguments down to getResourceBase? Either way,
>> that can be done later. This patch is
>>
>> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>
>