Skip to content

Commit

Permalink
libxdma: fix next adjacent descriptors
Browse files Browse the repository at this point in the history 
Fix the setting of the next adjacent fields in descriptors.

Following commit 5faf23e the next_adj field of all descriptors is set
according to the index of the descriptor rather than its address which
causes issues when dma_alloc_coherent doesn't return an address which is
page aligned (which happens).
Moreover, in the case of a transfer which number of descriptors is
bigger than a full page, the next_adj field is set to the maximum (63)
for all descriptors untill the last page of descriptors where it starts
decreasing.
Last, even before this commit, the next_adj field inside a block of
adjacent descriptors is not decreasing untill coming near page end,
which is not compliant with what the documentation says :

"Every descriptor in the descriptor list must accurately describe the descriptor
or block ofdescriptors that follows. In a block of adjacent descriptors, the
Nxt_adj value decrements from the first descriptor to the second to last
descriptor which has a value of zero. Likewise, eachdescriptor in the block
points to the next descriptor in the block, except for the last descriptor
which might point to a new block or might terminate the list."

This commit aligns the blocks of adjacent descriptors to
XDMA_MAX_ADJ_BLOCK_SIZE and makes the next_adj field decrease inside
each block untill the second to last descriptor in the block or in the
full transfer. The size of the page being a multiple of the size of the
block (4096 = sizeof(xdma_desc) * 128 =
sizeof(xdma_desc) * 2 * XDMA_MAX_ADJ_BLOCK_SIZE
  • Loading branch information
@jberaud
jberaud committed Sep 15, 2020
1 parent 8a5bb0e commit 209ee7c
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 35 deletions.
88 changes: 54 additions & 34 deletions XDMA/linux-kernel/libxdma/libxdma.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -601,6 +601,35 @@ static int engine_start_mode_config(struct xdma_engine *engine)
return 0;
}

/**
* xdma_get_next_adj()
*
* Get the number for adjacent descriptors to set in a descriptor, based on the
* remaining number of descriptors and the lower bits of the address of the
* next descriptor.
* Since the number of descriptors in a page (XDMA_PAGE_SIZE) is 128 and the
* maximum size of a block of adjacent descriptors is 64 (63 max adjacent
* descriptors for any descriptor), align the blocks of adjacent descriptors
* to the block size.
*/
static u32 xdma_get_next_adj(unsigned int remaining, u32 next_lo)
{
unsigned int next_index;

dbg_desc("%s: remaining_desc %u, next_lo 0x%x\n",__func__, remaining,
next_lo);

if (remaining <= 1)
return 0;

/* shift right 5 times corresponds to a division by
* sizeof(xdma_desc) = 32
*/
next_index = ((next_lo & (XDMA_PAGE_SIZE - 1)) >> 5) %
XDMA_MAX_ADJ_BLOCK_SIZE;
return min(XDMA_MAX_ADJ_BLOCK_SIZE - next_index - 1, remaining - 1);
}

/**
* engine_start() - start an idle engine with its first transfer on queue
*
Expand All @@ -620,8 +649,7 @@ static int engine_start_mode_config(struct xdma_engine *engine)
static struct xdma_transfer *engine_start(struct xdma_engine *engine)
{
struct xdma_transfer *transfer;
u32 w;
int extra_adj = 0;
u32 w, next_adj;
int rv;

if (!engine) {
Expand Down Expand Up @@ -681,15 +709,14 @@ static struct xdma_transfer *engine_start(struct xdma_engine *engine)
(unsigned long)(&engine->sgdma_regs->first_desc_hi) -
(unsigned long)(&engine->sgdma_regs));

if (transfer->desc_adjacent > 0) {
extra_adj = transfer->desc_adjacent - 1;
if (extra_adj > MAX_EXTRA_ADJ)
extra_adj = MAX_EXTRA_ADJ;
}
dbg_tfr("iowrite32(0x%08x to 0x%p) (first_desc_adjacent)\n", extra_adj,
next_adj = xdma_get_next_adj(transfer->desc_adjacent,
cpu_to_le32(PCI_DMA_L(transfer->desc_bus)));

dbg_tfr("iowrite32(0x%08x to 0x%p) (first_desc_adjacent)\n", next_adj,
(void *)&engine->sgdma_regs->first_desc_adjacent);

write_register(
extra_adj, &engine->sgdma_regs->first_desc_adjacent,
next_adj, &engine->sgdma_regs->first_desc_adjacent,
(unsigned long)(&engine->sgdma_regs->first_desc_adjacent) -
(unsigned long)(&engine->sgdma_regs));

Expand Down Expand Up @@ -2439,6 +2466,7 @@ static int transfer_desc_init(struct xdma_transfer *transfer, int count)
desc_virt[i].next_lo = cpu_to_le32(PCI_DMA_L(desc_bus));
desc_virt[i].next_hi = cpu_to_le32(PCI_DMA_H(desc_bus));
desc_virt[i].bytes = cpu_to_le32(0);

desc_virt[i].control = cpu_to_le32(DESC_MAGIC);
}
/* { i = number - 1 } */
Expand Down Expand Up @@ -2491,16 +2519,12 @@ static void xdma_desc_link(struct xdma_desc *first, struct xdma_desc *second,
}

