[Mesa-dev] [PATCH 4/8] mesa: Only validate shaders that can exist in the context
Ian Romanick
idr at freedesktop.org
Mon Dec 22 14:41:21 PST 2014
On 12/22/2014 11:03 AM, Ian Romanick wrote:
> On 12/21/2014 10:03 PM, Kenneth Graunke wrote:
>> On Friday, December 19, 2014 02:20:55 PM Ian Romanick wrote:
>>> From: Ian Romanick <ian.d.romanick at intel.com>
>>>
>>> On Bay Trail-D using Fedora 20 compile flags (-m64 -O2 -mtune=generic
>>> for 64-bit and -m32 -march=i686 -mtune=atom for 32-bit), affects
>>> Gl32Batch7:
>>>
>>> 32-bit: Difference at 95.0% confidence 0.495267% +/- 0.202063% (n=40)
>>> 64-bit: Difference at 95.0% confidence 3.57576% +/- 0.288175% (n=40)
>>>
>>> Signed-off-by: Ian Romanick <ian.d.romanick at intel.com>
>>
>> I'm guessing it would help legacy context programs even more.
>
> Dang it. That was actually a typo. Gl32Batch7 had no difference.
> These were the timings for Gl21Batch7. I'll fix that in the commit message.
>
>>> ---
>>> src/mesa/main/context.c | 78 +++++++++++++++++++++++++++++++------------------
>>> 1 file changed, 49 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/src/mesa/main/context.c b/src/mesa/main/context.c
>>> index 400c158..74f9004 100644
>>> --- a/src/mesa/main/context.c
>>> +++ b/src/mesa/main/context.c
>>> @@ -1903,49 +1903,69 @@ shader_linked_or_absent(struct gl_context *ctx,
>>> GLboolean
>>> _mesa_valid_to_render(struct gl_context *ctx, const char *where)
>>> {
>>> - bool from_glsl_shader[MESA_SHADER_COMPUTE] = { false };
>>> unsigned i;
>>>
>>> /* This depends on having up to date derived state (shaders) */
>>> if (ctx->NewState)
>>> _mesa_update_state(ctx);
>>>
>>> - for (i = 0; i < MESA_SHADER_COMPUTE; i++) {
>>> - if (!shader_linked_or_absent(ctx, ctx->_Shader->CurrentProgram[i],
>>> - &from_glsl_shader[i], where))
>>> - return GL_FALSE;
>>> - }
>>> + if (ctx->API == API_OPENGL_CORE || ctx->API == API_OPENGLES2) {
>>> + bool from_glsl_shader[MESA_SHADER_COMPUTE] = { false };
>>>
>>> - /* Any shader stages that are not supplied by the GLSL shader and have
>>> - * assembly shaders enabled must now be validated.
>>> - */
>>> - if (!from_glsl_shader[MESA_SHADER_VERTEX]
>>> - && ctx->VertexProgram.Enabled && !ctx->VertexProgram._Enabled) {
>>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>>> - "%s(vertex program not valid)", where);
>>> - return GL_FALSE;
>>> - }
>>> + for (i = 0; i < MESA_SHADER_COMPUTE; i++) {
>>> + if (!shader_linked_or_absent(ctx, ctx->_Shader->CurrentProgram[i],
>>> + &from_glsl_shader[i], where))
>>> + return GL_FALSE;
>>> + }
>>>
>>> - /* FINISHME: If GL_NV_geometry_program4 is ever supported, the current
>>> - * FINISHME: geometry program should validated here.
>>> - */
>>> - (void) from_glsl_shader[MESA_SHADER_GEOMETRY];
>>> + /* In OpenGL Core Profile and OpenGL ES 2.0 / 3.0, there are no assembly
>>> + * shaders. Don't check state related to those.
>>> + */
>>
>> FWIW, I don't think we actually enforce this restriction. Although we don't
>> advertise the extension, the API functions don't appear to contain any
>> API_OPENGL_CORE -> raise GL error code.
>
> I thought we had it configured so that those functions weren't even in
> the dispatch tables for core profile or ES. I can whip up a piglit test...
>
>> We might want to do that. In the unlikely event that there are applications
>> attempting to use ARB programs in core profile without checking for the
>> extension, landing those checks first would make debugging the problem easier:
>>
>> 1. broken app somehow works
>> 2. broken app starts returning INVALID_OPERATION
>> 3. code to support broken applications is removed.
>>
>> *shrug*. It probably doesn't actually matter.
>
> I wasn't too worried about it. Pretty much all applications that use
> the assembly shaders also use fixed-function state that doesn't exist in
> core profile. I think you'd really have to work to make an application
> that was both core profile and assembly shader.
Writing a piglit test that calls an unsupported function is hard. :( I
verified (by inspection) that _mesa_initialize_exec_tables only
initializes the dispatch table entry for glProgramStringARB when
ctx->API == API_OPENGL_COMPAT. Without that function, there are no
assembly shaders, so I think we're safe.
>>> + } else {
>>> + bool has_vertex_shader = false;
>>> + bool has_fragment_shader = false;
>>> +
>>> + /* In OpenGL Compatibility Profile, there is only vertex shader and
>>> + * fragment shader. We take this path also for API_OPENGLES because
>>
>> "there is only vertex shader" sounds a little odd. Perhaps "there are only
>> vertex and fragment shaders" or "there are only vertex and fragment shader stages"?
>>
>> Whatever you decide is fine.
>
> Yes... that is weird. I'll reword it.
I ended up with:
/* In OpenGL Compatibility Profile, there are only vertex shaders and
* fragment shaders (i.e., no geometry or tessellation shaders). We take
* this path also for API_OPENGLES because optimizing that path would
* make the other (more common) paths slightly slower.
*/
>>> + * optimizing that path would make the other (more common) paths
>>> + * slightly slower.
>>> + */
>>> + if (!shader_linked_or_absent(ctx,
>>> + ctx->_Shader->CurrentProgram[MESA_SHADER_VERTEX],
>>> + &has_vertex_shader, where))
>>> + return GL_FALSE;
>>>
>>> - if (!from_glsl_shader[MESA_SHADER_FRAGMENT]) {
>>> - if (ctx->FragmentProgram.Enabled && !ctx->FragmentProgram._Enabled) {
>>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>>> - "%s(fragment program not valid)", where);
>>> - return GL_FALSE;
>>> - }
>>> + if (!shader_linked_or_absent(ctx,
>>> + ctx->_Shader->CurrentProgram[MESA_SHADER_FRAGMENT],
>>> + &has_fragment_shader, where))
>>> + return GL_FALSE;
>>>
>>> - /* If drawing to integer-valued color buffers, there must be an
>>> - * active fragment shader (GL_EXT_texture_integer).
>>> + /* Any shader stages that are not supplied by the GLSL shader and have
>>> + * assembly shaders enabled must now be validated.
>>> */
>>> - if (ctx->DrawBuffer && ctx->DrawBuffer->_IntegerColor) {
>>> + if (!has_vertex_shader
>>> + && ctx->VertexProgram.Enabled && !ctx->VertexProgram._Enabled) {
>>> _mesa_error(ctx, GL_INVALID_OPERATION,
>>> - "%s(integer format but no fragment shader)", where);
>>> + "%s(vertex program not valid)", where);
>>> return GL_FALSE;
>>> }
>>> +
>>> + if (!has_fragment_shader) {
>>> + if (ctx->FragmentProgram.Enabled && !ctx->FragmentProgram._Enabled) {
>>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>>> + "%s(fragment program not valid)", where);
>>> + return GL_FALSE;
>>> + }
>>> +
>>> + /* If drawing to integer-valued color buffers, there must be an
>>> + * active fragment shader (GL_EXT_texture_integer).
>>> + */
>>> + if (ctx->DrawBuffer && ctx->DrawBuffer->_IntegerColor) {
>>> + _mesa_error(ctx, GL_INVALID_OPERATION,
>>> + "%s(integer format but no fragment shader)", where);
>>> + return GL_FALSE;
>>> + }
>>> + }
>>> }
>>>
>>> /* A pipeline object is bound */
>>
>> I like how this handles compatibily and core separately. It's much easier to
>> read that way. We might also be able to merge in some of check_valid_to_render
>> someday...
>>
>> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20141222/6dd3dca1/attachment-0001.sig>
More information about the mesa-dev
mailing list