[mesa,3/3] nouveau: Add support for clover / OpenCL kernel input parameters

Submitted by Hans de Goede on March 10, 2016, 3:14 p.m.

Details

Message ID 1457622887-19328-4-git-send-email-hdegoede@redhat.com
State New
Headers show
Series "tgsi and nouveau global / local / opencl-input mem support" ( rev: 1 ) in Nouveau

Not browsing as part of any series.

Commit Message

Hans de Goede March 10, 2016, 3:14 p.m.
Add support for clover / OpenCL kernel input parameters.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp      | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Patch hide | download patch | download mbox

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

Comments

On Thu, Mar 10, 2016 at 10:14 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Add support for clover / OpenCL kernel input parameters.
>
> Signed-off-by: Hans de Goede <hdegoede@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@redhat.com> wrote:
>> Add support for clover / OpenCL kernel input parameters.
>>
>> Signed-off-by: Hans de Goede <hdegoede@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
>>
On Thu, Mar 10, 2016 at 10:27 AM, Samuel Pitoiset
<samuel.pitoiset@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@redhat.com>
>> wrote:
>>>
>>> Add support for clover / OpenCL kernel input parameters.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@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@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@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@redhat.com>
>>> wrote:
>>>>
>>>> Add support for clover / OpenCL kernel input parameters.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@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@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@redhat.com> wrote:
> >>Add support for clover / OpenCL kernel input parameters.
> >>
> >>Signed-off-by: Hans de Goede <hdegoede@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
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@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@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@redhat.com>
>>> wrote:
>>>>
>>>> Add support for clover / OpenCL kernel input parameters.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@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@alum.mit.edu>
>
On Thu, Mar 10, 2016 at 11:03 AM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> You might want to increment the address by at least
> `info->prop.cp.inputOffset`, and if inputs still end up in shared on Tesla,

There's a cp.sharedOffset just for that :) However it doesn't appear
to get set anywhere...
On 03/10/2016 05:03 PM, Pierre Moreau wrote:
> 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@redhat.com> wrote:
>>>> Add support for clover / OpenCL kernel input parameters.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@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.

cf. my previous comment. :-)

>
>>>> +         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?

Sure for fermi, and probably for Tesla.

>
> 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@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
On Thu, Mar 10, 2016 at 11:05 AM, Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>> If I understand correctly, the goal is to have user inputs in a
>> `screen->uniform_bo`, and so for all generations?
>
> Sure for fermi, and probably for Tesla.

I think continuing to use the USER_PARAMS or whatever mechanism on
telsa makes sense. That's why I agreed to keep the MEMORY, INPUT
concept.

  -ilia
On 11:05 AM - Mar 10 2016, Ilia Mirkin wrote:
> On Thu, Mar 10, 2016 at 11:03 AM, Pierre Moreau <pierre.morrow@free.fr> wrote:
> > You might want to increment the address by at least
> > `info->prop.cp.inputOffset`, and if inputs still end up in shared on Tesla,
> 
> There's a cp.sharedOffset just for that :) However it doesn't appear
> to get set anywhere...

Oh really?! I completely missed that oneā€¦ Well, I have some changes to make on
my own code then! :-D Thanks for pointing that out!

Pierre


> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
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@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@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@redhat.com>
>>>> wrote:
>>>>>
>>>>> Add support for clover / OpenCL kernel input parameters.
>>>>>
>>>>> Signed-off-by: Hans de Goede <hdegoede@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@alum.mit.edu>
>>
>