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

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?

  -ilia
> +         break;
> +      default:
> +         assert(0); /* TODO: Add support for global and local memory */
> +      }
> +   }
>
>     if (idx >= 0) {
>        if (sym->reg.file == FILE_SHADER_INPUT)
> --
> 2.7.2
>

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.
>
>    -ilia
>
>> +         break;
>> +      default:
>> +         assert(0); /* TODO: Add support for global and local memory
*/
>> +      }
>> +   }
>>
>>      if (idx >= 0) {
>>         if (sym->reg.file == FILE_SHADER_INPUT)
>> --
>> 2.7.2
>>
-- 
-Samuel

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 04:27 PM - Mar 10 2016, Samuel Pitoiset 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);
You might want to increment the address by at least
`info->prop.cp.inputOffset`, and if inputs still end up in shared on Tesla,
then increment further by the input size. This input offset of 0x10 (or is it
0x20?) is due to the card sticking the size of a block and of the grid inside
`s[0x0..0x10]` (or maybe Nouveau is doing that, but I doubt it.). So even if
the user inputs end up somewhere else in memory, you most likely still don't
want to overwrite the grid information. This should be necessary only for Tesla
cards.
> >>+         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.
If I understand correctly, the goal is to have user inputs in a
`screen->uniform_bo`, and so for all generations?

Pierre

> 
> >
> >   -ilia
> >
> >>+         break;
> >>+      default:
> >>+         assert(0); /* TODO: Add support for global and local
memory */
> >>+      }
> >>+   }
> >>
> >>     if (idx >= 0) {
> >>        if (sym->reg.file == FILE_SHADER_INPUT)
> >>--
> >>2.7.2
> >>
> 
> -- 
> -Samuel
> _______________________________________________
> Nouveau mailing list
> Nouveau at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20160310/8fd0b68e/attachment.sig>