[Mesa-dev] [PATCH 3/3] radeon/winsys: add async dma infrastructure

[Jerome Glisse]

On Sat, Jan 5, 2013 at 9:49 AM, Christian König <deathsimple at vodafone.de> wrote:
> On 04.01.2013 23:19, j.glisse at gmail.com wrote:
> [SNIP]
>
>> diff --git a/src/gallium/drivers/r300/r300_emit.c
>> b/src/gallium/drivers/r300/r300_emit.c
>> index d1ed4b3..c824821 100644
>> --- a/src/gallium/drivers/r300/r300_emit.c
>> +++ b/src/gallium/drivers/r300/r300_emit.c
>> @@ -1184,7 +1184,8 @@ validate:
>>               assert(tex && tex->buf && "cbuf is marked, but NULL!");
>>               r300->rws->cs_add_reloc(r300->cs, tex->cs_buf,
>>                                       RADEON_USAGE_READWRITE,
>> -                                    r300_surface(fb->cbufs[i])->domain);
>> +                                    r300_surface(fb->cbufs[i])->domain,
>> +                                    RADEON_RING_DMA);
>
>
> ??? DMA ring on R300? At least on first glance that looks quite odd, should
> probably be GFX ring instead.

Yeah it's cut and paste error i catched up that when testing on r3xx

>
>>           }
>>           /* ...depth buffer... */
>>           if (fb->zsbuf) {
>> @@ -1192,7 +1193,8 @@ validate:
>>               assert(tex && tex->buf && "zsbuf is marked, but NULL!");
>>               r300->rws->cs_add_reloc(r300->cs, tex->cs_buf,
>>                                       RADEON_USAGE_READWRITE,
>> -                                    r300_surface(fb->zsbuf)->domain);
>> +                                    r300_surface(fb->zsbuf)->domain,
>> +                                    RADEON_RING_DMA);
>
>
> Same here and repeats on a couple of more places.
> [SNIP]
>
>
>> diff --git a/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> b/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> index 16536dc..5ff463e 100644
>> --- a/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> +++ b/src/gallium/winsys/radeon/drm/radeon_winsys.h
>> @@ -43,11 +43,13 @@
>>   #include "pipebuffer/pb_buffer.h"
>>   #include "libdrm/radeon_surface.h"
>>   -#define RADEON_MAX_CMDBUF_DWORDS (16 * 1024)
>> +#define RADEON_MAX_CMDBUF_DWORDS        (16 * 1024)
>>   -#define RADEON_FLUSH_ASYNC           (1 << 0)
>> -#define RADEON_FLUSH_KEEP_TILING_FLAGS (1 << 1) /* needs DRM 2.12.0 */
>> -#define RADEON_FLUSH_COMPUTE           (1 << 2)
>> +#define RADEON_FLUSH_ASYNC              (1 << 0)
>> +#define RADEON_FLUSH_KEEP_TILING_FLAGS  (1 << 1) /* needs DRM 2.12.0 */
>> +#define RADEON_FLUSH_COMPUTE            (1 << 2)
>> +#define RADEON_FLUSH_DMA                (1 << 3)
>> +#define RADEON_FLUSH_GFX                (1 << 4)
>>     /* Tiling flags. */
>>   enum radeon_bo_layout {
>> @@ -137,12 +139,19 @@ enum chip_class {
>>       TAHITI,
>>   };
>>   +enum radeon_ring_type {
>> +    RADEON_RING_PM4 = 0,
>> +    RADEON_RING_DMA = 1,
>> +};
>> +
>
>
> Don't use PM4 as identifier here, the PM4 packet format is used for other
> ring types beside GFX/Compute as well, but those rings can't necessary
> execute GFX/Compute commands.

I was looking for a 3 letter name that encompass gfx and compute

>>   struct winsys_handle;
>>   struct radeon_winsys_cs_handle;
>>     struct radeon_winsys_cs {
>> -    unsigned cdw;  /* Number of used dwords. */
>> -    uint32_t *buf; /* The command buffer. */
>> +    unsigned                    cdw;      /* Number of used dwords. */
>> +    uint32_t                    *buf;     /* The command buffer. */
>> +    unsigned                    dma_cdw;  /* Number of used dwords. */
>> +    uint32_t                    *dma_buf; /* The command buffer. */
>>   };
>
>
> Why like this? Can't we just have separate instances of the radeon_winsys_cs
> structure for each ring type we are dealing with?
>
> The rest looks quite good,
> Christian.

No we can't we need to keep track at the same time for same context of
the dma ring and the gfx/compute/uvd/other ring It's the relocation
code that needs that.

Cheers,
Jerome