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

[Hans de Goede]

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