Re: [Mesa-dev] [PATCH mesa] dri: use a supported API in driCreateNewContext

2018-02-15 Thread Ilia Mirkin 
On Thu, Feb 15, 2018 at 8:10 AM, Emil Velikov  wrote:
> On 15 February 2018 at 09:51, Eric Engestrom  
> wrote:
>> On Wednesday, 2018-02-14 09:06:41 -0500, Ilia Mirkin wrote:
>>> On Feb 14, 2018 7:38 AM, "Eric Engestrom"  wrote:
>>>
>>> From: Brendan King 
>>>
>>> Don't assume the screen supports OpenGL when creating a new context,
>>> use an API that the screen supports.
>>>
>>> Signed-off-by: Brendan King 
>>> Reviewed-by: Eric Engestrom 
>>> [Eric: rebased on current git HEAD]
>>> Signed-off-by: Eric Engestrom 
>>> ---
>>>  src/mesa/drivers/dri/common/dri_util.c | 14 +-
>>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/common/dri_util.c
>>> b/src/mesa/drivers/dri/common/dri_util.c
>>> index e6a7d2391a78c45d45a1..3f32b34132e75228e0e0 100644
>>> --- a/src/mesa/drivers/dri/common/dri_util.c
>>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>>> @@ -49,6 +49,7 @@
>>>  #include "main/debug_output.h"
>>>  #include "main/errors.h"
>>>  #include "main/macros.h"
>>> +#include "util/bitscan.h"
>>>
>>>  const char __dri2ConfigOptions[] =
>>> DRI_CONF_BEGIN
>>> @@ -325,7 +326,11 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
>>> mesa_api = API_OPENGLES;
>>> break;
>>>  case __DRI_API_GLES2:
>>> +   ctx_config.major_version = 2;
>>> +   mesa_api = API_OPENGLES2;
>>> +   break;
>>>  case __DRI_API_GLES3:
>>> +   ctx_config.major_version = 3;
>>>
>>>
>>> Are you sure about this change? Doesn't seem related, and I'm about 20%
>>> sure there was some reason for the current thing.
>>
>> I take the point that these are two separate bugs, and I'll split the
>> change and re-send, but I don't see the "reason for the current thing".
>> I'm betting on the 80% :P
>>
>> Without this `major_version` being set, validate_context_version()
>> further down could accept a gles3 context when only gles2 is supported,
>> because `major_version` would be 1 for both and never `>2` or `>3`.
>>
>> This is normally be hidden by the fact an attribute list with
>> __DRI_CTX_ATTRIB_MAJOR_VERSION would be passed, but
>> driCreateNewContextForAPI() doesn't pass an attribute list, which was in
>> turn hidden by driCreateNewContext() always requesting OPENGL, for which
>> 1.0 is valid, if you support OpenGL (which a lot of dri drivers do, but
>> we don't anymore).
>>
> Not quite sure about the version thingy, bth. IIRC the spec says that
> you can get higher version, as long as it's compatible.
> Thus using 2 for GLES2 doesn't sound right. I think this is simply
> masking an existing bug coming from the second hunk below.
>
> If we do need something like this, it should be handled right next to
> were we handle the desktop GL case.

As I recall, this code is a lot more subtle than it looks, and I'm
fairly sure correct as-is. Please ensure that you understand how it's
getting called before trying to change it. (e.g. I don't think
__DRI_API_GLES3 is used today, and it's a compat holdover, which we
decided should go to the same handling as GLES2.) All this stuff needs
to get checked *very* carefully.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa] dri: use a supported API in driCreateNewContext

