Nouveau - Mar 2016 - [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized

Hi,

On 14-03-16 14:01, Samuel Pitoiset wrote:
>
>
> On 03/14/2016 01:50 PM, Hans de Goede wrote:
>> After pipe_grid_info.indirect was introduced, clover was not modified
>> to set it causing it to pass uninitialized memory for it to
launch_grid.
>>
>> This commit fixes this by zero-ing the entire pipe_grid_info struct
when
>> declaring it, to avoid similar problems popping-up in the future.
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>   src/gallium/state_trackers/clover/core/kernel.cpp | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
b/src/gallium/state_trackers/clover/core/kernel.cpp
>> index 8396be9..dad66aa 100644
>> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>> @@ -55,7 +55,7 @@ kernel::launch(command_queue &q,
>>      const auto reduced_grid_size >>         map(divides(),
grid_size, block_size);
>>      void *st = exec.bind(&q, grid_offset);
>> -   struct pipe_grid_info info;
>> +   struct pipe_grid_info info = { 0, };
>
> Right, good catch, it's my fault.
>
> = { 0 }; is enough btw.
I prefer to add the "," to make clear that we are initializing the
entire struct,
I read it as  ", ...".
> This should be backported to mesa 11.2 I guess, could you please send a v2
with this minor fix and add the cc thing?
Sure, as soon as we're done bikeshedding on the "," :)

Regards,

Hans

On 03/14/2016 02:26 PM, Hans de Goede wrote:
> Hi,
>
> On 14-03-16 14:01, Samuel Pitoiset wrote:
>>
>>
>> On 03/14/2016 01:50 PM, Hans de Goede wrote:
>>> After pipe_grid_info.indirect was introduced, clover was not
modified
>>> to set it causing it to pass uninitialized memory for it to
launch_grid.
>>>
>>> This commit fixes this by zero-ing the entire pipe_grid_info struct
when
>>> declaring it, to avoid similar problems popping-up in the future.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>   src/gallium/state_trackers/clover/core/kernel.cpp | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
>>> b/src/gallium/state_trackers/clover/core/kernel.cpp
>>> index 8396be9..dad66aa 100644
>>> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>>> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>>> @@ -55,7 +55,7 @@ kernel::launch(command_queue &q,
>>>      const auto reduced_grid_size >>>        
map(divides(), grid_size, block_size);
>>>      void *st = exec.bind(&q, grid_offset);
>>> -   struct pipe_grid_info info;
>>> +   struct pipe_grid_info info = { 0, };
>>
>> Right, good catch, it's my fault.
>>
>> = { 0 }; is enough btw.
>
> I prefer to add the "," to make clear that we are initializing
the
> entire struct,
> I read it as  ", ...".
Well, usually we use { 0 } in mesa, try to grep and you will see. :-)
There is only 3 occurrences of { 0, }, but I think they are quite old.
>
>> This should be backported to mesa 11.2 I guess, could you please send
>> a v2 with this minor fix and add the cc thing?
>
> Sure, as soon as we're done bikeshedding on the "," :)
>
> Regards,
>
> Hans
-- 
-Samuel

On 03/14/2016 02:29 PM, Samuel Pitoiset wrote:
>
>
> On 03/14/2016 02:26 PM, Hans de Goede wrote:
>> Hi,
>>
>> On 14-03-16 14:01, Samuel Pitoiset wrote:
>>>
>>>
>>> On 03/14/2016 01:50 PM, Hans de Goede wrote:
>>>> After pipe_grid_info.indirect was introduced, clover was not
modified
>>>> to set it causing it to pass uninitialized memory for it to
>>>> launch_grid.
>>>>
>>>> This commit fixes this by zero-ing the entire pipe_grid_info
struct
>>>> when
>>>> declaring it, to avoid similar problems popping-up in the
future.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>>> ---
>>>>   src/gallium/state_trackers/clover/core/kernel.cpp | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
>>>> b/src/gallium/state_trackers/clover/core/kernel.cpp
>>>> index 8396be9..dad66aa 100644
>>>> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
>>>> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
>>>> @@ -55,7 +55,7 @@ kernel::launch(command_queue &q,
>>>>      const auto reduced_grid_size >>>>        
map(divides(), grid_size, block_size);
>>>>      void *st = exec.bind(&q, grid_offset);
>>>> -   struct pipe_grid_info info;
>>>> +   struct pipe_grid_info info = { 0, };
>>>
>>> Right, good catch, it's my fault.
>>>
>>> = { 0 }; is enough btw.
>>
>> I prefer to add the "," to make clear that we are
initializing the
>> entire struct,
>> I read it as  ", ...".
>
> Well, usually we use { 0 } in mesa, try to grep and you will see. :-)
> There is only 3 occurrences of { 0, }, but I think they are quite old.
Of course, I'm not really against this ",", but I just want
consistency
with the other parts.
>
>>
>>> This should be backported to mesa 11.2 I guess, could you please
send
>>> a v2 with this minor fix and add the cc thing?
>>
>> Sure, as soon as we're done bikeshedding on the "," :)
>>
>> Regards,
>>
>> Hans
>
-- 
-Samuel