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

[Kenneth Graunke]

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>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20150223/49b58102/attachment.sig>