2018-02-15 Thread Emil Velikov 
On 15 February 2018 at 09:51, Eric Engestrom  wrote:
> On Wednesday, 2018-02-14 09:06:41 -0500, Ilia Mirkin wrote:
>> On Feb 14, 2018 7:38 AM, "Eric Engestrom"  wrote:
>>
>> From: Brendan King 
>>
>> Don't assume the screen supports OpenGL when creating a new context,
>> use an API that the screen supports.
>>
>> Signed-off-by: Brendan King 
>> Reviewed-by: Eric Engestrom 
>> [Eric: rebased on current git HEAD]
>> Signed-off-by: Eric Engestrom 
>> ---
>>  src/mesa/drivers/dri/common/dri_util.c | 14 +-
>>  1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/drivers/dri/common/dri_util.c
>> b/src/mesa/drivers/dri/common/dri_util.c
>> index e6a7d2391a78c45d45a1..3f32b34132e75228e0e0 100644
>> --- a/src/mesa/drivers/dri/common/dri_util.c
>> +++ b/src/mesa/drivers/dri/common/dri_util.c
>> @@ -49,6 +49,7 @@
>>  #include "main/debug_output.h"
>>  #include "main/errors.h"
>>  #include "main/macros.h"
>> +#include "util/bitscan.h"
>>
>>  const char __dri2ConfigOptions[] =
>> DRI_CONF_BEGIN
>> @@ -325,7 +326,11 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
>> mesa_api = API_OPENGLES;
>> break;
>>  case __DRI_API_GLES2:
>> +   ctx_config.major_version = 2;
>> +   mesa_api = API_OPENGLES2;
>> +   break;
>>  case __DRI_API_GLES3:
>> +   ctx_config.major_version = 3;
>>
>>
>> Are you sure about this change? Doesn't seem related, and I'm about 20%
>> sure there was some reason for the current thing.
>
> I take the point that these are two separate bugs, and I'll split the
> change and re-send, but I don't see the "reason for the current thing".
> I'm betting on the 80% :P
>
> Without this `major_version` being set, validate_context_version()
> further down could accept a gles3 context when only gles2 is supported,
> because `major_version` would be 1 for both and never `>2` or `>3`.
>
> This is normally be hidden by the fact an attribute list with
> __DRI_CTX_ATTRIB_MAJOR_VERSION would be passed, but
> driCreateNewContextForAPI() doesn't pass an attribute list, which was in
> turn hidden by driCreateNewContext() always requesting OPENGL, for which
> 1.0 is valid, if you support OpenGL (which a lot of dri drivers do, but
> we don't anymore).
>
Not quite sure about the version thingy, bth. IIRC the spec says that
you can get higher version, as long as it's compatible.
Thus using 2 for GLES2 doesn't sound right. I think this is simply
masking an existing bug coming from the second hunk below.

If we do need something like this, it should be handled right next to
were we handle the desktop GL case.

>>
>> mesa_api = API_OPENGLES2;
>> break;
>>  case __DRI_API_OPENGL_CORE:
>> @@ -514,7 +519,14 @@ static __DRIcontext *
>>  driCreateNewContext(__DRIscreen *screen, const __DRIconfig *config,
>>  __DRIcontext *shared, void *data)
>>  {
>> -return driCreateNewContextForAPI(screen, __DRI_API_OPENGL,
>> +int apifs;
>> +
>> +apifs = ffs(screen->api_mask);
>> +
>> +if (!apifs)
>> +return NULL;
>> +
>> +return driCreateNewContextForAPI(screen, apifs - 1,
>>   config, shared, data);

This doesn't seem right:
 - the __DRI_API_GL* to API_OPENGL* mapping is not linear
 - this ends up picking an fairly "arbitrary" API

Instead, I would address the callers to feed the correct API.
From a quick look you're getting hit by either the gbm or the loader one?

Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa] dri: use a supported API in driCreateNewContext

2018-02-15 Thread Eric Engestrom 
On Wednesday, 2018-02-14 09:06:41 -0500, Ilia Mirkin wrote:
> On Feb 14, 2018 7:38 AM, "Eric Engestrom"  wrote:
> 
> From: Brendan King 
> 
> Don't assume the screen supports OpenGL when creating a new context,
> use an API that the screen supports.
> 
> Signed-off-by: Brendan King 
> Reviewed-by: Eric Engestrom 
> [Eric: rebased on current git HEAD]
> Signed-off-by: Eric Engestrom 
> ---
>  src/mesa/drivers/dri/common/dri_util.c | 14 +-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/common/dri_util.c
> b/src/mesa/drivers/dri/common/dri_util.c
> index e6a7d2391a78c45d45a1..3f32b34132e75228e0e0 100644
> --- a/src/mesa/drivers/dri/common/dri_util.c
> +++ b/src/mesa/drivers/dri/common/dri_util.c
> @@ -49,6 +49,7 @@
>  #include "main/debug_output.h"
>  #include "main/errors.h"
>  #include "main/macros.h"
> +#include "util/bitscan.h"
> 
>  const char __dri2ConfigOptions[] =
> DRI_CONF_BEGIN
> @@ -325,7 +326,11 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
> mesa_api = API_OPENGLES;
> break;
>  case __DRI_API_GLES2:
> +   ctx_config.major_version = 2;
> +   mesa_api = API_OPENGLES2;
> +   break;
>  case __DRI_API_GLES3:
> +   ctx_config.major_version = 3;
> 
> 
> Are you sure about this change? Doesn't seem related, and I'm about 20%
> sure there was some reason for the current thing.

I take the point that these are two separate bugs, and I'll split the
change and re-send, but I don't see the "reason for the current thing".
I'm betting on the 80% :P

Without this `major_version` being set, validate_context_version()
further down could accept a gles3 context when only gles2 is supported,
because `major_version` would be 1 for both and never `>2` or `>3`.

This is normally be hidden by the fact an attribute list with
__DRI_CTX_ATTRIB_MAJOR_VERSION would be passed, but
driCreateNewContextForAPI() doesn't pass an attribute list, which was in
turn hidden by driCreateNewContext() always requesting OPENGL, for which
1.0 is valid, if you support OpenGL (which a lot of dri drivers do, but
we don't anymore).

> 
> mesa_api = API_OPENGLES2;
> break;
>  case __DRI_API_OPENGL_CORE:
> @@ -514,7 +519,14 @@ static __DRIcontext *
>  driCreateNewContext(__DRIscreen *screen, const __DRIconfig *config,
>  __DRIcontext *shared, void *data)
>  {
> -return driCreateNewContextForAPI(screen, __DRI_API_OPENGL,
> +int apifs;
> +
> +apifs = ffs(screen->api_mask);
> +
> +if (!apifs)
> +return NULL;
> +
> +return driCreateNewContextForAPI(screen, apifs - 1,
>   config, shared, data);
>  }
> 
> --
> Cheers,
>   Eric
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH mesa] dri: use a supported API in driCreateNewContext

2018-02-14 Thread Ilia Mirkin 
On Feb 14, 2018 7:38 AM, "Eric Engestrom"  wrote:

From: Brendan King 

Don't assume the screen supports OpenGL when creating a new context,
use an API that the screen supports.

Signed-off-by: Brendan King 
Reviewed-by: Eric Engestrom 
[Eric: rebased on current git HEAD]
Signed-off-by: Eric Engestrom 
---
 src/mesa/drivers/dri/common/dri_util.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/common/dri_util.c
b/src/mesa/drivers/dri/common/dri_util.c
index e6a7d2391a78c45d45a1..3f32b34132e75228e0e0 100644
--- a/src/mesa/drivers/dri/common/dri_util.c
+++ b/src/mesa/drivers/dri/common/dri_util.c
@@ -49,6 +49,7 @@
 #include "main/debug_output.h"
 #include "main/errors.h"
 #include "main/macros.h"
+#include "util/bitscan.h"

 const char __dri2ConfigOptions[] =
DRI_CONF_BEGIN
@@ -325,7 +326,11 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
mesa_api = API_OPENGLES;
break;
 case __DRI_API_GLES2:
+   ctx_config.major_version = 2;
+   mesa_api = API_OPENGLES2;
+   break;
 case __DRI_API_GLES3:
+   ctx_config.major_version = 3;


Are you sure about this change? Doesn't seem related, and I'm about 20%
sure there was some reason for the current thing.

mesa_api = API_OPENGLES2;
break;
 case __DRI_API_OPENGL_CORE:
@@ -514,7 +519,14 @@ static __DRIcontext *
 driCreateNewContext(__DRIscreen *screen, const __DRIconfig *config,
 __DRIcontext *shared, void *data)
 {
-return driCreateNewContextForAPI(screen, __DRI_API_OPENGL,
+int apifs;
+
+apifs = ffs(screen->api_mask);
+
+if (!apifs)
+return NULL;
+
+return driCreateNewContextForAPI(screen, apifs - 1,
  config, shared, data);
 }

--
Cheers,
  Eric

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH mesa] dri: use a supported API in driCreateNewContext

2018-02-14 Thread Eric Engestrom 
From: Brendan King 

Don't assume the screen supports OpenGL when creating a new context,
use an API that the screen supports.

Signed-off-by: Brendan King 
Reviewed-by: Eric Engestrom 
[Eric: rebased on current git HEAD]
Signed-off-by: Eric Engestrom 
---
 src/mesa/drivers/dri/common/dri_util.c | 14 +-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/common/dri_util.c 
b/src/mesa/drivers/dri/common/dri_util.c
index e6a7d2391a78c45d45a1..3f32b34132e75228e0e0 100644
--- a/src/mesa/drivers/dri/common/dri_util.c
+++ b/src/mesa/drivers/dri/common/dri_util.c
@@ -49,6 +49,7 @@
 #include "main/debug_output.h"
 #include "main/errors.h"
 #include "main/macros.h"
+#include "util/bitscan.h"
 
 const char __dri2ConfigOptions[] =
DRI_CONF_BEGIN
@@ -325,7 +326,11 @@ driCreateContextAttribs(__DRIscreen *screen, int api,
mesa_api = API_OPENGLES;
break;
 case __DRI_API_GLES2:
+   ctx_config.major_version = 2;
+   mesa_api = API_OPENGLES2;
+   break;
 case __DRI_API_GLES3:
+   ctx_config.major_version = 3;
mesa_api = API_OPENGLES2;
break;
 case __DRI_API_OPENGL_CORE:
@@ -514,7 +519,14 @@ static __DRIcontext *
 driCreateNewContext(__DRIscreen *screen, const __DRIconfig *config,
 __DRIcontext *shared, void *data)
 {
-return driCreateNewContextForAPI(screen, __DRI_API_OPENGL,
+int apifs;
+
+apifs = ffs(screen->api_mask);
+
+if (!apifs)
+return NULL;
+
+return driCreateNewContextForAPI(screen, apifs - 1,
  config, shared, data);
 }
 
-- 
Cheers,
  Eric

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev