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

[Tapani Pälli]

On 11/23/2015 10:18 AM, Timothy Arceri wrote:
> On Mon, 2015-11-23 at 08:28 +0200, Tapani Pälli wrote:
>>
>> 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.
>
> Right thanks, I wasn't saying it wasn't correct to do the check just
> that I couldn't see where name was set. I see how it works now, thanks.
>
> Not a big deal but the check should probably be moved into the validate
> function as it is one of the validation rules.

There is check in validation if pipeline is bound because validation can 
be also run against a pipeline that is not bound. In this case there is 
some difference in error reporting compared to running validation when 
pipeline is bound.

"If validation succeeded, no INVALID_OPERATION validation error is 
generated if pipeline is bound and no program is made current via 
UseProgram, given the current state. If validation failed, such errors 
are generated under the current state."

So error reporting is only happening if the pipeline is bound, otherwise 
it should just return validation result for that pipeline.

>>
>>> 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