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

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>

On Thu, Mar 10, 2016 at 11:03 AM, Pierre Moreau <pierre.morrow at 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 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.
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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/nouveau
-- 
-Samuel

On Thu, Mar 10, 2016 at 11:05 AM, Samuel Pitoiset
<samuel.pitoiset at 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 at
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 at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- 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/d8c35257/attachment.sig>