Re: [PATCH 08/21] drm/etnaviv: add 'sync point' support

[Lucas Stach <l.stach@pengutronix.de>]

Am Freitag, den 09.06.2017, 12:21 +0200 schrieb Christian Gmeiner:
> In order to support performance counters in a sane way we need to
> provide
> a method to sync the GPU with the CPU. The GPU can process multpile
> command
> buffers/events per irq. With the help of a 'sync point' we can
> trigger an event
> and stop the GPU/FE immediately. When the CPU is done with is
> processing it
> simply needs to restart the FE and the GPU will continue.
> 
> Signed-off-by: Christian Gmeiner <christian.gmeiner@gmail.com>
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_buffer.c | 36
> ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/etnaviv/etnaviv_drv.h    |  1 +
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.c    | 14 +++++++++++++
>  drivers/gpu/drm/etnaviv/etnaviv_gpu.h    |  2 ++
>  4 files changed, 53 insertions(+)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> index ed9588f..9e7098e 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_buffer.c
> @@ -250,6 +250,42 @@ void etnaviv_buffer_end(struct etnaviv_gpu *gpu)
>  	}
>  }
>  
> +/* Append a 'sync point' to the ring buffer. */
> +void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int
> event)
> +{
> +	struct etnaviv_cmdbuf *buffer = gpu->buffer;
> +	unsigned int waitlink_offset = buffer->user_size - 16;
> +	u32 dwords, target;
> +
> +	/*
> +	 * We need at most 3 dwords in the return target:
> +	 * 1 event + 1 end + 1 wait + 1 link.
> +	 */
> +	dwords = 4;
> +	target = etnaviv_buffer_reserve(gpu, buffer, dwords);
> +
> +	/* Signal sync point event */
> +	CMD_LOAD_STATE(buffer, VIVS_GL_EVENT,
> VIVS_GL_EVENT_EVENT_ID(event) |
> +		       VIVS_GL_EVENT_FROM_PE);
> +
> +	/* Stop the FE to 'pause' the GPU */
> +	CMD_END(buffer);
> +
> +	/* Append waitlink */
> +	CMD_WAIT(buffer);
> +	CMD_LINK(buffer, 2, etnaviv_cmdbuf_get_va(buffer) +
> +			    buffer->user_size - 4);
> +
> +	/*
> +	 * Kick off the 'sync point' command by replacing the
> previous
> +	 * WAIT with a link to the address in the ring buffer.
> +	 */
> +	etnaviv_buffer_replace_wait(buffer, waitlink_offset,
> +				    VIV_FE_LINK_HEADER_OP_LINK |
> +				    VIV_FE_LINK_HEADER_PREFETCH(dwor
> ds),
> +				    target);
> +}
> +
>  /* Append a command buffer to the ring buffer. */
>  void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int
> event,
>  	struct etnaviv_cmdbuf *cmdbuf)
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> index 058389f..f6cdd69 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_drv.h
> @@ -101,6 +101,7 @@ int etnaviv_gem_new_userptr(struct drm_device
> *dev, struct drm_file *file,
>  u16 etnaviv_buffer_init(struct etnaviv_gpu *gpu);
>  u16 etnaviv_buffer_config_mmuv2(struct etnaviv_gpu *gpu, u32
> mtlb_addr, u32 safe_addr);
>  void etnaviv_buffer_end(struct etnaviv_gpu *gpu);
> +void etnaviv_sync_point_queue(struct etnaviv_gpu *gpu, unsigned int
> event);
>  void etnaviv_buffer_queue(struct etnaviv_gpu *gpu, unsigned int
> event,
>  	struct etnaviv_cmdbuf *cmdbuf);
>  void etnaviv_validate_init(void);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 037087e..803fcf4 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -25,6 +25,7 @@
>  #include "etnaviv_gpu.h"
>  #include "etnaviv_gem.h"
>  #include "etnaviv_mmu.h"
> +#include "etnaviv_perfmon.h"
>  #include "common.xml.h"
>  #include "state.xml.h"
>  #include "state_hi.xml.h"
> @@ -1349,6 +1350,7 @@ int etnaviv_gpu_submit(struct etnaviv_gpu *gpu,
>  	}
>  
>  	gpu->event[event].fence = fence;
> +	gpu->event[event].sync_point = NULL;
>  	submit->fence = dma_fence_get(fence);
>  	gpu->active_fence = submit->fence->seqno;
>  
> @@ -1394,6 +1396,15 @@ int etnaviv_gpu_submit(struct etnaviv_gpu
> *gpu,
>  	return ret;
>  }
>  
> +static void etnaviv_process_sync_point(struct etnaviv_gpu *gpu,
> +	struct etnaviv_event *event)
> +{
> +	u32 addr = gpu_read(gpu, VIVS_FE_DMA_ADDRESS);
> +
> +	event->sync_point(gpu, event);
> +	etnaviv_gpu_start_fe(gpu, addr + 2, 2);
> +}
> +
>  /*
>   * Init/Cleanup:
>   */
> @@ -1440,6 +1451,9 @@ static irqreturn_t irq_handler(int irq, void
> *data)
>  
>  			dev_dbg(gpu->dev, "event %u\n", event);
>  
> +			if (gpu->event[event].sync_point)
> +				etnaviv_process_sync_point(gpu,
> &gpu->event[event]);
> +

Sorry, but this part is a no-go. This means you are doing all the PM
register reads/writes (which might be many) from the IRQ handler. This
is a crucial fast path in the driver, which affects the realtime
capabilities of the whole system, not just the GPU driver. I'll reject
any change which adds significant overhead here.

In general the sync point idea is fine, but what you might need to do
here is move the PM register handling and FE restart to a work item,
queued on the etnaviv WQ. This might add some latency to the GPU
restart after a sync point, but at least it won't affect the system as
a whole.
>  			fence = gpu->event[event].fence;
>  			gpu->event[event].fence = NULL;
>  			dma_fence_signal(fence);
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> index 689cb8f..fee6ed9 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.h
> @@ -90,6 +90,8 @@ struct etnaviv_chip_identity {
>  struct etnaviv_event {
>  	bool used;
>  	struct dma_fence *fence;
> +
> +	void (*sync_point)(struct etnaviv_gpu *gpu, struct
> etnaviv_event *event);
>  };
>  
>  struct etnaviv_cmdbuf_suballoc;
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel