[Mesa-dev] [RFC][PATCH 00/11] gallium: Basic compute infrastructure.

[Jose Fonseca]

Francisco,

Sorry for the delay reviewing this, but I  haven't been able to dedicate some time until now.

Overall, it's a great piece of work! Just a few relatively minor comments/requests.

----- Original Message -----
> This patch series is part of the ongoing work to put together a
> compute stack on top of Gallium3D.  What we have been doing until now
> to that end could be divided in 6 building blocks:
> 
> 1/ Gallium API and TGSI changes (this patch series).
> 2/ Other fixes and additions to the gallium helper libraries that the
>    state tracker needs to function.
> 3/ OpenCL state tracker.
> 4/ Implementation of this API for the r600g driver, and work on AMD's
>    LLVM back-end.
> 5/ Implementation of this API for the nv50 driver, and related
>    compiler changes based on Christoph's nv50/nvc0 codegen code.
> 6/ LLVM back-end that will be used along with clang to translate
>    compute programs from the OpenCL language into TGSI.
> 
> Block 2 will be sent for review as a separate patch series soon.  I'm
> not sending block 3 in patch form because it's probably too bulky for
> this mailing list, and it's quite self-contained anyway, so, I've
> pushed it together with the other changes it depends on to the new
> 'gallium-compute' branch.
> 
> Blocks 4-6 are still rather immature, but, for the curious: The
> radeon-specific bits can be found in the 'clover-r600-master' branch
> of Tom's mesa tree [1], and some of the work done for nouveau can be
> found in the 'nv50-compute' branch of my tree [2].
> 
> About this patch series: patches 1 and 11 are API changes that give
> compute powers to pipe_context and pipe_screen, patches 3-10 are the
> extensions to the TGSI language that will be used by the state
> tracker
> to represent compute programs, patch 2 is a refactoring required by
> patch 10.
> 
> Comments, questions and r-b's are much appreciated.
> 
> [1] http://cgit.freedesktop.org/~tstellar/mesa/
> [2] https://github.com/curro/mesa
> 
> Francisco Jerez (11):
>       gallium: Basic compute interface.

Is the
 
  unsigned start_slot

parameter in bind_compute_sampler_states/set_compute_sampler_views methods really necessary? I rather have these methods consistent with the non-compute ones.  If/when we add such parameter, it should be done for all methods.

>       gallium/tgsi: Move interpolation info from tgsi_declaration to
>       a separate token.
>       gallium/tgsi: Introduce the compute processor.
>       gallium/tgsi: Define the TGSI_BUFFER texture target.

>       gallium/tgsi: Add support for raw resources.

What's the difference between raw resources and buffers? Shouldn't all buffers be raw, and non-buffers resources non-row?

>       gallium/tgsi: Add resource write-back support.
>       gallium/tgsi: Define system values used to query the compute
>       grid parameters.
>       gallium/tgsi: Add support for barriers.
>       gallium/tgsi: Add support for atomic opcodes.
>       gallium/tgsi: Introduce the "LOCAL" register declaration
>       modifier.


>       gallium/compute: Drop TGSI dependency.

This fine in principle, but I don't understand what PIPE_COMPUTE_CAP_IR_TARGET's triplets are supposed to be (a string?), and how would this scheme work, e.g., if a driver wanted to consume GLSL IR in the future.

I think that an enum would be preferrable.

Jose