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

[Ilia Mirkin]

On Thu, Feb 15, 2018 at 8:10 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
> On 15 February 2018 at 09:51, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
>> On Wednesday, 2018-02-14 09:06:41 -0500, Ilia Mirkin wrote:
>>> On Feb 14, 2018 7:38 AM, "Eric Engestrom" <eric.engestrom at imgtec.com> wrote:
>>>
>>> From: Brendan King <Brendan.King at imgtec.com>
>>>
>>> Don't assume the screen supports OpenGL when creating a new context,
>>> use an API that the screen supports.
>>>
>>> Signed-off-by: Brendan King <Brendan.King at imgtec.com>
>>> Reviewed-by: Eric Engestrom <eric.engestrom at imgtec.com>
>>> [Eric: rebased on current git HEAD]
>>> Signed-off-by: Eric Engestrom <eric.engestrom at imgtec.com>
>>> ---
>>>  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.