2017-06-26 15:30 GMT+02:00 Lucas Stach <l.stach@xxxxxxxxxxxxxx>:
> 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@xxxxxxxxx>
>> ---
>> 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.
I totally get your point and will switch to a work item.
>> 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;
greets
--
Christian Gmeiner, MSc
https://www.youtube.com/user/AloryOFFICIAL
https://soundcloud.com/christian-gmeiner
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
