[Mesa-dev] [PATCH 2/3] mesa: build up a mask of active shader stages in a pipeline

[Tapani Pälli]

On 12/08/2015 06:55 AM, Timothy Arceri wrote:
> On Tue, 2015-12-08 at 15:21 +1100, Timothy Arceri wrote:
>> On Tue, 2015-12-08 at 10:55 +1100, Timothy Arceri wrote:
>>> On Mon, 2015-12-07 at 11:29 +0200, Tapani Pälli wrote:
>>>> This will be used for validating SSO pipeline where all active
>>>> stages
>>>> in linked programs should be in use when rendering.
>>>>
>>>> Signed-off-by: Tapani Pälli <tapani.palli at intel.com>
>>>> ---
>>>>   src/mesa/main/mtypes.h      |  2 ++
>>>>   src/mesa/main/pipelineobj.c | 39
>>>> +++++++++++++++++++++++++++++++++++++++
>>>>   2 files changed, 41 insertions(+)
>>>>
>>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>>> index fa7ead0..d5a22c9 100644
>>>> --- a/src/mesa/main/mtypes.h
>>>> +++ b/src/mesa/main/mtypes.h
>>>> @@ -2824,6 +2824,8 @@ struct gl_pipeline_object
>>>>      GLboolean Validated;                 /**< Pipeline Validation
>>>> status */
>>>>   
>>>>      GLchar *InfoLog;
>>>> +
>>>> +   uint8_t ActiveStages;                /**< Stages used,
>>>> (glUseProgramStages) */
>>>>   };
>>>>   
>>>>   /**
>>>> diff --git a/src/mesa/main/pipelineobj.c
>>>> b/src/mesa/main/pipelineobj.c
>>>> index 6710d0d..c510ee8 100644
>>>> --- a/src/mesa/main/pipelineobj.c
>>>> +++ b/src/mesa/main/pipelineobj.c
>>>> @@ -218,6 +218,43 @@ _mesa_reference_pipeline_object_(struct
>>>> gl_context *ctx,
>>>>      }
>>>>   }
>>>>   
>>>> +static GLenum
>>>> +shader_bit_from_shader_stage(gl_shader_stage stage)
>>>> +{
>>>> +   switch (stage) {
>>>> +   case MESA_SHADER_VERTEX:
>>>> +      return GL_VERTEX_SHADER_BIT;
>>>> +   case MESA_SHADER_FRAGMENT:
>>>> +      return GL_FRAGMENT_SHADER_BIT;
>>>> +   case MESA_SHADER_GEOMETRY:
>>>> +      return GL_GEOMETRY_SHADER_BIT;
>>>> +   case MESA_SHADER_TESS_CTRL:
>>>> +      return GL_TESS_CONTROL_SHADER_BIT;
>>>> +   case MESA_SHADER_TESS_EVAL:
>>>> +      return GL_TESS_EVALUATION_SHADER_BIT;
>>>> +   case MESA_SHADER_COMPUTE:
>>>> +      return GL_COMPUTE_SHADER_BIT;
>>>> +   default:
>>>> +      unreachable("bad value in
>>>> _mesa_shader_bit_from_shader_stage()");
>>>> +   }
>>>> +}
>>>> +
>>>> +static void
>>>> +update_active_pipeline_stages(struct gl_pipeline_object *pipe,
>>>> +                              struct gl_shader_program *shProg,
>>>> +                              GLbitfield stages)
>>>> +{
>>>> +   unsigned i;
>>>> +   for (i = 0; i < MESA_SHADER_STAGES; i++) {
>>>> +      if ((stages & shader_bit_from_shader_stage(i)) != 0) {
>>>> +         if (shProg && shProg->ActiveStages & (1 << i))
>>>> +            pipe->ActiveStages |= (1 << i);
>>>> +         else
>>>> +            pipe->ActiveStages &= ~(1 << i);
>>>> +      }
>>>> +   }
>>> I think you could do most of the validation at this point which
>>> would
>>> reduce the amount of work required during rendering. You also have
>>> forgotten about GL_ALL_SHADER_BITS.
>> No you didn't I was just looking at to code for to long :P However
>> unless I'm missing something I believe my other comments are still
>> valid.
>>
>>> For example could you do something like this?
>>>
>>> Note: You would need to call this function after the
>>>   _mesa_use_shader_program() calls.
>>>
>>> unsigned i;
>>> unsigned j;
>>>
>>> for (i = 0; i < MESA_SHADER_STAGES; i++) {
>>>     if (!pipe->CurrentProgram[i]) {
>>>        struct gl_shader_program *shProg = pipe->CurrentProgram[i];
>>>        for (j = 0; j < MESA_SHADER_STAGES; j++) {
>>>           if (!shProg->_LinkedShaders[j])
>>>           continue;
>>>
>>>           /* Make sure that any shader stage is also used in the
>>> pipeline */
>>>           if (shProg != pipe->CurrentProgram[j]) {
>>>              pipe->ValidProgramUse = false;
>>>              return;
>>>           }
>>>        }
>>>     }
>>>
>>>     pipe->ValidProgramUse = true;
>>> }
> Thinking about this some more you would also need to revalidate if the
> program has been relinked. I guess ValidProgramUse could be stored for
> each program object rather than just for the pipe, then you could loop
> over the stages in patch 3 and only do the full validation check when
>   ValidProgramUse is false (you would reset to false on a relink).

Relink is handled by having ActiveStages mask in gl_shader_program (1st 
patch) that changes during linking and then this validation will fail.

There is also case where linking fails to program which is not set 
active, I have a patch for this that uses the mask also here, did not 
include it yet in the series since it was not quite ready at that time:

http://cgit.freedesktop.org/~tpalli/mesa/commit/?h=fix_sso&id=cffd08b93f56244ec7f7fbbca69d7f0d6d0a6243

> I'm also just about to add another comment to patch 3 as I think your c
> urrent patch will miss some use cases.
>
>>>> +}
>>>> +
>>>>   /**
>>>>    * Bound program to severals stages of the pipeline
>>>>    */
>>>> @@ -311,6 +348,8 @@ _mesa_UseProgramStages(GLuint pipeline,
>>>> GLbitfield stages, GLuint program)
>>>>         }
>>>>      }
>>>>   
>>>> +   update_active_pipeline_stages(pipe, shProg, stages);
>>>> +
>>>>      /* Enable individual stages from the program as requested by
>>>> the
>>>>       * application.  If there is no shader for a requested stage
>>>> in
>>>> the
>>>>       * program, _mesa_use_shader_program will enable fixed
>>>> -function
>>>> processing
>>> _______________________________________________
>>> mesa-dev mailing list
>>> mesa-dev at lists.freedesktop.org
>>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev