Re: [Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized
On 03/14/2016 09:49 PM, Francisco Jerez wrote:
Samuel Pitoiset writes:
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
---
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.
In C++ '{}' is standard, more concise, and works for a wider range of
types regardless of their layout ('{ 0 }' is valid or not depending on
what the first member of the struct is, while '{}' works regardless, in
C++11 it can even be used to initialize non-POD types with custom
constructors), so it should be generally preferred instead.
Don't bother to resend just because of my nitpicking, I'll fix it up
before I push the last revision of your change, which is:
I didn't know that, thanks for this very good explanation. :-)
Reviewed-by: Francisco Jerez
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
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized
Samuel Pitoiset writes:
> 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
> ---
> 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.
>
In C++ '{}' is standard, more concise, and works for a wider range of
types regardless of their layout ('{ 0 }' is valid or not depending on
what the first member of the struct is, while '{}' works regardless, in
C++11 it can even be used to initialize non-POD types with custom
constructors), so it should be generally preferred instead.
Don't bother to resend just because of my nitpicking, I'll fix it up
before I push the last revision of your change, which is:
Reviewed-by: Francisco Jerez
>>
>>>
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
> ___
> Nouveau mailing list
> Nouveau@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nouveau
signature.asc
Description: PGP signature
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized
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
---
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
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized
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
---
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
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [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
---
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
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
Re: [Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized
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
---
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.
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?
Thanks.
// The handles are created during exec_context::bind(), so we need make
// sure to call exec_context::bind() before retrieving them.
--
-Samuel
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
[Nouveau] [PATCH mesa] clover: Fix pipe_grid_info.indirect not being initialized
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
---
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, };
// The handles are created during exec_context::bind(), so we need make
// sure to call exec_context::bind() before retrieving them.
--
2.7.2
___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau
