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

[Timothy Arceri]

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.

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