[Mesa-dev] [PATCH 4/4] program: remove _mesa_init_*_program wrappers

[Emil Velikov]

On 9 October 2015 at 17:55, Marek Olšák <maraeo at gmail.com> wrote:
> On Thu, Oct 8, 2015 at 3:49 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 8 October 2015 at 01:12, Marek Olšák <maraeo at gmail.com> wrote:
>>> From: Marek Olšák <marek.olsak at amd.com>
>>>
>>> They didn't do anything useful.
>>> ---
>>>  src/mesa/drivers/dri/i915/i915_fragprog.c          |   7 +-
>>>  src/mesa/drivers/dri/i965/brw_program.c            |  10 +-
>>>  .../drivers/dri/i965/test_fs_cmod_propagation.cpp  |   2 +-
>>>  .../dri/i965/test_fs_saturate_propagation.cpp      |   2 +-
>>>  .../dri/i965/test_vec4_copy_propagation.cpp        |   2 +-
>>>  .../dri/i965/test_vec4_register_coalesce.cpp       |   2 +-
>>>  src/mesa/drivers/dri/r200/r200_vertprog.c          |   4 +-
>>>  src/mesa/program/program.c                         | 133 +++------------------
>>>  src/mesa/program/program.h                         |  29 +----
>>>  src/mesa/state_tracker/st_cb_program.c             |  43 +++----
>>>  10 files changed, 50 insertions(+), 184 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i915/i915_fragprog.c b/src/mesa/drivers/dri/i915/i915_fragprog.c
>>> index 1a5943c..237d219 100644
>>> --- a/src/mesa/drivers/dri/i915/i915_fragprog.c
>>> +++ b/src/mesa/drivers/dri/i915/i915_fragprog.c
>>> @@ -1316,8 +1316,8 @@ i915NewProgram(struct gl_context * ctx, GLenum target, GLuint id)
>>>  {
>>>     switch (target) {
>>>     case GL_VERTEX_PROGRAM_ARB:
>>> -      return _mesa_init_vertex_program(ctx, CALLOC_STRUCT(gl_vertex_program),
>>> -                                       target, id);
>>> +      return _mesa_init_gl_program(CALLOC_STRUCT(gl_vertex_program),
>>> +                                   target, id);
>>>
>> Worth doing the memory allocation to a temporary variable, and feeding
>> &prog->base to _mesa_init_gl_program ?
>
> I tried that first, but it required more lines of code.
>
The ones that you'll gain, will be balanced out by the other suggestions :-)

>>
>>> --- a/src/mesa/drivers/dri/r200/r200_vertprog.c
>>> +++ b/src/mesa/drivers/dri/r200/r200_vertprog.c
>>> @@ -1205,9 +1205,9 @@ r200NewProgram(struct gl_context *ctx, GLenum target, GLuint id)
>>>     switch(target){
>>>     case GL_VERTEX_PROGRAM_ARB:
>>>        vp = CALLOC_STRUCT(r200_vertex_program);
>>> -      return _mesa_init_vertex_program(ctx, &vp->mesa_program, target, id);
>>> +      return _mesa_init_gl_program(&vp->mesa_program, target, id);
>>>     case GL_FRAGMENT_PROGRAM_ARB:
>>> -      return _mesa_init_fragment_program( ctx, CALLOC_STRUCT(gl_fragment_program), target, id );
>>> +      return _mesa_init_gl_program(CALLOC_STRUCT(gl_fragment_program), target, id);
>> Ditto.
>>
>>
>>> --- a/src/mesa/program/program.c
>>> +++ b/src/mesa/program/program.c
>>
>>> @@ -309,34 +217,29 @@ _mesa_new_program(struct gl_context *ctx, GLenum target, GLuint id)
>>>     struct gl_program *prog;
>>>     switch (target) {
>>>     case GL_VERTEX_PROGRAM_ARB: /* == GL_VERTEX_PROGRAM_NV */
>>> -      prog = _mesa_init_vertex_program(ctx, CALLOC_STRUCT(gl_vertex_program),
>>> -                                       target, id );
>>> +      prog = _mesa_init_gl_program(CALLOC_STRUCT(gl_vertex_program),
>>> +                                   target, id);
>> Ditto. + early return similar to st/mesa ?
>>
>>> --- a/src/mesa/program/program.h
>>> +++ b/src/mesa/program/program.h
>>
>>> -extern struct gl_program *
>>> -_mesa_init_compute_program(struct gl_context *ctx,
>>> -                           struct gl_compute_program *prog,
>>> -                           GLenum target, GLuint id);
>>> +_mesa_init_gl_program(void *prog, GLenum target, GLuint id);
>>>
>> If you go for my suggestion you can keep prog as struct gl_program *.
>>
>>> --- a/src/mesa/state_tracker/st_cb_program.c
>>> +++ b/src/mesa/state_tracker/st_cb_program.c
>>
>>> +   switch (target) {
>>> +   case GL_VERTEX_PROGRAM_ARB:
>>> +      prog = (struct gl_program*)ST_CALLOC_STRUCT(st_vertex_program);
>>> +      break;
>>> +   case GL_FRAGMENT_PROGRAM_ARB:
>> Worth adding case GL_FRAGMENT_PROGRAM_NV, or perhaps nuking it from
>> mesa/program/program.c above ?
>
> I think GL_FRAGMENT_PROGRAM_NV is from an NV extension supported by
> some classic drivers but not st/mesa.
>
Seems to be part of GL_NV_fragment_program, which was removed a while ago with

commit 2f350f360b00e786e140d9ccb9db83bf23269d8f
Author: Kenneth Graunke <kenneth at whitecape.org>
Date:   Sun Oct 14 15:53:06 2012 -0700

   mesa: Remove get and enable bits for NV_fragment_program.

and a few other follow-up commits.

Seems like we still have a few remnants which I will clean-up after
this series lands.

>> Can we keep the variables as is and avoid the casts ?
>
> ST_CALLOC_STRUCT needs a cast.
> Or &ST_CALLOC_STRUCT(...)->Base.Base if I fix the macro to allow that.
>
I was thinking about retaining the original style/approach and not use
CALLOC_STRUCT(foo) as a function argument. Declaring a function
(_mesa_init_gl_program) argument as void * only to down and up cast
it, depending on the scenario, seems a bit ugly. If you don't mind I
can follow up and address these separately ?

-Emil