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

[Nicolai Hähnle]

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>

>
> Marek
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>