/* xdma_desc_adjacent -- Set how many descriptors are adjacent to this one */
static void xdma_desc_adjacent(struct xdma_desc *desc, int next_adjacent)
static void xdma_desc_adjacent(struct xdma_desc *desc, u32 next_adjacent)
{
/* remember reserved and control bits */
u32 control = le32_to_cpu(desc->control) & 0xffffc0ffUL;

if (next_adjacent)
next_adjacent = next_adjacent - 1;
if (next_adjacent > MAX_EXTRA_ADJ)
next_adjacent = MAX_EXTRA_ADJ;
control |= (next_adjacent << 8);
u32 control = le32_to_cpu(desc->control) & 0x0000f0ffUL;

This comment has been minimized.

Sign in to view

Copy link
@mpb27

mpb27 Nov 24, 2020

I think the mask here should be 0x0000c0ffUL since max value for next_adjacent is 0x3F.

This comment has been minimized.

Sign in to view

Copy link
@jberaud

jberaud Nov 24, 2020

Author Owner

You are right thanks. This code is in master branch on Xilinx's github. That said, this initialization is actually useless (thus the absence of real bug caused) since transfer_desc_init already does:
desc_virt[i].control = cpu_to_le32(DESC_MAGIC); for each descriptor. It means that the function can be replaced by:

/* xdma_desc_adjacent -- Set how many descriptors are adjacent to this one */
static void xdma_desc_adjacent(struct xdma_desc *desc, u32 next_adjacent)
{
        desc->control = cpu_to_le32(le32_to_cpu(desc->control) | next_adjacent << 8);
}

I'll send a PR.
FYI the most up to date public version of the dma_ip_driver we have is https://github.com/sasfermat/dma_ip_drivers
and we have some additional code to restore the cyclic routine on a private version.

This comment has been minimized.

Sign in to view

Copy link
@jberaud

jberaud Nov 24, 2020

Author Owner

Xilinx#92
/* merge adjacent and control field */
control |= 0xAD4B0000UL | (next_adjacent << 8);
/* write control and next_adjacent */
desc->control = cpu_to_le32(control);
}
Expand Down Expand Up @@ -3146,7 +3170,6 @@ static int transfer_init(struct xdma_engine *engine,
unsigned int desc_max = min_t(unsigned int,
req->sw_desc_cnt - req->sw_desc_idx,
XDMA_TRANSFER_MAX_DESC);
unsigned int desc_align = 0;
int i = 0;
int last = 0;
u32 control;
Expand Down Expand Up @@ -3184,16 +3207,7 @@ static int transfer_init(struct xdma_engine *engine,
xfer, (u64)xfer->desc_bus);
transfer_build(engine, req, xfer, desc_max);

/*
* Contiguous descriptors cannot cross PAGE boundary
* The 1st descriptor may start in the middle of the page,
* calculate the 1st block of adj desc accordingly
*/
desc_align = 128 - (engine->desc_idx % 128) - 1;
if (desc_align > (desc_max - 1))
desc_align = desc_max - 1;

xfer->desc_adjacent = desc_align;
xfer->desc_adjacent = desc_max;

/* terminate last descriptor */
last = desc_max - 1;
Expand All @@ -3209,11 +3223,13 @@ static int transfer_init(struct xdma_engine *engine,
engine->desc_used += desc_max;

/* fill in adjacent numbers */
for (i = 0; i < xfer->desc_num && desc_align; i++, desc_align--)
xdma_desc_adjacent(xfer->desc_virt + i, desc_align);
for (i = 0; i < xfer->desc_num; i++) {
u32 next_adj = xdma_get_next_adj(xfer->desc_num - i - 1,
(xfer->desc_virt + i)->next_lo);

for (; i < xfer->desc_num; i++)
xdma_desc_adjacent(xfer->desc_virt + i, xfer->desc_num - i - 1);
dbg_desc("set next adj at index %d to %u\n", i, next_adj);
xdma_desc_adjacent(xfer->desc_virt + i, next_adj);
}

spin_unlock_irqrestore(&engine->lock, flags);
return 0;
Expand Down Expand Up @@ -3271,8 +3287,12 @@ static int transfer_init_cyclic(struct xdma_engine *engine,

dbg_sg("transfer 0x%p has %d descriptors\n", xfer, xfer->desc_num);
/* fill in adjacent numbers */
for (i = 0; i < xfer->desc_num; i++)
xdma_desc_adjacent(xfer->desc_virt + i, xfer->desc_num - i - 1);
for (i = 0; i < xfer->desc_num; i++) {
u32 next_adj = xdma_get_next_adj(xfer->desc_num - i - 1,
(xfer->desc_virt + i)->next_lo);
dbg_desc("set next adj at index %d to %u\n", i, next_adj);
xdma_desc_adjacent(xfer->desc_virt + i, next_adj);
}

return 0;
}
Expand Down
3 changes: 2 additions & 1 deletion XDMA/linux-kernel/libxdma/libxdma.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@
* .REG_IRQ_OUT (reg_irq_from_ch[(channel*2) +: 2]),
*/
#define XDMA_ENG_IRQ_NUM (1)
#define MAX_EXTRA_ADJ (0x3F)
#define XDMA_MAX_ADJ_BLOCK_SIZE 0x40
#define XDMA_PAGE_SIZE 0x1000
#define RX_STATUS_EOP (1)

/* Target internal components on XDMA control BAR */
Expand Down

0 comments on commit 209ee7c

Please sign in to comment.