[Mesa-dev] [PATCH 3/3] mesa: return initial value for VALIDATE_STATUS if pipe not bound

[Tapani Pälli]

On 11/22/2015 01:25 PM, Timothy Arceri wrote:
> On Sun, 2015-11-22 at 11:00 +0100, gregory hainaut wrote:
>> On Sun, 22 Nov 2015 14:25:31 +1100
>> Timothy Arceri <t_arceri at yahoo.com.au> wrote:
>>
>>> On Tue, 2015-09-01 at 13:53 +0300, Tapani Pälli wrote:
>>>>  From OpenGL 4.5 Core spec (7.13):
>>>>
>>>>      "If pipeline is a name that has been generated (without
>>>> subsequent
>>>>      deletion) by GenProgramPipelines, but refers to a program
>>>> pipeline
>>>>      object that has not been previously bound, the GL first
>>>> creates
>>>> a new state vector in the same manner as when BindProgramPipeline
>>>>      creates a new program pipeline object."
>>>>
>>>> I interpret this as "If GetProgramPipelineiv gets called without
>>>> a
>>>> bound (but valid) pipeline object, the state should reflect
>>>> initial
>>>> state of a new pipeline object." This is also expected behaviour
>>>> by
>>>> ES31-CTS.sepshaderobjs.PipelineApi conformance test.
>>>>
>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>> ---
>>>>   src/mesa/main/pipelineobj.c | 3 ++-
>>>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/mesa/main/pipelineobj.c
>>>> b/src/mesa/main/pipelineobj.c
>>>> index 07acbf1..c2e1d29 100644
>>>> --- a/src/mesa/main/pipelineobj.c
>>>> +++ b/src/mesa/main/pipelineobj.c
>>>> @@ -614,7 +614,8 @@ _mesa_GetProgramPipelineiv(GLuint pipeline,
>>>> GLenum pname, GLint *params)
>>>>         *params = pipe->InfoLog ? strlen(pipe->InfoLog) + 1 : 0;
>>>>         return;
>>>>      case GL_VALIDATE_STATUS:
>>>> -      *params = pipe->Validated;
>>>> +      /* If pipeline is not bound, return initial value 0. */
>>>> +      *params = (ctx->_Shader->Name != pipe->Name)
>>>> ? 0 : pipe->Validated;
>>>
>>> Hi Tapani,
>>>
>>> As Gregory has pointed out this change causes a large number of the
>>> SSO deqp tests to fail.
>>>
>>> I'm not sure what the solution is yet but one thing I have noticed
>>> is
>>> that with this change we will always return a value of 0 as nothing
>>> ever sets the value of ctx->_Shader->Name.
>>>
>>> I did try setting it when the bind is done but the deqp tests still
>>> fail as the bind is done after the validate in those tests. Are you
>>> sure that this must return 0 if the pipe is not bound?
>>>
>>> Tim
>>>
>>>>         return;
>>>>      case GL_VERTEX_SHADER:
>>>>         *params = pipe->CurrentProgram[MESA_SHADER_VERTEX]
>>
>> Hello,
>>
>> Here my understanding of the specification (with my point of view of
>> GL
>> users).
>>
>> On various objects, gen* cmds reserve a key in a hash table but
>> objects
>> aren't created. Generally the object are created when they're bound
>> (often call bind to
>> create). However SSO API is (closer of the/a) direct state access
>> API. It is really
>> awkward to bind the object to modify it. Besides there is now full
>> DSA.
>>
>> So for me, the following pipeline command must created the object if
>> it
>> wasn't created (read if it wasn't bound)
>>
>> 1/ BindProgramPipeline
>> 2/ UseProgramStages
>> 3/ CreateProgramPipelines (???: not sure, but likely)
>> 4/ ActiveShaderProgram
>> 5/ GetProgramPipelineiv
>> 6/ ValidateProgramPipeline
>>
>> Note: can be read as all pipeline commands, so it is nearly
>> equivalent to
>> create the object in GenProgramPipelines. The only exception is
>> IsProgramPipeline.
>>
>>
>> So GetProgramPipelineiv (and others gl cmd) must start with:
>> if (! pipe->IsCreated ) {
>>      pipe->Init();
>>      pipe->IsCreated = true;
>> }
>>
>>
>> For example:
>> GenProgramPipelines(1, &pipe);
>> GetProgramPipelineiv(pipe, X, X); // Must create the object and
>> therefore will return the initial value (0)
>>
>> GenProgramPipelines(1, &pipe2);
>> ValidateProgramPipeline(pipe2); // Will create the object
>> GetProgramPipelineiv(pipe2, X, X); // object was created by
>> ValidateProgramPipeline, we can return the property
>>
>> As a side note, if Mesa is updated. Piglit test will need some
>> updates too.
>>
>> Best regards,
>> Gregory
>
> I did some digging in the khronos private bugzilla and it seems this
> issue was resolved and the spec updated to include the following
> failure case to validation:
>
> "There is no current program object specified byUseProgram, there is
> acurrent program pipeline object, and that object is empty (no
> executable code is installed for any stage)."
>
> So it seems the CTS is correct and the DEQP tests are wrong.
>
> However as I pointed out I think this patch is still broken as _Shader
> ->Name is always 0 currently as far as I could tell.

IMO the check done by this patch is valid, Name field has the name of 
currently bound program pipeline, it changes when current pipeline 
object gets changed. If name does not match the pipeline given to 
GetProgramPipelineiv it means it is not bound. If you call 
BindProgramPipeline, Name changes.

> Does anyone know if its possible to report bugs against DEQP? I
> couldn't find anywhere to do it.

Don't know about this one, it's part of Android so I guess the process 
is just sending changes through their patch submission process (?)

// Tapani