I've sent my substantive feedback in other messages in the thread.
Below is just some nitpicky formatting kinds of stuff.
On 03/06/2015 01:58 PM, Chris Wilson wrote:
> When rendering to an fbo, even though it may be acting as a winsys
> frontbuffer or just generally, we never throttle. However, when rendering
> to an fbo, there is no natural frame boundary. Conventionally we use
> SwapBuffers and glFinish, but potential callers avoid often glFinish for
> being too heavy handed (waiting on all outstanding rendering to complete).
> The kernel provides a soft-throttling option for this case that waits for
> rendering older than 20ms to be complete (that's a little too lax to be
> used for swapbuffers, but is here a useful safety net). The remaining
> choice is then either never to throttle, throttle after every draw call,
> or at after intermediate user defined point such as glFlush and thus all the
> implied flushes. This patch opts for the latter as that is the current
> method used for flushing to front buffers.
>
> v2: Defer the throttling from inside the flush to the next
> intel_prepare_render() and switch non-fbo frontbuffer throttling over to
> use the same lax method. The issuing being that
> glFlush()/intel_prepare_read() is just as likely to be called inside a
> tight loop and not at "frame" boundaries.
>
> v3: Rename from need_front_throttle to need_flush_throttle to avoid any
> ambiguity between front buffer rendering and fbo rendering. (Chad)
>
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Daniel Vetter <daniel.vet...@ffwll.ch>
> Cc: Kenneth Graunke <kenn...@whitecape.org>
> Cc: Ben Widawsky <b...@bwidawsk.net>
> Cc: Kristian Høgsberg <k...@bitplanet.net>
> Cc: Chad Versace <chad.vers...@linux.intel.com>
> Reviewed-by: Chad Versace <chad.vers...@linux.intel.com>
> ---
> src/mesa/drivers/dri/i965/brw_context.c | 16 ++++++++++++----
> src/mesa/drivers/dri/i965/brw_context.h | 12 +++++++++++-
> src/mesa/drivers/dri/i965/intel_screen.c | 8 ++++----
> 3 files changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
> b/src/mesa/drivers/dri/i965/brw_context.c
> index 972e458..bfda55f 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -232,8 +232,8 @@ intel_glFlush(struct gl_context *ctx)
>
> intel_batchbuffer_flush(brw);
> intel_flush_front(ctx);
> - if (brw_is_front_buffer_drawing(ctx->DrawBuffer))
> - brw->need_throttle = true;
> +
> + brw->need_flush_throttle = true;
> }
>
> static void
> @@ -1238,12 +1238,20 @@ intel_prepare_render(struct brw_context *brw)
> * the swap, and getting our hands on that doesn't seem worth it,
> * so we just us the first batch we emitted after the last swap.
> */
> - if (brw->need_throttle && brw->first_post_swapbuffers_batch) {
> + if (brw->need_swap_throttle && brw->first_post_swapbuffers_batch) {
> if (!brw->disable_throttling)
> drm_intel_bo_wait_rendering(brw->first_post_swapbuffers_batch);
> drm_intel_bo_unreference(brw->first_post_swapbuffers_batch);
> brw->first_post_swapbuffers_batch = NULL;
> - brw->need_throttle = false;
> + brw->need_swap_throttle = false;
> + /* Throttling here is more precise than the throttle ioctl, so skip it
> */
> + brw->need_flush_throttle = false;
> + }
> +
> + if (brw->need_flush_throttle) {
> + __DRIscreen *psp = brw->intelScreen->driScrnPriv;
> + drmCommandNone(psp->fd, DRM_I915_GEM_THROTTLE);
> + brw->need_flush_throttle = false;
> }
> }
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
> index 682fbe9..7854300 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1031,7 +1031,17 @@ struct brw_context
>
> /** Framerate throttling: @{ */
> drm_intel_bo *first_post_swapbuffers_batch;
> - bool need_throttle;
Blank line here.
> + /* Limit the number of outstanding SwapBuffers by waiting for an earlier
> + * frame of rendering to complete. This gives a very precise cap to the
> + * latency between input and output such that rendering never gets more
> + * than a frame behind the user. (With the caveat that we technically are
> + * not using the SwapBuffers itself as a barrier but the first batch
> + * submitted afterwards, which may be immediately prior to the next
> + * SwapBuffers.)
> + */
> + bool need_swap_throttle;
> + /** General throttling, not caught by throttling between SwapBuffers */
> + bool need_flush_throttle;
> /** @} */
>
> GLuint stats_wm;
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c
> b/src/mesa/drivers/dri/i965/intel_screen.c
> index cea7ddf..3640b67 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -174,10 +174,10 @@ intel_dri2_flush_with_flags(__DRIcontext *cPriv,
> if (flags & __DRI2_FLUSH_DRAWABLE)
> intel_resolve_for_dri2_flush(brw, dPriv);
>
> - if (reason == __DRI2_THROTTLE_SWAPBUFFER ||
> - reason == __DRI2_THROTTLE_FLUSHFRONT) {
> - brw->need_throttle = true;
> - }
> + if (reason == __DRI2_THROTTLE_SWAPBUFFER)
> + brw->need_swap_throttle = true;
> + if (reason == __DRI2_THROTTLE_FLUSHFRONT)
> + brw->need_flush_throttle = true;
>
> intel_batchbuffer_flush(brw);
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
