[Mesa-dev] [PATCH 01/10] gallium: add a common uploader to pipe_context

[Brian Paul]

On 01/30/2017 10:06 AM, Nicolai Hähnle wrote:
> On 27.01.2017 16:02, Marek Olšák wrote:
>> On Fri, Jan 27, 2017 at 3:38 PM, Roland Scheidegger
>> <sroland at vmware.com> wrote:
>>> Am 27.01.2017 um 12:02 schrieb Marek Olšák:
>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>
>>>> For lower memory usage and more efficient updates of the buffer
>>>> residency
>>>> list. (e.g. if drivers keep seeing the same buffer for many consecutive
>>>> "add" calls, the calls can be turned into no-ops trivially)
>>>> ---
>>>>  src/gallium/include/pipe/p_context.h | 7 +++++++
>>>>  1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/src/gallium/include/pipe/p_context.h
>>>> b/src/gallium/include/pipe/p_context.h
>>>> index 45098c9..5876968 100644
>>>> --- a/src/gallium/include/pipe/p_context.h
>>>> +++ b/src/gallium/include/pipe/p_context.h
>>>> @@ -69,33 +69,40 @@ struct pipe_stream_output_target;
>>>>  struct pipe_surface;
>>>>  struct pipe_transfer;
>>>>  struct pipe_vertex_buffer;
>>>>  struct pipe_vertex_element;
>>>>  struct pipe_video_buffer;
>>>>  struct pipe_video_codec;
>>>>  struct pipe_viewport_state;
>>>>  struct pipe_compute_state;
>>>>  union pipe_color_union;
>>>>  union pipe_query_result;
>>>> +struct u_upload_mgr;
>>>>
>>>>  /**
>>>>   * Gallium rendering context.  Basically:
>>>>   *  - state setting functions
>>>>   *  - VBO drawing functions
>>>>   *  - surface functions
>>>>   */
>>>>  struct pipe_context {
>>>>     struct pipe_screen *screen;
>>>>
>>>>     void *priv;  /**< context private data (for DRI for example) */
>>>>     void *draw;  /**< private, for draw module (temporary?) */
>>>>
>>>> +   /**
>>>> +    * Stream uploader created by the driver. All drivers, state
>>>> trackers, and
>>>> +    * modules should use it.
>>>> +    */
>>>> +   struct u_upload_mgr *stream_uploader;
>>>> +
>>>>     void (*destroy)( struct pipe_context * );
>>>>
>>>>     /**
>>>>      * VBO drawing
>>>>      */
>>>>     /*@{*/
>>>>     void (*draw_vbo)( struct pipe_context *pipe,
>>>>                       const struct pipe_draw_info *info );
>>>>     /*@}*/
>>>>
>>>>
>>>
>>> I suppose this makes sense. However, this makes util interfaces
>>> effectively part of the gallium interface, not sure how I feel about
>>> that as this seems to violate the contract that util code is optional.
>>
>> To be honest, I don't care much. Putting the uploader into
>> pipe_context has an obvious practical advantage and we want exactly
>> one instance between drivers and state trackers.
>>
>> Also, all utilities that had to use pipe_buffer_create just to upload
>> something for one use can now use this.
>>
>> If somebody wants a proper formal interface, feel free.
>>
>> I do see a small decrease in CPU overhead with legacy GL apps thanks
>> to amdgpu_cs_add_buffer getting the same buffer over and over again.
>
> I agree that this is a nice cleanup and improvement at the same time.
>
> Roland has a point, of course, and in theory one could add an
> upload_alloc function to the pipe_context, but in practice, that just
> adds an indirect call for no benefit. I think we should go ahead with this.
>
> I have some comments on patch #3, but apart from that, the series is
>
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>

I've been off-line for a while and am catching up on things.  I'd like 
to look more closely at this when I get time, but I don't want to hold 
things up.

Marek, could you at least add some documentation for this in 
src/gallium/docs/source/context.rst to explain how it should be used?

-Brian