Re: [PATCH v2 03/13] gpu: host1x: Support 40-bit addressing

Mikko Perttunen
2019-01-25T09:32:57Z

On 25.1.2019 11.20, Thierry Reding wrote:
> On Fri, Jan 25, 2019 at 11:13:41AM +0200, Mikko Perttunen wrote:
>> On 24.1.2019 20.02, Thierry Reding wrote:
>>> From: Thierry Reding <treding@nvidia.com>
>>>
>>> Tegra186 and later support 40 bits of address space. Additional
>>> registers need to be programmed to store the full 40 bits of push
>>> buffer addresses.
>>>
>>> Since command stream gathers can also reside in buffers in a 40-bit
>>> address space, a new variant of the GATHER opcode is also introduced.
>>> It takes two parameters: the first parameter contains the lower 32
>>> bits of the address and the second parameter contains bits 32 to 39.
>>>
>>> Signed-off-by: Thierry Reding <treding@nvidia.com>
>>> ---
>>> drivers/gpu/host1x/hw/cdma_hw.c | 13 ++++++++---
>>> drivers/gpu/host1x/hw/channel_hw.c | 28 +++++++++++++++++++----
>>> drivers/gpu/host1x/hw/host1x06_hardware.h | 5 ++++
>>> drivers/gpu/host1x/hw/host1x07_hardware.h | 5 ++++
>>> 4 files changed, 44 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/gpu/host1x/hw/cdma_hw.c b/drivers/gpu/host1x/hw/cdma_hw.c
>>> index ce320534cbed..6d2b7af2af89 100644
>>> --- a/drivers/gpu/host1x/hw/cdma_hw.c
>>> +++ b/drivers/gpu/host1x/hw/cdma_hw.c
>>> @@ -68,20 +68,27 @@ static void cdma_timeout_cpu_incr(struct host1x_cdma *cdma, u32 getptr,
>>> static void cdma_start(struct host1x_cdma *cdma)
>>> {
>>> struct host1x_channel *ch = cdma_to_channel(cdma);
>>> + u64 start, end;
>>> if (cdma->running)
>>> return;
>>> cdma->last_pos = cdma->push_buffer.pos;
>>> + start = cdma->push_buffer.dma;
>>> + end = cdma->push_buffer.dma + cdma->push_buffer.size + 4;
>>> host1x_ch_writel(ch, HOST1X_CHANNEL_DMACTRL_DMASTOP,
>>> HOST1X_CHANNEL_DMACTRL);
>>> /* set base, put and end pointer */
>>> - host1x_ch_writel(ch, cdma->push_buffer.dma, HOST1X_CHANNEL_DMASTART);
>>> + host1x_ch_writel(ch, lower_32_bits(start), HOST1X_CHANNEL_DMASTART);
>>> host1x_ch_writel(ch, cdma->push_buffer.pos, HOST1X_CHANNEL_DMAPUT);
>>> - host1x_ch_writel(ch, cdma->push_buffer.dma + cdma->push_buffer.size + 4,
>>> - HOST1X_CHANNEL_DMAEND);
>>> + host1x_ch_writel(ch, lower_32_bits(end), HOST1X_CHANNEL_DMAEND);
>>> +
>>> +#if HOST1X_HW >= 6
>>> + host1x_ch_writel(ch, upper_32_bits(start), HOST1X_CHANNEL_DMASTART_HI);
>>> + host1x_ch_writel(ch, upper_32_bits(end), HOST1X_CHANNEL_DMAEND_HI);
>>> +#endif
>>> /* reset GET */
>>> host1x_ch_writel(ch, HOST1X_CHANNEL_DMACTRL_DMASTOP |
>>> diff --git a/drivers/gpu/host1x/hw/channel_hw.c b/drivers/gpu/host1x/hw/channel_hw.c
>>> index ff137fe72d34..78fb49539e8c 100644
>>> --- a/drivers/gpu/host1x/hw/channel_hw.c
>>> +++ b/drivers/gpu/host1x/hw/channel_hw.c
>>> @@ -64,11 +64,31 @@ static void submit_gathers(struct host1x_job *job)
>>> for (i = 0; i < job->num_gathers; i++) {
>>> struct host1x_job_gather *g = &job->gathers[i];
>>> - u32 op1 = host1x_opcode_gather(g->words);
>>> - u32 op2 = g->base + g->offset;
>>> + dma_addr_t addr = g->base + g->offset;
>>> + u32 op2, op3;
>>> - trace_write_gather(cdma, g->bo, g->offset, op1 & 0xffff);
>>> - host1x_cdma_push(cdma, op1, op2);
>>> + op2 = lower_32_bits(addr);
>>> + op3 = upper_32_bits(addr);
>>> +
>>> + trace_write_gather(cdma, g->bo, g->offset, g->words);
>>> +
>>> + if (op3 != 0) {
>>> +#if HOST1X_HW >= 6
>>> + u32 op1 = host1x_opcode_gather_wide(g->words);
>>> + u32 op4 = HOST1X_OPCODE_NOP;
>>> +
>>> + host1x_cdma_push(cdma, op1, op2);
>>> + host1x_cdma_push(cdma, op3, op4);
>>
>> This will break if the first push goes as the last slot of the ringbuffer
>> and the second push goes as the first slot of the ringbuffer.
>>
>> Otherwise looks good to me.
>
> Why would that break? Isn't the purpose of a ringbuffer to behave as if
> it was infinitely sequential? If this really is a problem, how do we fix
> it? Would we have to stash NOPs into the pushbuffer until we wrap
> around?

That's because it's not actually a ringbuffer. It's actually just a
buffer with a 'RESTART 0' opcode in the last slot. As such, the GATHER_W
would incorrectly use the 'RESTART 0' opcode as its third word.

Cheers,
Mikko

>
> Thierry
>