[Mesa-dev] [PATCH 01/10] winsys/radeon: remove cs_write_reloc, add simpler cs_get_reloc
Christian König
deathsimple at vodafone.de
Tue Apr 15 04:12:18 PDT 2014
Well the question isn't if we need a synchronous flush, which as far as
I can see is clear that we need it. But instead if we should implement
the synchronous flush if with an extra flag while calling cs_flush_sync
directly after the submission is equivalent.
It is possible that upper layers need the flag it simplifies the logic,
but just looking at the winsys interface it seems superfluous.
Anyway I'm fine either way it just looked like it's unnecessary to me,
Christian.
Am 15.04.2014 01:21, schrieb Marek Olšák:
> Actually, I take it back. There are several reasons for having an explicit flag.
>
> 1) Flushes from the state tracker are always synchronous. It's because
> the CS ioctl must be finished before libGL sends the SwapBuffers
> request to the X server.
>
> 2) Flushes before calling buffer_map where synchronization is expected
> are also synchronous. The buffer_map and buffer_wait functions don't
> call cs_sync_flush, instead, they loop and call sched_yield(). This is
> probably fixable now that there is a global CS queue (originally, each
> CS instance had its own thread).
>
> 3) I guess there is some overhead in switching threads, and doing
> command submission in the main thread is probably easier for the CPU.
>
> Marek
>
>
> On Mon, Apr 14, 2014 at 2:41 PM, Marek Olšák <maraeo at gmail.com> wrote:
>> Yes, I will remove the flag in the next patch series.
>>
>> Marek
>>
>> On Sun, Apr 13, 2014 at 10:45 AM, Christian König
>> <deathsimple at vodafone.de> wrote:
>>> For this series: Reviewed-by: Christian König <christian.koenig at amd.com>
>>>
>>> BTW: Can we get rid of RADEON_FLUSH_ASYNC and always flush asynchronously?
>>> Just calling cs_sync_flush directly after the flush should have the same
>>> effect as synchronous flushing.
>>>
>>> Christian.
>>>
>>> Am 12.04.2014 18:34, schrieb Marek Olšák:
>>>
>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>
>>>> The only difference is that it doesn't write to the CS and only returns
>>>> the index.
>>>> ---
>>>> src/gallium/drivers/r300/r300_cs.h | 3 ++-
>>>> src/gallium/drivers/r300/r300_emit.c | 4 ++--
>>>> src/gallium/winsys/radeon/drm/radeon_drm_cs.c | 26
>>>> +++++++++-----------------
>>>> src/gallium/winsys/radeon/drm/radeon_winsys.h | 19 ++++++++++---------
>>>> 4 files changed, 23 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/src/gallium/drivers/r300/r300_cs.h
>>>> b/src/gallium/drivers/r300/r300_cs.h
>>>> index 744e19e..748d6ea 100644
>>>> --- a/src/gallium/drivers/r300/r300_cs.h
>>>> +++ b/src/gallium/drivers/r300/r300_cs.h
>>>> @@ -109,7 +109,8 @@
>>>> #define OUT_CS_RELOC(r) do { \
>>>> assert((r)); \
>>>> assert((r)->cs_buf); \
>>>> - cs_winsys->cs_write_reloc(cs_copy, (r)->cs_buf); \
>>>> + OUT_CS(0xc0001000); /* PKT3_NOP */ \
>>>> + OUT_CS(cs_winsys->cs_get_reloc(cs_copy, (r)->cs_buf) * 4); \
>>>> CS_USED_DW(2); \
>>>> } while (0)
>>>> diff --git a/src/gallium/drivers/r300/r300_emit.c
>>>> b/src/gallium/drivers/r300/r300_emit.c
>>>> index d99b919..9f16413 100644
>>>> --- a/src/gallium/drivers/r300/r300_emit.c
>>>> +++ b/src/gallium/drivers/r300/r300_emit.c
>>>> @@ -1043,8 +1043,8 @@ void r300_emit_vertex_arrays_swtcl(struct
>>>> r300_context *r300, boolean indexed)
>>>> OUT_CS(0);
>>>> assert(r300->vbo_cs);
>>>> - cs_winsys->cs_write_reloc(cs_copy, r300->vbo_cs);
>>>> - CS_USED_DW(2);
>>>> + OUT_CS(0xc0001000); /* PKT3_NOP */
>>>> + OUT_CS(r300->rws->cs_get_reloc(r300->cs, r300->vbo_cs) * 4);
>>>> END_CS;
>>>> }
>>>> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
>>>> b/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
>>>> index b55eb80..4ce1717 100644
>>>> --- a/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
>>>> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
>>>> @@ -313,6 +313,14 @@ static unsigned radeon_drm_cs_add_reloc(struct
>>>> radeon_winsys_cs *rcs,
>>>> return index;
>>>> }
>>>> +static int radeon_drm_cs_get_reloc(struct radeon_winsys_cs *rcs,
>>>> + struct radeon_winsys_cs_handle *buf)
>>>> +{
>>>> + struct radeon_drm_cs *cs = radeon_drm_cs(rcs);
>>>> +
>>>> + return radeon_get_reloc(cs->csc, (struct radeon_bo*)buf, NULL);
>>>> +}
>>>> +
>>>> static boolean radeon_drm_cs_validate(struct radeon_winsys_cs *rcs)
>>>> {
>>>> struct radeon_drm_cs *cs = radeon_drm_cs(rcs);
>>>> @@ -359,22 +367,6 @@ static boolean
>>>> radeon_drm_cs_memory_below_limit(struct radeon_winsys_cs *rcs, ui
>>>> return status;
>>>> }
>>>> -static void radeon_drm_cs_write_reloc(struct radeon_winsys_cs *rcs,
>>>> - struct radeon_winsys_cs_handle
>>>> *buf)
>>>> -{
>>>> - struct radeon_drm_cs *cs = radeon_drm_cs(rcs);
>>>> - struct radeon_bo *bo = (struct radeon_bo*)buf;
>>>> - unsigned index = radeon_get_reloc(cs->csc, bo, NULL);
>>>> -
>>>> - if (index == -1) {
>>>> - fprintf(stderr, "radeon: Cannot get a relocation in %s.\n",
>>>> __func__);
>>>> - return;
>>>> - }
>>>> -
>>>> - OUT_CS(&cs->base, 0xc0001000);
>>>> - OUT_CS(&cs->base, index * RELOC_DWORDS);
>>>> -}
>>>> -
>>>> void radeon_drm_cs_emit_ioctl_oneshot(struct radeon_drm_cs *cs, struct
>>>> radeon_cs_context *csc)
>>>> {
>>>> unsigned i;
>>>> @@ -650,9 +642,9 @@ void radeon_drm_cs_init_functions(struct
>>>> radeon_drm_winsys *ws)
>>>> ws->base.cs_create = radeon_drm_cs_create;
>>>> ws->base.cs_destroy = radeon_drm_cs_destroy;
>>>> ws->base.cs_add_reloc = radeon_drm_cs_add_reloc;
>>>> + ws->base.cs_get_reloc = radeon_drm_cs_get_reloc;
>>>> ws->base.cs_validate = radeon_drm_cs_validate;
>>>> ws->base.cs_memory_below_limit = radeon_drm_cs_memory_below_limit;
>>>> - ws->base.cs_write_reloc = radeon_drm_cs_write_reloc;
>>>> ws->base.cs_flush = radeon_drm_cs_flush;
>>>> ws->base.cs_set_flush_callback = radeon_drm_cs_set_flush;
>>>> ws->base.cs_is_buffer_referenced = radeon_bo_is_referenced;
>>>> diff --git a/src/gallium/winsys/radeon/drm/radeon_winsys.h
>>>> b/src/gallium/winsys/radeon/drm/radeon_winsys.h
>>>> index 485e925..320989c 100644
>>>> --- a/src/gallium/winsys/radeon/drm/radeon_winsys.h
>>>> +++ b/src/gallium/winsys/radeon/drm/radeon_winsys.h
>>>> @@ -450,6 +450,16 @@ struct radeon_winsys {
>>>> enum radeon_bo_priority priority);
>>>> /**
>>>> + * Return the index of an already-added buffer.
>>>> + *
>>>> + * \param cs Command stream
>>>> + * \param buf Buffer
>>>> + * \return The buffer index, or -1 if the buffer has not
>>>> been added.
>>>> + */
>>>> + int (*cs_get_reloc)(struct radeon_winsys_cs *cs,
>>>> + struct radeon_winsys_cs_handle *buf);
>>>> +
>>>> + /**
>>>> * Return TRUE if there is enough memory in VRAM and GTT for the
>>>> relocs
>>>> * added so far. If the validation fails, all the relocations which
>>>> have
>>>> * been added since the last call of cs_validate will be removed and
>>>> @@ -470,15 +480,6 @@ struct radeon_winsys {
>>>> boolean (*cs_memory_below_limit)(struct radeon_winsys_cs *cs,
>>>> uint64_t vram, uint64_t gtt);
>>>> /**
>>>> - * Write a relocated dword to a command buffer.
>>>> - *
>>>> - * \param cs A command stream the relocation is written to.
>>>> - * \param buf A winsys buffer to write the relocation for.
>>>> - */
>>>> - void (*cs_write_reloc)(struct radeon_winsys_cs *cs,
>>>> - struct radeon_winsys_cs_handle *buf);
>>>> -
>>>> - /**
>>>> * Flush a command stream.
>>>> *
>>>> * \param cs A command stream to flush.
>>>
More information about the mesa-dev
mailing list