Tom Stellard <thomas.stell...@amd.com> writes: > When probing for devices, clover will call pipe_loader_probe() twice. > The first time to retrieve the number of devices, and then second time > to retrieve the device structures. > > We currently assume that the return value of both calls will be the > same, but this will not be the case if a device happens to disappear > between the two calls. >
Assuming that the two sequential calls will give the same result is bad,
but the likelihood of a device disappearing between the two calls is so
small it's unlikely to happen more than once in a million years. If
this problem happens deterministically to you there's almost certainly a
more serious bug in pipe-loader that will cause it to return an
incorrect device count during the first call.
Indeed, pipe_loader_sw_probe() seems to return 1 unconditionally if ndev
is zero, so it will give a different device count depending on ndev if
pipe_loader_sw_probe_null() happens to fail. Seems like a bug we don't
want to simply paper over.
Anyway I'm not saying that clover's assumption shouldn't be fixed too --
See comment below.
> This patch removes this assumption and checks the return value of the
> second pipe_loader_probe() call to ensure it does not try to initialize
> devices that no longer exits.
>
> CC: <mesa-sta...@lists.freedesktop.org>
> ---
> src/gallium/state_trackers/clover/core/platform.cpp | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/platform.cpp
> b/src/gallium/state_trackers/clover/core/platform.cpp
> index 328b71c..689d692 100644
> --- a/src/gallium/state_trackers/clover/core/platform.cpp
> +++ b/src/gallium/state_trackers/clover/core/platform.cpp
> @@ -28,9 +28,10 @@ platform::platform() : adaptor_range(evals(), devs) {
> int n = pipe_loader_probe(NULL, 0);
> std::vector<pipe_loader_device *> ldevs(n);
>
> - pipe_loader_probe(&ldevs.front(), n);
> + n = pipe_loader_probe(&ldevs.front(), n);
>
> - for (pipe_loader_device *ldev : ldevs) {
> + for (int i = 0; i < n; ++i) {
> + pipe_loader_device *ldev = ldevs[i];
There are many reasons to prefer range loops to induction variables
(including iterators), and there's no need to change it here: Because
ldevs is zero-initialized you can just wrap the push_back(create()) call
around an 'if (ldev)' to make this a one-liner.
> try {
> devs.push_back(create<device>(*this, ldev));
> } catch (error &) {
> --
> 2.0.4
>
> _______________________________________________
> mesa-stable mailing list
> mesa-sta...@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-stable
signature.asc
Description: PGP signature
_______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
