[Mesa-dev] [PATCH 6/9] mesa/es3.1: Allow Multisampled FrameBufferTextures

[Ian Romanick]

On 05/11/2015 01:43 PM, Ilia Mirkin wrote:
> On Mon, May 11, 2015 at 4:34 PM, Ian Romanick <idr at freedesktop.org> wrote:
>> On 05/11/2015 07:57 AM, Ilia Mirkin wrote:
>>> On Mon, May 11, 2015 at 10:08 AM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>>>> On Mon, May 11, 2015 at 3:03 PM, Marta Lofstedt
>>>> <marta.lofstedt at linux.intel.com> wrote:
>>>>> From: Marta Lofstedt <marta.lofstedt at intel.com>
>>>>>
>>>>> GLES 3.1 must be allowed to use multisampled
>>>>> frambuffer textures.
>>>>>
>>>>> Signed-off-by: Marta Lofstedt <marta.lofstedt at intel.com>
>>>>> ---
>>>>>  src/mesa/main/fbobject.c | 5 +++--
>>>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/src/mesa/main/fbobject.c b/src/mesa/main/fbobject.c
>>>>> index 27cf97f..14a015e 100644
>>>>> --- a/src/mesa/main/fbobject.c
>>>>> +++ b/src/mesa/main/fbobject.c
>>>>> @@ -2756,8 +2756,9 @@ _mesa_FramebufferTexture2D(GLenum target, GLenum attachment,
>>>>>           break;
>>>>>        case GL_TEXTURE_2D_MULTISAMPLE:
>>>>>        case GL_TEXTURE_2D_MULTISAMPLE_ARRAY:
>>>>> -         error = _mesa_is_gles(ctx)
>>>>> -            || !ctx->Extensions.ARB_texture_multisample;
>>>>> +         error = (_mesa_is_gles(ctx)
>>>>> +            || !ctx->Extensions.ARB_texture_multisample) &&
>>>>> +            !_mesa_is_gles31(ctx);
>>>
>>> This seems correct. error = true when old condition, but not when
>>> es3.1 even if the old condition holds true. If the old condition is
>>> false, then the new addition doesn't matter.
>>>
>>> Personally I would have written this as
>>>
>>> error = _mesa_is_gles(ctx) && !mesa_is_gles31(ctx) ||
>>> !ctx->Extensions.ARB_texture_multisample;
>>>
>>> The nice thing about this is that it will force an error even in the
>>> very hypothetical situation where a driver doesn't expose
>>> ARB_texture_multisample, but a GLES3.1 context was created (e.g. via
>>> an override flag).
>>
>> I think this is a mis-feature.  Most of the ARB extensions are supersets
>> of the ES3.1 functionality, so we could someday have a driver that does
>> one but not the other.
> 
> OK, well, I won't fight this further -- I think I've made my point,
> and I believe you've understood it. Since you disagree, happy to let
> it go.
> 
> I can't help but add though that in the situation where the ARB
> extension is a superset in terms of actual backend driver
> functionality, not just small frontend differences (e.g.
> ARB_gpu_shader5), we should add more enables that allow the (in this
> case) ES3.1 functionality. This enables a driver to be developed over
> time and lets people use version overrides without resulting in
> crashes or other driver weirdness.

Perhaps.  Similar situations have been handled in a couple different
ways in the past.  Sometimes we'll add a more complex function to
determine whether something is enabled (e.g.,
_mesa_has_geometry_shaders).  Sometimes a driver will on enable a
certain extension for a certain kind of context (e.g., ARB_base_instance
in the i965 driver).  And there are other cases where we have multiple
extension bits (e.g., OES_texture_float).

>   -ilia