[Mesa-dev] [PATCH] i965: Perform program state upload outside of atom handling

[Kristian Høgsberg]

On Mon, Feb 23, 2015 at 1:23 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Monday, February 23, 2015 11:29:47 AM Carl Worth wrote:
>> Across the board of the various generations, the intial few atoms in
>> all of the atom lists are basically the same, (performing uploads for
>> the various programs). The only difference is that prior to gen6
>> there's an ff_gs upload in place of the later gs upload.
>>
>> In this commit, instead of using the atom lists for this program state
>> upload, we add a new function brw_upload_programs that calls into the
>> per-stage upload functions which in turn check dirty bits and return
>> immediately if nothing needs to be done.
>>
>> This commit is intended to have no functional change. The motivation
>> is that future code, (such as the shader cache), wants to have a
>> single function within which to perform various operations before and
>> after program upload, (with some local variables holding state across
>> the upload).
>>
>> It may be worth looking at whether some of the other functionality
>> currently handled via atoms might also be more cleanly handled in a
>> similar fashion.
>> ---
>>
>> In my second revision, I wrote:
>>
>> > I'm still not a huge fan of the resulting conditional expressions,
>> > (and the number of parentheses involved in them). It would not be too
>> > hard to imagine one of those getting edited incorrectly.
>>
>> Ken had the good idea to improve the readability/maintainability of
>> these conditions by adding a new brw_state_dirty function which
>> performs the masking and checks that the result is non-zero.
>>
>> So this third revision of the patch uses that approach.
>>
>>  src/mesa/drivers/dri/i965/brw_ff_gs.c        | 22 ++++++-------
>>  src/mesa/drivers/dri/i965/brw_ff_gs.h        |  3 ++
>>  src/mesa/drivers/dri/i965/brw_gs.c           | 24 ++++++---------
>>  src/mesa/drivers/dri/i965/brw_gs.h           |  5 +++
>>  src/mesa/drivers/dri/i965/brw_state.h        |  7 +++++
>>  src/mesa/drivers/dri/i965/brw_state_upload.c | 35 +++++++++++----------
>>  src/mesa/drivers/dri/i965/brw_vs.c           | 33 +++++++++-----------
>>  src/mesa/drivers/dri/i965/brw_vs.h           |  3 ++
>>  src/mesa/drivers/dri/i965/brw_wm.c           | 46 +++++++++++++---------------
>>  src/mesa/drivers/dri/i965/brw_wm.h           |  3 ++
>>  10 files changed, 97 insertions(+), 84 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_ff_gs.c b/src/mesa/drivers/dri/i965/brw_ff_gs.c
>> index 653c4b6..a5ef1ae 100644
>> --- a/src/mesa/drivers/dri/i965/brw_ff_gs.c
>> +++ b/src/mesa/drivers/dri/i965/brw_ff_gs.c
>> @@ -221,10 +221,20 @@ static void populate_key(struct brw_context *brw,
>>
>>  /* Calculate interpolants for triangle and line rasterization.
>>   */
>> -static void
>> +void
>>  brw_upload_ff_gs_prog(struct brw_context *brw)
>>  {
>>     struct brw_ff_gs_prog_key key;
>> +
>> +   if (! brw_state_dirty(brw,
>
>          ^^^ no space here
>
>> +                      (_NEW_LIGHT),
>
> I'm not really a fan of the extra parenthesis, and I recently removed
> them all from the brw_tracked_state_atoms...but I suppose it's a matter
> of taste.
>
>> +                      (BRW_NEW_PRIMITIVE |
>> +                       BRW_NEW_TRANSFORM_FEEDBACK |
>> +                       BRW_NEW_VS_PROG_DATA)))
>> +   {
>
> We don't generally put { on its own line - though, I do agree, that
> makes it a lot more readable here.
>
>> +      return;
>> +   }
>
> I would probably write this as:
>
> if (!brw_state_dirty(brw,
>                      _NEW_LIGHT,
>                      BRW_NEW_PRIMITIVE |
>                      BRW_NEW_TRANSFORM_FEEDBACK |
>                      BRW_NEW_VS_PROG_DATA))
>    return;
>
> but I suppose that's a matter of preference.  Same comments elsewhere.
>
>> +
>>     /* Populate the key:
>>      */
>>     populate_key(brw, &key);
> [snip]
>> diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h
>> index f195407..c25b0d5 100644
>> --- a/src/mesa/drivers/dri/i965/brw_state.h
>> +++ b/src/mesa/drivers/dri/i965/brw_state.h
>> @@ -152,6 +152,13 @@ extern const struct brw_tracked_state gen8_vertices;
>>  extern const struct brw_tracked_state gen8_vf_topology;
>>  extern const struct brw_tracked_state gen8_vs_state;
>>
>> +static inline bool
>> +brw_state_dirty(struct brw_context *brw, GLuint mesa_flags, uint64_t brw_flags)
>> +{
>> +     return (((brw->state.dirty.mesa & mesa_flags) |
>
> ^^^ return should have a 3-space indent.  There's also an extra set of
> parenthesis here.
>
>> +              (brw->state.dirty.brw & brw_flags)) != 0);
>
> Perhaps:
>
>    return ((brw->state.dirty.mesa & mesa_flags) |
>            (brw->state.dirty.brw & brw_flags)) != 0;
>
> FWIW, The bool return will also implicitly do != 0 for you (thanks to
> C99's stdbool.h), but it's also reasonable to leave it.
>
>> +}
>> +
>>  /* brw_misc_state.c */
>>  void brw_upload_invariant_state(struct brw_context *brw);
>>  uint32_t
>
> This looks good to me.  Thanks, Carl!
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>

Ken already mentioned all the stylistic issues I saw, so I'll just add my

Reviewed-by: Kristian Høgsberg <krh at bitplanet.net>