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

[Samuel Pitoiset <samuel.pitoiset-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

-- 
-Samuel
_______________________________________________
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau