[Mesa-dev] [Bug 93126] wrongly claim supporting GL_EXT_texture_rg
https://bugs.freedesktop.org/show_bug.cgi?id=93126 Ilia Mirkin changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #3 from Ilia Mirkin --- Pushed as commit 0396eaaf80c5d7955d7926c4e448f006c7682d2e Author: Ilia Mirkin Date: Thu Nov 26 10:32:57 2015 -0500 mesa: support GL_RED/GL_RG in ES2 contexts when driver support exists Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93126 Signed-off-by: Ilia Mirkin Reviewed-by: Eduardo Lima Mitev Cc: "11.0 11.1" -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91888] EGL Wayland software rendering no longer work after regression
https://bugs.freedesktop.org/show_bug.cgi?id=91888 --- Comment #7 from nerdopol...@verizon.net --- It seems that mesa master's software rendering also causes some problems with SDL v2, and the testgl2 application that is in their repository for Wayland Reverting Mesa to 10.6 allows this application to work otherwise I get libEGL Debug: EGL user error 0x3001 (EGL_NOT_INITIALIZED) in eglInitilize -- You are receiving this mail because: You are the QA Contact for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: support GL_RED/GL_RG in ES2 contexts when driver support exists
On Sat, Nov 28, 2015 at 1:14 PM, Eduardo Lima Mitev wrote:
> Patch is:
>
> Reviewed-by: Eduardo Lima Mitev
Thanks! I realized that this should also return invalid for GLES 1...
I'm adding ctx->API == API_OPENGLES and pushing.
>
> On 11/26/2015 04:57 PM, Ilia Mirkin wrote:
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93126
>> Signed-off-by: Ilia Mirkin
>> Cc: "11.0 11.1"
>> ---
>> src/mesa/main/glformats.c | 8 +++-
>> src/mesa/main/glformats.h | 3 ++-
>> src/mesa/main/readpix.c | 2 +-
>> src/mesa/main/teximage.c | 2 +-
>> 4 files changed, 11 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
>> index 2ed42ea..7b264eb 100644
>> --- a/src/mesa/main/glformats.c
>> +++ b/src/mesa/main/glformats.c
>> @@ -2077,12 +2077,18 @@ _mesa_error_check_format_and_type(const struct
>> gl_context *ctx,
>> * \return error code, or GL_NO_ERROR.
>> */
>> GLenum
>> -_mesa_es_error_check_format_and_type(GLenum format, GLenum type,
>> +_mesa_es_error_check_format_and_type(const struct gl_context *ctx,
>> + GLenum format, GLenum type,
>> unsigned dimensions)
>> {
>> GLboolean type_valid = GL_TRUE;
>>
>> switch (format) {
>> + case GL_RED:
>> + case GL_RG:
>> + if (!ctx->Extensions.ARB_texture_rg)
>> + return GL_INVALID_VALUE;
>> + /* fallthrough */
>> case GL_ALPHA:
>> case GL_LUMINANCE:
>> case GL_LUMINANCE_ALPHA:
>> diff --git a/src/mesa/main/glformats.h b/src/mesa/main/glformats.h
>> index 92f4bc6..b366855 100644
>> --- a/src/mesa/main/glformats.h
>> +++ b/src/mesa/main/glformats.h
>> @@ -127,7 +127,8 @@ _mesa_error_check_format_and_type(const struct
>> gl_context *ctx,
>>GLenum format, GLenum type);
>>
>> extern GLenum
>> -_mesa_es_error_check_format_and_type(GLenum format, GLenum type,
>> +_mesa_es_error_check_format_and_type(const struct gl_context *ctx,
>> + GLenum format, GLenum type,
>> unsigned dimensions);
>>
>> extern GLenum
>> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
>> index 81bb912..8cdc9fe 100644
>> --- a/src/mesa/main/readpix.c
>> +++ b/src/mesa/main/readpix.c
>> @@ -1043,7 +1043,7 @@ _mesa_ReadnPixelsARB( GLint x, GLint y, GLsizei width,
>> GLsizei height,
>>_mesa_get_color_read_type(ctx) == type) {
>> err = GL_NO_ERROR;
>>} else if (ctx->Version < 30) {
>> - err = _mesa_es_error_check_format_and_type(format, type, 2);
>> + err = _mesa_es_error_check_format_and_type(ctx, format, type, 2);
>> if (err == GL_NO_ERROR) {
>> if (type == GL_FLOAT || type == GL_HALF_FLOAT_OES) {
>> err = GL_INVALID_OPERATION;
>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>> index ac7599f..37dbe26 100644
>> --- a/src/mesa/main/teximage.c
>> +++ b/src/mesa/main/teximage.c
>> @@ -1699,7 +1699,7 @@ texture_format_error_check_gles(struct gl_context
>> *ctx, GLenum format,
>>}
>> }
>> else {
>> - err = _mesa_es_error_check_format_and_type(format, type, dimensions);
>> + err = _mesa_es_error_check_format_and_type(ctx, format, type,
>> dimensions);
>>if (err != GL_NO_ERROR) {
>> _mesa_error(ctx, err, "%s(format = %s, type = %s)",
>> callerName, _mesa_enum_to_string(format),
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] travis: Initial import of travis instructions.
On 25 November 2015 at 07:20, Jose Fonseca wrote: > BTW, I setup Mesa with Appveyor (like Travis for Windows) > > https://ci.appveyor.com/project/jrfonseca/mesa > > I'll try to get that going and commited too. > As a person who has broken the Windows build on an occasion or two, yes please. > The nice thing about Appveyor is that it can clone the git from anywhere, > not just GitHub. > > > Would it be OK to have email notifications to mesa-commits? > I'm wondering if mesa-dev wouldn't be more suitable. Not many of us (or is it just me?) don't follow mesa-commits. One thing no one mentioned is that one can automatically feed the results from Travis-CI/github into Coverity. I know Vinson has been doing the latter, although I'm not sure how much of it is automated. Thank you Jose! -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] travis: Initial import of travis instructions.
On Tue, Nov 24, 2015 at 4:38 PM, Rob Clark wrote: > On Tue, Nov 24, 2015 at 4:10 PM, Eric Anholt wrote: >> Rob Clark writes: >> >>> On Tue, Nov 24, 2015 at 2:10 PM, Ilia Mirkin wrote: Any reason not to add freedreno and nouveau in here? Too bad these packages don't all have "latest" symlinks... >>> >>> hmm, I was going to say that tinderbox can figure out how to just >>> build latest (plus we at one point had builders for several different >>> arch's for xserver and mesa, etc..) >>> >>> but then I noticed that http://tinderbox.x.org/ is in kind of dire shape.. >>> :-( >> >> I've run tinderbox servers before. It's a bunch of work, and then >> nobody looks at it anyway, including me. It also only tests your pushes >> once they've hit master, which to me misses the whole point. >> >> The github integration means that people doing things on github see CI >> state in their interfaces (well, in pull requests. If you just want >> current state, you have to go to the travis-ci.org dashboard). It also >> sends me emails when my branch pushes fail. > > yeah, I've run tinderbox builder too, so I know the issues.. > > All the same, it's probably still a good idea to resurrect tinderbox > to get build coverage on different archs.. the clever thing would be > if we could also work in piglit runs as well (ie. at least for > whatever gpu(s)) the various builders have.. In related news, I've managed to get an armv7 builder going again.. http://tinderbox.x.org/builds/machines/robclark-reptile/ it's building freedreno+vc4 gallium drivers, along w/ freedreno/armsoc/omap/modesetting ddx, etc. Once etnaviv finds it's way to upstream I'll switch it on as well. I guess I should try and figure out enough jhbuild to trick it into building/running piglit after mesa, then we could at least get some piglit coverage on one device. I do have one armv8 device that I could perhaps spare for an armv8 builder, but short a spare power supply for that one so would have to unplug it from time to time.. which would be a bit annoying. If someone has some armv8 hw to dedicate to the cause, I could send you my notes/config.. BR, -R > It may just end up being best to have both, at least as long as this > is low effort to keep going, it doesn't hurt. And the convenience for > personal git trees is a win. > > BR, > -R ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: support GL_RED/GL_RG in ES2 contexts when driver support exists
Patch is:
Reviewed-by: Eduardo Lima Mitev
On 11/26/2015 04:57 PM, Ilia Mirkin wrote:
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93126
> Signed-off-by: Ilia Mirkin
> Cc: "11.0 11.1"
> ---
> src/mesa/main/glformats.c | 8 +++-
> src/mesa/main/glformats.h | 3 ++-
> src/mesa/main/readpix.c | 2 +-
> src/mesa/main/teximage.c | 2 +-
> 4 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/main/glformats.c b/src/mesa/main/glformats.c
> index 2ed42ea..7b264eb 100644
> --- a/src/mesa/main/glformats.c
> +++ b/src/mesa/main/glformats.c
> @@ -2077,12 +2077,18 @@ _mesa_error_check_format_and_type(const struct
> gl_context *ctx,
> * \return error code, or GL_NO_ERROR.
> */
> GLenum
> -_mesa_es_error_check_format_and_type(GLenum format, GLenum type,
> +_mesa_es_error_check_format_and_type(const struct gl_context *ctx,
> + GLenum format, GLenum type,
> unsigned dimensions)
> {
> GLboolean type_valid = GL_TRUE;
>
> switch (format) {
> + case GL_RED:
> + case GL_RG:
> + if (!ctx->Extensions.ARB_texture_rg)
> + return GL_INVALID_VALUE;
> + /* fallthrough */
> case GL_ALPHA:
> case GL_LUMINANCE:
> case GL_LUMINANCE_ALPHA:
> diff --git a/src/mesa/main/glformats.h b/src/mesa/main/glformats.h
> index 92f4bc6..b366855 100644
> --- a/src/mesa/main/glformats.h
> +++ b/src/mesa/main/glformats.h
> @@ -127,7 +127,8 @@ _mesa_error_check_format_and_type(const struct gl_context
> *ctx,
>GLenum format, GLenum type);
>
> extern GLenum
> -_mesa_es_error_check_format_and_type(GLenum format, GLenum type,
> +_mesa_es_error_check_format_and_type(const struct gl_context *ctx,
> + GLenum format, GLenum type,
> unsigned dimensions);
>
> extern GLenum
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index 81bb912..8cdc9fe 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -1043,7 +1043,7 @@ _mesa_ReadnPixelsARB( GLint x, GLint y, GLsizei width,
> GLsizei height,
>_mesa_get_color_read_type(ctx) == type) {
> err = GL_NO_ERROR;
>} else if (ctx->Version < 30) {
> - err = _mesa_es_error_check_format_and_type(format, type, 2);
> + err = _mesa_es_error_check_format_and_type(ctx, format, type, 2);
> if (err == GL_NO_ERROR) {
> if (type == GL_FLOAT || type == GL_HALF_FLOAT_OES) {
> err = GL_INVALID_OPERATION;
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index ac7599f..37dbe26 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -1699,7 +1699,7 @@ texture_format_error_check_gles(struct gl_context *ctx,
> GLenum format,
>}
> }
> else {
> - err = _mesa_es_error_check_format_and_type(format, type, dimensions);
> + err = _mesa_es_error_check_format_and_type(ctx, format, type,
> dimensions);
>if (err != GL_NO_ERROR) {
> _mesa_error(ctx, err, "%s(format = %s, type = %s)",
> callerName, _mesa_enum_to_string(format),
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
On 28 November 2015 at 17:06, Francisco Jerez wrote: > ... you're free to propose such a change as a > separate series as long as you fix the documentation, all users and > back-ends of pipe-loader, if you consider it worth doing -- I personally > don't. > That what I was wondering about. Thanks > I don't think it boils down to that. ... I got the idea that you approve of the patch the first time around, despite it (the approval) blurring with the lengthy justification of the improved version. Perhaps we (I'm also guilty on that one) can use a more explicit "looks good, but let's we use X, because ..." approach. Otherwise things tend to get a bit strange ? Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
Emil Velikov writes:
> On 28 November 2015 at 15:10, Francisco Jerez wrote:
>> Emil Velikov writes:
>>
>>> Hi Francisco,
>>>
>>> In all due respect, I am surprised with the volume of justification
>>> you put into some changes.
>>>
>> And apparently it wasn't enough? :P
>>
>>> On 28 November 2015 at 11:52, Francisco Jerez wrote:
Francisco Jerez writes:
> Tom Stellard 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
>>> Err... have to love this estimate :-P With some recent chances that
>>> probability increases ever so slightly, as we push the "can we find
>>> the module, does it have the entry point" from .create_screen and
>>> .configure further up to probe screen.
>>>
>>
>> I was referring to the probability of a device actually disappearing
>> From the system between the two calls.
>>
> 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.
>
>>> Care to give an example. I'm not sure where/how this is possible.
>>>
>> The immediately following paragraph explained how and where it's
>> possible: pipe_loader_sw_probe() returns one regardless of whether the
>> software device is actually available.
>>
> That's why we have the "two stage" approach
> 1) Query the total count, allocate memory and 2) populate the memory
> with information.
>
> The alternative is to build a separate pipe-loader for each target,
> which ... doesn't sound optimal. Neither is forcing every target to
> have be with the same set of drivers.
>
> 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.
>
>>> Can you elaborate what exactly the problem here ? If we ask "how many
>>> we have" sure we can provide that [fixed] number. On the other hand,
>>> if we want more information about it (them), there is no guarantee
>>> that we'll succeed.
>>
>> The return value of pipe_loader_probe() is supposed to be the number of
>> devices actually available on the system, it doesn't depend on the devs
>> pointer or ndev parameter -- See the doxygen comment in pipe_loader.h
>> for the expected behaviour and pipe_loader_drm_probe() for a correct
>> implementation.
>>
> True, the documentation does say that. Yet the EGL Devices approach is
> what I perceive as better (clearer, more intuitive) approach - if your
> storage pointer is null return the total count. Otherwise upon
> information retrieval cap up-to the specified device count.
>
That's a valid yet somewhat inconsistent alternative approach. In
pipe-loader the return value of pipe_loader_probe() always has the same
semantics. I don't think I see why you consider it more intuitive for
the return value to have inconsistent semantics depending on what the
function arguments are, but you're free to propose such a change as a
separate series as long as you fix the documentation, all users and
back-ends of pipe-loader, if you consider it worth doing -- I personally
don't.
> Admittedly the EGL spec came long after the pipe-loader.
>
>>>
> 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:
>> ---
>> 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 ldevs(n);
>>
>> - pipe_loader_probe(&ldevs.front(), n);
>> + n = pipe_loader_probe(&ldevs.front(), n);
>>
>> - for (pipe_loader_device *ldev : ldevs) {
>> + for (int i =
Re: [Mesa-dev] [PATCH 8/9] nir: move to compiler
On 27 November 2015 at 20:45, Jason Ekstrand wrote: > On Nov 27, 2015 11:26 AM, "Matt Turner" wrote: >> On Fri, Nov 27, 2015 at 6:50 AM, Emil Velikov >> wrote: >> > On 25 November 2015 at 22:01, Matt Turner wrote: >> >> On Wed, Nov 25, 2015 at 1:32 PM, Emil Velikov >> >> wrote: >> > >> >>> --- a/src/Makefile.am >> >>> +++ b/src/Makefile.am >> >>> @@ -23,6 +23,7 @@ SUBDIRS = . gtest util mapi/glapi/gen mapi >> >>> >> >>> # XXX: conditionally include >> >>> SUBDIRS += compiler >> >>> +SUBDIRS += compiler/nir >> >> >> >> We have a non-recursive build in src/glsl today. I don't want to go >> >> backwards. >> > Not sure I fully get that can you elaborate ? Are you concerned that >> > things won't build in parallel, increasing the compilation times ? >> > >> > On my dual core system running with -j2 results in approx 15 seconds >> > increase. I'm willing to take that trade off for the improved >> > readability. What is the difference on your system ? >> >> src/glsl has single Makefile that builds libglcpp, glcpp, libglsl, >> glsl_compiler, glsl_test, libnir, and various test programs, allowing >> all of these things to happen in parallel. The Makefile is perfectly >> maintainable as it is and there's no advantage of splitting it, >> especially when the work has been done to get things to this state >> (commits 86d30dea, efd201ca) and NIR was added without an additional >> Makefile. > > I would tend to agree. Making things hierarchical is nice but, > unfortunately, autotools makes this and parallelization mutually exclusive. Actually I have some ancient work where we benefit from both. Namely have a single top level Makefile.am, which directly includes the subdirectory Automake.mk files, resulting in one big Makefile at the very end. That aside can we get some quantitative representation of the penalty you guys see. On my (old) machine the difference is negligible ~15 sec of a ~11 minute `make all' and ~16 minute `make distcheck'. Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 0/9] Move nir/glsl to src/compiler
On 27 November 2015 at 18:48, Jason Ekstrand wrote: > On Nov 27, 2015 10:44 AM, "Emil Velikov" wrote: >> Suggestions on how to make it clearer are appreciated :-) > > Meh, you clarified :-) > Hope next time around I can reach you a bit earlier :-P Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
On 28 November 2015 at 15:10, Francisco Jerez wrote:
> Emil Velikov writes:
>
>> Hi Francisco,
>>
>> In all due respect, I am surprised with the volume of justification
>> you put into some changes.
>>
> And apparently it wasn't enough? :P
>
>> On 28 November 2015 at 11:52, Francisco Jerez wrote:
>>> Francisco Jerez writes:
>>>
Tom Stellard 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
>> Err... have to love this estimate :-P With some recent chances that
>> probability increases ever so slightly, as we push the "can we find
>> the module, does it have the entry point" from .create_screen and
>> .configure further up to probe screen.
>>
>
> I was referring to the probability of a device actually disappearing
> From the system between the two calls.
>
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.
>> Care to give an example. I'm not sure where/how this is possible.
>>
> The immediately following paragraph explained how and where it's
> possible: pipe_loader_sw_probe() returns one regardless of whether the
> software device is actually available.
>
That's why we have the "two stage" approach
1) Query the total count, allocate memory and 2) populate the memory
with information.
The alternative is to build a separate pipe-loader for each target,
which ... doesn't sound optimal. Neither is forcing every target to
have be with the same set of drivers.
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.
>> Can you elaborate what exactly the problem here ? If we ask "how many
>> we have" sure we can provide that [fixed] number. On the other hand,
>> if we want more information about it (them), there is no guarantee
>> that we'll succeed.
>
> The return value of pipe_loader_probe() is supposed to be the number of
> devices actually available on the system, it doesn't depend on the devs
> pointer or ndev parameter -- See the doxygen comment in pipe_loader.h
> for the expected behaviour and pipe_loader_drm_probe() for a correct
> implementation.
>
True, the documentation does say that. Yet the EGL Devices approach is
what I perceive as better (clearer, more intuitive) approach - if your
storage pointer is null return the total count. Otherwise upon
information retrieval cap up-to the specified device count.
Admittedly the EGL spec came long after the pipe-loader.
>>
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:
> ---
> 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 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.
>>> Let me be more explicit about what could go wrong with this change in
>>> its current form: The return value of pipe_loader_probe() is the number
>>> of devices available on the system, so basically the same race condition
>>> is still possible if 'n' increases from the first ca
[Mesa-dev] [PATCH 2/3] radeon: const correctness
Add missing `const` specifier for pointer pointing to a const struct. Signed-off-by: Giuseppe Bilotta --- src/mesa/drivers/dri/radeon/radeon_swtcl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/drivers/dri/radeon/radeon_swtcl.c b/src/mesa/drivers/dri/radeon/radeon_swtcl.c index ed6b25c..adc1468 100644 --- a/src/mesa/drivers/dri/radeon/radeon_swtcl.c +++ b/src/mesa/drivers/dri/radeon/radeon_swtcl.c @@ -414,7 +414,7 @@ static GLboolean radeon_run_render( struct gl_context *ctx, r100ContextPtr rmesa = R100_CONTEXT(ctx); TNLcontext *tnl = TNL_CONTEXT(ctx); struct vertex_buffer *VB = &tnl->vb; - tnl_render_func *tab = TAG(render_tab_verts); + const tnl_render_func *tab = TAG(render_tab_verts); GLuint i; if (rmesa->radeon.swtcl.RenderIndex != 0 || -- 2.6.0.rc2.233.g6dd8a9a.dirty ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] radeon: whitespace cleanup
Signed-off-by: Giuseppe Bilotta --- src/mesa/drivers/dri/radeon/radeon_swtcl.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/mesa/drivers/dri/radeon/radeon_swtcl.c b/src/mesa/drivers/dri/radeon/radeon_swtcl.c index 1e19cf7..ed6b25c 100644 --- a/src/mesa/drivers/dri/radeon/radeon_swtcl.c +++ b/src/mesa/drivers/dri/radeon/radeon_swtcl.c @@ -417,9 +417,9 @@ static GLboolean radeon_run_render( struct gl_context *ctx, tnl_render_func *tab = TAG(render_tab_verts); GLuint i; - if (rmesa->radeon.swtcl.RenderIndex != 0 || + if (rmesa->radeon.swtcl.RenderIndex != 0 || !radeon_dma_validate_render( ctx, VB )) - return GL_TRUE; + return GL_TRUE; radeon_prepare_render(&rmesa->radeon); if (rmesa->radeon.NewGLState) -- 2.6.0.rc2.233.g6dd8a9a.dirty ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/3] misc janitorial
The second and third patches are mostly aimed at removing non-spurious compiler warnings. The first one is just minor whitespace cleanup in the general area of code touched by the second patch. Giuseppe Bilotta (3): radeon: whitespace cleanup radeon: const correctness xvmc: force assertion in XvMC tests src/gallium/state_trackers/xvmc/tests/test_blocks.c | 2 ++ src/gallium/state_trackers/xvmc/tests/test_context.c| 2 ++ src/gallium/state_trackers/xvmc/tests/test_rendering.c | 2 ++ src/gallium/state_trackers/xvmc/tests/test_subpicture.c | 2 ++ src/gallium/state_trackers/xvmc/tests/test_surface.c| 2 ++ src/mesa/drivers/dri/radeon/radeon_swtcl.c | 6 +++--- 6 files changed, 13 insertions(+), 3 deletions(-) -- 2.6.0.rc2.233.g6dd8a9a.dirty ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/3] xvmc: force assertion in XvMC tests
This follows the src/util/u_atomic_test model of undefining NDEBUG unconditionally throughouth the XvMC tests, to force asserts regardless of debug mode. Signed-off-by: Giuseppe Bilotta --- src/gallium/state_trackers/xvmc/tests/test_blocks.c | 2 ++ src/gallium/state_trackers/xvmc/tests/test_context.c| 2 ++ src/gallium/state_trackers/xvmc/tests/test_rendering.c | 2 ++ src/gallium/state_trackers/xvmc/tests/test_subpicture.c | 2 ++ src/gallium/state_trackers/xvmc/tests/test_surface.c| 2 ++ 5 files changed, 10 insertions(+) diff --git a/src/gallium/state_trackers/xvmc/tests/test_blocks.c b/src/gallium/state_trackers/xvmc/tests/test_blocks.c index a35838f..4c89951 100644 --- a/src/gallium/state_trackers/xvmc/tests/test_blocks.c +++ b/src/gallium/state_trackers/xvmc/tests/test_blocks.c @@ -25,6 +25,8 @@ * **/ +/* Force assertions, even on debug builds. */ +#undef NDEBUG #include #include #include diff --git a/src/gallium/state_trackers/xvmc/tests/test_context.c b/src/gallium/state_trackers/xvmc/tests/test_context.c index 344ac76..aa7c2c7 100644 --- a/src/gallium/state_trackers/xvmc/tests/test_context.c +++ b/src/gallium/state_trackers/xvmc/tests/test_context.c @@ -25,6 +25,8 @@ * **/ +/* Force assertions, even on debug builds. */ +#undef NDEBUG #include #include #include diff --git a/src/gallium/state_trackers/xvmc/tests/test_rendering.c b/src/gallium/state_trackers/xvmc/tests/test_rendering.c index b3b3794..8b61c65 100644 --- a/src/gallium/state_trackers/xvmc/tests/test_rendering.c +++ b/src/gallium/state_trackers/xvmc/tests/test_rendering.c @@ -25,6 +25,8 @@ * **/ +/* Force assertions, even on debug builds. */ +#undef NDEBUG #include #include #include diff --git a/src/gallium/state_trackers/xvmc/tests/test_subpicture.c b/src/gallium/state_trackers/xvmc/tests/test_subpicture.c index 57ba1c7..564c561 100644 --- a/src/gallium/state_trackers/xvmc/tests/test_subpicture.c +++ b/src/gallium/state_trackers/xvmc/tests/test_subpicture.c @@ -25,6 +25,8 @@ * **/ +/* Force assertions, even on debug builds. */ +#undef NDEBUG #include #include #include diff --git a/src/gallium/state_trackers/xvmc/tests/test_surface.c b/src/gallium/state_trackers/xvmc/tests/test_surface.c index 964ca82..9ee8d3c 100644 --- a/src/gallium/state_trackers/xvmc/tests/test_surface.c +++ b/src/gallium/state_trackers/xvmc/tests/test_surface.c @@ -25,6 +25,8 @@ * **/ +/* Force assertions, even on debug builds. */ +#undef NDEBUG #include #include #include -- 2.6.0.rc2.233.g6dd8a9a.dirty ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
Emil Velikov writes:
> Hi Francisco,
>
> In all due respect, I am surprised with the volume of justification
> you put into some changes.
>
And apparently it wasn't enough? :P
> On 28 November 2015 at 11:52, Francisco Jerez wrote:
>> Francisco Jerez writes:
>>
>>> Tom Stellard 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
> Err... have to love this estimate :-P With some recent chances that
> probability increases ever so slightly, as we push the "can we find
> the module, does it have the entry point" from .create_screen and
> .configure further up to probe screen.
>
I was referring to the probability of a device actually disappearing
From the system between the two calls.
>>> 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.
>>>
> Care to give an example. I'm not sure where/how this is possible.
>
The immediately following paragraph explained how and where it's
possible: pipe_loader_sw_probe() returns one regardless of whether the
software device is actually available.
>>> 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.
>>>
> Can you elaborate what exactly the problem here ? If we ask "how many
> we have" sure we can provide that [fixed] number. On the other hand,
> if we want more information about it (them), there is no guarantee
> that we'll succeed.
The return value of pipe_loader_probe() is supposed to be the number of
devices actually available on the system, it doesn't depend on the devs
pointer or ndev parameter -- See the doxygen comment in pipe_loader.h
for the expected behaviour and pipe_loader_drm_probe() for a correct
implementation.
>
>>> 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:
---
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 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.
>>>
>> Let me be more explicit about what could go wrong with this change in
>> its current form: The return value of pipe_loader_probe() is the number
>> of devices available on the system, so basically the same race condition
>> is still possible if 'n' increases from the first call to the second,
>> and in that case you'll end up reading uninitialized memory past the end
>> of the array -- Worse than a null dereference. Range loops make this
>> kind of situation impossible.
>>
> If I understood you correctly, you're saying that on the second call
> can return count greater than the initial one.
>
> Errm... this looks like a bug in the pipe-loader. I don't think it's
> normal to claim that X devices are available only to return
> information for fewer devices. OK, calling it a bug might not be
> politically correct, although it is a misleading design decision, at
> the very least.
>
What I was saying is that if the rationale of this patch is correct and
clover may not make any assumptions about the number of available
devices remaining the same between calls t
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
Hi Francisco,
In all due respect, I am surprised with the volume of justification
you put into some changes.
On 28 November 2015 at 11:52, Francisco Jerez wrote:
> Francisco Jerez writes:
>
>> Tom Stellard 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
Err... have to love this estimate :-P With some recent chances that
probability increases ever so slightly, as we push the "can we find
the module, does it have the entry point" from .create_screen and
.configure further up to probe screen.
>> 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.
>>
Care to give an example. I'm not sure where/how this is possible.
>> 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.
>>
Can you elaborate what exactly the problem here ? If we ask "how many
we have" sure we can provide that [fixed] number. On the other hand,
if we want more information about it (them), there is no guarantee
that we'll succeed.
>> 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:
>>> ---
>>> 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 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.
>>
> Let me be more explicit about what could go wrong with this change in
> its current form: The return value of pipe_loader_probe() is the number
> of devices available on the system, so basically the same race condition
> is still possible if 'n' increases from the first call to the second,
> and in that case you'll end up reading uninitialized memory past the end
> of the array -- Worse than a null dereference. Range loops make this
> kind of situation impossible.
>
If I understood you correctly, you're saying that on the second call
can return count greater than the initial one.
Errm... this looks like a bug in the pipe-loader. I don't think it's
normal to claim that X devices are available only to return
information for fewer devices. OK, calling it a bug might not be
politically correct, although it is a misleading design decision, at
the very least.
-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
Francisco Jerez writes:
> Tom Stellard 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:
>> ---
>> 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 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.
>
Let me be more explicit about what could go wrong with this change in
its current form: The return value of pipe_loader_probe() is the number
of devices available on the system, so basically the same race condition
is still possible if 'n' increases from the first call to the second,
and in that case you'll end up reading uninitialized memory past the end
of the array -- Worse than a null dereference. Range loops make this
kind of situation impossible.
>>try {
>> devs.push_back(create(*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
Re: [Mesa-dev] [Mesa-stable] [PATCH] clover: Check the return value of pipe_loader_probe() when probing for devices
Tom Stellard 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:
> ---
> 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 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(*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
